On 04/02/2018 10:18 AM, Brijesh Singh wrote:

QEMU version >= 2.12 provides support for launching an encrypted VMs on AMD x86 platform using Secure Encrypted Virtualization (SEV) feature. This patch adds support to query the SEV capability from the qemu.

Reviewed-by: "Daniel P. Berrangé" <berrange@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- src/conf/domain_capabilities.h | 13 ++++ src/qemu/qemu_capabilities.c | 38 +++++++++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_capspriv.h | 4 ++ src/qemu/qemu_monitor.c | 9 +++ src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 73 ++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + .../caps_2.12.0.x86_64.replies | 10 +++ tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 3 +- 10 files changed, 156 insertions(+), 1 deletion(-)

Should have noted the first time - should have 2 blank lines around new methods... e.g.:

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e54dde6..0f6e6fb 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -466,6 +466,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 285 */ "virtio-mouse-ccw", "virtio-tablet-ccw", + "sev-guest", );

@@ -532,6 +533,8 @@ struct _virQEMUCaps { size_t ngicCapabilities; virGICCapability *gicCapabilities;

+ virSEVCapability *sevCapabilities; + virQEMUCapsHostCPUData kvmCPU; virQEMUCapsHostCPUData tcgCPU; }; @@ -1705,6 +1708,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "virtio-keyboard-ccw", QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW }, { "virtio-mouse-ccw", QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW }, { "virtio-tablet-ccw", QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW }, + { "sev-guest", QEMU_CAPS_SEV_GUEST }, };

static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { @@ -2784,6 +2788,21 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, qemuCaps->ngicCapabilities = ncapabilities; }

Extra blank line here...

+void +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, + virSEVCapability *capabilities) +{ + virSEVCapability *cap = qemuCaps->sevCapabilities; + + if (cap) { + VIR_FREE(cap->pdh); + VIR_FREE(cap->cert_chain); + } + + VIR_FREE(qemuCaps->sevCapabilities); + + qemuCaps->sevCapabilities = capabilities; +}

and here

static int virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps, @@ -3287,6 +3306,19 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps, return 0; }

and here

+static int +virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps, + qemuMonitorPtr mon) +{ + virSEVCapability *caps = NULL; + + if (qemuMonitorGetSEVCapabilities(mon, &caps) < 0) + return -1; + + virQEMUCapsSetSEVCapabilities(qemuCaps, caps); + + return 0; +}

and here

bool virQEMUCapsCPUFilterFeatures(const char *name,

[...] John (I would normally fix it for you; however, I have more questions and comments as I've gone on, so if a v6 will be needed, then you'd need to fix. I may be able to alter for you depending on some decisions in the middle patches).

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

QEMU version >= 2.12 provides support for launching an encrypted VMs on AMD x86 platform using Secure Encrypted Virtualization (SEV) feature. This patch adds support to query the SEV capability from the qemu.

Reviewed-by: "Daniel P. Berrangé" <berrange@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- src/conf/domain_capabilities.h | 13 ++++ src/qemu/qemu_capabilities.c | 38 +++++++++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_capspriv.h | 4 ++ src/qemu/qemu_monitor.c | 9 +++ src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 73 ++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + .../caps_2.12.0.x86_64.replies | 10 +++ tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 3 +- 10 files changed, 156 insertions(+), 1 deletion(-)

I ran the changes through coverity as a last thing to do... ....

+void +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, + virSEVCapability *capabilities) +{ + virSEVCapability *cap = qemuCaps->sevCapabilities; +

This hunk ...

+ if (cap) { + VIR_FREE(cap->pdh); + VIR_FREE(cap->cert_chain); + } + + VIR_FREE(qemuCaps->sevCapabilities);

... should be a virQEMUSevCapabilitiesFree() type function which can be called from qemuMonitorJSONGetSEVCapabilities where the function would : if (!cap) return; VIR_FREE(cap->pdh); VIR_FREE(cap->cert_chain); VIR_FREE(capabilities); and the callers would need to ensure to 'overwrite' sevCapabilities with something new or NULL.

+ + qemuCaps->sevCapabilities = capabilities; +}

[...]

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d80c4f1..e67f7b7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6398,6 +6398,79 @@ qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, return ret; }

