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

On Wed, May 23, 2018 at 04:18:27PM -0500, 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 > (PDH) 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; > --- ...and this one should be IMHO named conf: Expose SEV in domain capabilities > 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(+) > > diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in > index b68ae4b4f1f3..f37b059ba6b1 100644 > --- a/docs/formatdomaincaps.html.in > +++ b/docs/formatdomaincaps.html.in > @@ -434,6 +434,12 @@ > &lt;/enum&gt; > &lt;/gic&gt; > &lt;vmcoreinfo supported='yes'/&gt; > + &lt;sev&gt; > + &lt;pdh&gt;UWxKSlNrVlRTRk5KVGtkSVFVMUU=&lt;/pdh&gt; > + &lt;cert-chain&gt;VVd4S1NsTnJWbFJUUms1S1ZHdGtTVkZWTVVVPQ==&lt;/cert-chain&gt; > + &lt;cbitpos&gt;47&lt;/cbitpos&gt; > + &lt;reduced-phys-bits&gt;1&lt;/reduced-phys-bits&gt; > + &lt;/sev&gt; > &lt;/features&gt; > &lt;/domainCapabilities&gt; > </pre> > @@ -462,5 +468,39 @@ > > <p>Reports whether the vmcoreinfo feature can be enabled</p> > > + <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.</p> > + > + <p> > + For more details on SEV feature see: > + <a href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specificat... > + SEV API spec</a> and <a href="http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12... > + SEV White Paper</a> > + </p> > + > + <dl> > + <dt><code>pdh</code></dt> > + <dd>A base64 encoded platform Diffie-Hellman public key which can be > + exported to remote entities that desire to establish a secure transport > + context with the SEV platform in order to transmit data securely.</dd> > + <dt><code>cert-chain</code></dt> > + <dd> A base64 encoded platform certificate chain that includes the platform > + endorsement key (PEK), owners certificate authority (OCD), and chip > + endorsement key (CEK).</dd> > + <dt><code>cbitpos</code></dt> > + <dd>When memory encryption is enabled, one of the physical address 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 lose certain bits in physical > + address space. The number of bits we lose is hypervisor dependent.</dd> > + </dl> > + > </body> > </html> > diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng > index 5913d711a3fe..26265645b82a 100644 > --- a/docs/schemas/domaincaps.rng > +++ b/docs/schemas/domaincaps.rng > @@ -184,6 +184,9 @@ > <interleave> > <ref name='gic'/> > <ref name='vmcoreinfo'/> > + <optional> > + <ref name='sev'/> needs 1 more level of indent...

> + </optional> > </interleave> > </element> > </define> > @@ -201,6 +204,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> > + > <define name='value'> > <zeroOrMore> > <element name='value'> > diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c > index 6e2ab0a28796..3b767c45cbb3 100644 > --- a/src/conf/domain_capabilities.c > +++ b/src/conf/domain_capabilities.c > @@ -542,6 +542,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); > + virBufferEscapeString(buf, "<pdh>%s</pdh>\n", sev->pdh); > + virBufferEscapeString(buf, "<cert-chain>%s</cert-chain>\n", > + sev->cert_chain); As I said, I have to agree with Dan here that reporting the 8k string in capabilities is a good idea, so we can store it, we just wouldn't format it ^here...

> + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</sev>\n"); > +} > + > > char * > virDomainCapsFormat(virDomainCapsPtr const caps) > @@ -585,6 +604,7 @@ virDomainCapsFormat(virDomainCapsPtr const caps) > virDomainCapsFeatureGICFormat(&buf, &caps->gic); > virBufferAsprintf(&buf, "<vmcoreinfo supported='%s'/>\n", > caps->vmcoreinfo ? "yes" : "no"); > + 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 c1093234ceb8..e33bef525ef4 100644 > --- a/src/conf/domain_capabilities.h > +++ b/src/conf/domain_capabilities.h > @@ -172,6 +172,7 @@ struct _virDomainCaps { > > virDomainCapsFeatureGIC gic; > bool vmcoreinfo; > + virSEVCapabilityPtr sev; > /* add new domain features here */ > }; > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 49b74f7e12c1..3345b09fa384 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -4998,6 +4998,8 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps, > virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0 || > virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0) > return -1; > + > + domCaps->sev = qemuCaps->sevCapabilities; domCaps is a one-time-use object needed when you format the qemuCaps into domCaps which is returned to the user and is therefore cleaned up on the API exit, so ^this shouldn't be considered safe and you should create virQEMUCapsFillDomainFeatureSEVCaps where you allocate a fresh domCaps->sev structure and then you'll need to put virSEVCapabilityFree I mentioned in patch 1 to virDomainCapsDispose so as not to leak any memory related to copying the SEV data.