On 02/27/2018 02:09 AM, Peter Krempa wrote:

On Mon, Feb 26, 2018 at 11:53:33 -0600, 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.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> ---

QEMU SEV v9 patch does not have implementation of query-sev-capabilities command and I am will be adding this command in next QEMU patch round. Command result will look like this:

{ "execute": "query-sev-capabilities" } { "return": { "sev": 1, "pdh": "....", "cert-chain": "...", "cbitpos": 47, "reduced-phys-bits": 5}}

This patch fails both 'make check' and 'make syntax-check' please make sure that for the next posting you send a series where every single patch passes both test suites.

Will do thanks.

...

src/conf/domain_capabilities.h | 14 +++++++ src/qemu/qemu_capabilities.c | 28 +++++++++++++ src/qemu/qemu_capspriv.h | 4 ++ src/qemu/qemu_monitor.c | 9 +++++ src/qemu/qemu_monitor.h | 3 ++ src/qemu/qemu_monitor_json.c | 92 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 ++ 7 files changed, 153 insertions(+)

diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index fa4c1e442f57..e13a7fd6ba1b 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -137,6 +137,20 @@ struct _virDomainCapsCPU { virDomainCapsCPUModelsPtr custom; };

+/* + * SEV capabilities + */ +typedef struct _virSEVCapability virSEVCapability; +typedef virSEVCapability *virSEVCapabilityPtr; +struct _virSEVCapability { + bool sev; + char *pdh; + char *cert_chain; + int cbitpos; + int reduced_phys_bits; +}; + + struct _virDomainCaps { virObjectLockable parent;

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b5eb8cf46a52..2c680528deb8 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -525,6 +525,8 @@ struct _virQEMUCaps { size_t ngicCapabilities; virGICCapability *gicCapabilities;

+ virSEVCapability *sevCapabilities; + virQEMUCapsHostCPUData kvmCPU; virQEMUCapsHostCPUData tcgCPU; }; @@ -2811,6 +2813,14 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, qemuCaps->ngicCapabilities = ncapabilities; }

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

This structure contains also some pointers which would be leaked.

Ah good catch, I will fix them in next rev.

...

+ + qemuCaps->sevCapabilities = capabilities; +}

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

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

bool virQEMUCapsCPUFilterFeatures(const char *name, @@ -4951,6 +4974,11 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) virQEMUCapsSet(qemuCaps, QEMU_CAPS_CPU_CACHE);

+ /* SEV capabilities */ + if (ARCH_IS_X86(qemuCaps->arch)) { + virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon); + }

I think you can add a capability bit for the presence of the query-sev-capabilities command and call this function based on this fact.

I will explore this option and add new CAP.

...

+ ret = 0; cleanup: return ret; diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 222f3368e3b6..1fa85cc14f07 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -86,6 +86,10 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, virGICCapability *capabilities, size_t ncapabilities);

+void +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, + virSEVCapability *capabilities); + int virQEMUCapsParseHelpStr(const char *qemu, const char *str, diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ad5c572aeefb..195248c88ae1 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4007,6 +4007,15 @@ qemuMonitorGetGICCapabilities(qemuMonitorPtr mon, return qemuMonitorJSONGetGICCapabilities(mon, capabilities); }

+int +qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon, + virSEVCapability **capabilities) +{ + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONGetSEVCapabilities(mon, capabilities); +} +

