Secure Encrypted Virtualization (sev) element is used to provide the guest
owners input parameters used for creating an encrypted VM using AMD SEV
feature. SEV feature supports running encrypted VM under the control of
KVM. Encrypted VMs have their pages (code and data) secured such that only
the guest itself has access to the unencrypted version. Each encrypted VM
is associated with a unique encryption key; if its data is accessed to a
different entity using a different key the encrypted guests data will be
incorrectly decrypted, leading to unintelligible data.

QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted
VMs. A typical command line

# $QEMU ... \
   -machine memory-encryption=sev0 \
   -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \
   ...

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++
 src/conf/domain_conf.c    | 64 +++++++++++++++++++++++++++++++++++++++
 src/conf/domain_conf.h    | 18 +++++++++++
 src/qemu/qemu_command.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 230 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6fd2189cd2f4..d18e3fb1d976 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null
 
     <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
 
+    <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3>
+
+    <p>
+       The contents of the <code>sev</code> element is used to provide the
+       guest owners input used for creating an encrypted VM using the AMD
+       Secure Encrypted Virtualization (SEV) feature.
+
+       SEV is an extension to the AMD-V architecture which supports running
+       encrypted virtual machine (VMs) under the control of KVM. Encrypted
+       VMs have their pages (code and data) secured such that only the guest
+       itself has access to the unencrypted version. Each encrypted VM is
+       associated with a unique encryption key; if its data is accessed to a
+       different entity using a different key the encrypted guests data will
+       be incorrectly decrypted, leading to unintelligible data.
+       </p>
+    <pre>
+&lt;domain&gt;
+  ...
+  &lt;sev&gt;
+    &lt;policy&gt; 1 &lt;/policy&gt;
+    &lt;cbitpos&gt; 47 &lt;/cbitpos&gt;
+    &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt;
+    &lt;session&gt; ... &lt;/session&gt;
+    &lt;dh-cert&gt; ... &lt;/dh&gt;
+  &lt;/sev&gt;
+  ...
+&lt;/domain&gt;
+</pre>
+
+    <p>
+    A least <code>cbitpos</code> and <code>reduced-phys-bits</code> must be nested
+    within the <code>sev</code> element.
+    </p>
+    <dl>
+      <dt><code>cbitpos</code></dt>
+      <dd>The <code>cbitpos</code> attribute provides the C-bit (aka encryption bit)
+      location in guest page table entry. The value of <code>cbitpos</code> is
+      hypervisor dependent and can be obtained through the <code>sev</code> element
+      from domaincapabilities.
+      </dd>
+      <dt><code>reduced-phys-bits</code></dt>
+      <dd>The <code>reduced-phys-bits</code> attribute provides the physical
+      address bit reducation. Similar to <code>cbitpos</code> the value of <code>
+      reduced-phys-bit</code> is hypervisor dependent and can be obtained
+      through the <code>sev</code> element from domaincapabilities.
+      </dd>
+      <dt><code>policy</code></dt>
+      <dd>The <code>policy</code> attribute provides the guest policy which must
+      be maintained by the SEV firmware. This policy is enforced by the firmware
+      and restricts what configuration and operational commands can be performed
+      on this guest by the hypervisor. The guest policy provided during guest
+      launch is bound to the guest and cannot be changed throughout the lifetime
+      of the guest. The policy is also transmitted during snapshot and migration
+      flows and enforced on the destination platform.
+      </dd>
+      <dt><code>dh-cert</code></dt>
+      <dd>The <code>dh-cert</code> attribute provides the guest owners public
+      Diffie-Hellman (DH) key. The key is used to negotiate a master secret
+      key between the SEV firmware and guest owner. This master secret key is
+      then used to establish a trusted channel between SEV firmware and guest
+      owner. The value must be encoded in base64.
+      </dd>
+      <dt><code>session</code></dt>
+      <dd>The <code>session</code> attribute provides the guest owners session
+      blob defined in SEV API spec. The value must be encoded in base64.
+      </dd>
+    </dl>
+
+    <p>Note: More information about <code>policy</code> bit definition, <code>
+    dh</code> and <code>session</code> is available in SEV API spec.</p>
+
     <h2><a id="examples">Example configs</a></h2>
 
     <p>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d96b012b98f0..4c9921b5dca6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15539,6 +15539,61 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
     return ret;
 }
 
+static void
+virDomainSevDefFree(virDomainSevDefPtr def)
+{
+    VIR_FREE(def->dh_cert);
+    VIR_FREE(def->session);
+
+    VIR_FREE(def);
+}
+
+static virDomainSevDefPtr
+virDomainSevDefParseXML(xmlNodePtr sevNode,
+                        xmlXPathContextPtr ctxt)
+{
+    char *tmp = NULL;
+    xmlNodePtr save = ctxt->node;
+    virDomainSevDefPtr def;
+    unsigned long policy;
+
+    ctxt->node = sevNode;
+
+    if (VIR_ALLOC(def) < 0)
+        return NULL;
+
+    if ((tmp = virXPathString("string(./dh-cert)", ctxt))) {
+        if (VIR_STRDUP(def->dh_cert, tmp) < 0)
+            goto error;
+
+        VIR_FREE(tmp);
+    }
+
+    if ((tmp = virXPathString("string(./session)", ctxt))) {
+        if (VIR_STRDUP(def->session, tmp) < 0)
+            goto error;
+
+        VIR_FREE(tmp);
+    }
+
+    if (virXPathULongHex("string(./policy)", ctxt, &policy) == 0) {
+        def->policy = policy;
+    } else {
+        def->policy = -1;
+    }
+
+    virXPathInt("string(./cbitpos)", ctxt, &def->cbitpos);
+    virXPathInt("string(./reduced-phys-bits)", ctxt, &def->reduced_phys_bits);
+
+    ctxt->node = save;
+    return def;
+
+error:
+    VIR_FREE(tmp);
+    virDomainSevDefFree(def);
+    ctxt->node = save;
+    return NULL;
+}
 
 static virDomainMemoryDefPtr
 virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
@@ -20212,6 +20267,15 @@ virDomainDefParseXML(xmlDocPtr xml,
     ctxt->node = node;
     VIR_FREE(nodes);
 
+    /* Check for SEV feature */
+    if ((n = virXPathNodeSet("./sev", ctxt, &nodes)) < 0)
+        goto error;
+
+    if (n) {
+        def->sev = virDomainSevDefParseXML(nodes[0], ctxt);
+        VIR_FREE(nodes);
+    }
+
     /* analysis of memory devices */
     if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) < 0)
         goto error;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 368f16f3fbf9..f0f267b28f40 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -142,6 +142,9 @@ typedef virDomainPanicDef *virDomainPanicDefPtr;
 typedef struct _virDomainMemoryDef virDomainMemoryDef;
 typedef virDomainMemoryDef *virDomainMemoryDefPtr;
 
+typedef struct _virDomainSevDef virDomainSevDef;
+typedef virDomainSevDef *virDomainSevDefPtr;
+
 /* forward declarations virDomainChrSourceDef, required by
  * virDomainNetDef
  */
@@ -2289,6 +2292,18 @@ struct _virDomainKeyWrapDef {
     int dea; /* enum virTristateSwitch */
 };
 