+int +qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, + virSEVCapability **capabilities) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr caps; + virSEVCapability *capability = NULL; + const char *pdh = NULL, *cert_chain = NULL; + int cbitpos, reduced_phys_bits; + + *capabilities = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-sev-capabilities", + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + caps = virJSONValueObjectGetObject(reply, "return"); + + if (virJSONValueObjectGetNumberInt(caps, "cbitpos", &cbitpos) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'cbitpos' field is missing")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberInt(caps, "reduced-phys-bits", + &reduced_phys_bits) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'reduced-phys-bits' field is missing")); + goto cleanup; + } + + if (!(pdh = virJSONValueObjectGetString(caps, "pdh"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'pdh' field is missing")); + goto cleanup; + } + + if (!(cert_chain = virJSONValueObjectGetString(caps, "cert-chain"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'cert-chain' field is missing")); + goto cleanup; + } + + if (VIR_ALLOC(capability) < 0) + goto cleanup; + + if (VIR_STRDUP(capability->pdh, pdh) < 0) + goto cleanup; + + if (VIR_STRDUP(capability->cert_chain, cert_chain) < 0) + goto cleanup; + + capability->cbitpos = cbitpos; + capability->reduced_phys_bits = reduced_phys_bits; + *capabilities = capability;

VIR_STEAL_PTR(*capabilities, capability);

+ ret = 0; + + cleanup:

virQEMUSevCapabilitiesFree(capability); John

+ virJSONValueFree(cmd); + virJSONValueFree(reply); + + return ret; +} +

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

Extend hypervisor capabilities to include sev feature. When available, hypervisor supports launching an encrypted VM on AMD platform. The sev feature tag provides additional details like platform diffie-hellman

Diffie-Hellman right?

key and certificate chain which can be used by the guest owner to establish a cryptographic session with the SEV firmware to negotiate keys used for attestation or to provide secret during launch.

Reviewed-by: "Daniel P. Berrangé" <berrange@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- docs/formatdomaincaps.html.in | 40 ++++++++++++++++++++++++++++++++++++++++ docs/schemas/domaincaps.rng | 20 ++++++++++++++++++++ src/conf/domain_capabilities.c | 20 ++++++++++++++++++++ src/conf/domain_capabilities.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ 5 files changed, 83 insertions(+)

I see this has Daniel's R-by, but I have a few notes and questions...

diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 6bfcaf6..f383141 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -417,6 +417,12 @@ <value>3</value> </enum> </gic> + <sev> + <pdh> </pdh> + <cert-chain> </cert-chain> + <cbitpos> </cbitpos> + <reduced-phys-bits> </reduced-phys-bits> + </sev>

The example output should have some sort of example output and not an empty space.

</features> </domainCapabilities> </pre> @@ -441,5 +447,39 @@ <code>gic</code> element.</dd> </dl>

+ <h4><a id="elementsSEV">SEV capabilities</a></h4> + + <p>AMD Secure Encrypted Virtualization (SEV) capabilities are exposed under + the <code>sev</code> element. + SEV is an extension to the AMD-V architecture which supports running + virtual machines (VMs) under the control of a hypervisor. When supported, + guest owner can create a VM whose memory contents will be transparently + encrypted with a key unique to that VM.

I think it would be cleaner to add a </p> to after VM and then a new <p> on the next line

+ + For more details on SEV feature see: + <a href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf"> + SEV API spec</a> and <a href="http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf"> + SEV White Paper</a> + + </p> + + <dl> + <dt><code>pdh</code></dt> + <dd>Platform diffie-hellman key, which can be exported to remote entities

Again, I think this should be Diffie-Hellman ? And it's the public key right - so "A base64 encoded platform Diffie-Hellman public key which..."

+ which wish to establish a secure transport context with the SEV platform

"which wish" reads strange - how about "that desire"

+ in order to transmit data securely. The key is encoded in base64</dd>

Add "A base64 encoded" up front makes the last sentence duplicitous.

+ <dt><code>cert-chain</code></dt> + <dd> Platform certificate chain -- which includes platform endorsement key + (PEK), owners certificate authory (OCA) and chip endorsement key (CEK). + The certificate chain is encoded in base64.</dd>

A base64 encoded platform certificate chain that includes the platform endorsement key (PEK), owners certificate authority (OCD), and chip endorsement key (CEK).

+ <dt><code>cbitpos</code></dt> + <dd>When memory encryption is enabled, one of the physical address bit

s/bit/bits

+ (aka the C-bit) is utilized to mark if a memory page is protected. The + C-bit position is Hypervisor dependent.</dd> + <dt><code>reduced-phys-bits</code></dt> + <dd>When memory encryption is enabled, we loose certain bits in physical + address space. The number of bits we loose is hypervisor dependent.</dd> + </dl> +

s/loose/lose

</body> </html> diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 3905318..53b33eb 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -173,6 +173,9 @@ <element name='features'> <interleave> <ref name='gic'/> + <optional> + <ref name='sev'/> + </optional> </interleave> </element> </define> @@ -184,6 +187,23 @@ </element> </define>

+ <define name='sev'> + <element name='sev'> + <element name='pdh'> + <data type='string'/> + </element> + <element name='cert-chain'> + <data type='string'/> + </element> + <element name='cbitpos'> + <data type='unsignedInt'/> + </element> + <element name='reduced-phys-bits'> + <data type='unsignedInt'/> + </element> + </element> + </define> +

I want to make sure of recent preferences which aren't documented, but sometimes draw attention of certain reviewers... Do we want dashes or camelCase for new names? e.g. "cert-chain" or "certChain"? And likewise for reduced-phys-bits or reducedPhysBits?

<define name='value'> <zeroOrMore> <element name='value'> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index f7d9be5..082065f 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -549,6 +549,25 @@ virDomainCapsFeatureGICFormat(virBufferPtr buf, FORMAT_EPILOGUE(gic); }

+static void +virDomainCapsFeatureSEVFormat(virBufferPtr buf, + virSEVCapabilityPtr const sev) +{ + if (!sev) + return; + + virBufferAddLit(buf, "<sev supported='yes'>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); + virBufferAsprintf(buf, "<reduced-phys-bits>%d</reduced-phys-bits>\n", + sev->reduced_phys_bits); + virBufferAsprintf(buf, "<pdh>%s</pdh>\n", sev->pdh); + virBufferAsprintf(buf, "<cert-chain>%s</cert-chain>\n", + sev->cert_chain);

When formatting strings, use virBufferEscapeString Yes, I know they're base64 encoded, but not taking any chances... Depending on whether there's an opinion w/r/t camelCase or not, I can either fix up the nits or wait for a v6 with adjustments to the user facing values. John

+ virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</sev>\n"); +} +

char * virDomainCapsFormat(virDomainCapsPtr const caps) @@ -587,6 +606,7 @@ virDomainCapsFormat(virDomainCapsPtr const caps) virBufferAdjustIndent(&buf, 2);

virDomainCapsFeatureGICFormat(&buf, &caps->gic); + virDomainCapsFeatureSEVFormat(&buf, caps->sev);

virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</features>\n"); diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 72e9daf..2e8596c 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -170,6 +170,7 @@ struct _virDomainCaps { /* add new domain devices here */

virDomainCapsFeatureGIC gic; + virSEVCapabilityPtr sev; /* add new domain features here */ };

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 0f6e6fb..3fd4911 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5787,6 +5787,8 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps, virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0 || virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0) return -1; + + domCaps->sev = qemuCaps->sevCapabilities; return 0; }

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

The launch-security element can be used to define the security model to use when launching a domain. Currently we support 'sev'.

When 'sev' is used, the VM will be launched with AMD SEV feature enabled. SEV feature supports running encrypted VM under the control of KVM. Encrypted VMs have their pages (code and data) secured such that only the guest itself has access to the unencrypted version. Each encrypted VM is associated with a unique encryption key; if its data is accessed to a different entity using a different key the encrypted guests data will be incorrectly decrypted, leading to unintelligible data.

Reviewed-by: "Daniel P. Berrangé" <berrange@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- docs/formatdomain.html.in | 120 ++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 39 ++++++++++++++ src/conf/domain_conf.c | 110 ++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 26 +++++++++ 4 files changed, 295 insertions(+)

Again - I see the R-by from Daniel, but I have more comments for the patches... Also since this patch is where we introduce the XML, the xml2xml changes that are in the last patch should move into here so that we keep everything together. We're probably getting beyond the point of me doing the movement, so a v6 will be needed.

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 82e7d7c..2a6bed7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -8200,6 +8200,126 @@ qemu-kvm -net nic,model=? /dev/null

<p>Note: DEA/TDEA is synonymous with DES/TDES.</p>

+ <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3> + + <p> + The contents of the <code><launch-security type='sev'></code> element + is used to provide the guest owners input used for creating an encrypted + VM using the AMD SEV feature. + + SEV is an extension to the AMD-V architecture which supports running + encrypted virtual machine (VMs) under the control of KVM. Encrypted + VMs have their pages (code and data) secured such that only the guest + itself has access to the unencrypted version. Each encrypted VM is + associated with a unique encryption key; if its data is accessed to a + different entity using a different key the encrypted guests data will + be incorrectly decrypted, leading to unintelligible data. + + For more information see various input parameters and its format see SEV API spec

s/see/see the/

+ <a href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf"> https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf </a> + <span class="since">Since 4.2.0</span>

I'll be 4.3.0 now that 4.2.0 is released.

+ </p> + <pre> +<domain> + ... + <launch-security type='sev'> + <policy> 0 </policy> + <cbitpos> 47 </cbitpos> + <reduced-phys-bits> 5 </reduced-phys-bits> + <session> ... </session> + <dh-cert> ... </dh> + </sev> + ... +</domain> +</pre>

Your example should provide some more realistic values.

+ + <p> + A least <code>cbitpos</code> and <code>reduced-phys-bits</code> must be + nested within the <code>launch-security</code> element. + </p>

The above won't be necessary as long as you note required below, e.g.

+ <dl> + <dt><code>cbitpos</code></dt> + <dd>The <code>cbitpos</code> element provides the C-bit (aka encryption bit)

s/The/The required/

+ location in guest page table entry. The value of <code>cbitpos</code> is + hypervisor dependent and can be obtained through the <code>sev</code> element + from domaincapabilities.

s/from domaincapabilities/from the domain capabilities/

+ </dd> + <dt><code>reduced-phys-bits</code></dt> + <dd>The <code>reduced-phys-bits</code> element provides the physical

s/The/The required/

+ address bit reducation. Similar to <code>cbitpos</code> the value of <code>

s/reducation/reduction/

+ reduced-phys-bit</code> is hypervisor dependent and can be obtained + through the <code>sev</code> element from domaincapabilities.

s/from domaincapabilities/from the domain capabilities/

+ </dd> + <dt><code>policy</code></dt> + <dd>The optional <code>policy</code> element provides the guest policy + which must be maintained by the SEV firmware. This policy is enforced by + the firmware and restricts what configuration and operational commands + can be performed on this guest by the hypervisor. The guest policy + provided during guest launch is bound to the guest and cannot be changed + throughout the lifetime of the guest. The policy is also transmitted + during snapshot and migration flows and enforced on the destination platform. +

There's some long lines above and below - while not a policy - for those using 80 character wide displays, we try to keep lines in the 70-75 char range.

+ The guest policy is a 4-byte structure with the fields shown in Table:

How about 4 unsigned byte

+ + <table class="top_table"> + <tr> + <th> Bit(s) </th> + <th> Description </th> + </tr> + <tr> + <td> 0 </td> + <td> Debugging of the guest is disallowed when set </td> + </tr> + <tr> + <td> 1 </td> + <td> Sharing keys with other guests is disallowed when set </td> + </tr> + <tr> + <td> 2 </td> + <td> SEV-ES is required when set</td> + </tr> + <tr> + <td> 3 </td> + <td> Sending the guest to another platform is disallowed when set</td> + </tr> + <tr> + <td> 4 </td> + <td> The guest must not be transmitted to another platform that is + not in the domain when set. </td> + </tr> + <tr> + <td> 5 </td> + <td> The guest must not be transmitted to another platform that is + not SEV capable when set. </td> + </tr> + <tr> + <td> 15:6 </td> + <td> reserved </td> + </tr> + <tr> + <td> 16:32 </td> + <td> The guest must not be transmitted to another platform with a + lower firmware version. </td>

I don't see this in the qemu policies: #define SEV_POLICY_NODBG 0x1 #define SEV_POLICY_NOKS 0x2 #define SEV_POLICY_ES 0x4 #define SEV_POLICY_NOSEND 0x8 #define SEV_POLICY_DOMAIN 0x10 #define SEV_POLICY_SEV 0x20 Sois there just one bit for the lower firmware thing or is this a word field with some value expected?

+ </tr> + </table> + Default value is 0x1

So this is the QEMU default currently; however, that could change in the future. I think we could just not say what the default is. It'll make some changes a bit later on easier too.

+ + </dd> + <dt><code>dh-cert</code></dt> + <dd>The optional <code>dh-cert</code> element provides the guest owners public

s/owners/owners base64 encoded/

+ Diffie-Hellman (DH) key. The key is used to negotiate a master secret + key between the SEV firmware and guest owner. This master secret key is + then used to establish a trusted channel between SEV firmware and guest

s/SEV/the SEV/

+ owner. The value must be encoded in base64.

and the last sentence wouldn't be needed any more...

+ </dd> + <dt><code>session</code></dt> + <dd>The optional <code>session</code> element provides the guest owners + session blob defined in SEV API spec. The value must be encoded in base64.

s/session/base64 encoded session/ s/in/in the/ and the last sentence wouldn't be needed any more...

+ + See SEV spec LAUNCH_START section for session blob format.

s/for/for the/

+ </dd> + </dl> + <h2><a id="examples">Example configs</a></h2>

<p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a72c919..6a0e129 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -77,6 +77,9 @@ <optional> <ref name='keywrap'/> </optional> + <optional> + <ref name='launch-security'/>> + </optional>> </interleave> </element> </define> @@ -436,6 +439,42 @@ </element> </define>

+ <define name="launch-security"> + <element name="launch-security"> + <attribute name="type"> + <value>sev</value> + </attribute> + <interleave> + <element name="cbitpos"> + <data type='unsignedInt'/> + </element> + <element name="reduced-phys-bits"> + <data type='unsignedInt'/> + </element> + <optional> + <element name="policy"> + <ref name='hexuint'/> + </element> + </optional>

Hopefully hexuint will suffice over time... On the other hand, this patch uses virXPathULongHex in order to parse.

+ <optional> + <element name="handle"> + <ref name='unsignedInt'/> + </element> + </optional> + <optional> + <element name="dh-cert"> + <data type="string"/> + </element> + </optional> + <optional> + <element name="session"> + <data type="string"/> + </element> + </optional> + </interleave> + </element> + </define> +

Similar to patch 2 questions... Should this be "launchSecurity" "reducedPhysBits" "DHCert" Changing has fairly major implications, but I'd rather not have it baked in and then have someone throw a complaint and have to redo things. Since many people are off today, let's give them at least until tomorrow to provide feedback before spinning a v6...

<!-- Enable or disable perf events for the domain. For each of the events the following rules apply: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7c0d9..4acf45c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -929,6 +929,10 @@ VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST, "ivshmem-plain", "ivshmem-doorbell")

