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

["Chen, Xiaogang" <Xiaogang.Chen@xxxxxxx>]

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@xxxxxxx>
---
  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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list