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.