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@xxxxxxx>
> ---
> 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.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
