Re: [PATCH 2/7] conf: support crypto device

6 Jan 2023

On 1/4/23 04:29, zhenwei pi wrote:
...
Support a new device type 'crypto'.
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 src/conf/domain_conf.c         | 191 +++++++++++++++++++++++++++++++++
 src/conf/domain_conf.h         |  40 +++++++
 src/conf/domain_postparse.c    |   1 +
 src/conf/domain_validate.c     |  18 ++++
 src/conf/virconftypes.h        |   2 +
 src/libvirt_private.syms       |   1 +
 src/qemu/qemu_command.c        |   1 +
 src/qemu/qemu_domain.c         |   3 +
 src/qemu/qemu_domain_address.c |  26 +++++
 src/qemu/qemu_driver.c         |   5 +
 src/qemu/qemu_hotplug.c        |   3 +
 src/qemu/qemu_validate.c       |  22 ++++
 12 files changed, 313 insertions(+)
What I'm missing here is qemuxml2xmltest test case. We surely want to
test whether parsing and formatting of the new XML works.
...

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6c088ff295..74448fe627 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -332,6 +332,7 @@ VIR_ENUM_IMPL(virDomainDevice,
               "iommu",
               "vsock",
               "audio",
+              "crypto",
 );
VIR_ENUM_IMPL(virDomainDiskDevice,
@@ -1314,6 +1315,22 @@ VIR_ENUM_IMPL(virDomainVsockModel,
               "virtio-non-transitional",
 );
+VIR_ENUM_IMPL(virDomainCryptoModel,
+              VIR_DOMAIN_CRYPTO_MODEL_LAST,
+              "virtio",
+);
+
+VIR_ENUM_IMPL(virDomainCryptoType,
+              VIR_DOMAIN_CRYPTO_TYPE_LAST,
+              "qemu",
+);
+
+VIR_ENUM_IMPL(virDomainCryptoBackend,
+              VIR_DOMAIN_CRYPTO_BACKEND_LAST,
+              "builtin",
+              "lkcf",
+);
+
 VIR_ENUM_IMPL(virDomainDiskDiscard,
               VIR_DOMAIN_DISK_DISCARD_LAST,
               "default",
@@ -3464,6 +3481,9 @@ void virDomainDeviceDefFree(virDomainDeviceDef *def)
     case VIR_DOMAIN_DEVICE_AUDIO:
         virDomainAudioDefFree(def->data.audio);
         break;
+    case VIR_DOMAIN_DEVICE_CRYPTO:
+        virDomainCryptoDefFree(def->data.crypto);
+        break;
     case VIR_DOMAIN_DEVICE_LAST:
     case VIR_DOMAIN_DEVICE_NONE:
         break;
@@ -3807,6 +3827,10 @@ void virDomainDefFree(virDomainDef *def)
         virDomainPanicDefFree(def->panics[i]);
     g_free(def->panics);
+    for (i = 0; i < def->ncryptos; i++)
+        virDomainCryptoDefFree(def->cryptos[i]);
+    g_free(def->cryptos);
+
     virDomainIOMMUDefFree(def->iommu);
g_free(def->idmap.uidmap);
@@ -4360,6 +4384,8 @@ virDomainDeviceGetInfo(const virDomainDeviceDef *device)
         return &device->data.iommu->info;
     case VIR_DOMAIN_DEVICE_VSOCK:
         return &device->data.vsock->info;
+    case VIR_DOMAIN_DEVICE_CRYPTO:
+        return &device->data.crypto->info;
/* The following devices do not contain virDomainDeviceInfo */
     case VIR_DOMAIN_DEVICE_LEASE:
@@ -4462,6 +4488,9 @@ virDomainDeviceSetData(virDomainDeviceDef *device,
     case VIR_DOMAIN_DEVICE_AUDIO:
         device->data.audio = devicedata;
         break;
+    case VIR_DOMAIN_DEVICE_CRYPTO:
+        device->data.crypto = devicedata;
+        break;
     case VIR_DOMAIN_DEVICE_NONE:
     case VIR_DOMAIN_DEVICE_LAST:
         break;
@@ -4673,6 +4702,13 @@ virDomainDeviceInfoIterateFlags(virDomainDef *def,
             return rc;
     }
+    device.type = VIR_DOMAIN_DEVICE_CRYPTO;
+    for (i = 0; i < def->ncryptos; i++) {
+        device.data.crypto = def->cryptos[i];
+        if ((rc = cb(def, &device, &def->cryptos[i]->info, opaque)) != 0)
+            return rc;
+    }
+
     /* If the flag below is set, make sure @cb can handle @info being NULL */
     if (iteratorFlags & DOMAIN_DEVICE_ITERATE_MISSING_INFO) {
         device.type = VIR_DOMAIN_DEVICE_GRAPHICS;
@@ -4731,6 +4767,7 @@ virDomainDeviceInfoIterateFlags(virDomainDef *def,
     case VIR_DOMAIN_DEVICE_IOMMU:
     case VIR_DOMAIN_DEVICE_VSOCK:
     case VIR_DOMAIN_DEVICE_AUDIO:
+    case VIR_DOMAIN_DEVICE_CRYPTO:
         break;
     }
 #endif
@@ -13417,6 +13454,94 @@ virDomainVsockDefParseXML(virDomainXMLOption *xmlopt,
     return g_steal_pointer(&vsock);
 }
+
+static virDomainCryptoDef *
+virDomainCryptoDefParseXML(virDomainXMLOption *xmlopt,
+                           xmlNodePtr node,
+                           xmlXPathContextPtr ctxt,
+                           unsigned int flags)
+{
+    virDomainCryptoDef *def;
+    VIR_XPATH_NODE_AUTORESTORE(ctxt)
+    int nbackends;
+    g_autofree xmlNodePtr *backends = NULL;
+    g_autofree char *model = NULL;
+    g_autofree char *backend = NULL;
+    g_autofree char *type = NULL;
+
+    def = g_new0(virDomainCryptoDef, 1);
+
+    if (!(model = virXMLPropString(node, "model"))) {
+        virReportError(VIR_ERR_XML_ERROR, "%s", _("missing crypto device model"));
+        goto error;
+    }
+
+    if ((def->model = virDomainCryptoModelTypeFromString(model)) < 0) {
Problem with this is that compiler may decide that def->model is
unsigned (because it's declared as: virDomainCryptoModel model.
Now, if virXXXTypeFromString() fails and returns -1, this is then
typecased into unsigned int (or whatever unsigned type compiler decided
on) and < 0 check is never true.

Fortunately, we have a conencient function for getting attribute values
and translating them into enums: virXMLPropEnum().
...
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown crypto model '%s'"), model);
+        goto error;
+    }
+
+    if (!(type = virXMLPropString(node, "type"))) {
+        virReportError(VIR_ERR_XML_ERROR, "%s", _("missing crypto device type"));
+        goto error;
+    }
+
+    if ((def->type = virDomainCryptoTypeTypeFromString(type)) < 0) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown crypto type '%s'"), model);
+        goto error;
+    }
+
+    ctxt->node = node;
+
+    if ((nbackends = virXPathNodeSet("./backend", ctxt, &backends)) < 0)
+        goto error;
+
+    if (nbackends != 1) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("only one crypto backend is supported"));
+        goto error;
+    }
+
+    if (!(backend = virXMLPropString(backends[0], "model"))) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("missing crypto device backend model"));
+        goto error;
+    }
+
+    if ((def->backend = virDomainCryptoBackendTypeFromString(backend)) < 0) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unknown crypto backend model '%s'"), backend);
+        goto error;
+    }
+
+    if (virXMLPropUInt(backends[0], "queues", 10, VIR_XML_PROP_NONE, &def->queues) < 0) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("parsing crypto device queues failed"));
Nope, this overwrites more specific error message reported by
virXMLPropUInt().
...
+        goto error;
+    }
+
+    switch ((virDomainCryptoBackend) def->backend) {
+    case VIR_DOMAIN_CRYPTO_BACKEND_BUILTIN:
+    case VIR_DOMAIN_CRYPTO_BACKEND_LKCF:
+    case VIR_DOMAIN_CRYPTO_BACKEND_LAST:
+        break;
+    }
What's the purpose of this statement?
...
+
+    if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, &def->info, flags) < 0)
+        goto error;
+
+    if (virDomainVirtioOptionsParseXML(virXPathNode("./driver", ctxt),
+                                       &def->virtio) < 0)
+        goto error;
+
+    return def;
+
+ error:
+    g_clear_pointer(&def, virDomainCryptoDefFree);
How about declaring @def as g_autoptr() and dropping this label completely?
...
+    return NULL;
+}
+
+
 virDomainDeviceDef *
 virDomainDeviceDefParse(const char *xmlStr,
                         const virDomainDef *def,
@@ -13578,6 +13703,11 @@ virDomainDeviceDefParse(const char *xmlStr,
                                                           flags)))
             return NULL;
         break;
