[libvirt] [PATCH 2/4] qemu: introduce SEV feature in hypervisor capabilities
Chen, Xiaogang
Xiaogang.Chen at amd.com
Tue Feb 27 16:45:00 UTC 2018
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 at 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180227/f673f97b/attachment-0001.htm>
More information about the libvir-list
mailing list