Re: [libvirt] [PATCH 2/4] qemu: introduce SEV feature in hypervisor capabilities

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(a)amd.com&gt; > --- > 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 @@ > &lt;value&gt;3&lt;/value&gt; > &lt;/enum&gt; > &lt;/gic&gt; > + &lt;sev supported='yes'&gt; > + &lt;pdh&gt; &lt;/pdh&gt; > + &lt;cert-chain&gt; &lt;/cert-chain&gt; > + &lt;cbitpos&gt; &lt;/cbitpos&gt; > + &lt;reduced-phys-bits&gt; &lt;/reduced-phys-bits&gt; > + &lt;/sev&gt; > &lt;/features&gt; > &lt;/domainCapabilities&gt; > </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.