1. Patchew
  2. Libvirt
  3. [PATCH v3 0/6] Support for launchSecurity type s390-pv
  4. View patch

[PATCH v3 3/6] conf: refactor launch security to allow more types

Boris Fiuczynski posted 6 patches 4 years, 7 months ago
There is a newer version of this series
[PATCH v3 3/6] conf: refactor launch security to allow more types
Posted by Boris Fiuczynski 4 years, 7 months ago 
Adding virDomainSecDef for general launch security data
and moving virDomainSEVDef as an element for SEV data.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
 src/conf/domain_conf.c      | 127 +++++++++++++++++++++++-------------
 src/conf/domain_conf.h      |  11 +++-
 src/conf/virconftypes.h     |   2 +
 src/qemu/qemu_cgroup.c      |   4 +-
 src/qemu/qemu_command.c     |  44 +++++++++++--
 src/qemu/qemu_driver.c      |   3 +-
 src/qemu/qemu_firmware.c    |  33 ++++++----
 src/qemu/qemu_namespace.c   |  20 ++++--
 src/qemu/qemu_process.c     |  33 ++++++++--
 src/qemu/qemu_validate.c    |  22 +++++--
 src/security/security_dac.c |   6 +-
 11 files changed, 218 insertions(+), 87 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 93ec50ff5d..2bd5210a16 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3502,6 +3502,19 @@ virDomainSEVDefFree(virDomainSEVDef *def)
     g_free(def);
 }
 
+
+void
+virDomainSecDefFree(virDomainSecDef *def)
+{
+    if (!def)
+        return;
+
+    virDomainSEVDefFree(def->sev);
+
+    g_free(def);
+}
+
+
 static void
 virDomainOSDefClear(virDomainOSDef *os)
 {
@@ -3703,7 +3716,7 @@ void virDomainDefFree(virDomainDef *def)
     if (def->namespaceData && def->ns.free)
         (def->ns.free)(def->namespaceData);
 
-    virDomainSEVDefFree(def->sev);
+    virDomainSecDefFree(def->sec);
 
     xmlFreeNode(def->metadata);
 
@@ -14720,57 +14733,72 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
 {
     VIR_XPATH_NODE_AUTORESTORE(ctxt)
     unsigned long policy;
-    g_autofree char *type = NULL;
     int rc = -1;
 
     g_autoptr(virDomainSEVDef) def = g_new0(virDomainSEVDef, 1);
 
     ctxt->node = sevNode;
 
-    if (!(type = virXMLPropString(sevNode, "type"))) {
+    if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
         virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("missing launch security type"));
+                       _("failed to get launch security policy for "
+                         "launch security type SEV"));
         return NULL;
     }
 
-    def->sectype = virDomainLaunchSecurityTypeFromString(type);
-    switch ((virDomainLaunchSecurity) def->sectype) {
-    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
-        if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
-            virReportError(VIR_ERR_XML_ERROR, "%s",
-                           _("failed to get launch security policy for "
-                             "launch security type SEV"));
-            return NULL;
-        }
+    /* the following attributes are platform dependent and if missing,
+     * we can autofill them from domain capabilities later
+     */
+    rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
+    if (rc == 0) {
+        def->haveCbitpos = true;
+    } else if (rc == -2) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("Invalid format for launch security cbitpos"));
+        return NULL;
+    }
 
-        /* the following attributes are platform dependent and if missing,
-         * we can autofill them from domain capabilities later
-         */
-        rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
-        if (rc == 0) {
-            def->haveCbitpos = true;
-        } else if (rc == -2) {
-            virReportError(VIR_ERR_XML_ERROR, "%s",
-                           _("Invalid format for launch security cbitpos"));
-            return NULL;
-        }
+    rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
+                      &def->reduced_phys_bits);
+    if (rc == 0) {
+        def->haveReducedPhysBits = true;
+    } else if (rc == -2) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("Invalid format for launch security "
+                         "reduced-phys-bits"));
+        return NULL;
+    }
 
-        rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
-                          &def->reduced_phys_bits);
-        if (rc == 0) {
-            def->haveReducedPhysBits = true;
-        } else if (rc == -2) {
-            virReportError(VIR_ERR_XML_ERROR, "%s",
-                           _("Invalid format for launch security "
-                             "reduced-phys-bits"));
-            return NULL;
-        }
+    def->policy = policy;
+    def->dh_cert = virXPathString("string(./dhCert)", ctxt);
+    def->session = virXPathString("string(./session)", ctxt);
+
+    return g_steal_pointer(&def);
+}
+
+
+static virDomainSecDef *
+virDomainSecDefParseXML(xmlNodePtr lsecNode,
+                        xmlXPathContextPtr ctxt)
+{
+    g_autoptr(virDomainSecDef) sec = g_new0(virDomainSecDef, 1);
+    g_autofree char *type = NULL;
 
-        def->policy = policy;
-        def->dh_cert = virXPathString("string(./dhCert)", ctxt);
-        def->session = virXPathString("string(./session)", ctxt);
+    ctxt->node = lsecNode;
 
-        return g_steal_pointer(&def);
+    if (!(type = virXMLPropString(lsecNode, "type"))) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("missing launch security type"));
+        return NULL;
+    }
+
+    sec->sectype = virDomainLaunchSecurityTypeFromString(type);
+    switch ((virDomainLaunchSecurity) sec->sectype) {
+    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
+        sec->sev = virDomainSEVDefParseXML(lsecNode, ctxt);
+        if (!sec->sev)
+            return NULL;
+        break;
     case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
     case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
     default:
@@ -14779,6 +14807,8 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
                        type);
         return NULL;
     }
+
+    return g_steal_pointer(&sec);
 }
 
 