+typedef struct _virDomainSevDef virDomainSevDef;
+typedef virDomainSevDef *virDomainSevDefPtr;
+
+struct _virDomainSevDef {
+    char *dh_cert;
+    char *session;
+    int policy;
+    int cbitpos;
+    int reduced_phys_bits;
+};
+
+
 typedef enum {
     VIR_DOMAIN_IOMMU_MODEL_INTEL,
 
@@ -2454,6 +2469,9 @@ struct _virDomainDef {
 
     virDomainKeyWrapDefPtr keywrap;
 
+    /* SEV-specific domain */
+    virDomainSevDefPtr sev;
+
     /* Application-specific custom metadata */
     xmlNodePtr metadata;
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fa0aa5d5c3d4..653bbe154332 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9663,6 +9663,80 @@ qemuBuildTPMCommandLine(virCommandPtr cmd,
     return 0;
 }
 
+static char *
+qemuBuildSevCreateFile(const virDomainDef *def, const char *name, char *data)
+{
+    char *base = virGetUserConfigDirectory();
+    char *configDir, *configFile;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+    virUUIDFormat(def->uuid, uuidstr);
+
+    if (virAsprintf(&configDir, "%s/sev/%s", base, uuidstr) < 0)
+        goto error;
+    VIR_FREE(base);
+
+    if (virFileMakePathWithMode(configDir, S_IRWXU) < 0) {
+        virReportSystemError(errno, _("cannot create config directory '%s'"),
+                             configDir);
+        goto error;
+    }
+
+    if (!(configFile = virFileBuildPath(configDir, name, ".base64")))
+        goto error;
+
+    if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) {
+        virReportSystemError(errno, _("failed to write data to config '%s'"),
+                             configFile);
+        goto error;
+    }
+
+    return configFile;
+
+error:
+    return NULL;
+}
+
+static int
+qemuBuildSevCommandLine(virCommandPtr cmd,
+                        const virDomainDef *def)
+{
+    virDomainSevDefPtr sev = def->sev;
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    virBuffer obj = VIR_BUFFER_INITIALIZER;
+    char *dh_cert_file = NULL;
+    char *session_file = NULL;
+
+    /* qemu accepts DH and session blob as file, create a temporary file */
+    if (sev->dh_cert &&
+        !(dh_cert_file = qemuBuildSevCreateFile(def, "dh_cert", sev->dh_cert)))
+        return -1;
+
+    if (sev->session &&
+        !(session_file = qemuBuildSevCreateFile(def, "session", sev->session)))
+        return -1;
+
+    virCommandAddArg(cmd, "-machine");
+    virBufferAddLit(&buf, "memory-encryption=sev0");
+    virCommandAddArgBuffer(cmd, &buf);
+
+    virCommandAddArg(cmd, "-object");
+    virBufferAddLit(&obj, "sev-guest,id=sev0");
+    if (sev->policy > 0)
+        virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
+    virBufferAsprintf(&obj, ",cbitpos=%d", sev->cbitpos);
+    virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
+    if (dh_cert_file)
+        virBufferAsprintf(&obj, ",dh-cert-file=%s", dh_cert_file);
+    if (session_file)
+        virBufferAsprintf(&obj, ",session-file=%s", session_file);
+    virCommandAddArgBuffer(cmd, &obj);
+
+    VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d dh=%s session=%s",
+             sev->policy, sev->cbitpos, sev->reduced_phys_bits, dh_cert_file,
+             session_file);
+    return 0;
+}
 
 static int
 qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
@@ -10108,6 +10182,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
     if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
         goto error;
 
+    if (def->sev && qemuBuildSevCommandLine(cmd, def) < 0)
+        goto error;
+
     if (snapshot)
         virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
 
-- 
2.14.3

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

On Mon, Feb 26, 2018 at 11:53:35 -0600, Brijesh Singh wrote:
> Secure Encrypted Virtualization (sev) element is used to provide the guest
> owners input parameters used for creating an encrypted VM using AMD SEV
> feature. SEV feature supports running encrypted VM under the control of
> KVM. Encrypted VMs have their pages (code and data) secured such that only
> the guest itself has access to the unencrypted version. Each encrypted VM
> is associated with a unique encryption key; if its data is accessed to a
> different entity using a different key the encrypted guests data will be
> incorrectly decrypted, leading to unintelligible data.
> 
> QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted
> VMs. A typical command line
> 
> # $QEMU ... \
>    -machine memory-encryption=sev0 \
>    -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \
>    ...

A general question. How does this work with migration? Since you state
that only the guest has access to unencrypted state I presume that the
qemu process sees it only encrypted data. Thus the encryption keys need
to be transferred to the other host machine.

