10:45 a.m.
On 2/27/2018 10:29 AM, Brijesh Singh wrote:
On 02/27/2018 02:15 AM, Peter Krempa wrote:
> 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(a)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?
How about something like:
<memory-encryption type='sev'>
<pdh> ..</pdh>
....
....
</memory-encryption>
if other manufacture implements memory encryption then we can
introduce a new type to handle new memory encryption feature. The
inputs to the memory encryption type is going to be vendor-specific.
>
>> + </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.
>
I will add some more details to clarify this.
>> + </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.
>
I must admit that I am new to libvirt hence need some help. Sorry I am
not able to follow your this comment. Do I need to update
domaincap.rng or not ? If yes, where do I need to define the names so
that we don't break the schema test. Any tips is appreciated. thanks
I think Peter is talking virt-xml-validate or similar scheme test. In
one of our internal review I have put the def for sev tags at
/docs/schemas/domaincommon.rng. If it is what he talked about we will
add def for new sev tags and verify correspondent XML files with
virt-xml-validate.
>> + </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.
OK, actually I was taking similar approach as 'gic' support -- which
leaves the empty element in case if gic is not present. Will now take
your recommendation and 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?
Yes they are same structure, will reuse it.
>
>> +};
>> +
>> 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.
>
The function was modeled after gic feature which always returns 0
regardless whether gic is available or not. Similarly we don't want to
fail just because SEV feature is not available. I can look into
improving this.