@@ -20098,10 +20128,10 @@ virDomainDefParseXML(xmlDocPtr xml,
     ctxt->node = node;
     VIR_FREE(nodes);
 
-    /* Check for SEV feature */
+    /* Check for launch security e.g. SEV feature */
     if ((node = virXPathNode("./launchSecurity", ctxt)) != NULL) {
-        def->sev = virDomainSEVDefParseXML(node, ctxt);
-        if (!def->sev)
+        def->sec = virDomainSecDefParseXML(node, ctxt);
+        if (!def->sec)
             goto error;
     }
 
@@ -26832,15 +26862,19 @@ virDomainKeyWrapDefFormat(virBuffer *buf, virDomainKeyWrapDef *keywrap)
 
 
 static void
-virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev)
+virDomainSecDefFormat(virBuffer *buf, virDomainSecDef *sec)
 {
-    if (!sev)
+    if (!sec)
         return;
 
-    switch ((virDomainLaunchSecurity) sev->sectype) {
+    switch ((virDomainLaunchSecurity) sec->sectype) {
     case VIR_DOMAIN_LAUNCH_SECURITY_SEV: {
+        virDomainSEVDef *sev = sec->sev;
+        if (!sev)
+            return;
+
         virBufferAsprintf(buf, "<launchSecurity type='%s'>\n",
-                          virDomainLaunchSecurityTypeToString(sev->sectype));
+                          virDomainLaunchSecurityTypeToString(sec->sectype));
         virBufferAdjustIndent(buf, 2);
 
         if (sev->haveCbitpos)
@@ -26860,6 +26894,7 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev)
         virBufferAddLit(buf, "</launchSecurity>\n");
         break;
     }
+
     case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
     case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
         break;
@@ -28272,7 +28307,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
     if (def->keywrap)
         virDomainKeyWrapDefFormat(buf, def->keywrap);
 
-    virDomainSEVDefFormat(buf, def->sev);
+    virDomainSecDefFormat(buf, def->sec);
 
     if (def->namespaceData && def->ns.format) {
         if ((def->ns.format)(buf, def->namespaceData) < 0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 512c7c8bd7..fa7ab1895d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2651,7 +2651,6 @@ typedef enum {
 
 
 struct _virDomainSEVDef {
-    int sectype; /* enum virDomainLaunchSecurity */
     char *dh_cert;
     char *session;
     unsigned int policy;
@@ -2661,6 +2660,10 @@ struct _virDomainSEVDef {
     unsigned int reduced_phys_bits;
 };
 
+struct _virDomainSecDef {
+    int sectype; /* enum virDomainLaunchSecurity */
+    virDomainSEVDef *sev;
+};
 
 typedef enum {
     VIR_DOMAIN_IOMMU_MODEL_INTEL,
@@ -2871,8 +2874,8 @@ struct _virDomainDef {
 
     virDomainKeyWrapDef *keywrap;
 
-    /* SEV-specific domain */
-    virDomainSEVDef *sev;
+    /* launch security e.g. SEV */
+    virDomainSecDef *sec;
 
     /* Application-specific custom metadata */
     xmlNodePtr metadata;
@@ -3287,6 +3290,8 @@ void virDomainShmemDefFree(virDomainShmemDef *def);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainShmemDef, virDomainShmemDefFree);
 void virDomainSEVDefFree(virDomainSEVDef *def);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSEVDef, virDomainSEVDefFree);
+void virDomainSecDefFree(virDomainSecDef *def);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSecDef, virDomainSecDefFree);
 void virDomainDeviceDefFree(virDomainDeviceDef *def);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree);
diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
index b21068486e..21420ba8ea 100644
--- a/src/conf/virconftypes.h
+++ b/src/conf/virconftypes.h
@@ -202,6 +202,8 @@ typedef struct _virDomainResourceDef virDomainResourceDef;
 
 typedef struct _virDomainSEVDef virDomainSEVDef;
 
+typedef struct _virDomainSecDef virDomainSecDef;
+
 typedef struct _virDomainShmemDef virDomainShmemDef;
 
 typedef struct _virDomainSmartcardDef virDomainSmartcardDef;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 038d6478b2..f2d99abcfa 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -856,7 +856,9 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
             return -1;
     }
 
-    if (vm->def->sev && qemuSetupSEVCgroup(vm) < 0)
+    if (vm->def->sec &&
+        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV &&
+        qemuSetupSEVCgroup(vm) < 0)
         return -1;
 
     return 0;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ea513693f7..4135a8444a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6966,11 +6966,20 @@ qemuBuildMachineCommandLine(virCommand *cmd,
     if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
         qemuAppendLoadparmMachineParm(&buf, def);
 
-    if (def->sev) {
-        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) {
-            virBufferAddLit(&buf, ",confidential-guest-support=sev0");
-        } else {
-            virBufferAddLit(&buf, ",memory-encryption=sev0");
+    if (def->sec) {
+        switch ((virDomainLaunchSecurity) def->sec->sectype) {
+        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
+            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) {
+                virBufferAddLit(&buf, ",confidential-guest-support=sev0");
+            } else {
+                virBufferAddLit(&buf, ",memory-encryption=sev0");
+            }
+            break;
+        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
+            break;
+        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
+            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
+            return -1;
         }
     }
 
@@ -9860,6 +9869,29 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand *cmd,
     return 0;
 }
 
+
+static int
+qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
+                        virDomainSecDef *sec)
+{
+    if (!sec)
+        return 0;
+
+    switch ((virDomainLaunchSecurity) sec->sectype) {
+    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
+        return qemuBuildSEVCommandLine(vm, cmd, sec->sev);
+        break;
+    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
+        break;
+    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
+        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
+        return -1;
+    }
+
+    return 0;
+}
+
+
 static int
 qemuBuildVMCoreInfoCommandLine(virCommand *cmd,
                                const virDomainDef *def)
@@ -10559,7 +10591,7 @@ qemuBuildCommandLine(virQEMUDriver *driver,
     if (qemuBuildVMCoreInfoCommandLine(cmd, def) < 0)
         return NULL;
 
-    if (qemuBuildSEVCommandLine(vm, cmd, def->sev) < 0)
+    if (qemuBuildSecCommandLine(vm, cmd, def->sec) < 0)
         return NULL;
 
     if (snapshot)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 235f575901..9973875092 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19830,7 +19830,8 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
     if (virDomainGetLaunchSecurityInfoEnsureACL(domain->conn, vm->def) < 0)
         goto cleanup;
 
-    if (vm->def->sev) {
+    if (vm->def->sec &&
+        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
         if (qemuDomainGetSEVMeasurement(driver, vm, params, nparams, flags) < 0)
             goto cleanup;
     }
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index e17b024b06..6d1bab181e 100644
--- a/src/qemu/qemu_firmware.c
+++ b/src/qemu/qemu_firmware.c
@@ -1053,19 +1053,28 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
         return false;
     }
 
-    if (def->sev &&
-        def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
-        if (!supportsSEV) {
-            VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it",
-                      path);
-            return false;
-        }
+    if (def->sec) {
+        switch ((virDomainLaunchSecurity) def->sec->sectype) {
+        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
+            if (!supportsSEV) {
+                VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it",
+                          path);
+                return false;
+            }
 
-        if (def->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY &&
-            !supportsSEVES) {
-            VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it",
-                      path);
-            return false;
+            if (def->sec->sev &&
+                def->sec->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY &&
+                !supportsSEVES) {
+                VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it",
+                          path);
+                return false;
+            }
+            break;
+        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
+            break;
+        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
+            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
+            return -1;
         }
     }
 
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 98495e8ef8..35c8eb83fd 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -594,16 +594,26 @@ static int
 qemuDomainSetupLaunchSecurity(virDomainObj *vm,
                               GSList **paths)
 {
-    virDomainSEVDef *sev = vm->def->sev;
+    virDomainSecDef *sec = vm->def->sec;
 
-    if (!sev || sev->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV)
+    if (!sec)
         return 0;
 
-    VIR_DEBUG("Setting up launch security");
+    switch ((virDomainLaunchSecurity) sec->sectype) {
+    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
+        VIR_DEBUG("Setting up launch security for SEV");
 
-    *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV));
+        *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV));
+
+        VIR_DEBUG("Set up launch security for SEV");
+        break;
+    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
+        break;
+    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
+        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
+        return -1;
+    }
 