Also this means that we need to make sure that the migration will not
work if the destination host of the migration does not support this
(you'll need to add a migration cookie flag for this). Or if migration
is not supported at all we'll need to reject it right away.

This means that the virDomainMemoryPeek will probably need to be fixed
to return error too.

> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.c    | 64 +++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h    | 18 +++++++++++
>  src/qemu/qemu_command.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 230 insertions(+)

It would be preferable if you'd split the qemu bits from the config
part.

> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 6fd2189cd2f4..d18e3fb1d976 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null
>  
>      <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
>  
> +    <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3>
> +
> +    <p>
> +       The contents of the <code>sev</code> element is used to provide the
> +       guest owners input used for creating an encrypted VM using the AMD
> +       Secure Encrypted Virtualization (SEV) feature.
> +
> +       SEV is an extension to the AMD-V architecture which supports running
> +       encrypted virtual machine (VMs) under the control of KVM. Encrypted
> +       VMs have their pages (code and data) secured such that only the guest
> +       itself has access to the unencrypted version. Each encrypted VM is
> +       associated with a unique encryption key; if its data is accessed to a
> +       different entity using a different key the encrypted guests data will
> +       be incorrectly decrypted, leading to unintelligible data.

As said in the previous review I'd prefer if we'd agree on a less-vendor
specific XML for this.

> +       </p>
> +    <pre>
> +&lt;domain&gt;
> +  ...
> +  &lt;sev&gt;
> +    &lt;policy&gt; 1 &lt;/policy&gt;
> +    &lt;cbitpos&gt; 47 &lt;/cbitpos&gt;
> +    &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt;
> +    &lt;session&gt; ... &lt;/session&gt;
> +    &lt;dh-cert&gt; ... &lt;/dh&gt;
> +  &lt;/sev&gt;
> +  ...
> +&lt;/domain&gt;
> +</pre>
> +
> +    <p>
> +    A least <code>cbitpos</code> and <code>reduced-phys-bits</code> must be nested
> +    within the <code>sev</code> element.
> +    </p>
> +    <dl>
> +      <dt><code>cbitpos</code></dt>
> +      <dd>The <code>cbitpos</code> attribute provides the C-bit (aka encryption bit)
> +      location in guest page table entry. The value of <code>cbitpos</code> is
> +      hypervisor dependent and can be obtained through the <code>sev</code> element
> +      from domaincapabilities.

It sounds a bit more platform dependant. Is there any way where this
could be different from the data present in the capabilities? If not it
does not make much sense to allow configuring it.

Also does it change the machine ABI? I'm asking whether it's necessary
to expose this at all. (e.g. it's necessary to keep the value the same
accross migrations).


> +      </dd>
> +      <dt><code>reduced-phys-bits</code></dt>
> +      <dd>The <code>reduced-phys-bits</code> attribute provides the physical
> +      address bit reducation. Similar to <code>cbitpos</code> the value of <code>
> +      reduced-phys-bit</code> is hypervisor dependent and can be obtained
> +      through the <code>sev</code> element from domaincapabilities.

Same applies for this.

> +      </dd>
> +      <dt><code>policy</code></dt>
> +      <dd>The <code>policy</code> attribute provides the guest policy which must
> +      be maintained by the SEV firmware. This policy is enforced by the firmware
> +      and restricts what configuration and operational commands can be performed
> +      on this guest by the hypervisor. The guest policy provided during guest
> +      launch is bound to the guest and cannot be changed throughout the lifetime
> +      of the guest. The policy is also transmitted during snapshot and migration
> +      flows and enforced on the destination platform.

So this is a hex number. Any documentation on the possible values?

> +      </dd>
> +      <dt><code>dh-cert</code></dt>
> +      <dd>The <code>dh-cert</code> attribute provides the guest owners public
> +      Diffie-Hellman (DH) key. The key is used to negotiate a master secret
> +      key between the SEV firmware and guest owner. This master secret key is
> +      then used to establish a trusted channel between SEV firmware and guest
> +      owner. The value must be encoded in base64.
> +      </dd>
> +      <dt><code>session</code></dt>
> +      <dd>The <code>session</code> attribute provides the guest owners session
> +      blob defined in SEV API spec. The value must be encoded in base64.

This should also be more documented.

> +      </dd>
> +    </dl>
> +
> +    <p>Note: More information about <code>policy</code> bit definition, <code>
> +    dh</code> and <code>session</code> is available in SEV API spec.</p>

At least by providing the specification document. Preferably commiting
it to the libvirt repository so that it does not change URIs in the
future.

> +
>      <h2><a id="examples">Example configs</a></h2>
>  
>      <p>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d96b012b98f0..4c9921b5dca6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15539,6 +15539,61 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
>      return ret;
>  }
>  
> +static void
> +virDomainSevDefFree(virDomainSevDefPtr def)
> +{
> +    VIR_FREE(def->dh_cert);
> +    VIR_FREE(def->session);
> +
> +    VIR_FREE(def);
> +}
> +
> +static virDomainSevDefPtr
> +virDomainSevDefParseXML(xmlNodePtr sevNode,
> +                        xmlXPathContextPtr ctxt)
> +{
> +    char *tmp = NULL;
> +    xmlNodePtr save = ctxt->node;
> +    virDomainSevDefPtr def;
> +    unsigned long policy;
> +
> +    ctxt->node = sevNode;
> +
> +    if (VIR_ALLOC(def) < 0)
> +        return NULL;
> +
> +    if ((tmp = virXPathString("string(./dh-cert)", ctxt))) {
> +        if (VIR_STRDUP(def->dh_cert, tmp) < 0)
> +            goto error;
> +
> +        VIR_FREE(tmp);
> +    }
> +
> +    if ((tmp = virXPathString("string(./session)", ctxt))) {
> +        if (VIR_STRDUP(def->session, tmp) < 0)
> +            goto error;
> +
> +        VIR_FREE(tmp);
> +    }
> +
> +    if (virXPathULongHex("string(./policy)", ctxt, &policy) == 0) {
> +        def->policy = policy;
> +    } else {
> +        def->policy = -1;
> +    }
> +
> +    virXPathInt("string(./cbitpos)", ctxt, &def->cbitpos);
> +    virXPathInt("string(./reduced-phys-bits)", ctxt, &def->reduced_phys_bits);
> +
> +    ctxt->node = save;
> +    return def;
> +
> +error:
> +    VIR_FREE(tmp);
> +    virDomainSevDefFree(def);
> +    ctxt->node = save;
> +    return NULL;
> +}
>  
>  static virDomainMemoryDefPtr
>  virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
> @@ -20212,6 +20267,15 @@ virDomainDefParseXML(xmlDocPtr xml,
>      ctxt->node = node;
>      VIR_FREE(nodes);
>  
> +    /* Check for SEV feature */
> +    if ((n = virXPathNodeSet("./sev", ctxt, &nodes)) < 0)
> +        goto error;

virXPathNode since you are not interrested in more than one element.

> +
> +    if (n) {
> +        def->sev = virDomainSevDefParseXML(nodes[0], ctxt);
> +        VIR_FREE(nodes);
> +    }
> +
>      /* analysis of memory devices */
>      if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) < 0)
>          goto error;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 368f16f3fbf9..f0f267b28f40 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -142,6 +142,9 @@ typedef virDomainPanicDef *virDomainPanicDefPtr;
>  typedef struct _virDomainMemoryDef virDomainMemoryDef;
>  typedef virDomainMemoryDef *virDomainMemoryDefPtr;
>  
> +typedef struct _virDomainSevDef virDomainSevDef;
> +typedef virDomainSevDef *virDomainSevDefPtr;
> +
>  /* forward declarations virDomainChrSourceDef, required by
>   * virDomainNetDef
>   */
> @@ -2289,6 +2292,18 @@ struct _virDomainKeyWrapDef {
>      int dea; /* enum virTristateSwitch */
>  };
>  
> +typedef struct _virDomainSevDef virDomainSevDef;
> +typedef virDomainSevDef *virDomainSevDefPtr;
> +
> +struct _virDomainSevDef {
> +    char *dh_cert;
> +    char *session;
> +    int policy;
> +    int cbitpos;
> +    int reduced_phys_bits;
> +};
> +
> +
>  typedef enum {
>      VIR_DOMAIN_IOMMU_MODEL_INTEL,
>  
> @@ -2454,6 +2469,9 @@ struct _virDomainDef {
>  
>      virDomainKeyWrapDefPtr keywrap;
>  
> +    /* SEV-specific domain */
> +    virDomainSevDefPtr sev;
> +
>      /* Application-specific custom metadata */
>      xmlNodePtr metadata;
>  
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index fa0aa5d5c3d4..653bbe154332 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9663,6 +9663,80 @@ qemuBuildTPMCommandLine(virCommandPtr cmd,
>      return 0;
>  }
>  
> +static char *
> +qemuBuildSevCreateFile(const virDomainDef *def, const char *name, char *data)
> +{
> +    char *base = virGetUserConfigDirectory();
> +    char *configDir, *configFile;
> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> +    virUUIDFormat(def->uuid, uuidstr);
> +
> +    if (virAsprintf(&configDir, "%s/sev/%s", base, uuidstr) < 0)
> +        goto error;
> +    VIR_FREE(base);

I think we should put these in the VM file directory rather than into
the user config directory.

> +
> +    if (virFileMakePathWithMode(configDir, S_IRWXU) < 0) {
> +        virReportSystemError(errno, _("cannot create config directory '%s'"),
> +                             configDir);
> +        goto error;
> +    }
> +
> +    if (!(configFile = virFileBuildPath(configDir, name, ".base64")))
> +        goto error;
> +
> +    if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) {
> +        virReportSystemError(errno, _("failed to write data to config '%s'"),
> +                             configFile);
> +        goto error;
> +    }
> +
> +    return configFile;
> +
> +error:
> +    return NULL;
> +}
> +
> +static int
> +qemuBuildSevCommandLine(virCommandPtr cmd,
> +                        const virDomainDef *def)
> +{
> +    virDomainSevDefPtr sev = def->sev;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    virBuffer obj = VIR_BUFFER_INITIALIZER;
> +    char *dh_cert_file = NULL;
> +    char *session_file = NULL;
> +
> +    /* qemu accepts DH and session blob as file, create a temporary file */
> +    if (sev->dh_cert &&
> +        !(dh_cert_file = qemuBuildSevCreateFile(def, "dh_cert", sev->dh_cert)))
> +        return -1;
> +
> +    if (sev->session &&
> +        !(session_file = qemuBuildSevCreateFile(def, "session", sev->session)))
> +        return -1;

These files should not be created in the command line generator but
rather in 'qemuProcessPrepareHost'. Otherwise the test suite will leak
these.

> +
> +    virCommandAddArg(cmd, "-machine");
> +    virBufferAddLit(&buf, "memory-encryption=sev0");
> +    virCommandAddArgBuffer(cmd, &buf);
> +
> +    virCommandAddArg(cmd, "-object");
> +    virBufferAddLit(&obj, "sev-guest,id=sev0");

I don't see any capability check that qemu actually supports this
object. You'll need to add them ...

> +    if (sev->policy > 0)
> +        virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
> +    virBufferAsprintf(&obj, ",cbitpos=%d", sev->cbitpos);
> +    virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
> +    if (dh_cert_file)
> +        virBufferAsprintf(&obj, ",dh-cert-file=%s", dh_cert_file);
> +    if (session_file)
> +        virBufferAsprintf(&obj, ",session-file=%s", session_file);
> +    virCommandAddArgBuffer(cmd, &obj);
> +
> +    VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d dh=%s session=%s",
> +             sev->policy, sev->cbitpos, sev->reduced_phys_bits, dh_cert_file,
> +             session_file);

This should be right at the beginning of the func.

> +    return 0;
> +}
>  
>  static int
>  qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
> @@ -10108,6 +10182,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>      if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
>          goto error;
>  
> +    if (def->sev && qemuBuildSevCommandLine(cmd, def) < 0)
> +        goto error;
> +
>      if (snapshot)
>          virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);

Also this is lacking test cases for the qemuxml2argvtest,
qemuxml2xmltest/genericxml2xmltest.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02/27/2018 02:34 AM, Peter Krempa wrote:
> On Mon, Feb 26, 2018 at 11:53:35 -0600, Brijesh Singh wrote:
>> Secure Encrypted Virtualization (sev) element is used to provide the guest
>> owners input parameters used for creating an encrypted VM using AMD SEV
>> feature. SEV feature supports running encrypted VM under the control of
>> KVM. Encrypted VMs have their pages (code and data) secured such that only
>> the guest itself has access to the unencrypted version. Each encrypted VM
>> is associated with a unique encryption key; if its data is accessed to a
>> different entity using a different key the encrypted guests data will be
>> incorrectly decrypted, leading to unintelligible data.
>>
>> QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted
>> VMs. A typical command line
>>
>> # $QEMU ... \
>>     -machine memory-encryption=sev0 \
>>     -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \
>>     ...
> 
> A general question. How does this work with migration? Since you state
> that only the guest has access to unencrypted state I presume that the
> qemu process sees it only encrypted data. Thus the encryption keys need
> to be transferred to the other host machine.
> 

To protect the confidentiality of an SEV protected guest while in 
transit, its memory contents get encrypted with a key that can be 
recovered by the next platform to execute it. The SEV firmware
provides the interfaces to support this protection: SEND_START, 
SEND_UPDATE_DATA, and SEND_FINISH for the re-encrypting the memory 
contents in transit.

See SEV API spec [1]  Appendix A (migration flow)

[1] https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf

Please note that we do not send the keys from source to destination, 
instead data gets wrapped with transport key and unwrapped on 
destination. The transport keys are negotiated before we start the 
migration. I have not pushed any migration patches yet -- the patches 
will come after base SEV is accepted. Currently, we add a migration 
blocker in qemu for the SEV guest.


