On 1/4/23 04:29, zhenwei pi wrote:
> Support a new device type 'crypto'.
>
> Signed-off-by: zhenwei pi <pizhen...@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