-    VIR_DEBUG("Set up launch security");
     return 0;
 }
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 2b03b0ab98..d9073fb3a3 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6480,7 +6480,7 @@ qemuProcessUpdateSEVInfo(virDomainObj *vm)
 {
     qemuDomainObjPrivate *priv = vm->privateData;
     virQEMUCaps *qemuCaps = priv->qemuCaps;
-    virDomainSEVDef *sev = vm->def->sev;
+    virDomainSEVDef *sev = vm->def->sec->sev;
     virSEVCapability *sevCaps = NULL;
 
     /* if platform specific info like 'cbitpos' and 'reducedPhysBits' have
@@ -6636,7 +6636,8 @@ qemuProcessPrepareDomain(virQEMUDriver *driver,
     for (i = 0; i < vm->def->nshmems; i++)
         qemuDomainPrepareShmemChardev(vm->def->shmems[i]);
 
-    if (vm->def->sev) {
+    if (vm->def->sec &&
+        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
         VIR_DEBUG("Updating SEV platform info");
         if (qemuProcessUpdateSEVInfo(vm) < 0)
             return -1;
@@ -6674,10 +6675,10 @@ qemuProcessSEVCreateFile(virDomainObj *vm,
 static int
 qemuProcessPrepareSEVGuestInput(virDomainObj *vm)
 {
-    virDomainSEVDef *sev = vm->def->sev;
+    virDomainSEVDef *sev = vm->def->sec->sev;
 
     if (!sev)
-        return 0;
+        return -1;
 
     VIR_DEBUG("Preparing SEV guest");
 
@@ -6695,6 +6696,28 @@ qemuProcessPrepareSEVGuestInput(virDomainObj *vm)
 }
 
 
+static int
+qemuProcessPrepareLaunchSecurityGuestInput(virDomainObj *vm)
+{
+    virDomainSecDef *sec = vm->def->sec;
+
+    if (!sec)
+        return 0;
+
+    switch ((virDomainLaunchSecurity) sec->sectype) {
+    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
+        return qemuProcessPrepareSEVGuestInput(vm);
+    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
+        break;
+    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
+        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
+        return -1;
+    }
+
+    return 0;
+}
+
+
 static int
 qemuProcessPrepareHostStorage(virQEMUDriver *driver,
                               virDomainObj *vm,
@@ -6874,7 +6897,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver,
     if (qemuExtDevicesPrepareHost(driver, vm) < 0)
         return -1;
 
-    if (qemuProcessPrepareSEVGuestInput(vm) < 0)
+    if (qemuProcessPrepareLaunchSecurityGuestInput(vm) < 0)
         return -1;
 
     return 0;
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 382473d03b..957dbc906c 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1214,12 +1214,22 @@ qemuValidateDomainDef(const virDomainDef *def,
     if (qemuValidateDomainDefPanic(def, qemuCaps) < 0)
         return -1;
 
-    if (def->sev &&
-        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("SEV launch security is not supported with "
-                         "this QEMU binary"));
-        return -1;
+    if (def->sec) {
+        switch ((virDomainLaunchSecurity) def->sec->sectype) {
+        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
+            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("SEV launch security is not supported with "
+                                 "this QEMU binary"));
+                return -1;
+            }
+            break;
+        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
+            break;
+        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
+            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
+            return -1;
+        }
     }
 
     if (def->naudios > 1 &&
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 4909107fcc..b874dd4ab6 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1958,7 +1958,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr,
             rc = -1;
     }
 
-    if (def->sev) {
+    if (def->sec &&
+        def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
         if (virSecurityDACRestoreSEVLabel(mgr, def) < 0)
             rc = -1;
     }
@@ -2165,7 +2166,8 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr,
             return -1;
     }
 
-    if (def->sev) {
+    if (def->sec &&
+        def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
         if (virSecurityDACSetSEVLabel(mgr, def) < 0)
             return -1;
     }
-- 
2.30.2
Re: [PATCH v3 3/6] conf: refactor launch security to allow more types
Posted by Daniel Henrique Barboza 4 years, 7 months ago 

On 6/22/21 10:10 AM, Boris Fiuczynski wrote:
> Adding virDomainSecDef for general launch security data
> and moving virDomainSEVDef as an element for SEV data.
> 
> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   src/conf/domain_conf.c      | 127 +++++++++++++++++++++++-------------
>   src/conf/domain_conf.h      |  11 +++-
>   src/conf/virconftypes.h     |   2 +
>   src/qemu/qemu_cgroup.c      |   4 +-
>   src/qemu/qemu_command.c     |  44 +++++++++++--
>   src/qemu/qemu_driver.c      |   3 +-
>   src/qemu/qemu_firmware.c    |  33 ++++++----
>   src/qemu/qemu_namespace.c   |  20 ++++--
>   src/qemu/qemu_process.c     |  33 ++++++++--
>   src/qemu/qemu_validate.c    |  22 +++++--
>   src/security/security_dac.c |   6 +-
>   11 files changed, 218 insertions(+), 87 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 93ec50ff5d..2bd5210a16 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3502,6 +3502,19 @@ virDomainSEVDefFree(virDomainSEVDef *def)
>       g_free(def);
>   }
>   
> +
> +void
> +virDomainSecDefFree(virDomainSecDef *def)
> +{
> +    if (!def)
> +        return;
> +
> +    virDomainSEVDefFree(def->sev);
> +
> +    g_free(def);
> +}
> +
> +
>   static void
>   virDomainOSDefClear(virDomainOSDef *os)
>   {
> @@ -3703,7 +3716,7 @@ void virDomainDefFree(virDomainDef *def)
>       if (def->namespaceData && def->ns.free)
>           (def->ns.free)(def->namespaceData);
>   
> -    virDomainSEVDefFree(def->sev);
> +    virDomainSecDefFree(def->sec);
>   
>       xmlFreeNode(def->metadata);
>   
> @@ -14720,57 +14733,72 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>   {
>       VIR_XPATH_NODE_AUTORESTORE(ctxt)
>       unsigned long policy;
> -    g_autofree char *type = NULL;
>       int rc = -1;
>   
>       g_autoptr(virDomainSEVDef) def = g_new0(virDomainSEVDef, 1);
>   
>       ctxt->node = sevNode;
>   
> -    if (!(type = virXMLPropString(sevNode, "type"))) {
> +    if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
>           virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("missing launch security type"));
> +                       _("failed to get launch security policy for "
> +                         "launch security type SEV"));
>           return NULL;
>       }
>   
> -    def->sectype = virDomainLaunchSecurityTypeFromString(type);
> -    switch ((virDomainLaunchSecurity) def->sectype) {
> -    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> -        if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("failed to get launch security policy for "
> -                             "launch security type SEV"));
> -            return NULL;
> -        }
> +    /* the following attributes are platform dependent and if missing,
> +     * we can autofill them from domain capabilities later
> +     */
> +    rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
> +    if (rc == 0) {
> +        def->haveCbitpos = true;
> +    } else if (rc == -2) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Invalid format for launch security cbitpos"));
> +        return NULL;
> +    }
>   
> -        /* the following attributes are platform dependent and if missing,
> -         * we can autofill them from domain capabilities later
> -         */
> -        rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
> -        if (rc == 0) {
> -            def->haveCbitpos = true;
> -        } else if (rc == -2) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("Invalid format for launch security cbitpos"));
> -            return NULL;
> -        }
> +    rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
> +                      &def->reduced_phys_bits);
> +    if (rc == 0) {
> +        def->haveReducedPhysBits = true;
> +    } else if (rc == -2) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Invalid format for launch security "
> +                         "reduced-phys-bits"));
> +        return NULL;
> +    }
>   
> -        rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
> -                          &def->reduced_phys_bits);
> -        if (rc == 0) {
> -            def->haveReducedPhysBits = true;
> -        } else if (rc == -2) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("Invalid format for launch security "
> -                             "reduced-phys-bits"));
> -            return NULL;
> -        }
> +    def->policy = policy;
> +    def->dh_cert = virXPathString("string(./dhCert)", ctxt);
> +    def->session = virXPathString("string(./session)", ctxt);
> +
> +    return g_steal_pointer(&def);
> +}
> +
> +
> +static virDomainSecDef *
> +virDomainSecDefParseXML(xmlNodePtr lsecNode,
> +                        xmlXPathContextPtr ctxt)
> +{
> +    g_autoptr(virDomainSecDef) sec = g_new0(virDomainSecDef, 1);
> +    g_autofree char *type = NULL;
>   
> -        def->policy = policy;
> -        def->dh_cert = virXPathString("string(./dhCert)", ctxt);
> -        def->session = virXPathString("string(./session)", ctxt);
> +    ctxt->node = lsecNode;
>   
> -        return g_steal_pointer(&def);
> +    if (!(type = virXMLPropString(lsecNode, "type"))) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("missing launch security type"));
> +        return NULL;
> +    }
> +
> +    sec->sectype = virDomainLaunchSecurityTypeFromString(type);
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +        sec->sev = virDomainSEVDefParseXML(lsecNode, ctxt);
> +        if (!sec->sev)
> +            return NULL;
> +        break;
>       case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>       case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>       default:
> @@ -14779,6 +14807,8 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>                          type);
>           return NULL;
>       }
> +
> +    return g_steal_pointer(&sec);
>   }
>   
>   
> @@ -20098,10 +20128,10 @@ virDomainDefParseXML(xmlDocPtr xml,
>       ctxt->node = node;
>       VIR_FREE(nodes);
>   
> -    /* Check for SEV feature */
> +    /* Check for launch security e.g. SEV feature */
>       if ((node = virXPathNode("./launchSecurity", ctxt)) != NULL) {
> -        def->sev = virDomainSEVDefParseXML(node, ctxt);
> -        if (!def->sev)
> +        def->sec = virDomainSecDefParseXML(node, ctxt);
> +        if (!def->sec)
>               goto error;
>       }
>   
> @@ -26832,15 +26862,19 @@ virDomainKeyWrapDefFormat(virBuffer *buf, virDomainKeyWrapDef *keywrap)
>   
>   
>   static void
> -virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev)
> +virDomainSecDefFormat(virBuffer *buf, virDomainSecDef *sec)
>   {
> -    if (!sev)
> +    if (!sec)
>           return;
>   
> -    switch ((virDomainLaunchSecurity) sev->sectype) {
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
>       case VIR_DOMAIN_LAUNCH_SECURITY_SEV: {
> +        virDomainSEVDef *sev = sec->sev;
> +        if (!sev)
> +            return;
> +
>           virBufferAsprintf(buf, "<launchSecurity type='%s'>\n",
> -                          virDomainLaunchSecurityTypeToString(sev->sectype));
> +                          virDomainLaunchSecurityTypeToString(sec->sectype));
>           virBufferAdjustIndent(buf, 2);
>   
>           if (sev->haveCbitpos)
> @@ -26860,6 +26894,7 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev)
>           virBufferAddLit(buf, "</launchSecurity>\n");
>           break;
>       }
> +
>       case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>       case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>           break;
> @@ -28272,7 +28307,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
>       if (def->keywrap)
>           virDomainKeyWrapDefFormat(buf, def->keywrap);
>   
> -    virDomainSEVDefFormat(buf, def->sev);
> +    virDomainSecDefFormat(buf, def->sec);
>   
>       if (def->namespaceData && def->ns.format) {
>           if ((def->ns.format)(buf, def->namespaceData) < 0)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 512c7c8bd7..fa7ab1895d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2651,7 +2651,6 @@ typedef enum {
>   
>   
>   struct _virDomainSEVDef {
> -    int sectype; /* enum virDomainLaunchSecurity */
>       char *dh_cert;
>       char *session;
>       unsigned int policy;
> @@ -2661,6 +2660,10 @@ struct _virDomainSEVDef {
>       unsigned int reduced_phys_bits;
>   };
>   
> +struct _virDomainSecDef {
> +    int sectype; /* enum virDomainLaunchSecurity */
> +    virDomainSEVDef *sev;
> +};
>   
>   typedef enum {
>       VIR_DOMAIN_IOMMU_MODEL_INTEL,
> @@ -2871,8 +2874,8 @@ struct _virDomainDef {
>   
>       virDomainKeyWrapDef *keywrap;
>   
> -    /* SEV-specific domain */
> -    virDomainSEVDef *sev;
> +    /* launch security e.g. SEV */
> +    virDomainSecDef *sec;
>   
>       /* Application-specific custom metadata */
>       xmlNodePtr metadata;
> @@ -3287,6 +3290,8 @@ void virDomainShmemDefFree(virDomainShmemDef *def);
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainShmemDef, virDomainShmemDefFree);
>   void virDomainSEVDefFree(virDomainSEVDef *def);
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSEVDef, virDomainSEVDefFree);
> +void virDomainSecDefFree(virDomainSecDef *def);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSecDef, virDomainSecDefFree);
>   void virDomainDeviceDefFree(virDomainDeviceDef *def);
>   
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree);
> diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
> index b21068486e..21420ba8ea 100644
> --- a/src/conf/virconftypes.h
> +++ b/src/conf/virconftypes.h
> @@ -202,6 +202,8 @@ typedef struct _virDomainResourceDef virDomainResourceDef;
>   
>   typedef struct _virDomainSEVDef virDomainSEVDef;
>   
> +typedef struct _virDomainSecDef virDomainSecDef;
> +
>   typedef struct _virDomainShmemDef virDomainShmemDef;
>   
>   typedef struct _virDomainSmartcardDef virDomainSmartcardDef;
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 038d6478b2..f2d99abcfa 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -856,7 +856,9 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
>               return -1;
>       }
>   
> -    if (vm->def->sev && qemuSetupSEVCgroup(vm) < 0)
> +    if (vm->def->sec &&
> +        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV &&
> +        qemuSetupSEVCgroup(vm) < 0)
>           return -1;
>   
>       return 0;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ea513693f7..4135a8444a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6966,11 +6966,20 @@ qemuBuildMachineCommandLine(virCommand *cmd,
>       if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
>           qemuAppendLoadparmMachineParm(&buf, def);
>   
> -    if (def->sev) {
> -        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) {
> -            virBufferAddLit(&buf, ",confidential-guest-support=sev0");
> -        } else {
> -            virBufferAddLit(&buf, ",memory-encryption=sev0");
> +    if (def->sec) {
> +        switch ((virDomainLaunchSecurity) def->sec->sectype) {
> +        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) {
> +                virBufferAddLit(&buf, ",confidential-guest-support=sev0");
> +            } else {
> +                virBufferAddLit(&buf, ",memory-encryption=sev0");
> +            }
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
> +            return -1;
>           }
>       }
>   
> @@ -9860,6 +9869,29 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand *cmd,
>       return 0;
>   }
>   
> +
> +static int
> +qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
> +                        virDomainSecDef *sec)
> +{
> +    if (!sec)
> +        return 0;
> +
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +        return qemuBuildSEVCommandLine(vm, cmd, sec->sev);
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>   static int
>   qemuBuildVMCoreInfoCommandLine(virCommand *cmd,
>                                  const virDomainDef *def)
> @@ -10559,7 +10591,7 @@ qemuBuildCommandLine(virQEMUDriver *driver,
>       if (qemuBuildVMCoreInfoCommandLine(cmd, def) < 0)
>           return NULL;
>   
> -    if (qemuBuildSEVCommandLine(vm, cmd, def->sev) < 0)
> +    if (qemuBuildSecCommandLine(vm, cmd, def->sec) < 0)
>           return NULL;
>   
>       if (snapshot)
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 235f575901..9973875092 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19830,7 +19830,8 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
>       if (virDomainGetLaunchSecurityInfoEnsureACL(domain->conn, vm->def) < 0)
>           goto cleanup;
>   
> -    if (vm->def->sev) {
> +    if (vm->def->sec &&
> +        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>           if (qemuDomainGetSEVMeasurement(driver, vm, params, nparams, flags) < 0)
>               goto cleanup;
>       }
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index e17b024b06..6d1bab181e 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -1053,19 +1053,28 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
>           return false;
>       }
>   
> -    if (def->sev &&
> -        def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
> -        if (!supportsSEV) {
> -            VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it",
> -                      path);
> -            return false;
> -        }
> +    if (def->sec) {
> +        switch ((virDomainLaunchSecurity) def->sec->sectype) {
> +        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +            if (!supportsSEV) {
> +                VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it",
> +                          path);
> +                return false;
> +            }
>   
> -        if (def->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY &&
> -            !supportsSEVES) {
> -            VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it",
> -                      path);
> -            return false;
> +            if (def->sec->sev &&
> +                def->sec->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY &&
> +                !supportsSEVES) {
> +                VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it",
> +                          path);
> +                return false;
> +            }
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
> +            return -1;
>           }
>       }
>   
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index 98495e8ef8..35c8eb83fd 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -594,16 +594,26 @@ static int
>   qemuDomainSetupLaunchSecurity(virDomainObj *vm,
>                                 GSList **paths)
>   {
> -    virDomainSEVDef *sev = vm->def->sev;
> +    virDomainSecDef *sec = vm->def->sec;
>   
> -    if (!sev || sev->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV)
> +    if (!sec)
>           return 0;
>   
> -    VIR_DEBUG("Setting up launch security");
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +        VIR_DEBUG("Setting up launch security for SEV");
>   
> -    *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV));
> +        *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV));
> +
> +        VIR_DEBUG("Set up launch security for SEV");
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
> +        return -1;
> +    }
>   
> -    VIR_DEBUG("Set up launch security");
>       return 0;
>   }
>   
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 2b03b0ab98..d9073fb3a3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6480,7 +6480,7 @@ qemuProcessUpdateSEVInfo(virDomainObj *vm)
>   {
>       qemuDomainObjPrivate *priv = vm->privateData;
>       virQEMUCaps *qemuCaps = priv->qemuCaps;
> -    virDomainSEVDef *sev = vm->def->sev;
> +    virDomainSEVDef *sev = vm->def->sec->sev;
>       virSEVCapability *sevCaps = NULL;
>   
>       /* if platform specific info like 'cbitpos' and 'reducedPhysBits' have
> @@ -6636,7 +6636,8 @@ qemuProcessPrepareDomain(virQEMUDriver *driver,
>       for (i = 0; i < vm->def->nshmems; i++)
>           qemuDomainPrepareShmemChardev(vm->def->shmems[i]);
>   
> -    if (vm->def->sev) {
> +    if (vm->def->sec &&
> +        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>           VIR_DEBUG("Updating SEV platform info");
>           if (qemuProcessUpdateSEVInfo(vm) < 0)
>               return -1;
> @@ -6674,10 +6675,10 @@ qemuProcessSEVCreateFile(virDomainObj *vm,
>   static int
>   qemuProcessPrepareSEVGuestInput(virDomainObj *vm)
>   {
> -    virDomainSEVDef *sev = vm->def->sev;
> +    virDomainSEVDef *sev = vm->def->sec->sev;
>   
>       if (!sev)
> -        return 0;
> +        return -1;
>   
>       VIR_DEBUG("Preparing SEV guest");
>   
> @@ -6695,6 +6696,28 @@ qemuProcessPrepareSEVGuestInput(virDomainObj *vm)
>   }
>   
>   
> +static int
> +qemuProcessPrepareLaunchSecurityGuestInput(virDomainObj *vm)
> +{
> +    virDomainSecDef *sec = vm->def->sec;
> +
> +    if (!sec)
> +        return 0;
> +
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +        return qemuProcessPrepareSEVGuestInput(vm);
> +    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>   static int
>   qemuProcessPrepareHostStorage(virQEMUDriver *driver,
>                                 virDomainObj *vm,
> @@ -6874,7 +6897,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver,
>       if (qemuExtDevicesPrepareHost(driver, vm) < 0)
>           return -1;
>   
> -    if (qemuProcessPrepareSEVGuestInput(vm) < 0)
> +    if (qemuProcessPrepareLaunchSecurityGuestInput(vm) < 0)
>           return -1;
>   
>       return 0;
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 382473d03b..957dbc906c 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -1214,12 +1214,22 @@ qemuValidateDomainDef(const virDomainDef *def,
>       if (qemuValidateDomainDefPanic(def, qemuCaps) < 0)
>           return -1;
>   
> -    if (def->sev &&
> -        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("SEV launch security is not supported with "
> -                         "this QEMU binary"));
> -        return -1;
> +    if (def->sec) {
> +        switch ((virDomainLaunchSecurity) def->sec->sectype) {
> +        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("SEV launch security is not supported with "
> +                                 "this QEMU binary"));
> +                return -1;
> +            }
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
> +            return -1;
> +        }
>       }
>   
>       if (def->naudios > 1 &&
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 4909107fcc..b874dd4ab6 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1958,7 +1958,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr,
>               rc = -1;
>       }
>   
> -    if (def->sev) {
> +    if (def->sec &&
> +        def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>           if (virSecurityDACRestoreSEVLabel(mgr, def) < 0)
>               rc = -1;
>       }
> @@ -2165,7 +2166,8 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr,
>               return -1;
>       }
>   
> -    if (def->sev) {
> +    if (def->sec &&
> +        def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>           if (virSecurityDACSetSEVLabel(mgr, def) < 0)
>               return -1;
>       }
>
Re: [PATCH v3 3/6] conf: refactor launch security to allow more types
Posted by Pavel Hrdina 4 years, 7 months ago 
On Tue, Jun 22, 2021 at 03:10:46PM +0200, Boris Fiuczynski wrote:
> Adding virDomainSecDef for general launch security data
> and moving virDomainSEVDef as an element for SEV data.
> 
> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
>  src/conf/domain_conf.c      | 127 +++++++++++++++++++++++-------------
>  src/conf/domain_conf.h      |  11 +++-
>  src/conf/virconftypes.h     |   2 +
>  src/qemu/qemu_cgroup.c      |   4 +-
>  src/qemu/qemu_command.c     |  44 +++++++++++--
>  src/qemu/qemu_driver.c      |   3 +-
>  src/qemu/qemu_firmware.c    |  33 ++++++----
>  src/qemu/qemu_namespace.c   |  20 ++++--
>  src/qemu/qemu_process.c     |  33 ++++++++--
>  src/qemu/qemu_validate.c    |  22 +++++--
>  src/security/security_dac.c |   6 +-
>  11 files changed, 218 insertions(+), 87 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 93ec50ff5d..2bd5210a16 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3502,6 +3502,19 @@ virDomainSEVDefFree(virDomainSEVDef *def)
>      g_free(def);
>  }
>  
> +
> +void
> +virDomainSecDefFree(virDomainSecDef *def)
> +{
> +    if (!def)
> +        return;
> +
> +    virDomainSEVDefFree(def->sev);
> +
> +    g_free(def);
> +}
> +
> +
>  static void
>  virDomainOSDefClear(virDomainOSDef *os)
>  {
> @@ -3703,7 +3716,7 @@ void virDomainDefFree(virDomainDef *def)
>      if (def->namespaceData && def->ns.free)
>          (def->ns.free)(def->namespaceData);
>  
> -    virDomainSEVDefFree(def->sev);
> +    virDomainSecDefFree(def->sec);
>  
>      xmlFreeNode(def->metadata);
>  
> @@ -14720,57 +14733,72 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>  {
>      VIR_XPATH_NODE_AUTORESTORE(ctxt)
>      unsigned long policy;
> -    g_autofree char *type = NULL;
>      int rc = -1;
>  
>      g_autoptr(virDomainSEVDef) def = g_new0(virDomainSEVDef, 1);
>  
>      ctxt->node = sevNode;
>  
> -    if (!(type = virXMLPropString(sevNode, "type"))) {
> +    if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
>          virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("missing launch security type"));
> +                       _("failed to get launch security policy for "
> +                         "launch security type SEV"));
>          return NULL;
>      }
>  
> -    def->sectype = virDomainLaunchSecurityTypeFromString(type);
> -    switch ((virDomainLaunchSecurity) def->sectype) {
> -    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> -        if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("failed to get launch security policy for "
> -                             "launch security type SEV"));
> -            return NULL;
> -        }
> +    /* the following attributes are platform dependent and if missing,
> +     * we can autofill them from domain capabilities later
> +     */
> +    rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
> +    if (rc == 0) {
> +        def->haveCbitpos = true;
> +    } else if (rc == -2) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Invalid format for launch security cbitpos"));
> +        return NULL;
> +    }
>  
> -        /* the following attributes are platform dependent and if missing,
> -         * we can autofill them from domain capabilities later
> -         */
> -        rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
> -        if (rc == 0) {
> -            def->haveCbitpos = true;
> -        } else if (rc == -2) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("Invalid format for launch security cbitpos"));
> -            return NULL;
> -        }
> +    rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
> +                      &def->reduced_phys_bits);
> +    if (rc == 0) {
> +        def->haveReducedPhysBits = true;
> +    } else if (rc == -2) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Invalid format for launch security "
> +                         "reduced-phys-bits"));
> +        return NULL;
> +    }
>  
> -        rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
> -                          &def->reduced_phys_bits);
> -        if (rc == 0) {
> -            def->haveReducedPhysBits = true;
> -        } else if (rc == -2) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("Invalid format for launch security "
> -                             "reduced-phys-bits"));
> -            return NULL;
> -        }
> +    def->policy = policy;
> +    def->dh_cert = virXPathString("string(./dhCert)", ctxt);
> +    def->session = virXPathString("string(./session)", ctxt);
> +
> +    return g_steal_pointer(&def);
> +}
> +
> +
> +static virDomainSecDef *
> +virDomainSecDefParseXML(xmlNodePtr lsecNode,
> +                        xmlXPathContextPtr ctxt)
> +{
> +    g_autoptr(virDomainSecDef) sec = g_new0(virDomainSecDef, 1);
> +    g_autofree char *type = NULL;
>  
> -        def->policy = policy;
> -        def->dh_cert = virXPathString("string(./dhCert)", ctxt);
> -        def->session = virXPathString("string(./session)", ctxt);
> +    ctxt->node = lsecNode;
>  
> -        return g_steal_pointer(&def);
> +    if (!(type = virXMLPropString(lsecNode, "type"))) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("missing launch security type"));
> +        return NULL;
> +    }
> +
> +    sec->sectype = virDomainLaunchSecurityTypeFromString(type);
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +        sec->sev = virDomainSEVDefParseXML(lsecNode, ctxt);
> +        if (!sec->sev)
> +            return NULL;
> +        break;
>      case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>      case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>      default:
> @@ -14779,6 +14807,8 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>                         type);
>          return NULL;
>      }
> +
> +    return g_steal_pointer(&sec);
>  }
>  
>  
> @@ -20098,10 +20128,10 @@ virDomainDefParseXML(xmlDocPtr xml,
>      ctxt->node = node;
>      VIR_FREE(nodes);
>  
> -    /* Check for SEV feature */
> +    /* Check for launch security e.g. SEV feature */
>      if ((node = virXPathNode("./launchSecurity", ctxt)) != NULL) {
> -        def->sev = virDomainSEVDefParseXML(node, ctxt);
> -        if (!def->sev)
> +        def->sec = virDomainSecDefParseXML(node, ctxt);
> +        if (!def->sec)
>              goto error;
>      }
>  
> @@ -26832,15 +26862,19 @@ virDomainKeyWrapDefFormat(virBuffer *buf, virDomainKeyWrapDef *keywrap)
>  
>  
>  static void
> -virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev)
> +virDomainSecDefFormat(virBuffer *buf, virDomainSecDef *sec)
>  {
> -    if (!sev)
> +    if (!sec)
>          return;
>  
> -    switch ((virDomainLaunchSecurity) sev->sectype) {
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
>      case VIR_DOMAIN_LAUNCH_SECURITY_SEV: {
> +        virDomainSEVDef *sev = sec->sev;
> +        if (!sev)
> +            return;
> +
>          virBufferAsprintf(buf, "<launchSecurity type='%s'>\n",
> -                          virDomainLaunchSecurityTypeToString(sev->sectype));
> +                          virDomainLaunchSecurityTypeToString(sec->sectype));
>          virBufferAdjustIndent(buf, 2);
>  
>          if (sev->haveCbitpos)
> @@ -26860,6 +26894,7 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev)
>          virBufferAddLit(buf, "</launchSecurity>\n");
>          break;
>      }
> +
>      case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>      case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>          break;
> @@ -28272,7 +28307,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
>      if (def->keywrap)
>          virDomainKeyWrapDefFormat(buf, def->keywrap);
>  
> -    virDomainSEVDefFormat(buf, def->sev);
> +    virDomainSecDefFormat(buf, def->sec);
>  
>      if (def->namespaceData && def->ns.format) {
>          if ((def->ns.format)(buf, def->namespaceData) < 0)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 512c7c8bd7..fa7ab1895d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2651,7 +2651,6 @@ typedef enum {
>  
>  
>  struct _virDomainSEVDef {
> -    int sectype; /* enum virDomainLaunchSecurity */
>      char *dh_cert;
>      char *session;
>      unsigned int policy;
> @@ -2661,6 +2660,10 @@ struct _virDomainSEVDef {
>      unsigned int reduced_phys_bits;
>  };
>  
> +struct _virDomainSecDef {
> +    int sectype; /* enum virDomainLaunchSecurity */
> +    virDomainSEVDef *sev;

I would rather use union here like we do in other similar internal
structures:

    struct _virDomainSecDef {
        int sectype; /* enum virDomainLaunchSecurity */
        union data {
            virDomainSEVDef sev;
        }
    }

or

    struct _virDomainSecDef {
        int sectype; /* enum virDomainLaunchSecurity */
        union data {
            virDomainSEVDef *sev;
        }
    }

depending if we need to have the specific SEV structure as pointer or
not based on its usage. I personally think we can do it without the
pointer as it should not happen that sectype will be set to SEV but we
will not have any data.

Pavel

> +};
>  
>  typedef enum {
>      VIR_DOMAIN_IOMMU_MODEL_INTEL,
> @@ -2871,8 +2874,8 @@ struct _virDomainDef {
>  
>      virDomainKeyWrapDef *keywrap;
>  
> -    /* SEV-specific domain */
> -    virDomainSEVDef *sev;
> +    /* launch security e.g. SEV */
> +    virDomainSecDef *sec;
>  
>      /* Application-specific custom metadata */
>      xmlNodePtr metadata;
> @@ -3287,6 +3290,8 @@ void virDomainShmemDefFree(virDomainShmemDef *def);
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainShmemDef, virDomainShmemDefFree);
>  void virDomainSEVDefFree(virDomainSEVDef *def);
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSEVDef, virDomainSEVDefFree);
> +void virDomainSecDefFree(virDomainSecDef *def);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSecDef, virDomainSecDefFree);
>  void virDomainDeviceDefFree(virDomainDeviceDef *def);
>  
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree);
> diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
> index b21068486e..21420ba8ea 100644
> --- a/src/conf/virconftypes.h
> +++ b/src/conf/virconftypes.h
> @@ -202,6 +202,8 @@ typedef struct _virDomainResourceDef virDomainResourceDef;
>  
>  typedef struct _virDomainSEVDef virDomainSEVDef;
>  
> +typedef struct _virDomainSecDef virDomainSecDef;
> +
>  typedef struct _virDomainShmemDef virDomainShmemDef;
>  
>  typedef struct _virDomainSmartcardDef virDomainSmartcardDef;
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 038d6478b2..f2d99abcfa 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -856,7 +856,9 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
>              return -1;
>      }
>  
> -    if (vm->def->sev && qemuSetupSEVCgroup(vm) < 0)
> +    if (vm->def->sec &&
> +        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV &&
> +        qemuSetupSEVCgroup(vm) < 0)
>          return -1;
>  
>      return 0;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ea513693f7..4135a8444a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6966,11 +6966,20 @@ qemuBuildMachineCommandLine(virCommand *cmd,
>      if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
>          qemuAppendLoadparmMachineParm(&buf, def);
>  
> -    if (def->sev) {
> -        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) {
> -            virBufferAddLit(&buf, ",confidential-guest-support=sev0");
> -        } else {
> -            virBufferAddLit(&buf, ",memory-encryption=sev0");
> +    if (def->sec) {
> +        switch ((virDomainLaunchSecurity) def->sec->sectype) {
> +        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) {
> +                virBufferAddLit(&buf, ",confidential-guest-support=sev0");
> +            } else {
> +                virBufferAddLit(&buf, ",memory-encryption=sev0");
> +            }
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
> +            return -1;
>          }
>      }
>  
> @@ -9860,6 +9869,29 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand *cmd,
>      return 0;
>  }
>  
> +
> +static int
> +qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
> +                        virDomainSecDef *sec)
> +{
> +    if (!sec)
> +        return 0;
> +
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +        return qemuBuildSEVCommandLine(vm, cmd, sec->sev);
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static int
>  qemuBuildVMCoreInfoCommandLine(virCommand *cmd,
>                                 const virDomainDef *def)
> @@ -10559,7 +10591,7 @@ qemuBuildCommandLine(virQEMUDriver *driver,
>      if (qemuBuildVMCoreInfoCommandLine(cmd, def) < 0)
>          return NULL;
>  
> -    if (qemuBuildSEVCommandLine(vm, cmd, def->sev) < 0)
> +    if (qemuBuildSecCommandLine(vm, cmd, def->sec) < 0)
>          return NULL;
>  
>      if (snapshot)
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 235f575901..9973875092 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19830,7 +19830,8 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
>      if (virDomainGetLaunchSecurityInfoEnsureACL(domain->conn, vm->def) < 0)
>          goto cleanup;
>  
> -    if (vm->def->sev) {
> +    if (vm->def->sec &&
> +        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>          if (qemuDomainGetSEVMeasurement(driver, vm, params, nparams, flags) < 0)
>              goto cleanup;
>      }
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index e17b024b06..6d1bab181e 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -1053,19 +1053,28 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
>          return false;
>      }
>  
> -    if (def->sev &&
> -        def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
> -        if (!supportsSEV) {
> -            VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it",
> -                      path);
> -            return false;
> -        }
> +    if (def->sec) {
> +        switch ((virDomainLaunchSecurity) def->sec->sectype) {
> +        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +            if (!supportsSEV) {
> +                VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it",
> +                          path);
> +                return false;
> +            }
>  
> -        if (def->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY &&
> -            !supportsSEVES) {
> -            VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it",
> -                      path);
> -            return false;
> +            if (def->sec->sev &&
> +                def->sec->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY &&
> +                !supportsSEVES) {
> +                VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it",
> +                          path);
> +                return false;
> +            }
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
> +            return -1;
>          }
>      }
>  
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index 98495e8ef8..35c8eb83fd 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -594,16 +594,26 @@ static int
>  qemuDomainSetupLaunchSecurity(virDomainObj *vm,
>                                GSList **paths)
>  {
> -    virDomainSEVDef *sev = vm->def->sev;
> +    virDomainSecDef *sec = vm->def->sec;
>  
> -    if (!sev || sev->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV)
> +    if (!sec)
>          return 0;
>  
> -    VIR_DEBUG("Setting up launch security");
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +        VIR_DEBUG("Setting up launch security for SEV");
>  
> -    *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV));
> +        *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV));
> +
> +        VIR_DEBUG("Set up launch security for SEV");
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
> +        return -1;
> +    }
>  
> -    VIR_DEBUG("Set up launch security");
>      return 0;
>  }
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 2b03b0ab98..d9073fb3a3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6480,7 +6480,7 @@ qemuProcessUpdateSEVInfo(virDomainObj *vm)
>  {
>      qemuDomainObjPrivate *priv = vm->privateData;
>      virQEMUCaps *qemuCaps = priv->qemuCaps;
> -    virDomainSEVDef *sev = vm->def->sev;
> +    virDomainSEVDef *sev = vm->def->sec->sev;
>      virSEVCapability *sevCaps = NULL;
>  
>      /* if platform specific info like 'cbitpos' and 'reducedPhysBits' have
> @@ -6636,7 +6636,8 @@ qemuProcessPrepareDomain(virQEMUDriver *driver,
>      for (i = 0; i < vm->def->nshmems; i++)
>          qemuDomainPrepareShmemChardev(vm->def->shmems[i]);
>  
> -    if (vm->def->sev) {
> +    if (vm->def->sec &&
> +        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>          VIR_DEBUG("Updating SEV platform info");
>          if (qemuProcessUpdateSEVInfo(vm) < 0)
>              return -1;
> @@ -6674,10 +6675,10 @@ qemuProcessSEVCreateFile(virDomainObj *vm,
>  static int
>  qemuProcessPrepareSEVGuestInput(virDomainObj *vm)
>  {
> -    virDomainSEVDef *sev = vm->def->sev;
> +    virDomainSEVDef *sev = vm->def->sec->sev;
>  
>      if (!sev)
> -        return 0;
> +        return -1;

This should not happen as we would abort if allocation of
virDomainSEVDef failed. In addition if we go with the union where the
data would not be a pointer there is no need for this check at all.

Pavel

>      VIR_DEBUG("Preparing SEV guest");
>  
> @@ -6695,6 +6696,28 @@ qemuProcessPrepareSEVGuestInput(virDomainObj *vm)
>  }
>  
>  
> +static int
> +qemuProcessPrepareLaunchSecurityGuestInput(virDomainObj *vm)
> +{
> +    virDomainSecDef *sec = vm->def->sec;
> +
> +    if (!sec)
> +        return 0;
> +
> +    switch ((virDomainLaunchSecurity) sec->sectype) {
> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +        return qemuProcessPrepareSEVGuestInput(vm);
> +    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +        break;
> +    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static int
>  qemuProcessPrepareHostStorage(virQEMUDriver *driver,
>                                virDomainObj *vm,
> @@ -6874,7 +6897,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver,
>      if (qemuExtDevicesPrepareHost(driver, vm) < 0)
>          return -1;
>  
> -    if (qemuProcessPrepareSEVGuestInput(vm) < 0)
> +    if (qemuProcessPrepareLaunchSecurityGuestInput(vm) < 0)
>          return -1;
>  
>      return 0;
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 382473d03b..957dbc906c 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -1214,12 +1214,22 @@ qemuValidateDomainDef(const virDomainDef *def,
>      if (qemuValidateDomainDefPanic(def, qemuCaps) < 0)
>          return -1;
>  
> -    if (def->sev &&
> -        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("SEV launch security is not supported with "
> -                         "this QEMU binary"));
> -        return -1;
> +    if (def->sec) {
> +        switch ((virDomainLaunchSecurity) def->sec->sectype) {
> +        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("SEV launch security is not supported with "
> +                                 "this QEMU binary"));
> +                return -1;
> +            }
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> +            break;
> +        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
> +            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
> +            return -1;
> +        }
>      }
>  
>      if (def->naudios > 1 &&
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 4909107fcc..b874dd4ab6 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1958,7 +1958,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr,
>              rc = -1;
>      }
>  
> -    if (def->sev) {
> +    if (def->sec &&
> +        def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>          if (virSecurityDACRestoreSEVLabel(mgr, def) < 0)
>              rc = -1;
>      }
> @@ -2165,7 +2166,8 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr,
>              return -1;
>      }
>  
> -    if (def->sev) {
> +    if (def->sec &&
> +        def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>          if (virSecurityDACSetSEVLabel(mgr, def) < 0)
>              return -1;
>      }
> -- 
> 2.30.2
>
Re: [PATCH v3 3/6] conf: refactor launch security to allow more types
Posted by Boris Fiuczynski 4 years, 7 months ago 
On 6/25/21 10:51 AM, Pavel Hrdina wrote:
> On Tue, Jun 22, 2021 at 03:10:46PM +0200, Boris Fiuczynski wrote:
>> Adding virDomainSecDef for general launch security data
>> and moving virDomainSEVDef as an element for SEV data.
>>
>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>>   src/conf/domain_conf.c      | 127 +++++++++++++++++++++++-------------
>>   src/conf/domain_conf.h      |  11 +++-
>>   src/conf/virconftypes.h     |   2 +
>>   src/qemu/qemu_cgroup.c      |   4 +-
>>   src/qemu/qemu_command.c     |  44 +++++++++++--
>>   src/qemu/qemu_driver.c      |   3 +-
>>   src/qemu/qemu_firmware.c    |  33 ++++++----
>>   src/qemu/qemu_namespace.c   |  20 ++++--
>>   src/qemu/qemu_process.c     |  33 ++++++++--
>>   src/qemu/qemu_validate.c    |  22 +++++--
>>   src/security/security_dac.c |   6 +-
>>   11 files changed, 218 insertions(+), 87 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 93ec50ff5d..2bd5210a16 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -3502,6 +3502,19 @@ virDomainSEVDefFree(virDomainSEVDef *def)
>>       g_free(def);
>>   }
>>   
>> +
>> +void
>> +virDomainSecDefFree(virDomainSecDef *def)
>> +{
>> +    if (!def)
>> +        return;
>> +
>> +    virDomainSEVDefFree(def->sev);
>> +
>> +    g_free(def);
>> +}
>> +
>> +
>>   static void
>>   virDomainOSDefClear(virDomainOSDef *os)
>>   {
>> @@ -3703,7 +3716,7 @@ void virDomainDefFree(virDomainDef *def)
>>       if (def->namespaceData && def->ns.free)
>>           (def->ns.free)(def->namespaceData);
>>   
>> -    virDomainSEVDefFree(def->sev);
>> +    virDomainSecDefFree(def->sec);
>>   
>>       xmlFreeNode(def->metadata);
>>   
>> @@ -14720,57 +14733,72 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>>   {
>>       VIR_XPATH_NODE_AUTORESTORE(ctxt)
>>       unsigned long policy;
>> -    g_autofree char *type = NULL;
>>       int rc = -1;
>>   
>>       g_autoptr(virDomainSEVDef) def = g_new0(virDomainSEVDef, 1);
>>   
>>       ctxt->node = sevNode;
>>   
>> -    if (!(type = virXMLPropString(sevNode, "type"))) {
>> +    if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
>>           virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                       _("missing launch security type"));
>> +                       _("failed to get launch security policy for "
>> +                         "launch security type SEV"));
>>           return NULL;
>>       }
>>   
>> -    def->sectype = virDomainLaunchSecurityTypeFromString(type);
>> -    switch ((virDomainLaunchSecurity) def->sectype) {
>> -    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
>> -        if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
>> -            virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                           _("failed to get launch security policy for "
>> -                             "launch security type SEV"));
>> -            return NULL;
>> -        }
>> +    /* the following attributes are platform dependent and if missing,
>> +     * we can autofill them from domain capabilities later
>> +     */
>> +    rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
>> +    if (rc == 0) {
>> +        def->haveCbitpos = true;
>> +    } else if (rc == -2) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("Invalid format for launch security cbitpos"));
>> +        return NULL;
>> +    }
>>   
>> -        /* the following attributes are platform dependent and if missing,
>> -         * we can autofill them from domain capabilities later
>> -         */
>> -        rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
>> -        if (rc == 0) {
>> -            def->haveCbitpos = true;
>> -        } else if (rc == -2) {
>> -            virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                           _("Invalid format for launch security cbitpos"));
>> -            return NULL;
>> -        }
>> +    rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
>> +                      &def->reduced_phys_bits);
>> +    if (rc == 0) {
>> +        def->haveReducedPhysBits = true;
>> +    } else if (rc == -2) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("Invalid format for launch security "
>> +                         "reduced-phys-bits"));
>> +        return NULL;
>> +    }
>>   
>> -        rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
>> -                          &def->reduced_phys_bits);
>> -        if (rc == 0) {
>> -            def->haveReducedPhysBits = true;
>> -        } else if (rc == -2) {
>> -            virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                           _("Invalid format for launch security "
>> -                             "reduced-phys-bits"));
>> -            return NULL;
>> -        }
>> +    def->policy = policy;
>> +    def->dh_cert = virXPathString("string(./dhCert)", ctxt);
>> +    def->session = virXPathString("string(./session)", ctxt);
>> +
>> +    return g_steal_pointer(&def);
>> +}
>> +
>> +
>> +static virDomainSecDef *
>> +virDomainSecDefParseXML(xmlNodePtr lsecNode,
>> +                        xmlXPathContextPtr ctxt)
>> +{
>> +    g_autoptr(virDomainSecDef) sec = g_new0(virDomainSecDef, 1);
>> +    g_autofree char *type = NULL;
>>   
>> -        def->policy = policy;
>> -        def->dh_cert = virXPathString("string(./dhCert)", ctxt);
>> -        def->session = virXPathString("string(./session)", ctxt);
>> +    ctxt->node = lsecNode;
>>   
>> -        return g_steal_pointer(&def);
>> +    if (!(type = virXMLPropString(lsecNode, "type"))) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("missing launch security type"));
>> +        return NULL;
>> +    }
>> +
>> +    sec->sectype = virDomainLaunchSecurityTypeFromString(type);
>> +    switch ((virDomainLaunchSecurity) sec->sectype) {
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
>> +        sec->sev = virDomainSEVDefParseXML(lsecNode, ctxt);
>> +        if (!sec->sev)
>> +            return NULL;
>> +        break;
>>       case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>>       case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>>       default:
>> @@ -14779,6 +14807,8 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>>                          type);
>>           return NULL;
>>       }
>> +
>> +    return g_steal_pointer(&sec);
>>   }
>>   
>>   
>> @@ -20098,10 +20128,10 @@ virDomainDefParseXML(xmlDocPtr xml,
>>       ctxt->node = node;
>>       VIR_FREE(nodes);
>>   
>> -    /* Check for SEV feature */
>> +    /* Check for launch security e.g. SEV feature */
>>       if ((node = virXPathNode("./launchSecurity", ctxt)) != NULL) {
>> -        def->sev = virDomainSEVDefParseXML(node, ctxt);
>> -        if (!def->sev)
>> +        def->sec = virDomainSecDefParseXML(node, ctxt);
>> +        if (!def->sec)
>>               goto error;
>>       }
>>   
>> @@ -26832,15 +26862,19 @@ virDomainKeyWrapDefFormat(virBuffer *buf, virDomainKeyWrapDef *keywrap)
>>   
>>   
>>   static void
>> -virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev)
>> +virDomainSecDefFormat(virBuffer *buf, virDomainSecDef *sec)
>>   {
>> -    if (!sev)
>> +    if (!sec)
>>           return;
>>   
>> -    switch ((virDomainLaunchSecurity) sev->sectype) {
>> +    switch ((virDomainLaunchSecurity) sec->sectype) {
>>       case VIR_DOMAIN_LAUNCH_SECURITY_SEV: {
>> +        virDomainSEVDef *sev = sec->sev;
>> +        if (!sev)
>> +            return;
>> +
>>           virBufferAsprintf(buf, "<launchSecurity type='%s'>\n",
>> -                          virDomainLaunchSecurityTypeToString(sev->sectype));
>> +                          virDomainLaunchSecurityTypeToString(sec->sectype));
>>           virBufferAdjustIndent(buf, 2);
>>   
>>           if (sev->haveCbitpos)
>> @@ -26860,6 +26894,7 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev)
>>           virBufferAddLit(buf, "</launchSecurity>\n");
>>           break;
>>       }
>> +
>>       case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>>       case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>>           break;
>> @@ -28272,7 +28307,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
>>       if (def->keywrap)
>>           virDomainKeyWrapDefFormat(buf, def->keywrap);
>>   
>> -    virDomainSEVDefFormat(buf, def->sev);
>> +    virDomainSecDefFormat(buf, def->sec);
>>   
>>       if (def->namespaceData && def->ns.format) {
>>           if ((def->ns.format)(buf, def->namespaceData) < 0)
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 512c7c8bd7..fa7ab1895d 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2651,7 +2651,6 @@ typedef enum {
>>   
>>   
>>   struct _virDomainSEVDef {
>> -    int sectype; /* enum virDomainLaunchSecurity */
>>       char *dh_cert;
>>       char *session;
>>       unsigned int policy;
>> @@ -2661,6 +2660,10 @@ struct _virDomainSEVDef {
>>       unsigned int reduced_phys_bits;
>>   };
>>   
>> +struct _virDomainSecDef {
>> +    int sectype; /* enum virDomainLaunchSecurity */
>> +    virDomainSEVDef *sev;
> 
> I would rather use union here like we do in other similar internal
> structures:
> 
>      struct _virDomainSecDef {
>          int sectype; /* enum virDomainLaunchSecurity */
>          union data {
>              virDomainSEVDef sev;
>          }
>      }
> 
> or
> 
>      struct _virDomainSecDef {
>          int sectype; /* enum virDomainLaunchSecurity */
>          union data {
>              virDomainSEVDef *sev;
>          }
>      }
> 
> depending if we need to have the specific SEV structure as pointer or
> not based on its usage. I personally think we can do it without the
> pointer as it should not happen that sectype will be set to SEV but we
> will not have any data.

OK, will change it.

> 
> Pavel
> 
>> +};
>>   
>>   typedef enum {
>>       VIR_DOMAIN_IOMMU_MODEL_INTEL,
>> @@ -2871,8 +2874,8 @@ struct _virDomainDef {
>>   
>>       virDomainKeyWrapDef *keywrap;
>>   
>> -    /* SEV-specific domain */
>> -    virDomainSEVDef *sev;
>> +    /* launch security e.g. SEV */
>> +    virDomainSecDef *sec;
>>   
>>       /* Application-specific custom metadata */
>>       xmlNodePtr metadata;
>> @@ -3287,6 +3290,8 @@ void virDomainShmemDefFree(virDomainShmemDef *def);
>>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainShmemDef, virDomainShmemDefFree);
>>   void virDomainSEVDefFree(virDomainSEVDef *def);
>>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSEVDef, virDomainSEVDefFree);
>> +void virDomainSecDefFree(virDomainSecDef *def);
>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSecDef, virDomainSecDefFree);
>>   void virDomainDeviceDefFree(virDomainDeviceDef *def);
>>   
>>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree);
>> diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
>> index b21068486e..21420ba8ea 100644
>> --- a/src/conf/virconftypes.h
>> +++ b/src/conf/virconftypes.h
>> @@ -202,6 +202,8 @@ typedef struct _virDomainResourceDef virDomainResourceDef;
>>   
>>   typedef struct _virDomainSEVDef virDomainSEVDef;
>>   
>> +typedef struct _virDomainSecDef virDomainSecDef;
>> +
>>   typedef struct _virDomainShmemDef virDomainShmemDef;
>>   
>>   typedef struct _virDomainSmartcardDef virDomainSmartcardDef;
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index 038d6478b2..f2d99abcfa 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -856,7 +856,9 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
>>               return -1;
>>       }
>>   
>> -    if (vm->def->sev && qemuSetupSEVCgroup(vm) < 0)
>> +    if (vm->def->sec &&
>> +        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV &&
>> +        qemuSetupSEVCgroup(vm) < 0)
>>           return -1;
>>   
>>       return 0;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index ea513693f7..4135a8444a 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -6966,11 +6966,20 @@ qemuBuildMachineCommandLine(virCommand *cmd,
>>       if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
>>           qemuAppendLoadparmMachineParm(&buf, def);
>>   
>> -    if (def->sev) {
>> -        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) {
>> -            virBufferAddLit(&buf, ",confidential-guest-support=sev0");
>> -        } else {
>> -            virBufferAddLit(&buf, ",memory-encryption=sev0");
>> +    if (def->sec) {
>> +        switch ((virDomainLaunchSecurity) def->sec->sectype) {
>> +        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
>> +            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) {
>> +                virBufferAddLit(&buf, ",confidential-guest-support=sev0");
>> +            } else {
>> +                virBufferAddLit(&buf, ",memory-encryption=sev0");
>> +            }
>> +            break;
>> +        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>> +            break;
>> +        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>> +            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
>> +            return -1;
>>           }
>>       }
>>   
>> @@ -9860,6 +9869,29 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand *cmd,
>>       return 0;
>>   }
>>   
>> +
>> +static int
>> +qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
>> +                        virDomainSecDef *sec)
>> +{
>> +    if (!sec)
>> +        return 0;
>> +
>> +    switch ((virDomainLaunchSecurity) sec->sectype) {
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
>> +        return qemuBuildSEVCommandLine(vm, cmd, sec->sev);
>> +        break;
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>> +        break;
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>> +        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>>   static int
>>   qemuBuildVMCoreInfoCommandLine(virCommand *cmd,
>>                                  const virDomainDef *def)
>> @@ -10559,7 +10591,7 @@ qemuBuildCommandLine(virQEMUDriver *driver,
>>       if (qemuBuildVMCoreInfoCommandLine(cmd, def) < 0)
>>           return NULL;
>>   
>> -    if (qemuBuildSEVCommandLine(vm, cmd, def->sev) < 0)
>> +    if (qemuBuildSecCommandLine(vm, cmd, def->sec) < 0)
>>           return NULL;
>>   
>>       if (snapshot)
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 235f575901..9973875092 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -19830,7 +19830,8 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
>>       if (virDomainGetLaunchSecurityInfoEnsureACL(domain->conn, vm->def) < 0)
>>           goto cleanup;
>>   
>> -    if (vm->def->sev) {
>> +    if (vm->def->sec &&
>> +        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>>           if (qemuDomainGetSEVMeasurement(driver, vm, params, nparams, flags) < 0)
>>               goto cleanup;
>>       }
>> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
>> index e17b024b06..6d1bab181e 100644
>> --- a/src/qemu/qemu_firmware.c
>> +++ b/src/qemu/qemu_firmware.c
>> @@ -1053,19 +1053,28 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
>>           return false;
>>       }
>>   
>> -    if (def->sev &&
>> -        def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>> -        if (!supportsSEV) {
>> -            VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it",
>> -                      path);
>> -            return false;
>> -        }
>> +    if (def->sec) {
>> +        switch ((virDomainLaunchSecurity) def->sec->sectype) {
>> +        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
>> +            if (!supportsSEV) {
>> +                VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it",
>> +                          path);
>> +                return false;
>> +            }
>>   
>> -        if (def->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY &&
>> -            !supportsSEVES) {
>> -            VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it",
>> -                      path);
>> -            return false;
>> +            if (def->sec->sev &&
>> +                def->sec->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY &&
>> +                !supportsSEVES) {
>> +                VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it",
>> +                          path);
>> +                return false;
>> +            }
>> +            break;
>> +        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>> +            break;
>> +        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>> +            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
>> +            return -1;
>>           }
>>       }
>>   
>> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
>> index 98495e8ef8..35c8eb83fd 100644
>> --- a/src/qemu/qemu_namespace.c
>> +++ b/src/qemu/qemu_namespace.c
>> @@ -594,16 +594,26 @@ static int
>>   qemuDomainSetupLaunchSecurity(virDomainObj *vm,
>>                                 GSList **paths)
>>   {
>> -    virDomainSEVDef *sev = vm->def->sev;
>> +    virDomainSecDef *sec = vm->def->sec;
>>   
>> -    if (!sev || sev->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV)
>> +    if (!sec)
>>           return 0;
>>   
>> -    VIR_DEBUG("Setting up launch security");
>> +    switch ((virDomainLaunchSecurity) sec->sectype) {
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
>> +        VIR_DEBUG("Setting up launch security for SEV");
>>   
>> -    *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV));
>> +        *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV));
>> +
>> +        VIR_DEBUG("Set up launch security for SEV");
>> +        break;
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>> +        break;
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>> +        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
>> +        return -1;
>> +    }
>>   
>> -    VIR_DEBUG("Set up launch security");
>>       return 0;
>>   }
>>   
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 2b03b0ab98..d9073fb3a3 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -6480,7 +6480,7 @@ qemuProcessUpdateSEVInfo(virDomainObj *vm)
>>   {
>>       qemuDomainObjPrivate *priv = vm->privateData;
>>       virQEMUCaps *qemuCaps = priv->qemuCaps;
>> -    virDomainSEVDef *sev = vm->def->sev;
>> +    virDomainSEVDef *sev = vm->def->sec->sev;
>>       virSEVCapability *sevCaps = NULL;
>>   
>>       /* if platform specific info like 'cbitpos' and 'reducedPhysBits' have
>> @@ -6636,7 +6636,8 @@ qemuProcessPrepareDomain(virQEMUDriver *driver,
>>       for (i = 0; i < vm->def->nshmems; i++)
>>           qemuDomainPrepareShmemChardev(vm->def->shmems[i]);
>>   
>> -    if (vm->def->sev) {
>> +    if (vm->def->sec &&
>> +        vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>>           VIR_DEBUG("Updating SEV platform info");
>>           if (qemuProcessUpdateSEVInfo(vm) < 0)
>>               return -1;
>> @@ -6674,10 +6675,10 @@ qemuProcessSEVCreateFile(virDomainObj *vm,
>>   static int
>>   qemuProcessPrepareSEVGuestInput(virDomainObj *vm)
>>   {
>> -    virDomainSEVDef *sev = vm->def->sev;
>> +    virDomainSEVDef *sev = vm->def->sec->sev;
>>   
>>       if (!sev)
>> -        return 0;
>> +        return -1;
> 
> This should not happen as we would abort if allocation of
> virDomainSEVDef failed. In addition if we go with the union where the
> data would not be a pointer there is no need for this check at all.

OK, I will remove it.

> 
> Pavel
> 
>>       VIR_DEBUG("Preparing SEV guest");
>>   
>> @@ -6695,6 +6696,28 @@ qemuProcessPrepareSEVGuestInput(virDomainObj *vm)
>>   }
>>   
>>   
>> +static int
>> +qemuProcessPrepareLaunchSecurityGuestInput(virDomainObj *vm)
>> +{
>> +    virDomainSecDef *sec = vm->def->sec;
>> +
>> +    if (!sec)
>> +        return 0;
>> +
>> +    switch ((virDomainLaunchSecurity) sec->sectype) {
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
>> +        return qemuProcessPrepareSEVGuestInput(vm);
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>> +        break;
>> +    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>> +        virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype);
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>>   static int
>>   qemuProcessPrepareHostStorage(virQEMUDriver *driver,
>>                                 virDomainObj *vm,
>> @@ -6874,7 +6897,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver,
>>       if (qemuExtDevicesPrepareHost(driver, vm) < 0)
>>           return -1;
>>   
>> -    if (qemuProcessPrepareSEVGuestInput(vm) < 0)
>> +    if (qemuProcessPrepareLaunchSecurityGuestInput(vm) < 0)
>>           return -1;
>>   
>>       return 0;
>> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
>> index 382473d03b..957dbc906c 100644
>> --- a/src/qemu/qemu_validate.c
>> +++ b/src/qemu/qemu_validate.c
>> @@ -1214,12 +1214,22 @@ qemuValidateDomainDef(const virDomainDef *def,
>>       if (qemuValidateDomainDefPanic(def, qemuCaps) < 0)
>>           return -1;
>>   
>> -    if (def->sev &&
>> -        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                       _("SEV launch security is not supported with "
>> -                         "this QEMU binary"));
>> -        return -1;
>> +    if (def->sec) {
>> +        switch ((virDomainLaunchSecurity) def->sec->sectype) {
>> +        case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
>> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
>> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                               _("SEV launch security is not supported with "
>> +                                 "this QEMU binary"));
>> +                return -1;
>> +            }
>> +            break;
>> +        case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
>> +            break;
>> +        case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>> +            virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
>> +            return -1;
>> +        }
>>       }
>>   
>>       if (def->naudios > 1 &&
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 4909107fcc..b874dd4ab6 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -1958,7 +1958,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr,
>>               rc = -1;
>>       }
>>   
>> -    if (def->sev) {
>> +    if (def->sec &&
>> +        def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>>           if (virSecurityDACRestoreSEVLabel(mgr, def) < 0)
>>               rc = -1;
>>       }
>> @@ -2165,7 +2166,8 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr,
>>               return -1;
>>       }
>>   
>> -    if (def->sev) {
>> +    if (def->sec &&
>> +        def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>>           if (virSecurityDACSetSEVLabel(mgr, def) < 0)
>>               return -1;
>>       }
>> -- 
>> 2.30.2
>>


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.