> Also this means that we need to make sure that the migration will not
> work if the destination host of the migration does not support this
> (you'll need to add a migration cookie flag for this). Or if migration
> is not supported at all we'll need to reject it right away.
> 

Do we need to do anything special in libvirt, or having migration 
blocker in QEMU is good enough ?


> This means that the virDomainMemoryPeek will probably need to be fixed
> to return error too.
> 

I didn't know about the virDomainMemoryPeek module but from the name it 
sounds like it read the guest memory contents, does this go through QEMU 
monitor command (e.g x/xp) or it peeks directly into guest memory ?

If request goes through the QEMU monitor and sev guest policy allows 
debugging then QEMU monitor command will able to use SEV debug APIs to 
access the guest memory.


>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++
>>   src/conf/domain_conf.c    | 64 +++++++++++++++++++++++++++++++++++++++
>>   src/conf/domain_conf.h    | 18 +++++++++++
>>   src/qemu/qemu_command.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 230 insertions(+)
> 
> It would be preferable if you'd split the qemu bits from the config
> part.
> 

Sure, I will do that.


>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 6fd2189cd2f4..d18e3fb1d976 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null
>>   
>>       <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
>>   
>> +    <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3>
>> +
>> +    <p>
>> +       The contents of the <code>sev</code> element is used to provide the
>> +       guest owners input used for creating an encrypted VM using the AMD
>> +       Secure Encrypted Virtualization (SEV) feature.
>> +
>> +       SEV is an extension to the AMD-V architecture which supports running
>> +       encrypted virtual machine (VMs) under the control of KVM. Encrypted
>> +       VMs have their pages (code and data) secured such that only the guest
>> +       itself has access to the unencrypted version. Each encrypted VM is
>> +       associated with a unique encryption key; if its data is accessed to a
>> +       different entity using a different key the encrypted guests data will
>> +       be incorrectly decrypted, leading to unintelligible data.
> 
> As said in the previous review I'd prefer if we'd agree on a less-vendor
> specific XML for this.


Inputs to the memory encryption engine is vendor specific, see my 
previous comment, if the below approach works then I am all for it

<memory-encryption type='sev'>
  <dh> ...</dh>
  ...
  ...
</memory-encryption>


> 
>> +       </p>
>> +    <pre>
>> +&lt;domain&gt;
>> +  ...
>> +  &lt;sev&gt;
>> +    &lt;policy&gt; 1 &lt;/policy&gt;
>> +    &lt;cbitpos&gt; 47 &lt;/cbitpos&gt;
>> +    &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt;
>> +    &lt;session&gt; ... &lt;/session&gt;
>> +    &lt;dh-cert&gt; ... &lt;/dh&gt;
>> +  &lt;/sev&gt;
>> +  ...
>> +&lt;/domain&gt;
>> +</pre>
>> +
>> +    <p>
>> +    A least <code>cbitpos</code> and <code>reduced-phys-bits</code> must be nested
>> +    within the <code>sev</code> element.
>> +    </p>
>> +    <dl>
>> +      <dt><code>cbitpos</code></dt>
>> +      <dd>The <code>cbitpos</code> attribute provides the C-bit (aka encryption bit)
>> +      location in guest page table entry. The value of <code>cbitpos</code> is
>> +      hypervisor dependent and can be obtained through the <code>sev</code> element
>> +      from domaincapabilities.
> 
> It sounds a bit more platform dependant. Is there any way where this
> could be different from the data present in the capabilities? If not it
> does not make much sense to allow configuring it.
> 


Yes, this is platform/hypervisor dependent. It will be same as what is 
returned from capabilities. The reason why I expose this to user so that 
we can handle migration cases in which source and destination 
HV/platform have different C-bit positions. I am not clear on how 
libvirt VM migration works, I am thinking that in migration case we 
simply send the source XML file to destination and ask libvirt to create 
a guest using the newly provided XML documents. If this is case, we 
should be providing the sources C-bit position. While creating the SEV 
guest we validate the requested C-bit position and if we can't work with 
requested c-bit position then we fail to create a guest. In other words, 
we if set cbitspos equal to what is present in capabilities then we are 
not communicating the C-bit properly. I am open to other suggestions.


> Also does it change the machine ABI? I'm asking whether it's necessary
> to expose this at all. (e.g. it's necessary to keep the value the same
> accross migrations).


See my comments, the value is host family dependent and if we are
migrating across different AMD family then its value will change.


> 
> 
>> +      </dd>
>> +      <dt><code>reduced-phys-bits</code></dt>
>> +      <dd>The <code>reduced-phys-bits</code> attribute provides the physical
>> +      address bit reducation. Similar to <code>cbitpos</code> the value of <code>
>> +      reduced-phys-bit</code> is hypervisor dependent and can be obtained
>> +      through the <code>sev</code> element from domaincapabilities.
> 
> Same applies for this.
> 
>> +      </dd>
>> +      <dt><code>policy</code></dt>
>> +      <dd>The <code>policy</code> attribute provides the guest policy which must
>> +      be maintained by the SEV firmware. This policy is enforced by the firmware
>> +      and restricts what configuration and operational commands can be performed
>> +      on this guest by the hypervisor. The guest policy provided during guest
>> +      launch is bound to the guest and cannot be changed throughout the lifetime
>> +      of the guest. The policy is also transmitted during snapshot and migration
>> +      flows and enforced on the destination platform.
> 
> So this is a hex number. Any documentation on the possible values?


I can provide the link of the complete documentation and if needed then 
I can document here.


> 
>> +      </dd>
>> +      <dt><code>dh-cert</code></dt>
>> +      <dd>The <code>dh-cert</code> attribute provides the guest owners public
>> +      Diffie-Hellman (DH) key. The key is used to negotiate a master secret
>> +      key between the SEV firmware and guest owner. This master secret key is
>> +      then used to establish a trusted channel between SEV firmware and guest
>> +      owner. The value must be encoded in base64.
>> +      </dd>
>> +      <dt><code>session</code></dt>
>> +      <dd>The <code>session</code> attribute provides the guest owners session
>> +      blob defined in SEV API spec. The value must be encoded in base64.
> 
> This should also be more documented.
> 

 From x86 software point of view this is a blob of input which comes 
from the guest owner and must be passed as-is. The blob definition may 
change from one SEV firmware to another hence I don't see any reason for 
documenting this. I will provide link to SEV spec in case if someone 
want to find the details on blob and what exactly it contains etc etc.



>> +      </dd>
>> +    </dl>
>> +
>> +    <p>Note: More information about <code>policy</code> bit definition, <code>
>> +    dh</code> and <code>session</code> is available in SEV API spec.</p>
> 
> At least by providing the specification document. Preferably commiting
> it to the libvirt repository so that it does not change URIs in the
> future.
> 

Will do, so far the link has been static and I am okay with putting it 
either in here or commit description or both.


>> +
>>       <h2><a id="examples">Example configs</a></h2>
>>   
>>       <p>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index d96b012b98f0..4c9921b5dca6 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -15539,6 +15539,61 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
>>       return ret;
>>   }
>>   
>> +static void
>> +virDomainSevDefFree(virDomainSevDefPtr def)
>> +{
>> +    VIR_FREE(def->dh_cert);
>> +    VIR_FREE(def->session);
>> +
>> +    VIR_FREE(def);
>> +}
>> +
>> +static virDomainSevDefPtr
>> +virDomainSevDefParseXML(xmlNodePtr sevNode,
>> +                        xmlXPathContextPtr ctxt)
>> +{
>> +    char *tmp = NULL;
>> +    xmlNodePtr save = ctxt->node;
>> +    virDomainSevDefPtr def;
>> +    unsigned long policy;
>> +
>> +    ctxt->node = sevNode;
>> +
>> +    if (VIR_ALLOC(def) < 0)
>> +        return NULL;
>> +
>> +    if ((tmp = virXPathString("string(./dh-cert)", ctxt))) {
>> +        if (VIR_STRDUP(def->dh_cert, tmp) < 0)
>> +            goto error;
>> +
>> +        VIR_FREE(tmp);
>> +    }
>> +
>> +    if ((tmp = virXPathString("string(./session)", ctxt))) {
>> +        if (VIR_STRDUP(def->session, tmp) < 0)
>> +            goto error;
>> +
>> +        VIR_FREE(tmp);
>> +    }
>> +
>> +    if (virXPathULongHex("string(./policy)", ctxt, &policy) == 0) {
>> +        def->policy = policy;
>> +    } else {
>> +        def->policy = -1;
>> +    }
>> +
>> +    virXPathInt("string(./cbitpos)", ctxt, &def->cbitpos);
>> +    virXPathInt("string(./reduced-phys-bits)", ctxt, &def->reduced_phys_bits);
>> +
>> +    ctxt->node = save;
>> +    return def;
>> +
>> +error:
>> +    VIR_FREE(tmp);
>> +    virDomainSevDefFree(def);
>> +    ctxt->node = save;
>> +    return NULL;
>> +}
>>   
>>   static virDomainMemoryDefPtr
>>   virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
>> @@ -20212,6 +20267,15 @@ virDomainDefParseXML(xmlDocPtr xml,
>>       ctxt->node = node;
>>       VIR_FREE(nodes);
>>   
>> +    /* Check for SEV feature */
>> +    if ((n = virXPathNodeSet("./sev", ctxt, &nodes)) < 0)
>> +        goto error;
> 
> virXPathNode since you are not interrested in more than one element.
> 


Got it.


>> +
>> +    if (n) {
>> +        def->sev = virDomainSevDefParseXML(nodes[0], ctxt);
>> +        VIR_FREE(nodes);
>> +    }
>> +
>>       /* analysis of memory devices */
>>       if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) < 0)
>>           goto error;
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 368f16f3fbf9..f0f267b28f40 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -142,6 +142,9 @@ typedef virDomainPanicDef *virDomainPanicDefPtr;
>>   typedef struct _virDomainMemoryDef virDomainMemoryDef;
>>   typedef virDomainMemoryDef *virDomainMemoryDefPtr;
>>   
>> +typedef struct _virDomainSevDef virDomainSevDef;
>> +typedef virDomainSevDef *virDomainSevDefPtr;
>> +
>>   /* forward declarations virDomainChrSourceDef, required by
>>    * virDomainNetDef
>>    */
>> @@ -2289,6 +2292,18 @@ struct _virDomainKeyWrapDef {
>>       int dea; /* enum virTristateSwitch */
>>   };
>>   
>> +typedef struct _virDomainSevDef virDomainSevDef;
>> +typedef virDomainSevDef *virDomainSevDefPtr;
>> +
>> +struct _virDomainSevDef {
>> +    char *dh_cert;
>> +    char *session;
>> +    int policy;
>> +    int cbitpos;
>> +    int reduced_phys_bits;
>> +};
>> +
>> +
>>   typedef enum {
>>       VIR_DOMAIN_IOMMU_MODEL_INTEL,
>>   
>> @@ -2454,6 +2469,9 @@ struct _virDomainDef {
>>   
>>       virDomainKeyWrapDefPtr keywrap;
>>   
>> +    /* SEV-specific domain */
>> +    virDomainSevDefPtr sev;
>> +
>>       /* Application-specific custom metadata */
>>       xmlNodePtr metadata;
>>   
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index fa0aa5d5c3d4..653bbe154332 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -9663,6 +9663,80 @@ qemuBuildTPMCommandLine(virCommandPtr cmd,
>>       return 0;
>>   }
>>   
>> +static char *
>> +qemuBuildSevCreateFile(const virDomainDef *def, const char *name, char *data)
>> +{
>> +    char *base = virGetUserConfigDirectory();
>> +    char *configDir, *configFile;
>> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
>> +
>> +    virUUIDFormat(def->uuid, uuidstr);
>> +
>> +    if (virAsprintf(&configDir, "%s/sev/%s", base, uuidstr) < 0)
>> +        goto error;
>> +    VIR_FREE(base);
> 
> I think we should put these in the VM file directory rather than into
> the user config directory.
> 

Sure.


>> +
>> +    if (virFileMakePathWithMode(configDir, S_IRWXU) < 0) {
>> +        virReportSystemError(errno, _("cannot create config directory '%s'"),
>> +                             configDir);
>> +        goto error;
>> +    }
>> +
>> +    if (!(configFile = virFileBuildPath(configDir, name, ".base64")))
>> +        goto error;
>> +
>> +    if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) {
>> +        virReportSystemError(errno, _("failed to write data to config '%s'"),
>> +                             configFile);
>> +        goto error;
>> +    }
>> +
>> +    return configFile;
>> +
>> +error:
>> +    return NULL;
>> +}
>> +
>> +static int
>> +qemuBuildSevCommandLine(virCommandPtr cmd,
>> +                        const virDomainDef *def)
>> +{
>> +    virDomainSevDefPtr sev = def->sev;
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +    virBuffer obj = VIR_BUFFER_INITIALIZER;
>> +    char *dh_cert_file = NULL;
>> +    char *session_file = NULL;
>> +
>> +    /* qemu accepts DH and session blob as file, create a temporary file */
>> +    if (sev->dh_cert &&
>> +        !(dh_cert_file = qemuBuildSevCreateFile(def, "dh_cert", sev->dh_cert)))
>> +        return -1;
>> +
>> +    if (sev->session &&
>> +        !(session_file = qemuBuildSevCreateFile(def, "session", sev->session)))
>> +        return -1;
> 
> These files should not be created in the command line generator but
> rather in 'qemuProcessPrepareHost'. Otherwise the test suite will leak
> these.
> 

Okay will do so.


>> +
>> +    virCommandAddArg(cmd, "-machine");
>> +    virBufferAddLit(&buf, "memory-encryption=sev0");
>> +    virCommandAddArgBuffer(cmd, &buf);
>> +
>> +    virCommandAddArg(cmd, "-object");
>> +    virBufferAddLit(&obj, "sev-guest,id=sev0");
> 
> I don't see any capability check that qemu actually supports this
> object. You'll need to add them ...


Will do that.


> 
>> +    if (sev->policy > 0)
>> +        virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
>> +    virBufferAsprintf(&obj, ",cbitpos=%d", sev->cbitpos);
>> +    virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
>> +    if (dh_cert_file)
>> +        virBufferAsprintf(&obj, ",dh-cert-file=%s", dh_cert_file);
>> +    if (session_file)
>> +        virBufferAsprintf(&obj, ",session-file=%s", session_file);
>> +    virCommandAddArgBuffer(cmd, &obj);
>> +
>> +    VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d dh=%s session=%s",
>> +             sev->policy, sev->cbitpos, sev->reduced_phys_bits, dh_cert_file,
>> +             session_file);
> 
> This should be right at the beginning of the func.
> 

Will do.


>> +    return 0;
>> +}
>>   
>>   static int
>>   qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
>> @@ -10108,6 +10182,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>>       if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
>>           goto error;
>>   
>> +    if (def->sev && qemuBuildSevCommandLine(cmd, def) < 0)
>> +        goto error;
>> +
>>       if (snapshot)
>>           virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
> 
> Also this is lacking test cases for the qemuxml2argvtest,
> qemuxml2xmltest/genericxml2xmltest.
> 