+VIR_ENUM_IMPL(virDomainLaunchSecurity, VIR_DOMAIN_LAUNCH_SECURITY_LAST, + "", + "sev") + static virClassPtr virDomainObjClass; static virClassPtr virDomainXMLOptionClass; static void virDomainObjDispose(void *obj); @@ -2897,6 +2901,14 @@ virDomainCachetuneDefFree(virDomainCachetuneDefPtr cachetune) VIR_FREE(cachetune); }

again 2 blank lines between functions

+static void +virDomainSevDefFree(virDomainSevDefPtr def) +{

Rather than making callers know... if (!def) return;

+ VIR_FREE(def->dh_cert); + VIR_FREE(def->session); + + VIR_FREE(def); +}

void virDomainDefFree(virDomainDefPtr def) { @@ -3079,6 +3091,9 @@ void virDomainDefFree(virDomainDefPtr def) if (def->namespaceData && def->ns.free) (def->ns.free)(def->namespaceData);

+ if (def->sev)

Meaning this one won't be necessary

+ virDomainSevDefFree(def->sev); + xmlFreeNode(def->metadata);

VIR_FREE(def); @@ -15537,6 +15552,70 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, return ret; }

2 blank lines

+static virDomainSevDefPtr +virDomainSevDefParseXML(xmlNodePtr sevNode, xmlXPathContextPtr ctxt)

Two lines: virDomainSevDefParseXML(xmlNodePtr sevNode, xmlXPathContextPtr ctxt)

+{ + char *tmp = NULL, *type = NULL;

Prefer 2 lines here too: char *tmp = NULL; char *type = NULL;

+ xmlNodePtr save = ctxt->node; + virDomainSevDefPtr def; + unsigned long policy;

probably should initialize = 0;

+ + ctxt->node = sevNode; + + if (VIR_ALLOC(def) < 0) + return NULL; + + if (!(type = virXMLPropString(sevNode, "type"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing launch-security type")); + goto error; + } +

Assuming 'sev' for domain_conf isn't "normal" - that is it's not future proof...

+ if (virDomainLaunchSecurityTypeFromString(type) != + VIR_DOMAIN_LAUNCH_SECURITY_SEV) {

This produces a libvirt failed for some reason type error message...

+ goto error; + }

So, as ugly as this will look, using switch/case logic: def->sectype = virDomainLaunchSecurityTypeFromString(type); switch((virDomainLaunchSecurity) def->sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: break; case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: default: virReportError(VIR_ERR_XML_ERROR, _("unsupported launch-security type '%s'"), type); goto error; } NB: I've *added* ->sectype to @def [see below]

+ + if (virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to get cbitpos"));

s/get/get launch-security/

+ goto error; + } + + if (virXPathUInt("string(./reduced-phys-bits)", ctxt, + &def->reduced_phys_bits) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("failed to get reduced-phys-bits"));

s/get/get launch-security/

+ goto error; + } + + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0)> + policy = 0x1;

Hmmm... This one is optional which makes things a bit interesting. How do you know it was provided? Today the default could be 0x1, but sometime in the future if you decide upon a different default, then things get really complicated. Or for some other chip and/or hypervisor the default won't be 1. Also virXPathULongHex returns -1 or -2 w/ different meanings - if it's not there, You get a -1 when not provided and -2 when you have invalid value in field, which should be an error. Finally, ULongHex returns 64 bits and your field is 32 bits leading to possible overflow So, either you have to have a "policy_provided" boolean of some sort or I think preferably leave it as 0 (zero) indicating not provided and then when generating a command line check against 0 and provide the hypervisor dependent value on the command line *OR* just don't provided it an let the hypervisor choose it's default value because the value wasn't provided (that's a later patch though) Also see [1] below... So my suggestion is: if (virXPathULongHex("string(./policy)", ctxt, &policy) == -2 || policy > UINT_MAX) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid launch-security policy value")); goto error; } def->policy = policy; If -1 is returned, the def->policy = 0; otherwise, it's set to something.

+ + def->policy = policy; + + if ((tmp = virXPathString("string(./dh-cert)", ctxt))) { + if (VIR_STRDUP(def->dh_cert, tmp) < 0) + goto error; + + VIR_FREE(tmp); + } + + if ((tmp = virXPathString("string(./session)", ctxt))) { + if (VIR_STRDUP(def->session, tmp) < 0) + goto error; + + VIR_FREE(tmp); + } + + ctxt->node = save; + return def; + + error: + VIR_FREE(tmp); + virDomainSevDefFree(def); + ctxt->node = save; + return NULL; +}

