[libvirt] [PATCH v5 02/10] qemu: introduce SEV feature in hypervisor capabilities
Brijesh Singh
brijesh.singh at amd.com
Mon Apr 2 19:20:17 UTC 2018
On 04/02/2018 12:33 PM, John Ferlan wrote:
>
>
> 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?
>
Yes, I will use camel case in next patch.
>> 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 at redhat.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
>> ---
>> 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.
>
Noted, I will fill in some random values.
>> </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
>
Noted.
>> +
>> + 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..."
Noted.
>
>> + 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).
>
Noted, makes it much cleaner. thanks
>> + <dt><code>cbitpos</code></dt>
>> + <dd>When memory encryption is enabled, one of the physical address bit
>
> s/bit/bits
>
Noted.
>> + (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
>
Noted
>> </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?
>
I have been trying keep the same naming convension as sev-guest object
name (which uses dashes). I am flexible, if libvirt community prefers
camelCase over the dashes then I am okay with it.
>
>> <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.
>
Noted.
-Brijesh
More information about the libvir-list
mailing list