+    case VIR_DOMAIN_DEVICE_CRYPTO:
+        if (!(dev->data.crypto = virDomainCryptoDefParseXML(xmlopt, node, ctxt,
+                                                          flags)))
+            return NULL;
+        break;
     case VIR_DOMAIN_DEVICE_NONE:
     case VIR_DOMAIN_DEVICE_LAST:
         break;
@@ -18670,6 +18800,21 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,
     }
     VIR_FREE(nodes);
+    /* Parse the crypto devices */
+    if ((n = virXPathNodeSet("./devices/crypto", ctxt, &nodes)) < 0)
+        return NULL;
+    if (n)
+        def->cryptos = g_new0(virDomainCryptoDef *, n);
+    for (i = 0; i < n; i++) {
+        virDomainCryptoDef *crypto = virDomainCryptoDefParseXML(xmlopt, nodes[i],
+                                                       ctxt, flags);
+        if (!crypto)
+            return NULL;
+
+        def->cryptos[def->ncryptos++] = crypto;
+    }
+    VIR_FREE(nodes);
+
     /* Parse the TPM devices */
     if ((n = virXPathNodeSet("./devices/tpm", ctxt, &nodes)) < 0)
         return NULL;
@@ -21210,6 +21355,7 @@ virDomainDefCheckABIStabilityFlags(virDomainDef *src,
     case VIR_DOMAIN_DEVICE_IOMMU:
     case VIR_DOMAIN_DEVICE_VSOCK:
     case VIR_DOMAIN_DEVICE_AUDIO:
+    case VIR_DOMAIN_DEVICE_CRYPTO:
         break;
     }
 #endif