2 blank lines

static virDomainMemoryDefPtr virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, @@ -20210,6 +20289,13 @@ virDomainDefParseXML(xmlDocPtr xml, ctxt->node = node; VIR_FREE(nodes);

+ /* Check for SEV feature */ + if ((node = virXPathNode("./launch-security", ctxt)) != NULL) { + def->sev = virDomainSevDefParseXML(node, ctxt); + if (!def->sev) + goto error; + } + /* analysis of memory devices */ if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) < 0) goto error; @@ -26087,6 +26173,27 @@ virDomainKeyWrapDefFormat(virBufferPtr buf, virDomainKeyWrapDefPtr keywrap) }

2 blank lines

static void +virDomainSevDefFormat(virBufferPtr buf, virDomainSevDefPtr sev)

Two lines e.g. virDomainSevDefFormat(virBufferPtr buf, virDomainSevDefPtr sev)

+{

if (!sev) return;

+ virBufferAddLit(buf, "<launch-security type='sev'>\n");

Since we shouldn't be assuming 'sev' at this point virBufferAsprintf(buf, "<launch-security type='%s'>\n", virDomainLaunchSecurityTypeToString(def->sectype));

+ virBufferAdjustIndent(buf, 2); + + virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); + virBufferAsprintf(buf, "<reduced-phys-bits>%d</reduced-phys-bits>\n", + sev->reduced_phys_bits); + virBufferAsprintf(buf, "<policy>%d</policy>\n", sev->policy);

This has been defined as a "0x" number, but you're printing in decimal format. Let's print in 0x value. [1] Also if you leave this a 0 (zero), then you can compare sev->policy

0 before printing, e.g.:

if (sev->policy) virBufferAsprintf(buf, "<policy>0x%x</policy>\n", sev->policy); you may also want to do a "0x%04x"

+ if (sev->dh_cert) + virBufferAsprintf(buf, "<dh_cert>%s</dh_cert>\n", sev->dh_cert);

Use virBufferEscapeString

+ + if (sev->session) + virBufferAsprintf(buf, "<session>%s</session>\n", sev->session);

Use virBufferEscapeString

+ + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</launch-security>\n"); +} + + +static void virDomainPerfDefFormat(virBufferPtr buf, virDomainPerfDefPtr perf) { size_t i; @@ -27258,6 +27365,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->keywrap) virDomainKeyWrapDefFormat(buf, def->keywrap);

+ if (def->sev) + virDomainSevDefFormat(buf, def->sev); +

See above, no need for the if (def->sev)

virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</domain>\n");

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 61379e5..60f9dd2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -142,6 +142,9 @@ typedef virDomainPanicDef *virDomainPanicDefPtr; typedef struct _virDomainMemoryDef virDomainMemoryDef; typedef virDomainMemoryDef *virDomainMemoryDefPtr;

+typedef struct _virDomainSevDef virDomainSevDef; +typedef virDomainSevDef *virDomainSevDefPtr; + /* forward declarations virDomainChrSourceDef, required by * virDomainNetDef */ @@ -2290,6 +2293,25 @@ struct _virDomainKeyWrapDef { };

typedef enum { + VIR_DOMAIN_LAUNCH_SECURITY_NONE, + VIR_DOMAIN_LAUNCH_SECURITY_SEV, + + VIR_DOMAIN_LAUNCH_SECURITY_LAST, +} virDomainLaunchSecurity; + +typedef struct _virDomainSevDef virDomainSevDef; +typedef virDomainSevDef *virDomainSevDefPtr; + +struct _virDomainSevDef {

int sectype; /* enum virDomainLaunchSecurity */ John

+ char *dh_cert; + char *session; + unsigned int policy; + unsigned int cbitpos; + unsigned int reduced_phys_bits; +}; + + +typedef enum { VIR_DOMAIN_IOMMU_MODEL_INTEL,

VIR_DOMAIN_IOMMU_MODEL_LAST @@ -2454,6 +2476,9 @@ struct _virDomainDef {

virDomainKeyWrapDefPtr keywrap;

+ /* SEV-specific domain */ + virDomainSevDefPtr sev; + /* Application-specific custom metadata */ xmlNodePtr metadata;

@@ -3345,6 +3370,7 @@ VIR_ENUM_DECL(virDomainMemorySource) VIR_ENUM_DECL(virDomainMemoryAllocation) VIR_ENUM_DECL(virDomainIOMMUModel) VIR_ENUM_DECL(virDomainShmemModel) +VIR_ENUM_DECL(virDomainLaunchSecurity) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainNostateReason)

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

QEMU uses /dev/sev device while creating the SEV guest, lets add /dev/sev in the list of devices allowed to be accessed by the QEMU.

Signed-off-by: Brijesh Singh <<brijesh.singh@amd.com>> --- docs/drvqemu.html.in | 1 + src/qemu/qemu.conf | 2 +- src/qemu/qemu_cgroup.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)

You'll also need : diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 688e5b9fda..b94b5349c4 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -60,6 +60,7 @@ module Test_libvirtd_qemu = { "8" = "/dev/kqemu" } { "9" = "/dev/rtc" } { "10" = "/dev/hpet" } + { "11" = "/dev/sev" } } { "save_image_format" = "raw" } { "dump_image_format" = "raw" } Not everyone has that augeas package installed and thus fail to run the 'check-augeas-remote'... I think that's all that's necessary - cgroups and devices being added is perhaps something Daniel knows a bit more about... John

diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in index cbd159d..7c33a18 100644 --- a/docs/drvqemu.html.in +++ b/docs/drvqemu.html.in @@ -397,6 +397,7 @@ chmod o+x /path/to/directory /dev/random, /dev/urandom, /dev/ptmx, /dev/kvm, /dev/kqemu, /dev/rtc, /dev/hpet, /dev/net/tun +/dev/sev </pre>