Will look at those tests.

-Brijesh

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

[...]

> >
> > > +      </dd>
> > > +      <dt><code>policy</code></dt>
> > > +      <dd>The <code>policy</code> attribute provides the guest policy which must
> > > +      be maintained by the SEV firmware. This policy is enforced by the firmware
> > > +      and restricts what configuration and operational commands can be performed
> > > +      on this guest by the hypervisor. The guest policy provided during guest
> > > +      launch is bound to the guest and cannot be changed throughout the lifetime
> > > +      of the guest. The policy is also transmitted during snapshot and migration
> > > +      flows and enforced on the destination platform.
> >
> > So this is a hex number. Any documentation on the possible values?
>
>
> I can provide the link of the complete documentation and if needed then I
> can document here.
>
>
> >
> > > +      </dd>
> > > +      <dt><code>dh-cert</code></dt>
> > > +      <dd>The <code>dh-cert</code> attribute provides the guest owners public
> > > +      Diffie-Hellman (DH) key. The key is used to negotiate a master secret
> > > +      key between the SEV firmware and guest owner. This master secret key is
> > > +      then used to establish a trusted channel between SEV firmware and guest
> > > +      owner. The value must be encoded in base64.
> > > +      </dd>
> > > +      <dt><code>session</code></dt>
> > > +      <dd>The <code>session</code> attribute provides the guest owners session
> > > +      blob defined in SEV API spec. The value must be encoded in base64.
> >
> > This should also be more documented.
> >
>
> From x86 software point of view this is a blob of input which comes from the
> guest owner and must be passed as-is. The blob definition may change from
> one SEV firmware to another hence I don't see any reason for documenting
> this. I will provide link to SEV spec in case if someone want to find the
> details on blob and what exactly it contains etc etc.
>
>
>
> > > +      </dd>
> > > +    </dl>
> > > +
> > > +    <p>Note: More information about <code>policy</code> bit definition, <code>
> > > +    dh</code> and <code>session</code> is available in SEV API spec.</p>
> >
> > At least by providing the specification document. Preferably commiting
> > it to the libvirt repository so that it does not change URIs in the
> > future.
> >
>
> Will do, so far the link has been static and I am okay with putting it
> either in here or commit description or both.

