[v1] x86: Secure Encrypted Virtualization (AMD)

[libvirt] [PATCH 2/4] qemu: introduce SEV feature in hypervisor capabilities

Posted by Brijesh Singh 7 years, 2 months ago

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@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 @@
         &lt;value&gt;3&lt;/value&gt;
       &lt;/enum&gt;
     &lt;/gic&gt;
+    &lt;sev supported='yes'&gt;
+      &lt;pdh&gt; &lt;/pdh&gt;
+      &lt;cert-chain&gt; &lt;/cert-chain&gt;
+      &lt;cbitpos&gt; &lt;/cbitpos&gt;
+      &lt;reduced-phys-bits&gt; &lt;/reduced-phys-bits&gt;
+    &lt;/sev&gt;
   &lt;/features&gt;
 &lt;/domainCapabilities&gt;
 </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.
+    </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>
+    </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'/>
+    </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 (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;
+};
+
 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.
+ *
+ * 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;
+}
+
 
 /**
  * virQEMUCapsFillDomainFeatureGICCaps:
@@ -5958,7 +5996,8 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps,
         virQEMUCapsFillDomainDeviceGraphicsCaps(qemuCaps, graphics) < 0 ||
         virQEMUCapsFillDomainDeviceVideoCaps(qemuCaps, video) < 0 ||
         virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0 ||
-        virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0)
+        virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0 ||
+        virQEMUCapsFillDomainFeatureSEVCaps(qemuCaps, domCaps))
         return -1;
     return 0;
 }
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Peter Krempa 7 years, 2 months ago

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@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 @@
>          &lt;value&gt;3&lt;/value&gt;
>        &lt;/enum&gt;
>      &lt;/gic&gt;
> +    &lt;sev supported='yes'&gt;
> +      &lt;pdh&gt; &lt;/pdh&gt;
> +      &lt;cert-chain&gt; &lt;/cert-chain&gt;
> +      &lt;cbitpos&gt; &lt;/cbitpos&gt;
> +      &lt;reduced-phys-bits&gt; &lt;/reduced-phys-bits&gt;
> +    &lt;/sev&gt;
>    &lt;/features&gt;
>  &lt;/domainCapabilities&gt;
>  </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?

> +    </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.

> +    </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.

> +    </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.

> +
> +    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?

> +};
> +
>  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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Brijesh Singh 7 years, 2 months ago


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@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 @@
>>           &lt;value&gt;3&lt;/value&gt;
>>         &lt;/enum&gt;
>>       &lt;/gic&gt;
>> +    &lt;sev supported='yes'&gt;
>> +      &lt;pdh&gt; &lt;/pdh&gt;
>> +      &lt;cert-chain&gt; &lt;/cert-chain&gt;
>> +      &lt;cbitpos&gt; &lt;/cbitpos&gt;
>> +      &lt;reduced-phys-bits&gt; &lt;/reduced-phys-bits&gt;
>> +    &lt;/sev&gt;
>>     &lt;/features&gt;
>>   &lt;/domainCapabilities&gt;
>>   </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



>> +    </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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Chen, Xiaogang 7 years, 2 months ago


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@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 @@
>>>           &lt;value&gt;3&lt;/value&gt;
>>>         &lt;/enum&gt;
>>>       &lt;/gic&gt;
>>> +    &lt;sev supported='yes'&gt;
>>> +      &lt;pdh&gt; &lt;/pdh&gt;
>>> +      &lt;cert-chain&gt; &lt;/cert-chain&gt;
>>> +      &lt;cbitpos&gt; &lt;/cbitpos&gt;
>>> +      &lt;reduced-phys-bits&gt; &lt;/reduced-phys-bits&gt;
>>> +    &lt;/sev&gt;
>>>     &lt;/features&gt;
>>>   &lt;/domainCapabilities&gt;
>>>   </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.
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/4] qemu: provide support to query the SEV capability
[libvirt] [PATCH 2/4] qemu: introduce SEV feature in hypervisor capabilities
[libvirt] [PATCH 3/4] conf: introduce sev element in domain
[libvirt] [PATCH 4/4] libvirt-domain: add new virDomainGetSevVmMeasurement() API