<p> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 07eab7e..d328d96 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -451,7 +451,7 @@ # "/dev/null", "/dev/full", "/dev/zero", # "/dev/random", "/dev/urandom", # "/dev/ptmx", "/dev/kvm", "/dev/kqemu", -# "/dev/rtc","/dev/hpet" +# "/dev/rtc","/dev/hpet", "/dev/sev" #] # # RDMA migration requires the following extra files to be added to the list: diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index b604edb..1a271df 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -46,7 +46,7 @@ const char *const defaultDeviceACL[] = { "/dev/null", "/dev/full", "/dev/zero", "/dev/random", "/dev/urandom", "/dev/ptmx", "/dev/kvm", "/dev/kqemu", - "/dev/rtc", "/dev/hpet", + "/dev/rtc", "/dev/hpet", "/dev/sev", NULL, }; #define DEVICE_PTY_MAJOR 136

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

QEMU >= 2.12 provides 'sev-guest' object which is used to launch encrypted VMs on AMD platform using SEV feature. The various inputs required to launch SEV guest is provided through the <launch-security> tag. A typical SEV guest launch command line looks like this:

# $QEMU ...\ -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 ...\ -machine memory-encryption=sev0 \

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- src/qemu/qemu_command.c | 35 +++++++++++++++++++++++++++++ src/qemu/qemu_process.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+)

(slight delay for next part of review - today was rocket launch day and then we headed out for a bit ;-))

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 682d714..55bbfa2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7405,6 +7405,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM)) qemuAppendLoadparmMachineParm(&buf, def);

+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)

Since we already checked sev-guest at prepare host storage (mostly unconditionally), I don't think we have to make the check here as well - although I could be wrong...

+ virBufferAddLit(&buf, ",memory-encryption=sev0"); + virCommandAddArgBuffer(cmd, &buf); }

@@ -9750,6 +9753,35 @@ qemuBuildTPMCommandLine(virCommandPtr cmd, return 0; }

+static void

This probably should be an int...

+qemuBuildSevCommandLine(virDomainObjPtr vm, virCommandPtr cmd, + virDomainSevDefPtr sev) +{ + virBuffer obj = VIR_BUFFER_INITIALIZER; + qemuDomainObjPrivatePtr priv = vm->privateData; + char *path = NULL; +

if (!dev->sev) return 0; again, since prepare host storage checked the sev-guest capability we should be good to go here...

+ VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d", + sev->policy, sev->cbitpos, sev->reduced_phys_bits); + + virBufferAsprintf(&obj, "sev-guest,id=sev0,cbitpos=%d", sev->cbitpos); + virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits); + virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);

Here I would say: if (sev->policy > 0) virBufferAsprintf(&obj, ",policy=0x%x", sev->policy); and let qemu pick the default (which is 0x1 as I read that code).

+ + if (sev->dh_cert) { + ignore_value(virAsprintf(&path, "%s/dh_cert.base64", priv->libDir)); + virBufferAsprintf(&obj, ",dh-cert-file=%s", path); + VIR_FREE(path); + } + + if (sev->session) { + ignore_value(virAsprintf(&path, "%s/session.base64", priv->libDir)); + virBufferAsprintf(&obj, ",session-file=%s", path); + VIR_FREE(path); + }

...since I don't believe we can ignore_value on the paths - especially since we're using it to create a path to a file containing some sort of session info or DH key (base64 encrypted).

+ + virCommandAddArgList(cmd, "-object", virBufferContentAndReset(&obj), NULL); +}

static int qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd, @@ -10195,6 +10227,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0) goto error;

+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev) + qemuBuildSevCommandLine(vm, cmd, def->sev); +

I think we're save to change this to: if (qemuBuildSevCommandLine(vm, cmd, dev->sev) < 0) goto error;

if (snapshot) virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c0105c8..0c93f15 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5745,6 +5745,61 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, return ret; }

Two blank lines

+static int +qemuBuildSevCreateFile(const char *configDir, const char *name, + const char *data)

3 lines for args

+{ + char *configFile; + + if (!(configFile = virFileBuildPath(configDir, name, ".base64"))) + return -1; + + if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) { + virReportSystemError(errno, _("failed to write data to config '%s'"), + configFile); + goto error; + }

Check out storageBackendCreateQemuImgSecretPath which just goes straight to safewrite when writing to the file or qemuDomainWriteMasterKeyFile which is a similar w/r/t a single key file for the domain. The one thing to think about being the privileges for the file being created and written and the expectations for QEMU's usage. I think this is more like the storage secret code, but I could be wrong!

+ + VIR_FREE(configFile); + return 0; + + error: + VIR_FREE(configFile); + return -1; +} +

2 blank lines

+static int +qemuProcessPrepareSevGuestInput(virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr def = vm->def; + virQEMUCapsPtr qemuCaps = priv->qemuCaps; + virDomainSevDefPtr sev = def->sev; + + if (!sev) + return 0; + + VIR_DEBUG("Prepare SEV guest"); + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Domain %s asked for 'sev' launch but "

s/but/but this/

+ "QEMU does not support SEV feature"), vm->def->name); + return -1; + }

Since we check for this here - I think this should be good enough for command line building. That is - qemuProcessPrepareHostStorage will come before command line building - so the check is already made.

+ + if (sev->dh_cert) { + if (qemuBuildSevCreateFile(priv->libDir, "dh_cert", sev->dh_cert) < 0) + return -1; + } + + if (sev->session) { + if (qemuBuildSevCreateFile(priv->libDir, "session", sev->session) < 0) + return -1; + } + + return 0; +}

2 blank lines John

static int qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, @@ -5870,6 +5925,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, if (qemuProcessPrepareHostStorage(driver, vm, flags) < 0) goto cleanup;

+ if (qemuProcessPrepareSevGuestInput(vm) < 0) + goto cleanup; + ret = 0; cleanup: virObjectUnref(cfg);

On Mon, Apr 02, 2018 at 07:04:25PM -0400, John Ferlan wrote:

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

...

QEMU >= 2.12 provides 'sev-guest' object which is used to launch encrypted VMs on AMD platform using SEV feature. The various inputs required to launch SEV guest is provided through the <launch-security> tag. A typical SEV guest launch command line looks like this:

# $QEMU ...\ -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 ...\ -machine memory-encryption=sev0 \

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- src/qemu/qemu_command.c | 35 +++++++++++++++++++++++++++++ src/qemu/qemu_process.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+)

(slight delay for next part of review - today was rocket launch day and then we headed out for a bit ;-))

...

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 682d714..55bbfa2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7405,6 +7405,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM)) qemuAppendLoadparmMachineParm(&buf, def);

+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)

Since we already checked sev-guest at prepare host storage (mostly unconditionally), I don't think we have to make the check here as well - although I could be wrong...

I guess you surely meant qemuProcessPrepareSevGuestInput, but you're right, we don't need it. ...

...

static int qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd, @@ -10195,6 +10227,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0) goto error;

+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev) + qemuBuildSevCommandLine(vm, cmd, def->sev); +

I think we're save to change this to:

Yep. Erik

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

The API can be used outside the libvirt to get the launch security information. When SEV is enabled, the API can be used to get the measurement of the launch process.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- include/libvirt/libvirt-domain.h | 17 ++++++++++++++ src/driver-hypervisor.h | 7 ++++++ src/libvirt-domain.c | 48 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++++ 4 files changed, 77 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 12fd340..6870a1a 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -4764,4 +4764,21 @@ int virDomainSetLifecycleAction(virDomainPtr domain, unsigned int action, unsigned int flags);

+/** + * Launch Security API + */ + +/** + * VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT: + * + * Macro represents the launch measurement of the SEV guest, + * as VIR_TYPED_PARAM_STRING. + */ +#define VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT "sev-measurement"

syntax-check tells you that this is incorrectly spaced - should be "# define"