int qemuMonitorNBDServerStart(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 954ae88e4f64..1b2513650c58 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -755,6 +755,9 @@ int qemuMonitorSetMigrationCapability(qemuMonitorPtr mon, int qemuMonitorGetGICCapabilities(qemuMonitorPtr mon, virGICCapability **capabilities);

+int qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon, + virSEVCapability **capabilities); + typedef enum { QEMU_MONITOR_MIGRATE_BACKGROUND = 1 << 0, QEMU_MONITOR_MIGRATE_NON_SHARED_DISK = 1 << 1, /* migration with non-shared storage with full disk copy */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a09e93e464b3..4424abfa7148 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6362,6 +6362,98 @@ 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; + bool sev; + 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 the 'query-sev-capabilities' QMP command was not available + * we simply successfully return zero capabilities. + * This is the case for QEMU <2.12 */ + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + ret = 0; + goto cleanup;

Making this whole function depend on the presence of the new command will greatly simplify your pain with the qemucapabilitiestest which this will inflict. If you make sure that only new qemus call this command you won't have to fix all older test suites.

Also note that you'll need to add a test case for the new qemu with the command supported so we can test this capability detection.

Yes adding new cap will help this, and I will also look into adding test suite for this command.

...

+ } + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + caps = virJSONValueObjectGetObject(reply, "return"); + + if (virJSONValueObjectGetBoolean(caps, "sev", &sev) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'sev' field is missing")); + goto cleanup; + } + + if (!sev) { + goto cleanup; + } + + 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_N(capability, 1) < 0)

'VIR_ALLOC' should be enough

Agreed.

...

+ goto cleanup; + + if (VIR_STRDUP(capability->pdh, pdh) < 0) + goto cleanup; + + if (VIR_STRDUP(capability->cert_chain, cert_chain) < 0) + goto cleanup; + + capability->sev = true;

This field should not be necessary. The presence of the structure itself should be good enough in this case.

Will remove this field.

...

+ capability->cbitpos = cbitpos; + capability->reduced_phys_bits = reduced_phys_bits; + *capabilities = capability; + ret = 0; + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + + return ret; +} + static virJSONValuePtr qemuMonitorJSONBuildInetSocketAddress(const char *host, const char *port) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index ec243becc4ae..305f789902e9 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -152,6 +152,9 @@ int qemuMonitorJSONSetMigrationCapability(qemuMonitorPtr mon, int qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, virGICCapability **capabilities);

+int qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, + virSEVCapability **capabilities); + int qemuMonitorJSONMigrate(qemuMonitorPtr mon, unsigned int flags, const char *uri); -- 2.14.3

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Feb 26, 2018 at 11:53:34 -0600, 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 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.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- docs/formatdomaincaps.html.in | 31 +++++++++++++++++++++++++++++++ docs/schemas/domaincaps.rng | 10 ++++++++++ src/conf/domain_capabilities.c | 19 +++++++++++++++++++ src/conf/domain_capabilities.h | 11 +++++++++++ src/qemu/qemu_capabilities.c | 41 ++++++++++++++++++++++++++++++++++++++++- 5 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 6bfcaf61caae..8f833477772c 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -417,6 +417,12 @@ <value>3</value> </enum> </gic> + <sev supported='yes'> + <pdh> </pdh> + <cert-chain> </cert-chain> + <cbitpos> </cbitpos> + <reduced-phys-bits> </reduced-phys-bits> + </sev> </features> </domainCapabilities> </pre> @@ -441,5 +447,30 @@ <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.

So is it likely that anything similar will be introduced by other manufacturers too? I'd like to avoid having these to be per-manufacturer specific. Can we change this to be more generic?

+ </p> + + <dl> + <dt><code>pdh</code></dt> + <dd>Platform diffie-hellman key, which can be exported to remote entities + which wish to establish a secure transport context with the SEV platform + in order to transmit data securely. The key is encoded in base64</dd> + <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> + <dt><code>cbitpos</code></dt> + <dd> C-bit position in page-table entry</dd> + <dt><code>reduced-phys-bits</code></dt> + <dd> Physical Address bit reduction</dd>

So these really are not clear from this description.

+ </dl> + </body> </html> diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 39053181eb9a..6ce8d296c703 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -184,6 +184,16 @@ </element> </define>

+ <define name='sev'> + <element name='sev'> + <ref name='supported'/> + <ref name='pdh'/> + <ref name='cert-chain'/> + <ref name='cbitpos'/> + <ref name='reduced-phys-bits'/>

You are not really defining these names anywhere. This will most probably break the schema test.

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

+static void +virDomainCapsFeatureSEVFormat(virBufferPtr buf, + virDomainCapsFeatureSEVPtr const sev) +{ + FORMAT_PROLOGUE(sev);

If the feature is not supported, you should not format an empty element here. Just skip it completely.

+ + if (sev->supported) { + 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); + } + + FORMAT_EPILOGUE(sev); +} +

char * virDomainCapsFormat(virDomainCapsPtr const caps) @@ -587,6 +605,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 e13a7fd6ba1b..aed5ec28e9cc 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -102,6 +102,16 @@ struct _virDomainCapsFeatureGIC { virDomainCapsEnum version; /* Info about virGICVersion */ };