@@ -24562,6 +24708,47 @@ virDomainRNGDefFree(virDomainRNGDef *def)
 }
+static int
+virDomainCryptoDefFormat(virBuffer *buf,
+                         virDomainCryptoDef *def,
+                         unsigned int flags)
+{
+    const char *model = virDomainCryptoModelTypeToString(def->model);
+    const char *type = virDomainCryptoTypeTypeToString(def->model);
+    const char *backend = virDomainCryptoBackendTypeToString(def->backend);
+    g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER;
+
+    virBufferAsprintf(buf, "<crypto model='%s' type='%s'>\n", model, type);
+    virBufferAdjustIndent(buf, 2);
+    virBufferAsprintf(buf, "<backend model='%s'", backend);
+    if (def->queues)
+        virBufferAsprintf(buf, " queues='%d'", def->queues);
+    virBufferAddLit(buf, "/>\n");
+
+    virDomainVirtioOptionsFormat(&driverAttrBuf, def->virtio);
+
+    virXMLFormatElement(buf, "driver", &driverAttrBuf, NULL);
+
+    virDomainDeviceInfoFormat(buf, &def->info, flags);
+
+    virBufferAdjustIndent(buf, -2);
+    virBufferAddLit(buf, "</crypto>\n");
+
+    return 0;
This is all the function returns. Can this be made void instead?
...
+}
+
+void
+virDomainCryptoDefFree(virDomainCryptoDef *def)
+{
+    if (!def)
+        return;
+
+    virDomainDeviceInfoClear(&def->info);
+    g_free(def->virtio);
+    g_free(def);
+}
+
+
 static int
 virDomainMemorySourceDefFormat(virBuffer *buf,
                                virDomainMemoryDef *def)
@@ -27261,6 +27448,10 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
             return -1;
     }