+ +int virDomainGetLaunchSecurityInfo(virDomainPtr domain, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags); + #endif /* __VIR_LIBVIRT_DOMAIN_H__ */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index ce0e2b2..b066413 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1283,6 +1283,12 @@ typedef int unsigned int action, unsigned int flags);

+typedef int +(*virDrvDomainGetLaunchSecurityInfo)(virDomainPtr domain, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags); +

typedef struct _virHypervisorDriver virHypervisorDriver; typedef virHypervisorDriver *virHypervisorDriverPtr; @@ -1528,6 +1534,7 @@ struct _virHypervisorDriver { virDrvDomainSetVcpu domainSetVcpu; virDrvDomainSetBlockThreshold domainSetBlockThreshold; virDrvDomainSetLifecycleAction domainSetLifecycleAction; + virDrvDomainGetLaunchSecurityInfo domainGetLaunchSecurityInfo; };

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 63d2ae2..5b63a3c 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12101,3 +12101,51 @@ int virDomainSetLifecycleAction(virDomainPtr domain, virDispatchError(domain->conn); return -1; } + +/** + * virDomainGetLaunchSecurityInfo: + * @domain: a domain object + * @params: where to store security info + * @nparams: number of items in @params + * @flags: currently used, set to 0. + * + * Get the launch security info. In case of the SEV guest, this will + * return the launch measurement. + * + * Returns -1 in case of failure, 0 in case of success. + */ +int virDomainGetLaunchSecurityInfo(virDomainPtr domain, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + virConnectPtr conn = domain->conn; + + VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%p flags=0x%x", + params, nparams, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckNonNullArgGoto(params, error); + virCheckNonNullArgGoto(nparams, error); + virCheckReadOnlyGoto(conn->flags, error); + + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + flags |= VIR_TYPED_PARAM_STRING_OKAY; + + if (conn->driver->domainGetLaunchSecurityInfo) { + int ret; + ret = conn->driver->domainGetLaunchSecurityInfo(domain, params, + nparams, flags); + if (ret < 0) + goto error; + return ret; + } + virReportUnsupportedError(); + + error: + virDispatchError(domain->conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 95df3a0..caba286 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -785,4 +785,9 @@ LIBVIRT_4.1.0 { virStoragePoolLookupByTargetPath; } LIBVIRT_3.9.0;

+LIBVIRT_4.2.0 {

It's 4.3.0 now... Otherwise, I think this looks fine. John

+ global: + virDomainGetLaunchSecurityInfo; +} LIBVIRT_4.1.0; + # .... define new API here using predicted next version number ....

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

Add remote support for launch security info.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- src/remote/remote_daemon_dispatch.c | 47 +++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 42 ++++++++++++++++++++++++++++++++- src/remote/remote_protocol.x | 20 +++++++++++++++- src/remote_protocol-structs | 11 +++++++++ 4 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 121d114..0959604 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3088,6 +3088,53 @@ remoteDispatchNodeGetMemoryStats(virNetServerPtr server ATTRIBUTE_UNUSED, }

static int +remoteDispatchDomainGetLaunchSecurityInfo(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_get_launch_security_info_args *args, + remote_domain_get_launch_security_info_ret *ret) +{ + virDomainPtr dom = NULL; + virTypedParameterPtr params = NULL; + int nparams = 0; + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + if (virDomainGetLaunchSecurityInfo(dom, ¶ms, &nparams, args->flags) < 0) + goto cleanup; + + if (nparams > REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; + } + + if (virTypedParamsSerialize(params, nparams, + (virTypedParameterRemotePtr *) &ret->params.params_val, + &ret->params.params_len, + args->flags) < 0) + goto cleanup; + + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + virTypedParamsFree(params, nparams); + virObjectUnref(dom); + return rv; +} + +static int remoteDispatchDomainGetPerfEvents(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, virNetMessagePtr msg ATTRIBUTE_UNUSED, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 325ef3f..b52faa1 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1948,6 +1948,45 @@ remoteDomainGetNumaParameters(virDomainPtr domain, }

static int +remoteDomainGetLaunchSecurityInfo(virDomainPtr domain, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + int rv = -1; + remote_domain_get_launch_security_info_args args; + remote_domain_get_launch_security_info_ret ret; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, domain); + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_LAUNCH_SECURITY_INFO, + (xdrproc_t) xdr_remote_domain_get_launch_security_info_args, (char *) &args, + (xdrproc_t) xdr_remote_domain_get_launch_security_info_ret, (char *) &ret) == -1) + goto done; + + if (virTypedParamsDeserialize((virTypedParameterRemotePtr) ret.params.params_val, + ret.params.params_len, + REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX, + params, + nparams) < 0) + goto cleanup; + + rv = 0; + + cleanup: + xdr_free((xdrproc_t) xdr_remote_domain_get_launch_security_info_ret, + (char *) &ret); + done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteDomainGetPerfEvents(virDomainPtr domain, virTypedParameterPtr *params, int *nparams, @@ -8430,7 +8469,8 @@ static virHypervisorDriver hypervisor_driver = { .domainSetGuestVcpus = remoteDomainSetGuestVcpus, /* 2.0.0 */ .domainSetVcpu = remoteDomainSetVcpu, /* 3.1.0 */ .domainSetBlockThreshold = remoteDomainSetBlockThreshold, /* 3.2.0 */ - .domainSetLifecycleAction = remoteDomainSetLifecycleAction /* 3.9.0 */ + .domainSetLifecycleAction = remoteDomainSetLifecycleAction, /* 3.9.0 */ + .domainGetLaunchSecurityInfo = remoteDomainGetLaunchSecurityInfo /* 4.2.0 */

4.3.0 now... John

};

static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 9dbd497..4c0144c 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -253,6 +253,9 @@ const REMOTE_DOMAIN_IP_ADDR_MAX = 2048; /* Upper limit on number of guest vcpu information entries */ const REMOTE_DOMAIN_GUEST_VCPU_PARAMS_MAX = 64;

+/* Upper limit on number of launch security information entries */ +const REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX = 64; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];

@@ -3448,6 +3451,15 @@ struct remote_domain_set_lifecycle_action_args { unsigned int flags; };

+struct remote_domain_get_launch_security_info_args { + remote_nonnull_domain dom; + unsigned int flags; +}; + +struct remote_domain_get_launch_security_info_ret { + remote_typed_param params<REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX>; +}; + /*----- Protocol. -----*/