"so far" doesn't mean the link isn't going to die with newer revisions of SEV
and we really don't want to have dangling URIs in our documentation. However, I
don't agree with Peter here that we should be committing the spec into the
repo, the spec contains lots of details, many of whic are not necessarily
relevant to libvirt, I'd therefore suggest to document all the possible values
for each element here explicitly, adding a precise tag of which revision of SEV
this is compatible with and which version of libvirt introduced the support for
it in order to make absolutely clear what libvirt consumers can expect from us
enabling this feature, should AMD introduce more values in future revisions of
SEV, since everyone can use google to get to the current revision for any
particular details and differences.

Erik

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

On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote:
> Secure Encrypted Virtualization (sev) element is used to provide the guest
> owners input parameters used for creating an encrypted VM using AMD SEV
> feature. SEV feature supports running encrypted VM under the control of
> KVM. Encrypted VMs have their pages (code and data) secured such that only
> the guest itself has access to the unencrypted version. Each encrypted VM
> is associated with a unique encryption key; if its data is accessed to a
> different entity using a different key the encrypted guests data will be
> incorrectly decrypted, leading to unintelligible data.
> 
> QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted
> VMs. A typical command line
> 
> # $QEMU ... \
>    -machine memory-encryption=sev0 \
>    -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \
>    ...
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.c    | 64 +++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h    | 18 +++++++++++
>  src/qemu/qemu_command.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 230 insertions(+)

In general we'd expect to see additions to the test suite for any XML
changes. eg a qemuxml2xmltest and qemuxml2argvtest addition.

> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 6fd2189cd2f4..d18e3fb1d976 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null
>  
>      <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
>  
> +    <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3>
> +
> +    <p>
> +       The contents of the <code>sev</code> element is used to provide the
> +       guest owners input used for creating an encrypted VM using the AMD
> +       Secure Encrypted Virtualization (SEV) feature.
> +
> +       SEV is an extension to the AMD-V architecture which supports running
> +       encrypted virtual machine (VMs) under the control of KVM. Encrypted
> +       VMs have their pages (code and data) secured such that only the guest
> +       itself has access to the unencrypted version. Each encrypted VM is
> +       associated with a unique encryption key; if its data is accessed to a
> +       different entity using a different key the encrypted guests data will
> +       be incorrectly decrypted, leading to unintelligible data.
> +       </p>
> +    <pre>
> +&lt;domain&gt;
> +  ...
> +  &lt;sev&gt;
> +    &lt;policy&gt; 1 &lt;/policy&gt;
> +    &lt;cbitpos&gt; 47 &lt;/cbitpos&gt;
> +    &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt;
> +    &lt;session&gt; ... &lt;/session&gt;
> +    &lt;dh-cert&gt; ... &lt;/dh&gt;
> +  &lt;/sev&gt;

Minor nitpick - since this inheranted SEV specific, I think we could do
with having a generic top level element with a type=sev. eg

  <launch-security type="sev">
       <policy>...</policy>
       <cbitpos>..</cbitpos>
       ...etc...
  </launch>

then we can plug in custom data if other vendors invent competing
solutions to AMD's SEV.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

On 02/27/2018 05:10 AM, Daniel P. Berrangé wrote:
> On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote:
>> Secure Encrypted Virtualization (sev) element is used to provide the guest
>> owners input parameters used for creating an encrypted VM using AMD SEV
>> feature. SEV feature supports running encrypted VM under the control of
>> KVM. Encrypted VMs have their pages (code and data) secured such that only
>> the guest itself has access to the unencrypted version. Each encrypted VM
>> is associated with a unique encryption key; if its data is accessed to a
>> different entity using a different key the encrypted guests data will be
>> incorrectly decrypted, leading to unintelligible data.
>>
>> QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted
>> VMs. A typical command line
>>
>> # $QEMU ... \
>>     -machine memory-encryption=sev0 \
>>     -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \
>>     ...
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++
>>   src/conf/domain_conf.c    | 64 +++++++++++++++++++++++++++++++++++++++
>>   src/conf/domain_conf.h    | 18 +++++++++++
>>   src/qemu/qemu_command.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 230 insertions(+)
> 
> In general we'd expect to see additions to the test suite for any XML
> changes. eg a qemuxml2xmltest and qemuxml2argvtest addition.
> 


Sure, this is my first stab at libvirt and will look into getting 
familiar with test and add them in next round.


>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 6fd2189cd2f4..d18e3fb1d976 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null
>>   
>>       <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
>>   
>> +    <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3>
>> +
>> +    <p>
>> +       The contents of the <code>sev</code> element is used to provide the
>> +       guest owners input used for creating an encrypted VM using the AMD
>> +       Secure Encrypted Virtualization (SEV) feature.
>> +
>> +       SEV is an extension to the AMD-V architecture which supports running
>> +       encrypted virtual machine (VMs) under the control of KVM. Encrypted
>> +       VMs have their pages (code and data) secured such that only the guest
>> +       itself has access to the unencrypted version. Each encrypted VM is
>> +       associated with a unique encryption key; if its data is accessed to a
>> +       different entity using a different key the encrypted guests data will
>> +       be incorrectly decrypted, leading to unintelligible data.
>> +       </p>
>> +    <pre>
>> +&lt;domain&gt;
>> +  ...
>> +  &lt;sev&gt;
>> +    &lt;policy&gt; 1 &lt;/policy&gt;
>> +    &lt;cbitpos&gt; 47 &lt;/cbitpos&gt;
>> +    &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt;
>> +    &lt;session&gt; ... &lt;/session&gt;
>> +    &lt;dh-cert&gt; ... &lt;/dh&gt;
>> +  &lt;/sev&gt;
> 
> Minor nitpick - since this inheranted SEV specific, I think we could do
> with having a generic top level element with a type=sev. eg
> 
>    <launch-security type="sev">
>         <policy>...</policy>
>         <cbitpos>..</cbitpos>
>         ...etc...
>    </launch>
> 
> then we can plug in custom data if other vendors invent competing
> solutions to AMD's SEV.
> 

I am okay with this, how about <memory-encryption> instead of 
<launch-security>, are you okay with it ?


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

On Tue, Feb 27, 2018 at 11:07:25AM -0600, Brijesh Singh wrote:
> 
> 
> On 02/27/2018 05:10 AM, Daniel P. Berrangé wrote:
> > On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote:
> > > Secure Encrypted Virtualization (sev) element is used to provide the guest
> > > owners input parameters used for creating an encrypted VM using AMD SEV
> > > feature. SEV feature supports running encrypted VM under the control of
> > > KVM. Encrypted VMs have their pages (code and data) secured such that only
> > > the guest itself has access to the unencrypted version. Each encrypted VM
> > > is associated with a unique encryption key; if its data is accessed to a
> > > different entity using a different key the encrypted guests data will be
> > > incorrectly decrypted, leading to unintelligible data.
> > > 
> > > QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted
> > > VMs. A typical command line
> > > 
> > > # $QEMU ... \
> > >     -machine memory-encryption=sev0 \
> > >     -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \
> > >     ...
> > > 
> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > > ---
> > >   docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++
> > >   src/conf/domain_conf.c    | 64 +++++++++++++++++++++++++++++++++++++++
> > >   src/conf/domain_conf.h    | 18 +++++++++++
> > >   src/qemu/qemu_command.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++
> > >   4 files changed, 230 insertions(+)
> > 
> > In general we'd expect to see additions to the test suite for any XML
> > changes. eg a qemuxml2xmltest and qemuxml2argvtest addition.
> > 
> 
> 
> Sure, this is my first stab at libvirt and will look into getting familiar
> with test and add them in next round.
> 
> 
> > > 
> > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > > index 6fd2189cd2f4..d18e3fb1d976 100644
> > > --- a/docs/formatdomain.html.in
> > > +++ b/docs/formatdomain.html.in
> > > @@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null
> > >       <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
> > > +    <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3>
> > > +
> > > +    <p>
> > > +       The contents of the <code>sev</code> element is used to provide the
> > > +       guest owners input used for creating an encrypted VM using the AMD
> > > +       Secure Encrypted Virtualization (SEV) feature.
> > > +
> > > +       SEV is an extension to the AMD-V architecture which supports running
> > > +       encrypted virtual machine (VMs) under the control of KVM. Encrypted
> > > +       VMs have their pages (code and data) secured such that only the guest
> > > +       itself has access to the unencrypted version. Each encrypted VM is
> > > +       associated with a unique encryption key; if its data is accessed to a
> > > +       different entity using a different key the encrypted guests data will
> > > +       be incorrectly decrypted, leading to unintelligible data.
> > > +       </p>
> > > +    <pre>
> > > +&lt;domain&gt;
> > > +  ...
> > > +  &lt;sev&gt;
> > > +    &lt;policy&gt; 1 &lt;/policy&gt;
> > > +    &lt;cbitpos&gt; 47 &lt;/cbitpos&gt;
> > > +    &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt;
> > > +    &lt;session&gt; ... &lt;/session&gt;
> > > +    &lt;dh-cert&gt; ... &lt;/dh&gt;
> > > +  &lt;/sev&gt;
> > 
> > Minor nitpick - since this inheranted SEV specific, I think we could do
> > with having a generic top level element with a type=sev. eg
> > 
> >    <launch-security type="sev">
> >         <policy>...</policy>
> >         <cbitpos>..</cbitpos>
> >         ...etc...
> >    </launch>
> > 
> > then we can plug in custom data if other vendors invent competing
> > solutions to AMD's SEV.
> > 
> 
> I am okay with this, how about <memory-encryption> instead of
> <launch-security>, are you okay with it ?