+typedef struct _virDomainCapsFeatureSEV virDomainCapsFeatureSEV; +typedef virDomainCapsFeatureSEV *virDomainCapsFeatureSEVPtr; +struct _virDomainCapsFeatureSEV { + bool supported; + char *pdh; /* host platform-diffie hellman key */ + char *cert_chain; /* PDH certificate chain */ + int cbitpos; + int reduced_phys_bits;

This structure is basically the same as the one in the qemu driver. Can't you just use this one in the qemu driver?

+}; + typedef enum { VIR_DOMCAPS_CPU_USABLE_UNKNOWN, VIR_DOMCAPS_CPU_USABLE_YES, @@ -171,6 +181,7 @@ struct _virDomainCaps { /* add new domain devices here */

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

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2c680528deb8..ee8c542679eb 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5880,6 +5880,44 @@ virQEMUCapsSupportsGICVersion(virQEMUCapsPtr qemuCaps, return false; }

+/** + * virQEMUCapsFillDomainFeatureSEVCaps: + * @qemuCaps: QEMU capabilities + * @domCaps: domain capabilities + * + * Take the information about SEV capabilities that has been obtained + * using the 'query-sev-capabilities' QMP command and stored in @qemuCaps + * and convert it to a form suitable for @domCaps.

This function would not be necessary in that case.

+ * + * Returns: 0 on success, <0 on failure + */ +static int +virQEMUCapsFillDomainFeatureSEVCaps(virQEMUCapsPtr qemuCaps, + virDomainCapsPtr domCaps) +{ + virDomainCapsFeatureSEVPtr sev = &domCaps->sev; + virSEVCapability *cap = qemuCaps->sevCapabilities; + + if (!cap) + return 0; + + sev->supported = cap->sev; + + if (VIR_STRDUP(sev->pdh, cap->pdh) < 0) + goto failed; + + if (VIR_STRDUP(sev->cert_chain, cap->cert_chain) < 0) + goto failed; + + sev->cbitpos = cap->cbitpos; + sev->reduced_phys_bits = cap->reduced_phys_bits; + + return 0; +failed: + sev->supported = false; + return 0;

So why does this function return anything? Also we prefer the 'error' label in this case.

On 2/27/2018 10:29 AM, Brijesh Singh wrote:

On 02/27/2018 02:15 AM, Peter Krempa wrote:

...

On Mon, Feb 26, 2018 at 11:53:34 -0600, 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 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.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> ---   docs/formatdomaincaps.html.in  | 31 +++++++++++++++++++++++++++++++   docs/schemas/domaincaps.rng    | 10 ++++++++++   src/conf/domain_capabilities.c | 19 +++++++++++++++++++   src/conf/domain_capabilities.h | 11 +++++++++++   src/qemu/qemu_capabilities.c   | 41 ++++++++++++++++++++++++++++++++++++++++-   5 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 6bfcaf61caae..8f833477772c 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -417,6 +417,12 @@           <value>3</value>         </enum>       </gic> +    <sev supported='yes'> +      <pdh> </pdh> +      <cert-chain> </cert-chain> +      <cbitpos> </cbitpos> +      <reduced-phys-bits> </reduced-phys-bits> +    </sev>     </features>   </domainCapabilities>   </pre> @@ -441,5 +447,30 @@         <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.

So is it likely that anything similar will be introduced by other manufacturers too? I'd like to avoid having these to be per-manufacturer specific. Can we change this to be more generic?

How about something like:

<memory-encryption type='sev'>  <pdh> ..</pdh>  ....  .... </memory-encryption>

if other manufacture implements memory encryption then we can introduce a new type to handle new memory encryption feature. The inputs to the memory encryption type is going to be vendor-specific.

...

...

+    </p> + +    <dl> +      <dt><code>pdh</code></dt> +      <dd>Platform diffie-hellman key, which can be exported to remote entities +      which wish to establish a secure transport context with the SEV platform +      in order to transmit data securely. The key is encoded in base64</dd> + <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> +      <dt><code>cbitpos</code></dt> +      <dd> C-bit position in page-table entry</dd> + <dt><code>reduced-phys-bits</code></dt> +      <dd> Physical Address bit reduction</dd>

So these really are not clear from this description.

I will add some more details to clarify this.

...

...

+    </dl> +     </body>   </html> diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 39053181eb9a..6ce8d296c703 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -184,6 +184,16 @@       </element>     </define>   +  <define name='sev'> +    <element name='sev'> +      <ref name='supported'/> +      <ref name='pdh'/> +      <ref name='cert-chain'/> +      <ref name='cbitpos'/> +      <ref name='reduced-phys-bits'/>

You are not really defining these names anywhere. This will most probably break the schema test.

I must admit that I am new to libvirt hence need some help. Sorry I am not able to follow your this comment. Do I need to update domaincap.rng or not ? If yes, where do I need to define the names so that we don't break the schema test. Any tips is appreciated. thanks

I think Peter is talking virt-xml-validate or similar scheme test. In one of our internal review I have put the def for sev tags at /docs/schemas/domaincommon.rng. If it is what he talked about we will add def for new sev tags and verify correspondent XML files with virt-xml-validate.

...

...

+    </element> +  </define> +     <define name='value'>       <zeroOrMore>         <element name='value'> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index f7d9be50f82d..6a7a30877042 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -549,6 +549,24 @@ virDomainCapsFeatureGICFormat(virBufferPtr buf,       FORMAT_EPILOGUE(gic);   }   +static void +virDomainCapsFeatureSEVFormat(virBufferPtr buf, +                              virDomainCapsFeatureSEVPtr const sev) +{ +    FORMAT_PROLOGUE(sev);

If the feature is not supported, you should not format an empty element here. Just skip it completely.

OK, actually I was taking similar approach as 'gic' support -- which leaves the empty element in case if gic is not present. Will now take your recommendation and skip it completely.

...

...

+ +    if (sev->supported) { +        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); +    } + +    FORMAT_EPILOGUE(sev); +} +     char *   virDomainCapsFormat(virDomainCapsPtr const caps) @@ -587,6 +605,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 e13a7fd6ba1b..aed5ec28e9cc 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -102,6 +102,16 @@ struct _virDomainCapsFeatureGIC {       virDomainCapsEnum version; /* Info about virGICVersion */   };   +typedef struct _virDomainCapsFeatureSEV virDomainCapsFeatureSEV; +typedef virDomainCapsFeatureSEV *virDomainCapsFeatureSEVPtr; +struct _virDomainCapsFeatureSEV { +    bool supported; +    char *pdh;  /* host platform-diffie hellman key */ +    char *cert_chain;  /* PDH certificate chain */ +    int cbitpos; +    int reduced_phys_bits;

This structure is basically the same as the one in the qemu driver. Can't you just use this one in the qemu driver?

Yes they are same structure, will reuse it.

...

...

+}; +   typedef enum {       VIR_DOMCAPS_CPU_USABLE_UNKNOWN,       VIR_DOMCAPS_CPU_USABLE_YES, @@ -171,6 +181,7 @@ struct _virDomainCaps {       /* add new domain devices here */         virDomainCapsFeatureGIC gic; +    virDomainCapsFeatureSEV sev;       /* add new domain features here */   };   diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2c680528deb8..ee8c542679eb 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5880,6 +5880,44 @@ virQEMUCapsSupportsGICVersion(virQEMUCapsPtr qemuCaps,       return false;   }   +/** + * virQEMUCapsFillDomainFeatureSEVCaps: + * @qemuCaps: QEMU capabilities + * @domCaps: domain capabilities + * + * Take the information about SEV capabilities that has been obtained + * using the 'query-sev-capabilities' QMP command and stored in @qemuCaps + * and convert it to a form suitable for @domCaps.

This function would not be necessary in that case.

...

+ * + * Returns: 0 on success, <0 on failure + */ +static int +virQEMUCapsFillDomainFeatureSEVCaps(virQEMUCapsPtr qemuCaps, +                                    virDomainCapsPtr domCaps) +{ +    virDomainCapsFeatureSEVPtr sev = &domCaps->sev; +    virSEVCapability *cap = qemuCaps->sevCapabilities; + +    if (!cap) +        return 0; + +    sev->supported = cap->sev; + +    if (VIR_STRDUP(sev->pdh, cap->pdh) < 0) +        goto failed; + +    if (VIR_STRDUP(sev->cert_chain, cap->cert_chain) < 0) +        goto failed; + +    sev->cbitpos = cap->cbitpos; +    sev->reduced_phys_bits = cap->reduced_phys_bits; + +    return 0; +failed: +    sev->supported = false; +    return 0;

So why does this function return anything? Also we prefer the 'error' label in this case.

The function was modeled after gic feature which always returns 0 regardless whether gic is available or not. Similarly we don't want to fail just because SEV feature is not available. I can look into improving this.

On Mon, Feb 26, 2018 at 11:53:35 -0600, Brijesh Singh wrote:

Secure Encrypted Virtualization (sev) element is used to provide the guest owners input parameters used for creating an encrypted VM using AMD SEV feature. 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.

QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted VMs. A typical command line

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

A general question. How does this work with migration? Since you state that only the guest has access to unencrypted state I presume that the qemu process sees it only encrypted data. Thus the encryption keys need to be transferred to the other host machine. Also this means that we need to make sure that the migration will not work if the destination host of the migration does not support this (you'll need to add a migration cookie flag for this). Or if migration is not supported at all we'll need to reject it right away. This means that the virDomainMemoryPeek will probably need to be fixed to return error too.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 18 +++++++++++ src/qemu/qemu_command.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 230 insertions(+)

It would be preferable if you'd split the qemu bits from the config part.

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6fd2189cd2f4..d18e3fb1d976 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -8195,6 +8195,77 @@ 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>sev</code> element is used to provide the + guest owners input used for creating an encrypted VM using the AMD + Secure Encrypted Virtualization (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.

As said in the previous review I'd prefer if we'd agree on a less-vendor specific XML for this.

+ </p> + <pre> +<domain> + ... + <sev> + <policy> 1 </policy> + <cbitpos> 47 </cbitpos> + <reduced-phys-bits> 5 </reduced-phys-bits> + <session> ... </session> + <dh-cert> ... </dh> + </sev> + ... +</domain> +</pre> + + <p> + A least <code>cbitpos</code> and <code>reduced-phys-bits</code> must be nested + within the <code>sev</code> element. + </p> + <dl> + <dt><code>cbitpos</code></dt> + <dd>The <code>cbitpos</code> attribute provides the C-bit (aka encryption bit) + 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.

It sounds a bit more platform dependant. Is there any way where this could be different from the data present in the capabilities? If not it does not make much sense to allow configuring it. Also does it change the machine ABI? I'm asking whether it's necessary to expose this at all. (e.g. it's necessary to keep the value the same accross migrations).

+ </dd> + <dt><code>reduced-phys-bits</code></dt> + <dd>The <code>reduced-phys-bits</code> attribute provides the physical + address bit reducation. Similar to <code>cbitpos</code> the value of <code> + reduced-phys-bit</code> is hypervisor dependent and can be obtained + through the <code>sev</code> element from domaincapabilities.

Same applies for this.

+ </dd> + <dt><code>policy</code></dt> + <dd>The <code>policy</code> attribute 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.

So this is a hex number. Any documentation on the possible values?

+ </dd> + <dt><code>dh-cert</code></dt> + <dd>The <code>dh-cert</code> attribute provides the guest owners public + 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 + owner. The value must be encoded in base64. + </dd> + <dt><code>session</code></dt> + <dd>The <code>session</code> attribute provides the guest owners session + blob defined in SEV API spec. The value must be encoded in base64.

This should also be more documented.

+ </dd> + </dl> + + <p>Note: More information about <code>policy</code> bit definition, <code> + dh</code> and <code>session</code> is available in SEV API spec.</p>

At least by providing the specification document. Preferably commiting it to the libvirt repository so that it does not change URIs in the future.

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

<p> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d96b012b98f0..4c9921b5dca6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15539,6 +15539,61 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, return ret; }

+static void +virDomainSevDefFree(virDomainSevDefPtr def) +{ + VIR_FREE(def->dh_cert); + VIR_FREE(def->session); + + VIR_FREE(def); +} + +static virDomainSevDefPtr +virDomainSevDefParseXML(xmlNodePtr sevNode, + xmlXPathContextPtr ctxt) +{ + char *tmp = NULL; + xmlNodePtr save = ctxt->node; + virDomainSevDefPtr def; + unsigned long policy; + + ctxt->node = sevNode; + + if (VIR_ALLOC(def) < 0) + return NULL; + + 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); + } + + if (virXPathULongHex("string(./policy)", ctxt, &policy) == 0) { + def->policy = policy; + } else { + def->policy = -1; + } + + virXPathInt("string(./cbitpos)", ctxt, &def->cbitpos); + virXPathInt("string(./reduced-phys-bits)", ctxt, &def->reduced_phys_bits); + + ctxt->node = save; + return def; + +error: + VIR_FREE(tmp); + virDomainSevDefFree(def); + ctxt->node = save; + return NULL; +}

static virDomainMemoryDefPtr virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, @@ -20212,6 +20267,15 @@ virDomainDefParseXML(xmlDocPtr xml, ctxt->node = node; VIR_FREE(nodes);

+ /* Check for SEV feature */ + if ((n = virXPathNodeSet("./sev", ctxt, &nodes)) < 0) + goto error;

virXPathNode since you are not interrested in more than one element.

+ + if (n) { + def->sev = virDomainSevDefParseXML(nodes[0], ctxt); + VIR_FREE(nodes); + } + /* analysis of memory devices */ if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) < 0) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 368f16f3fbf9..f0f267b28f40 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 */ @@ -2289,6 +2292,18 @@ struct _virDomainKeyWrapDef { int dea; /* enum virTristateSwitch */ };

+typedef struct _virDomainSevDef virDomainSevDef; +typedef virDomainSevDef *virDomainSevDefPtr; + +struct _virDomainSevDef { + char *dh_cert; + char *session; + int policy; + int cbitpos; + int reduced_phys_bits; +}; + + typedef enum { VIR_DOMAIN_IOMMU_MODEL_INTEL,

@@ -2454,6 +2469,9 @@ struct _virDomainDef {

virDomainKeyWrapDefPtr keywrap;

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

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fa0aa5d5c3d4..653bbe154332 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9663,6 +9663,80 @@ qemuBuildTPMCommandLine(virCommandPtr cmd, return 0; }

+static char * +qemuBuildSevCreateFile(const virDomainDef *def, const char *name, char *data) +{ + char *base = virGetUserConfigDirectory(); + char *configDir, *configFile; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(def->uuid, uuidstr); + + if (virAsprintf(&configDir, "%s/sev/%s", base, uuidstr) < 0) + goto error; + VIR_FREE(base);

I think we should put these in the VM file directory rather than into the user config directory.

+ + if (virFileMakePathWithMode(configDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create config directory '%s'"), + configDir); + goto error; + } + + if (!(configFile = virFileBuildPath(configDir, name, ".base64"))) + goto error; + + if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) { + virReportSystemError(errno, _("failed to write data to config '%s'"), + configFile); + goto error; + } + + return configFile; + +error: + return NULL; +} + +static int +qemuBuildSevCommandLine(virCommandPtr cmd, + const virDomainDef *def) +{ + virDomainSevDefPtr sev = def->sev; + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBuffer obj = VIR_BUFFER_INITIALIZER; + char *dh_cert_file = NULL; + char *session_file = NULL; + + /* qemu accepts DH and session blob as file, create a temporary file */ + if (sev->dh_cert && + !(dh_cert_file = qemuBuildSevCreateFile(def, "dh_cert", sev->dh_cert))) + return -1; + + if (sev->session && + !(session_file = qemuBuildSevCreateFile(def, "session", sev->session))) + return -1;

These files should not be created in the command line generator but rather in 'qemuProcessPrepareHost'. Otherwise the test suite will leak these.

+ + virCommandAddArg(cmd, "-machine"); + virBufferAddLit(&buf, "memory-encryption=sev0"); + virCommandAddArgBuffer(cmd, &buf); + + virCommandAddArg(cmd, "-object"); + virBufferAddLit(&obj, "sev-guest,id=sev0");

I don't see any capability check that qemu actually supports this object. You'll need to add them ...

+ if (sev->policy > 0) + virBufferAsprintf(&obj, ",policy=0x%x", sev->policy); + virBufferAsprintf(&obj, ",cbitpos=%d", sev->cbitpos); + virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits); + if (dh_cert_file) + virBufferAsprintf(&obj, ",dh-cert-file=%s", dh_cert_file); + if (session_file) + virBufferAsprintf(&obj, ",session-file=%s", session_file); + virCommandAddArgBuffer(cmd, &obj); + + VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d dh=%s session=%s", + sev->policy, sev->cbitpos, sev->reduced_phys_bits, dh_cert_file, + session_file);

This should be right at the beginning of the func.

+ return 0; +}

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

+ if (def->sev && qemuBuildSevCommandLine(cmd, def) < 0) + goto error; + if (snapshot) virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);

Also this is lacking test cases for the qemuxml2argvtest, qemuxml2xmltest/genericxml2xmltest.

On 02/27/2018 05:10 AM, Daniel P. Berrangé wrote:

On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote:

...

Secure Encrypted Virtualization (sev) element is used to provide the guest owners input parameters used for creating an encrypted VM using AMD SEV feature. 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.

QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted VMs. A typical command line

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

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 18 +++++++++++ src/qemu/qemu_command.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 230 insertions(+)

In general we'd expect to see additions to the test suite for any XML changes. eg a qemuxml2xmltest and qemuxml2argvtest addition.

Sure, this is my first stab at libvirt and will look into getting familiar with test and add them in next round.

...

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6fd2189cd2f4..d18e3fb1d976 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -8195,6 +8195,77 @@ 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>sev</code> element is used to provide the + guest owners input used for creating an encrypted VM using the AMD + Secure Encrypted Virtualization (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. + </p> + <pre> +<domain> + ... + <sev> + <policy> 1 </policy> + <cbitpos> 47 </cbitpos> + <reduced-phys-bits> 5 </reduced-phys-bits> + <session> ... </session> + <dh-cert> ... </dh> + </sev>

Minor nitpick - since this inheranted SEV specific, I think we could do with having a generic top level element with a type=sev. eg

<launch-security type="sev"> <policy>...</policy> <cbitpos>..</cbitpos> ...etc... </launch>

then we can plug in custom data if other vendors invent competing solutions to AMD's SEV.

I am okay with this, how about <memory-encryption> instead of <launch-security>, are you okay with it ?

On 02/27/2018 11:15 AM, Daniel P. Berrangé wrote:

On Tue, Feb 27, 2018 at 11:07:25AM -0600, Brijesh Singh wrote:

...

On 02/27/2018 05:10 AM, Daniel P. Berrangé wrote:

...

On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote:

...

Secure Encrypted Virtualization (sev) element is used to provide the guest owners input parameters used for creating an encrypted VM using AMD SEV feature. 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.

QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted VMs. A typical command line

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

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 18 +++++++++++ src/qemu/qemu_command.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 230 insertions(+)

In general we'd expect to see additions to the test suite for any XML changes. eg a qemuxml2xmltest and qemuxml2argvtest addition.

Sure, this is my first stab at libvirt and will look into getting familiar with test and add them in next round.

...

...

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6fd2189cd2f4..d18e3fb1d976 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -8195,6 +8195,77 @@ 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>sev</code> element is used to provide the + guest owners input used for creating an encrypted VM using the AMD + Secure Encrypted Virtualization (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. + </p> + <pre> +<domain> + ... + <sev> + <policy> 1 </policy> + <cbitpos> 47 </cbitpos> + <reduced-phys-bits> 5 </reduced-phys-bits> + <session> ... </session> + <dh-cert> ... </dh> + </sev>

Minor nitpick - since this inheranted SEV specific, I think we could do with having a generic top level element with a type=sev. eg

<launch-security type="sev"> <policy>...</policy> <cbitpos>..</cbitpos> ...etc... </launch>

then we can plug in custom data if other vendors invent competing solutions to AMD's SEV.

I am okay with this, how about <memory-encryption> instead of <launch-security>, are you okay with it ?

Memory encryption is a very specific feature. It occurs to me that there could in future be other features that use launch time validation, that are not memory encryption related.

Understood, I am good with launch-security tag. thanks

On Wed, Feb 28, 2018 at 10:34:51AM +0100, Erik Skultety wrote:

On Tue, Feb 27, 2018 at 05:15:30PM +0000, Daniel P. Berrangé wrote:

...

On Tue, Feb 27, 2018 at 11:07:25AM -0600, Brijesh Singh wrote:

...

On 02/27/2018 05:10 AM, Daniel P. Berrangé wrote:

...

On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote:

...

Secure Encrypted Virtualization (sev) element is used to provide the guest owners input parameters used for creating an encrypted VM using AMD SEV feature. 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.

QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted VMs. A typical command line

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

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 18 +++++++++++ src/qemu/qemu_command.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 230 insertions(+)

In general we'd expect to see additions to the test suite for any XML changes. eg a qemuxml2xmltest and qemuxml2argvtest addition.

Sure, this is my first stab at libvirt and will look into getting familiar with test and add them in next round.

...

...

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6fd2189cd2f4..d18e3fb1d976 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -8195,6 +8195,77 @@ 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>sev</code> element is used to provide the + guest owners input used for creating an encrypted VM using the AMD + Secure Encrypted Virtualization (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. + </p> + <pre> +<domain> + ... + <sev> + <policy> 1 </policy> + <cbitpos> 47 </cbitpos> + <reduced-phys-bits> 5 </reduced-phys-bits> + <session> ... </session> + <dh-cert> ... </dh> + </sev>

Minor nitpick - since this inheranted SEV specific, I think we could do with having a generic top level element with a type=sev. eg

<launch-security type="sev"> <policy>...</policy> <cbitpos>..</cbitpos> ...etc... </launch>

then we can plug in custom data if other vendors invent competing solutions to AMD's SEV.

I am okay with this, how about <memory-encryption> instead of <launch-security>, are you okay with it ?

Memory encryption is a very specific feature. It occurs to me that there could in future be other features that use launch time validation, that are not memory encryption related.

<launch-security> is IMHO still rather specific than generic, since we might need to enable features in the future, which might/might no rely on security, but add additional attributes to the launch validation, in which case I think going for something like <launch-control> or simply <launch> and having a structure similar to the one below:

Any kind of launch validation is ultimately security related in some manner.

By having the separate <sev> element you can make the sub-elements depend on this parent element, since you can't expect other vendors to favour <cbitpos> which add burden to the documentation to make it clear. Of course, the price you pay for this is a more complex XML structure. <launch> <security> <sev> <sev_specific_elements/> </sev> </security>

This is not the way we usually do things - we wuld have a type="sev|..." which determines what child elements are permitted, as illustrated in the example above.

<other_security_unrelated_validation_options/> </launch>

I think the extra level of nesting is uneccessary Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Feb 26, 2018 at 11:53:36AM -0600, Brijesh Singh wrote:

The virDomainGetSevVmMeasurement() can be used to retrieve the measurement of encrypted VM launched using AMD SEV feature. The measurement is a signature of the memory contents that can be sent to the guest owner as an attestation that the memory was encrypted correctly by the firmware before booting the guest.

Signed-off-by: Xiaogang Chen <Xiaogang.Chen@amd.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- include/libvirt/libvirt-domain.h | 4 +++ src/driver-hypervisor.h | 4 +++ src/libvirt-domain.c | 41 +++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + src/qemu/qemu_driver.c | 57 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 8 ++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 33 +++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 2 ++ src/remote/remote_driver.c | 3 ++- src/remote/remote_protocol.x | 17 +++++++++++- 11 files changed, 171 insertions(+), 2 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 4048acf38aaf..c0bcfea4723c 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -4756,4 +4756,8 @@ int virDomainSetLifecycleAction(virDomainPtr domain, unsigned int action, unsigned int flags);

+char * +virDomainGetSevVmMeasurement(virDomainPtr domain, + unsigned int flags);

I'm a little concerned with introducing an API like this that is tied to the specific implementation that we have in AMD chips today, as it is unlikely to be usable when other vendors provide equivalent functionality. At the same time it is hard (impossible) to predict what information future impelementations will need to provide. So I wonder if we would be better making the API generic by using the virTypedParameters design. eg virDomaniGetLaunchSecurityInfo(virDOmaniPtr domain, virTypedParametersPtr *params, unsigned int *nparams, unsigned int flags); And then to start with we could just define a single named parameter #define VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT "sev-measurement"

diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 95df3a0dbc7b..6e956d965a26 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -783,6 +783,7 @@ LIBVIRT_3.9.0 { LIBVIRT_4.1.0 { global: virStoragePoolLookupByTargetPath; + virDomainGetSevVmMeasurement; } LIBVIRT_3.9.0;

Since we're in freeze for 4.1.0, it is going to be 4.2.0 at the earliest now.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 313d730c791f..852d1f0fd2f7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21254,6 +21254,62 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, return ret; }

+static char * +qemuDomainGetSevVmMeasurement(virDomainPtr dom, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + char *ret = NULL, *tmp; + + virCheckFlags(0, NULL); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + + if (virDomainGetSevVmMeasurementEnsureACL(dom->conn, vm->def) < 0){ + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("get sev vm measurement is not allowed")); + goto cleanup; + } + + if (vm->def->sev) { + goto endjob; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("domain is not SEV guest"));

Hehe, unreachable code due to inverted order :-)

+ } + + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + + VIR_DEBUG("query sev launch measurement"); + if(!(tmp = qemuMonitorGetSevMeasurement(QEMU_DOMAIN_PRIVATE(vm)->mon))){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to get measurement")); + goto endjob; + } + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; + + ret = tmp; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +}

Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|