+    for (n = 0; n < def->ncryptos; n++) {
+        if (virDomainCryptoDefFormat(buf, def->cryptos[n], flags))
+            return -1;
+    }
     if (def->iommu)
         virDomainIOMMUDefFormat(buf, def->iommu);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1404c55053..9062250d60 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -86,6 +86,7 @@ typedef enum {
     VIR_DOMAIN_DEVICE_IOMMU,
     VIR_DOMAIN_DEVICE_VSOCK,
     VIR_DOMAIN_DEVICE_AUDIO,
+    VIR_DOMAIN_DEVICE_CRYPTO,
VIR_DOMAIN_DEVICE_LAST
 } virDomainDeviceType;
@@ -118,6 +119,7 @@ struct _virDomainDeviceDef {
         virDomainIOMMUDef *iommu;
         virDomainVsockDef *vsock;
         virDomainAudioDef *audio;
+        virDomainCryptoDef *crypto;
     } data;
 };
@@ -2858,6 +2860,34 @@ struct _virDomainVsockDef {
     virDomainVirtioOptions *virtio;
 };
+typedef enum {
+    VIR_DOMAIN_CRYPTO_MODEL_VIRTIO,
+
+    VIR_DOMAIN_CRYPTO_MODEL_LAST
+} virDomainCryptoModel;
+
+typedef enum {
+    VIR_DOMAIN_CRYPTO_TYPE_QEMU,
+
+    VIR_DOMAIN_CRYPTO_TYPE_LAST
+} virDomainCryptoType;
+
+typedef enum {
+    VIR_DOMAIN_CRYPTO_BACKEND_BUILTIN,
+    VIR_DOMAIN_CRYPTO_BACKEND_LKCF,
+
+    VIR_DOMAIN_CRYPTO_BACKEND_LAST
+} virDomainCryptoBackend;
+
+struct _virDomainCryptoDef {
+    virDomainCryptoModel model;
+    virDomainCryptoType type;
+    virDomainCryptoBackend backend;
+    unsigned int queues;
+    virDomainDeviceInfo info;
+    virDomainVirtioOptions *virtio;
+};
+
 struct _virDomainVirtioOptions {
     virTristateSwitch iommu;
     virTristateSwitch ats;
@@ -3023,6 +3053,9 @@ struct _virDomainDef {
     size_t nsysinfo;
     virSysinfoDef **sysinfo;
+    size_t ncryptos;
+    virDomainCryptoDef **cryptos;
+
     /* At maximum 2 TPMs on the domain if a TPM Proxy is present. */
     size_t ntpms;
     virDomainTPMDef **tpms;
@@ -3274,6 +3307,7 @@ struct _virDomainXMLPrivateDataCallbacks {
     virDomainXMLPrivateDataNewFunc    vcpuNew;
     virDomainXMLPrivateDataNewFunc    chrSourceNew;
     virDomainXMLPrivateDataNewFunc    vsockNew;
+    virDomainXMLPrivateDataNewFunc    cryptoNew;
     virDomainXMLPrivateDataNewFunc    graphicsNew;
     virDomainXMLPrivateDataNewFunc    networkNew;
     virDomainXMLPrivateDataNewFunc    videoNew;
@@ -3440,6 +3474,9 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainIOMMUDef, virDomainIOMMUDefFree);
 virDomainVsockDef *virDomainVsockDefNew(virDomainXMLOption *xmlopt);
 void virDomainVsockDefFree(virDomainVsockDef *vsock);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainVsockDef, virDomainVsockDefFree);
+virDomainCryptoDef *virDomainCryptoDefNew(virDomainXMLOption *xmlopt);
This function is never defined, only declared here.
...
+void virDomainCryptoDefFree(virDomainCryptoDef *crypto);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainCryptoDef, virDomainCryptoDefFree);
 void virDomainNetTeamingInfoFree(virDomainNetTeamingInfo *teaming);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainNetTeamingInfo, virDomainNetTeamingInfoFree);
 void virDomainNetDefFree(virDomainNetDef *def);
Michal

    

Re: [PATCH 2/7] conf: support crypto device

Michal Prívozník