Memory encryption is a very specific feature. It occurs to me that there
could in future be other features that use launch time validation, that
are not memory encryption related.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

On 02/27/2018 11:15 AM, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2018 at 11:07:25AM -0600, Brijesh Singh wrote:
>>
>>
>> On 02/27/2018 05:10 AM, Daniel P. Berrangé wrote:
>>> On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote:
>>>> Secure Encrypted Virtualization (sev) element is used to provide the guest
>>>> owners input parameters used for creating an encrypted VM using AMD SEV
>>>> feature. SEV feature supports running encrypted VM under the control of
>>>> KVM. Encrypted VMs have their pages (code and data) secured such that only
>>>> the guest itself has access to the unencrypted version. Each encrypted VM
>>>> is associated with a unique encryption key; if its data is accessed to a
>>>> different entity using a different key the encrypted guests data will be
>>>> incorrectly decrypted, leading to unintelligible data.
>>>>
>>>> QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted
>>>> VMs. A typical command line
>>>>
>>>> # $QEMU ... \
>>>>      -machine memory-encryption=sev0 \
>>>>      -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \
>>>>      ...
>>>>
>>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>>> ---
>>>>    docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++
>>>>    src/conf/domain_conf.c    | 64 +++++++++++++++++++++++++++++++++++++++
>>>>    src/conf/domain_conf.h    | 18 +++++++++++
>>>>    src/qemu/qemu_command.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    4 files changed, 230 insertions(+)
>>>
>>> In general we'd expect to see additions to the test suite for any XML
>>> changes. eg a qemuxml2xmltest and qemuxml2argvtest addition.
>>>
>>
>>
>> Sure, this is my first stab at libvirt and will look into getting familiar
>> with test and add them in next round.
>>
>>
>>>>
>>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>>> index 6fd2189cd2f4..d18e3fb1d976 100644
>>>> --- a/docs/formatdomain.html.in
>>>> +++ b/docs/formatdomain.html.in
>>>> @@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null
>>>>        <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
>>>> +    <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3>
>>>> +
>>>> +    <p>
>>>> +       The contents of the <code>sev</code> element is used to provide the
>>>> +       guest owners input used for creating an encrypted VM using the AMD
>>>> +       Secure Encrypted Virtualization (SEV) feature.
>>>> +
>>>> +       SEV is an extension to the AMD-V architecture which supports running
>>>> +       encrypted virtual machine (VMs) under the control of KVM. Encrypted
>>>> +       VMs have their pages (code and data) secured such that only the guest
>>>> +       itself has access to the unencrypted version. Each encrypted VM is
>>>> +       associated with a unique encryption key; if its data is accessed to a
>>>> +       different entity using a different key the encrypted guests data will
>>>> +       be incorrectly decrypted, leading to unintelligible data.
>>>> +       </p>
>>>> +    <pre>
>>>> +&lt;domain&gt;
>>>> +  ...
>>>> +  &lt;sev&gt;
>>>> +    &lt;policy&gt; 1 &lt;/policy&gt;
>>>> +    &lt;cbitpos&gt; 47 &lt;/cbitpos&gt;
>>>> +    &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt;
>>>> +    &lt;session&gt; ... &lt;/session&gt;
>>>> +    &lt;dh-cert&gt; ... &lt;/dh&gt;
>>>> +  &lt;/sev&gt;
>>>
>>> Minor nitpick - since this inheranted SEV specific, I think we could do
>>> with having a generic top level element with a type=sev. eg
>>>
>>>     <launch-security type="sev">
>>>          <policy>...</policy>
>>>          <cbitpos>..</cbitpos>
>>>          ...etc...
>>>     </launch>
>>>
>>> then we can plug in custom data if other vendors invent competing
>>> solutions to AMD's SEV.
>>>
>>
>> I am okay with this, how about <memory-encryption> instead of
>> <launch-security>, are you okay with it ?
> 
> Memory encryption is a very specific feature. It occurs to me that there
> could in future be other features that use launch time validation, that
> are not memory encryption related.
> 


Understood, I am good with launch-security tag. thanks

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

On Tue, Feb 27, 2018 at 05:15:30PM +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2018 at 11:07:25AM -0600, Brijesh Singh wrote:
> >
> >
> > On 02/27/2018 05:10 AM, Daniel P. Berrangé wrote:
> > > On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote:
> > > > Secure Encrypted Virtualization (sev) element is used to provide the guest
> > > > owners input parameters used for creating an encrypted VM using AMD SEV
> > > > feature. SEV feature supports running encrypted VM under the control of
> > > > KVM. Encrypted VMs have their pages (code and data) secured such that only
> > > > the guest itself has access to the unencrypted version. Each encrypted VM
> > > > is associated with a unique encryption key; if its data is accessed to a
> > > > different entity using a different key the encrypted guests data will be
> > > > incorrectly decrypted, leading to unintelligible data.
> > > >
> > > > QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted
> > > > VMs. A typical command line
> > > >
> > > > # $QEMU ... \
> > > >     -machine memory-encryption=sev0 \
> > > >     -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \
> > > >     ...
> > > >
> > > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > > > ---
> > > >   docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++
> > > >   src/conf/domain_conf.c    | 64 +++++++++++++++++++++++++++++++++++++++
> > > >   src/conf/domain_conf.h    | 18 +++++++++++
> > > >   src/qemu/qemu_command.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++
> > > >   4 files changed, 230 insertions(+)
> > >
> > > In general we'd expect to see additions to the test suite for any XML
> > > changes. eg a qemuxml2xmltest and qemuxml2argvtest addition.
> > >
> >
> >
> > Sure, this is my first stab at libvirt and will look into getting familiar
> > with test and add them in next round.
> >
> >
> > > >
> > > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > > > index 6fd2189cd2f4..d18e3fb1d976 100644
> > > > --- a/docs/formatdomain.html.in
> > > > +++ b/docs/formatdomain.html.in
> > > > @@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null
> > > >       <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
> > > > +    <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3>
> > > > +
> > > > +    <p>
> > > > +       The contents of the <code>sev</code> element is used to provide the
> > > > +       guest owners input used for creating an encrypted VM using the AMD
> > > > +       Secure Encrypted Virtualization (SEV) feature.
> > > > +
> > > > +       SEV is an extension to the AMD-V architecture which supports running
> > > > +       encrypted virtual machine (VMs) under the control of KVM. Encrypted
> > > > +       VMs have their pages (code and data) secured such that only the guest
> > > > +       itself has access to the unencrypted version. Each encrypted VM is
> > > > +       associated with a unique encryption key; if its data is accessed to a
> > > > +       different entity using a different key the encrypted guests data will
> > > > +       be incorrectly decrypted, leading to unintelligible data.
> > > > +       </p>
> > > > +    <pre>
> > > > +&lt;domain&gt;
> > > > +  ...
> > > > +  &lt;sev&gt;
> > > > +    &lt;policy&gt; 1 &lt;/policy&gt;
> > > > +    &lt;cbitpos&gt; 47 &lt;/cbitpos&gt;
> > > > +    &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt;
> > > > +    &lt;session&gt; ... &lt;/session&gt;
> > > > +    &lt;dh-cert&gt; ... &lt;/dh&gt;
> > > > +  &lt;/sev&gt;
> > >
> > > Minor nitpick - since this inheranted SEV specific, I think we could do
> > > with having a generic top level element with a type=sev. eg
> > >
> > >    <launch-security type="sev">
> > >         <policy>...</policy>
> > >         <cbitpos>..</cbitpos>
> > >         ...etc...
> > >    </launch>
> > >
> > > then we can plug in custom data if other vendors invent competing
> > > solutions to AMD's SEV.
> > >
> >
> > I am okay with this, how about <memory-encryption> instead of
> > <launch-security>, are you okay with it ?
>
> Memory encryption is a very specific feature. It occurs to me that there
> could in future be other features that use launch time validation, that
> are not memory encryption related.