/* Define the program number, protocol version and procedure numbers here. */ @@ -6135,5 +6147,11 @@ enum remote_procedure { * @priority: high * @acl: storage_pool:getattr */ - REMOTE_PROC_STORAGE_POOL_LOOKUP_BY_TARGET_PATH = 391 + REMOTE_PROC_STORAGE_POOL_LOOKUP_BY_TARGET_PATH = 391, + + /** + * @generate: none + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_GET_LAUNCH_SECURITY_INFO = 392 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index f45aba2..8f19d98 100644

cannot tell you how many people miss this one...

--- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2877,6 +2877,16 @@ struct remote_domain_set_lifecycle_action_args { u_int action; u_int flags; }; +struct remote_domain_get_launch_security_info_args { + remote_nonnull_domain dom; + u_int flags; +}; +struct remote_domain_get_launch_security_info_ret { + struct { + u_int params_len; + remote_typed_param * params_val; + } params; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -3269,4 +3279,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_MANAGED_SAVE_DEFINE_XML = 389, REMOTE_PROC_DOMAIN_SET_LIFECYCLE_ACTION = 390, REMOTE_PROC_STORAGE_POOL_LOOKUP_BY_TARGET_PATH = 391, + REMOTE_PROC_DOMAIN_GET_LAUNCH_SECURITY_INFO = 392, };

s/qemu_driver/qemu s/add/Add/ On 04/02/2018 10:18 AM, Brijesh Singh wrote:

This patch implement the internal driver API for launch event into

s/implement/implements/

qemu driver. When SEV is enabled, execute 'query-sev-launch-measurement' to get the measurement of memory encrypted through launch sequence.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- src/qemu/qemu_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 8 ++++++ src/qemu/qemu_monitor.h | 3 ++ src/qemu/qemu_monitor_json.c | 32 +++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 2 ++ 5 files changed, 111 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 072eb54..898aaf0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21332,6 +21332,71 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, return ret; }

2 blank lines... static int qemuDomainGetSevMeasurement(type arg, type arg...)

+static int qemuDomainGetSevMeasurement(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + int ret = -1; + char *tmp; + int maxpar = 0; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + return -1;

If we don't get a job, no need to EndJob

+ + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + + tmp = qemuMonitorGetSevMeasurement(QEMU_DOMAIN_PRIVATE(vm)->mon); + if (tmp == NULL) + goto endjob; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; + + if (virTypedParamsAddString(params, nparams, &maxpar, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT, + tmp) < 0) + goto endjob; + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + return ret; +} + +

Could use some intro comments (inputs, outputs, etc)

+static int +qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + virQEMUDriverPtr driver = domain->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + + if (!(vm = qemuDomObjFromDomain(domain))) + goto cleanup; + + if (virDomainGetLaunchSecurityInfoEnsureACL(domain->conn, vm->def) < 0) + goto cleanup; + + if (vm->def->sev) { + if (qemuDomainGetSevMeasurement(driver, vm, params, nparams, flags) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +}

static virHypervisorDriver qemuHypervisorDriver = { .name = QEMU_DRIVER_NAME, @@ -21552,6 +21617,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainSetVcpu = qemuDomainSetVcpu, /* 3.1.0 */ .domainSetBlockThreshold = qemuDomainSetBlockThreshold, /* 3.2.0 */ .domainSetLifecycleAction = qemuDomainSetLifecycleAction, /* 3.9.0 */ + .domainGetLaunchSecurityInfo = qemuDomainGetLaunchSecurityInfo, /* 4.2.0 */

4.3.0 now...

};

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 44c2dff..877aaa56 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4417,3 +4417,11 @@ qemuMonitorSetWatchdogAction(qemuMonitorPtr mon,

return qemuMonitorJSONSetWatchdogAction(mon, action); } + +char * +qemuMonitorGetSevMeasurement(qemuMonitorPtr mon) +{ + QEMU_CHECK_MONITOR_NULL(mon); + + return qemuMonitorJSONGetSevMeasurement(mon); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index efd3427..c475b73 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1188,4 +1188,7 @@ virJSONValuePtr qemuMonitorQueryNamedBlockNodes(qemuMonitorPtr mon);

int qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, const char *action); +char * +qemuMonitorGetSevMeasurement(qemuMonitorPtr mon); + #endif /* QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e67f7b7..be5731b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7960,3 +7960,35 @@ qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon, virJSONValueFree(reply); return ret; }

2 blank lines Could also use some json output expections - you'll see some of the functions provide some comments... could add a few here too. John

+> +char * +qemuMonitorJSONGetSevMeasurement(qemuMonitorPtr mon) +{ + const char *tmp; + char *measurement = NULL; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-sev-launch-measure", NULL))) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + data = virJSONValueObjectGetObject(reply, "return"); + + if (!(tmp = virJSONValueObjectGetString(data, "data"))) + goto cleanup; + + if (VIR_STRDUP(measurement, tmp) < 0) + goto cleanup; + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return measurement; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index f30ff1f..7d5e1f0 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -342,6 +342,8 @@ int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon,

int qemuMonitorJSONSystemWakeup(qemuMonitorPtr mon);

+char *qemuMonitorJSONGetSevMeasurement(qemuMonitorPtr mon); + int qemuMonitorJSONGetVersion(qemuMonitorPtr mon, int *major, int *minor,

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

Add new 'launch-security' command, the command can be used to get or set the launch security information when booting encrypted VMs.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- tools/virsh-domain.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)

Need to modify tools/virsh.pod too in order to supply the man page information.

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2b775fc..4dca191 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13877,6 +13877,84 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) return ret >= 0; }

+/* + * "launch-security" command + */ +static const vshCmdInfo info_launch_security[] = { + {.name = "help", + .data = N_("Get or set launch-security information") + }, + {.name = "desc", + .data = N_("Get or set the current launch-security information for a guest" + " domain.\n" + " To get the launch-security information use following command: \n\n" + " virsh # launch-security <domain>") + },

Rather lengthy... don't think the last 2 lines are necessary. Is there another command doing the same? Probably should fill in the ".help" entry too like other commands.

+ {.name = NULL} +}; + +static const vshCmdOptDef opts_launch_security[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "get", + .type = VSH_OT_STRING, + .help = N_("Show the launch-security info") + }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = NULL} +}; +

2 lines...

+static void +virshPrintLaunchSecurityInfo(vshControl *ctl, virTypedParameterPtr params, + int nparams) +{ + size_t i; +

Should there perhaps be a header here for the columns?

+ for (i = 0; i < nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) + vshPrintExtra(ctl, "%-15s: %s\n", params[i].field, params[i].value.s); + } +} +

2 lines... John

+static bool +cmdLaunchSecurity(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + int nparams = 0; + virTypedParameterPtr params = NULL; + bool ret = false; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (virDomainGetLaunchSecurityInfo(dom, ¶ms, &nparams, flags) != 0) { + vshError(ctl, "%s", _("Unable to get launch security info")); + goto cleanup; + } + + virshPrintLaunchSecurityInfo(ctl, params, nparams); + + ret = true; + cleanup: + virTypedParamsFree(params, nparams); + virshDomainFree(dom); + return ret; +} + + const vshCmdDef domManagementCmds[] = { {.name = "attach-device", .handler = cmdAttachDevice, @@ -14492,5 +14570,11 @@ const vshCmdDef domManagementCmds[] = { .info = info_domblkthreshold, .flags = 0 }, + {.name = "launch-security", + .handler = cmdLaunchSecurity, + .opts = opts_launch_security, + .info = info_launch_security, + .flags = 0 + }, {.name = NULL} };

On Mon, Apr 02, 2018 at 09:18:55AM -0500, Brijesh Singh wrote:

Add new 'launch-security' command, the command can be used to get or set the launch security information when booting encrypted VMs.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- tools/virsh-domain.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2b775fc..4dca191 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13877,6 +13877,84 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) return ret >= 0; }

+/* + * "launch-security" command + */ +static const vshCmdInfo info_launch_security[] = { + {.name = "help", + .data = N_("Get or set launch-security information") + }, + {.name = "desc", + .data = N_("Get or set the current launch-security information for a guest" + " domain.\n" + " To get the launch-security information use following command: \n\n" + " virsh # launch-security <domain>")

As John has pointed out, you might want to shorten ^these 2 lines, however, I think it makes sense to make it obvious that running without any arguments/options this behaves like a getter, otherwise it's going to behave like a setter, right? (it's a common practice in libvirt, so nothing against conceptually).

+ }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_launch_security[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "get", + .type = VSH_OT_STRING, + .help = N_("Show the launch-security info") + }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = NULL} +};

Sorry if I missed the obvious, but what exactly is the --get <string> supposed to do? Erik

On Wed, Apr 04, 2018 at 07:36:37AM -0500, Brijesh Singh wrote:

On 4/3/18 9:32 AM, Erik Skultety wrote:

...

...

Add new 'launch-security' command, the command can be used to get or set the launch security information when booting encrypted VMs.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- tools/virsh-domain.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2b775fc..4dca191 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13877,6 +13877,84 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) return ret >= 0; }

+/* + * "launch-security" command + */ +static const vshCmdInfo info_launch_security[] = { + {.name = "help", + .data = N_("Get or set launch-security information") + }, + {.name = "desc", + .data = N_("Get or set the current launch-security information for a guest" + " domain.\n" + " To get the launch-security information use following command: \n\n" + " virsh # launch-security <domain>") As John has pointed out, you might want to shorten ^these 2 lines, however, I

On Mon, Apr 02, 2018 at 09:18:55AM -0500, Brijesh Singh wrote: think it makes sense to make it obvious that running without any arguments/options this behaves like a getter, otherwise it's going to behave like a setter, right? (it's a common practice in libvirt, so nothing against conceptually).

Yes, without any command line it should be getter otherwise settter. Currently, we don't have anything in setter yet.

...

...

+ }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_launch_security[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "get", + .type = VSH_OT_STRING, + .help = N_("Show the launch-security info") + }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = NULL} +}; Sorry if I missed the obvious, but what exactly is the --get <string> supposed to do?

The command will return a measurement of encrypted image. The measurement value can be used by guest owner to validate the image before it launches the VM. In a typical scenario we may have something like this:

# virsh create guest.xml --paused # virsh launch-security --domain guest // validate the measurement obtained through above command if measurement is wrong then;  destory the guest else  // optionally inject secret in guest using virsh launch-security set <....>  resume the guest

Yeah, but since you agreed that the command behaves like a getter without any parameters, I don't see a reason for the --get <string> parameter, that's why I asked about it. Also, the name of the command should be changed since this way, it's a bit deceptive as it hints that it queries/modifies the launch-security data which you got from querying qemu capabilities within the domain XML. I'm also going to respond to the other email to this patch I sent yesterday, because it's irrelevant given the fact that I got confused by the name (even though I saw what API it called) and created a false association. Other than what's already been noted, the logic in the virsh command itself looks solid, not much to it since the setter part will come later I presume. Erik

On 04/02/2018 10:18 AM, Brijesh Singh wrote:

From: Xiaogang Chen <Xiaogang.Chen@amd.com>

Update qemuxml2xmltest, genericxml2xmltest and qemuxml2argvtest to include sev specific tag, a typical SEV specific tag looks like

<launch-security type='sev> <cbitpos>47</cbitpos> <reduced-phys-bits>1</reduced-phys-bits> <policy>1</policy> </launch-security>

Signed-off-by: Xiaogang Chen <Xiaogang.Chen@amd.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- tests/genericxml2xmlindata/sev.xml | 20 +++++++++++++++++++ tests/genericxml2xmloutdata/sev.xml | 22 +++++++++++++++++++++ tests/genericxml2xmltest.c | 2 ++ tests/qemuxml2argvdata/sev.args | 24 +++++++++++++++++++++++ tests/qemuxml2argvdata/sev.xml | 35 +++++++++++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ tests/qemuxml2xmloutdata/sev.xml | 39 +++++++++++++++++++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 ++ 8 files changed, 146 insertions(+) create mode 100644 tests/genericxml2xmlindata/sev.xml create mode 100644 tests/genericxml2xmloutdata/sev.xml create mode 100644 tests/qemuxml2argvdata/sev.args create mode 100644 tests/qemuxml2argvdata/sev.xml create mode 100644 tests/qemuxml2xmloutdata/sev.xml

I'll do this one in two phases (and out of order in the review) This first phase focuses on the xml2xml processing which needs to be merged into patch3 First rename "sev.xml" to be "launch-security-sev.xml"... Just seeing 'sev' would make me wonder....

diff --git a/tests/genericxml2xmlindata/sev.xml b/tests/genericxml2xmlindata/sev.xml new file mode 100644 index 0000000..aeb0c6a --- /dev/null +++ b/tests/genericxml2xmlindata/sev.xml

Rename to launch-security-sev.xml

@@ -0,0 +1,20 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-1.0'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash>

Add the: <devices> </devices>

+ <launch-security type='sev'> + <cbitpos>47</cbitpos> + <reduced-phys-bits>1</reduced-phys-bits> + <policy>1</policy>

Should add some default values - just to prove parsing and formatting works. I used: <dh-cert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dh-cert> <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session>

+ </launch-security> +</domain> diff --git a/tests/genericxml2xmloutdata/sev.xml b/tests/genericxml2xmloutdata/sev.xml new file mode 100644 index 0000000..70065b8 --- /dev/null +++ b/tests/genericxml2xmloutdata/sev.xml

NB: By adding <devices></devices> to the genericxml2xmlindata then it doesn't seem having a genericxml2xmloutdata file is required...

@@ -0,0 +1,22 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-1.0'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + </devices> + <launch-security type='sev'> + <cbitpos>47</cbitpos> + <reduced-phys-bits>1</reduced-phys-bits> + <policy>1</policy> + </launch-security> +</domain> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index d8270a6..3b75b43 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -141,6 +141,8 @@ mymain(void) DO_TEST_FULL("cachetune-colliding-types", false, true, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);

+ DO_TEST_DIFFERENT("sev"); +

Change the name to 'launch-security-sev' and I used DO_TEST with just the in file w/ <devices> adjustment and things were good. I believe that's the "proper way".

virObjectUnref(caps); virObjectUnref(xmlopt);

diff --git a/tests/qemuxml2argvdata/sev.args b/tests/qemuxml2argvdata/sev.args new file mode 100644 index 0000000..312dbcf --- /dev/null +++ b/tests/qemuxml2argvdata/sev.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest1 \ +-S \ +-machine pc-1.0,accel=kvm,memory-encryption=sev0 \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x1 diff --git a/tests/qemuxml2argvdata/sev.xml b/tests/qemuxml2argvdata/sev.xml new file mode 100644 index 0000000..2476b58 --- /dev/null +++ b/tests/qemuxml2argvdata/sev.xml

Rename to launch-security-sev.xml

@@ -0,0 +1,35 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-1.0'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> + <launch-security type='sev'> + <cbitpos>47</cbitpos> + <reduced-phys-bits>1</reduced-phys-bits> + <policy>1</policy>

Similar to above add the fields to prove parse/format: <dh-cert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dh-cert> <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session>

+ </launch-security> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 308d71f..de0ac58 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3023,6 +3023,8 @@ mymain(void) QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW);

+ DO_TEST("sev", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_KVM, QEMU_CAPS_SEV_GUEST); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir);

diff --git a/tests/qemuxml2xmloutdata/sev.xml b/tests/qemuxml2xmloutdata/sev.xml new file mode 100644 index 0000000..80017fe --- /dev/null +++ b/tests/qemuxml2xmloutdata/sev.xml

Change the name to launch-security-sev.xml

@@ -0,0 +1,39 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-1.0'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> + <launch-security type='sev'> + <cbitpos>47</cbitpos> + <reduced-phys-bits>1</reduced-phys-bits> + <policy>1</policy>

Similar to above add the fields to prove parse/format: <dh-cert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dh-cert> <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session>

+ </launch-security> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0f56029..328ef66 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1265,6 +1265,8 @@ mymain(void) DO_TEST_STATUS("modern"); DO_TEST_STATUS("migration-out-nbd");

+ DO_TEST("sev", NONE); +

Change the name to launch-security-sev *and* move this up before the #define DO_TEST_STATUS John

if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir);