On 04/02/2018 10:18 AM, 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 Diffie-Hellman right? > 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. > > Reviewed-by: "Daniel P. Berrangé" <berrange@xxxxxxxxxx> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx> > --- > 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(+) > I see this has Daniel's R-by, but I have a few notes and questions... > diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in > index 6bfcaf6..f383141 100644 > --- a/docs/formatdomaincaps.html.in > +++ b/docs/formatdomaincaps.html.in > @@ -417,6 +417,12 @@ > <value>3</value> > </enum> > </gic> > + <sev> > + <pdh> </pdh> > + <cert-chain> </cert-chain> > + <cbitpos> </cbitpos> > + <reduced-phys-bits> </reduced-phys-bits> > + </sev> The example output should have some sort of example output and not an empty space. > </features> > </domainCapabilities> > </pre> > @@ -441,5 +447,39 @@ > <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. I think it would be cleaner to add a </p> to after VM and then a new <p> on the next line > + > + For more details on SEV feature see: > + <a href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf";> > + SEV API spec</a> and <a href="http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf";> > + SEV White Paper</a> > + > + </p> > + > + <dl> > + <dt><code>pdh</code></dt> > + <dd>Platform diffie-hellman key, which can be exported to remote entities Again, I think this should be Diffie-Hellman ? And it's the public key right - so "A base64 encoded platform Diffie-Hellman public key which..." > + which wish to establish a secure transport context with the SEV platform "which wish" reads strange - how about "that desire" > + in order to transmit data securely. The key is encoded in base64</dd> Add "A base64 encoded" up front makes the last sentence duplicitous. > + <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> A base64 encoded platform certificate chain that includes the platform endorsement key (PEK), owners certificate authority (OCD), and chip endorsement key (CEK). > + <dt><code>cbitpos</code></dt> > + <dd>When memory encryption is enabled, one of the physical address bit s/bit/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 loose certain bits in physical > + address space. The number of bits we loose is hypervisor dependent.</dd> > + </dl> > + s/loose/lose > </body> > </html> > diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng > index 3905318..53b33eb 100644 > --- a/docs/schemas/domaincaps.rng > +++ b/docs/schemas/domaincaps.rng > @@ -173,6 +173,9 @@ > <element name='features'> > <interleave> > <ref name='gic'/> > + <optional> > + <ref name='sev'/> > + </optional> > </interleave> > </element> > </define> > @@ -184,6 +187,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> > + I want to make sure of recent preferences which aren't documented, but sometimes draw attention of certain reviewers... Do we want dashes or camelCase for new names? e.g. "cert-chain" or "certChain"? And likewise for reduced-phys-bits or reducedPhysBits? > <define name='value'> > <zeroOrMore> > <element name='value'> > diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c > index f7d9be5..082065f 100644 > --- a/src/conf/domain_capabilities.c > +++ b/src/conf/domain_capabilities.c > @@ -549,6 +549,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); > + virBufferAsprintf(buf, "<pdh>%s</pdh>\n", sev->pdh); > + virBufferAsprintf(buf, "<cert-chain>%s</cert-chain>\n", > + sev->cert_chain); When formatting strings, use virBufferEscapeString Yes, I know they're base64 encoded, but not taking any chances... Depending on whether there's an opinion w/r/t camelCase or not, I can either fix up the nits or wait for a v6 with adjustments to the user facing values. John > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</sev>\n"); > +} > + > > char * > virDomainCapsFormat(virDomainCapsPtr const caps) > @@ -587,6 +606,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 72e9daf..2e8596c 100644 > --- a/src/conf/domain_capabilities.h > +++ b/src/conf/domain_capabilities.h > @@ -170,6 +170,7 @@ struct _virDomainCaps { > /* add new domain devices here */ > > virDomainCapsFeatureGIC gic; > + virSEVCapabilityPtr sev; > /* add new domain features here */ > }; > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 0f6e6fb..3fd4911 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -5787,6 +5787,8 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps, > virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0 || > virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0) > return -1; > + > + domCaps->sev = qemuCaps->sevCapabilities; > return 0; > } > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