<launch-security> is IMHO still rather specific than generic, since we might
need to enable features in the future, which might/might no rely on security,
but add additional attributes to the launch validation, in which case I think
going for something like <launch-control> or simply <launch> and having a
structure similar to the one below:

By having the separate <sev> element you can make the sub-elements depend on
this parent element, since you can't expect other vendors to favour <cbitpos>
which add burden to the documentation to make it clear. Of course, the price
you pay for this is a more complex XML structure.
<launch>
    <security>
        <sev>
            <sev_specific_elements/>
        </sev>
    </security>
    <other_security_unrelated_validation_options/>
</launch>

Erik

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

On Wed, Feb 28, 2018 at 10:34:51AM +0100, Erik Skultety wrote:
> On Tue, Feb 27, 2018 at 05:15:30PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Feb 27, 2018 at 11:07:25AM -0600, Brijesh Singh wrote:
> > >
> > >
> > > On 02/27/2018 05:10 AM, Daniel P. Berrangé wrote:
> > > > On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote:
> > > > > Secure Encrypted Virtualization (sev) element is used to provide the guest
> > > > > owners input parameters used for creating an encrypted VM using AMD SEV
> > > > > feature. SEV feature supports running encrypted VM under the control of
> > > > > KVM. Encrypted VMs have their pages (code and data) secured such that only
> > > > > the guest itself has access to the unencrypted version. Each encrypted VM
> > > > > is associated with a unique encryption key; if its data is accessed to a
> > > > > different entity using a different key the encrypted guests data will be
> > > > > incorrectly decrypted, leading to unintelligible data.
> > > > >
> > > > > QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted
> > > > > VMs. A typical command line
> > > > >
> > > > > # $QEMU ... \
> > > > >     -machine memory-encryption=sev0 \
> > > > >     -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \
> > > > >     ...
> > > > >
> > > > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > > > > ---
> > > > >   docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++
> > > > >   src/conf/domain_conf.c    | 64 +++++++++++++++++++++++++++++++++++++++
> > > > >   src/conf/domain_conf.h    | 18 +++++++++++
> > > > >   src/qemu/qemu_command.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > >   4 files changed, 230 insertions(+)
> > > >
> > > > In general we'd expect to see additions to the test suite for any XML
> > > > changes. eg a qemuxml2xmltest and qemuxml2argvtest addition.
> > > >
> > >
> > >
> > > Sure, this is my first stab at libvirt and will look into getting familiar
> > > with test and add them in next round.
> > >
> > >
> > > > >
> > > > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > > > > index 6fd2189cd2f4..d18e3fb1d976 100644
> > > > > --- a/docs/formatdomain.html.in
> > > > > +++ b/docs/formatdomain.html.in
> > > > > @@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null
> > > > >       <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
> > > > > +    <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3>
> > > > > +
> > > > > +    <p>
> > > > > +       The contents of the <code>sev</code> element is used to provide the
> > > > > +       guest owners input used for creating an encrypted VM using the AMD
> > > > > +       Secure Encrypted Virtualization (SEV) feature.
> > > > > +
> > > > > +       SEV is an extension to the AMD-V architecture which supports running
> > > > > +       encrypted virtual machine (VMs) under the control of KVM. Encrypted
> > > > > +       VMs have their pages (code and data) secured such that only the guest
> > > > > +       itself has access to the unencrypted version. Each encrypted VM is
> > > > > +       associated with a unique encryption key; if its data is accessed to a
> > > > > +       different entity using a different key the encrypted guests data will
> > > > > +       be incorrectly decrypted, leading to unintelligible data.
> > > > > +       </p>
> > > > > +    <pre>
> > > > > +&lt;domain&gt;
> > > > > +  ...
> > > > > +  &lt;sev&gt;
> > > > > +    &lt;policy&gt; 1 &lt;/policy&gt;
> > > > > +    &lt;cbitpos&gt; 47 &lt;/cbitpos&gt;
> > > > > +    &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt;
> > > > > +    &lt;session&gt; ... &lt;/session&gt;
> > > > > +    &lt;dh-cert&gt; ... &lt;/dh&gt;
> > > > > +  &lt;/sev&gt;
> > > >
> > > > Minor nitpick - since this inheranted SEV specific, I think we could do
> > > > with having a generic top level element with a type=sev. eg
> > > >
> > > >    <launch-security type="sev">
> > > >         <policy>...</policy>
> > > >         <cbitpos>..</cbitpos>
> > > >         ...etc...
> > > >    </launch>
> > > >
> > > > then we can plug in custom data if other vendors invent competing
> > > > solutions to AMD's SEV.
> > > >
> > >
> > > I am okay with this, how about <memory-encryption> instead of
> > > <launch-security>, are you okay with it ?
> >
> > Memory encryption is a very specific feature. It occurs to me that there
> > could in future be other features that use launch time validation, that
> > are not memory encryption related.
> 
> <launch-security> is IMHO still rather specific than generic, since we might
> need to enable features in the future, which might/might no rely on security,
> but add additional attributes to the launch validation, in which case I think
> going for something like <launch-control> or simply <launch> and having a
> structure similar to the one below:

Any kind of launch validation is ultimately security related in some
manner.

> By having the separate <sev> element you can make the sub-elements depend on
> this parent element, since you can't expect other vendors to favour <cbitpos>
> which add burden to the documentation to make it clear. Of course, the price
> you pay for this is a more complex XML structure.
> <launch>
>     <security>
>         <sev>
>             <sev_specific_elements/>
>         </sev>
>     </security>

This is not the way we usually do things - we wuld have a type="sev|..."
which determines what child elements are permitted, as illustrated in
the example above.

>     <other_security_unrelated_validation_options/>
> </launch>

I think the extra level of nesting is uneccessary

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

On Wed, Feb 28, 2018 at 09:40:11 +0000, Daniel Berrange wrote:
> On Wed, Feb 28, 2018 at 10:34:51AM +0100, Erik Skultety wrote:
> > On Tue, Feb 27, 2018 at 05:15:30PM +0000, Daniel P. Berrangé wrote:

[...]

> > By having the separate <sev> element you can make the sub-elements depend on
> > this parent element, since you can't expect other vendors to favour <cbitpos>
> > which add burden to the documentation to make it clear. Of course, the price
> > you pay for this is a more complex XML structure.

The parser can parse different things depending on the model name. Also
the schema has provisions for this. The only slightly more complicated
part is providing examples in the documentation, since you'll need to
repeat the block with different model.

> > <launch>
> >     <security>
> >         <sev>
> >             <sev_specific_elements/>
> >         </sev>
> >     </security>
> 
> This is not the way we usually do things - we wuld have a type="sev|..."
> which determines what child elements are permitted, as illustrated in
> the example above.
> 
> >     <other_security_unrelated_validation_options/>
> > </launch>
> 
> I think the extra level of nesting is uneccessary

Also new elements are ignored by older libvirt (since schema validation
is not turned on in all cases) while new values for an enum can be
properly validated and rejected.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

> Any kind of launch validation is ultimately security related in some
> manner.
>
> > By having the separate <sev> element you can make the sub-elements depend on
> > this parent element, since you can't expect other vendors to favour <cbitpos>
> > which add burden to the documentation to make it clear. Of course, the price
> > you pay for this is a more complex XML structure.
> > <launch>
> >     <security>
> >         <sev>
> >             <sev_specific_elements/>
> >         </sev>
> >     </security>
>
> This is not the way we usually do things - we wuld have a type="sev|..."
> which determines what child elements are permitted, as illustrated in
> the example above.

Oh, right. Also, having <sev> element wouldn't make it clear that you can only
have one type active, either 'sev' or some other solution.

Erik

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