2:21 a.m.
On Sun, Apr 27, 2025 at 19:48:03 +0800, honglei.wang(a)smartx.com wrote:
From: ray <honglei.wang(a)smartx.com>
This patch extends the disk bus support by introducing a new nvme-ns bus type.
The nvme-ns bus disk needs to be attached to nvme controller. A controller
can contain multiple nvme-ns disk devices.
Signed-off-by: ray <honglei.wang(a)smartx.com>
---
src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 7 +++++++
src/conf/domain_postparse.c | 2 ++
src/conf/domain_validate.c | 4 +++-
src/conf/virconftypes.h | 2 ++
src/hyperv/hyperv_driver.c | 2 ++
src/qemu/qemu_alias.c | 1 +
src/qemu/qemu_command.c | 26 ++++++++++++++++++++++++++
src/qemu/qemu_domain_address.c | 5 +++++
src/qemu/qemu_hotplug.c | 14 ++++++++++++--
src/qemu/qemu_postparse.c | 1 +
src/qemu/qemu_validate.c | 12 ++++++++++++
src/test/test_driver.c | 2 ++
src/util/virutil.c | 2 +-
src/vbox/vbox_common.c | 2 ++
src/vmx/vmx.c | 1 +
16 files changed, 118 insertions(+), 4 deletions(-)
Reviewing this would likely be simpler if addition of the controller
was split from the addition of the disk.
[...]
@@ -2563,6 +2565,7 @@
virDomainControllerDefNew(virDomainControllerType type)
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
break;
}
[....]
@@ -8832,6 +8845,8 @@
virDomainControllerDefParseXML(virDomainXMLOption *xmlopt,
int ntargetNodes = 0;
g_autofree xmlNodePtr *modelNodes = NULL;
int nmodelNodes = 0;
+ g_autofree xmlNodePtr *serialNodes = NULL;
+ int nserialNodes = 0;
int numaNode = -1;
int ports;
VIR_XPATH_NODE_AUTORESTORE(ctxt)
@@ -8969,6 +8984,18 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt,
if (virXMLPropInt(node, "ports", 10, VIR_XML_PROP_NONNEGATIVE, &ports,
-1) < 0)
return NULL;
+ if ((nserialNodes = virXPathNodeSet("./serial", ctxt, &serialNodes))
> 1) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Multiple <serial> elements in controller definition
not allowed"));
We ususally defer validation of minor infractions such as this to the
schema rather than having code for it.
+ return NULL;
+ }
+
+ if (nserialNodes == 1) {
+ if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_NVME) {
You have a switch statement checking type right below ...
+ def->opts.nvmeopts.serial =
virXMLNodeContentString(serialNodes[0]);
+ }
+ }
+
switch (def->type) {
case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: {
if (virXMLPropInt(node, "vectors", 10, VIR_XML_PROP_NONNEGATIVE,
@@ -9054,6 +9081,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt,
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
... so remove all of the above for:
def->opts.nvmeopts.serial = virXPathString("string(./serial)", ctxt);
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
default:
break;
[...]
@@ -24028,6 +24060,12 @@ virDomainControllerDefFormat(virBuffer
*buf,
}
break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
+ if (def->opts.nvmeopts.serial != NULL) {
+ virBufferAsprintf(&childBuf,
"<serial>%s</serial>\n", def->opts.nvmeopts.serial);
User provided strings must be formatted using 'virBufferEscapeString'
to ensure proper XML entity escaping. That function also skips the
formatting if the string argument is NULL so no check will be needed.
+ }
+ break;
+
case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
if (virDomainControllerDefFormatPCI(&childBuf, def, flags) < 0)
return -1;
[...]
@@ -766,6 +768,10 @@ struct _virDomainXenbusControllerOpts {
int maxEventChannels; /* -1 == undef */
};
+struct _virDomainNVMeControllerOpts {
+ char *serial;
I don't see a corresponding change to free this field so it will likely
be leaked.
+};
+
/* Stores the virtual disk controller configuration */
struct _virDomainControllerDef {
virDomainControllerType type;
[...]
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index d0d4bc0bf4..1ad8350117 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -267,6 +267,7 @@ virDomainDiskAddressDiskBusCompatibility(virDomainDiskBus bus,
case VIR_DOMAIN_DISK_BUS_UML:
case VIR_DOMAIN_DISK_BUS_SD:
case VIR_DOMAIN_DISK_BUS_NONE:
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
Since you are using a 'drive' address type here I think this is wrong.
As in you know that if the disk is of VIR_DOMAIN_DISK_BUS_NVME_NS type
it ought to use the 'drive' address type and thus should be categorized
same as SCSI disks.
case VIR_DOMAIN_DISK_BUS_LAST:
return true;
}
[...]
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 3e6bced4a8..5cffd9e5c8 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -258,6 +258,7 @@ qemuAssignDeviceDiskAlias(virDomainDef *def,
case VIR_DOMAIN_DISK_BUS_IDE:
case VIR_DOMAIN_DISK_BUS_SATA:
case VIR_DOMAIN_DISK_BUS_SCSI:
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
diskPriv->qomName = g_strdup(disk->info.alias);
break;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e6d308534f..d5f75fb3f4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -531,6 +531,17 @@ qemuBuildDeviceAddresDriveProps(virJSONValue *props,
}
break;
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
Please keep the spacing consistent ...
+ if (!(controllerAlias =
virDomainControllerAliasFind(domainDef,
+
VIR_DOMAIN_CONTROLLER_TYPE_NVME,
+
info->addr.drive.controller)))
Also the alignment here ..
+ return -1;
+
+ if (virJSONValueObjectAdd(&props,
+ "s:bus", controllerAlias,
+ NULL) < 0)
... and here is broken.
+ return -1;
+ break;
case VIR_DOMAIN_DISK_BUS_VIRTIO:
case VIR_DOMAIN_DISK_BUS_USB:
@@ -1722,6 +1733,10 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
driver = "floppy";
break;
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
+ driver = "nvme-ns";
+ break;
+
case VIR_DOMAIN_DISK_BUS_XEN:
case VIR_DOMAIN_DISK_BUS_UML:
case VIR_DOMAIN_DISK_BUS_SD:
@@ -2851,6 +2866,16 @@ qemuBuildControllerDevProps(const virDomainDef *domainDef,
break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
+ if (virJSONValueObjectAdd(&props,
+ "s:driver", "nvme",
+ "s:id", def->info.alias,
+ "s:serial", def->opts.nvmeopts.serial,
According to the parser 'serial' seems to be optional. Use of the 's:'
converter makes it mandatory. You likely need 'S:' if it's optional.
+ NULL) < 0)
+ return -1;
+
+ break;
+
case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
[...]
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 5326aba281..844cfc2e02 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -848,7 +848,8 @@ qemuDomainAttachControllerDevice(virDomainObj *vm,
bool releaseaddr = false;
if (controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI && \
- controller->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) {
+ controller->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL && \
+ controller->type != VIR_DOMAIN_CONTROLLER_TYPE_NVME) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("'%1$s' controller cannot be hot plugged."),
virDomainControllerTypeToString(controller->type));
@@ -1058,6 +1059,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
/* Note that SD card hotplug support should be added only once
* they support '-device' (don't require -drive only).
* See also: qemuDiskBusIsSD */
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
So what is the story regarding hotplug here? I presume that the
individual namespace devices need to exist before the controller is
attached, right?
case VIR_DOMAIN_DISK_BUS_NONE:
case VIR_DOMAIN_DISK_BUS_LAST:
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
@@ -5782,6 +5784,7 @@ qemuDomainDetachPrepDisk(virDomainObj *vm,
case VIR_DOMAIN_DISK_BUS_UML:
case VIR_DOMAIN_DISK_BUS_SATA:
case VIR_DOMAIN_DISK_BUS_SD:
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("This type of disk cannot be hot unplugged"));
return -1;
Hmm, so if you unplug the controller we'd have to remove all the disks.
Which IMO could work although IIRC the disk unplug code would not be
able to handle this properly yet.
Either way it's okay to forbid libvirt-initiated unplug requests. Also
guest-initiated unplug requests should be complied with and thus the
code for unplug will need to be fixed. Although that is not strictly
required for your series because we have the same kind of issue when
unplugging SCSI controller.
[...]
@@ -5934,7 +5943,8 @@ qemuDomainDetachPrepController(virDomainObj
*vm,
int idx;
virDomainControllerDef *controller = NULL;
- if (match->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ if (match->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
+ match->type != VIR_DOMAIN_CONTROLLER_TYPE_NVME) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("'%1$s' controller cannot be hot
unplugged."),
virDomainControllerTypeToString(match->type));
[...]
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index b2c3c9e2f6..9985b2e2c1 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
[...]
@@ -3194,6 +3196,14 @@ qemuValidateDomainDeviceDefDiskFrontend(const
virDomainDiskDef *disk,
break;
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
+ if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("unexpected address type for nvme-ns disk"));
Based on the error message this is not an _INTERNAL_ERROR, but rather
one of those stating that the user messed up with the configuration.
+ return -1;
+ }
+ break;
+
case VIR_DOMAIN_DISK_BUS_XEN:
case VIR_DOMAIN_DISK_BUS_SD:
case VIR_DOMAIN_DISK_BUS_NONE:
[...]
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 2abcb282fe..02494f1061 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -327,7 +327,7 @@ int virDiskNameParse(const char *name, int *disk, int *partition)
const char *ptr = NULL;
char *rem;
int idx = 0;
- static char const* const drive_prefix[] = {"fd", "hd",
"vd", "sd", "xvd", "ubd"};
+ static char const* const drive_prefix[] = {"fd", "hd",
"vd", "sd", "xvd", "ubd", "nvmens"};
I'm not sure I like the 'nvmens' prefix. Let's discuss that in the patch
adding the XML
size_t i;
size_t n_digits;