From: ray <honglei.wang@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@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(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 542d6ade91..e4b3bf8720 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -372,6 +372,7 @@ VIR_ENUM_IMPL(virDomainDiskBus,
"uml",
"sata",
"sd",
+ "nvme-ns",
);
VIR_ENUM_IMPL(virDomainDiskCache,
@@ -420,6 +421,7 @@ VIR_ENUM_IMPL(virDomainController,
"pci",
"xenbus",
"isa",
+ "nvme",
);
VIR_ENUM_IMPL(virDomainControllerModelPCI,
@@ -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;
}
@@ -6869,6 +6872,14 @@ virDomainDiskDefAssignAddress(virDomainXMLOption *xmlopt G_GNUC_UNUSED,
def->info.addr.drive.unit = idx % 2;
break;
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
+ /* For NVME-NS, each nvme controller has a maximum of 256 nvme-ns */
+ def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+ def->info.addr.drive.controller = idx / 256;
+ def->info.addr.drive.bus = 0;
+ def->info.addr.drive.unit = idx % 256;
+ break;
+
case VIR_DOMAIN_DISK_BUS_NONE:
case VIR_DOMAIN_DISK_BUS_VIRTIO:
case VIR_DOMAIN_DISK_BUS_XEN:
@@ -8784,6 +8795,7 @@ virDomainControllerModelTypeFromString(const virDomainControllerDef *def,
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
return -1;
}
@@ -8812,6 +8824,7 @@ virDomainControllerModelTypeToString(virDomainControllerDef *def,
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
return NULL;
}
@@ -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"));
+ return NULL;
+ }
+
+ if (nserialNodes == 1) {
+ if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_NVME) {
+ 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:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
default:
break;
@@ -14998,6 +15026,10 @@ virDomainDiskControllerMatch(int controller_type, int disk_bus)
disk_bus == VIR_DOMAIN_DISK_BUS_SATA)
return true;
+ if (controller_type == VIR_DOMAIN_CONTROLLER_TYPE_NVME &&
+ disk_bus == VIR_DOMAIN_DISK_BUS_NVME_NS)
+ return true;
+
return false;
}
@@ -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);
+ }
+ break;
+
case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
if (virDomainControllerDefFormatPCI(&childBuf, def, flags) < 0)
return -1;
@@ -29662,6 +29700,7 @@ virDiskNameToBusDeviceIndex(virDomainDiskDef *disk,
case VIR_DOMAIN_DISK_BUS_NONE:
case VIR_DOMAIN_DISK_BUS_SATA:
case VIR_DOMAIN_DISK_BUS_UML:
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
case VIR_DOMAIN_DISK_BUS_LAST:
default:
*busIdx = 0;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 58b97a2b54..e9db1ea896 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -401,6 +401,7 @@ typedef enum {
VIR_DOMAIN_DISK_BUS_UML,
VIR_DOMAIN_DISK_BUS_SATA,
VIR_DOMAIN_DISK_BUS_SD,
+ VIR_DOMAIN_DISK_BUS_NVME_NS,
VIR_DOMAIN_DISK_BUS_LAST
} virDomainDiskBus;
@@ -611,6 +612,7 @@ typedef enum {
VIR_DOMAIN_CONTROLLER_TYPE_PCI,
VIR_DOMAIN_CONTROLLER_TYPE_XENBUS,
VIR_DOMAIN_CONTROLLER_TYPE_ISA,
+ VIR_DOMAIN_CONTROLLER_TYPE_NVME,
VIR_DOMAIN_CONTROLLER_TYPE_LAST
} virDomainControllerType;
@@ -766,6 +768,10 @@ struct _virDomainXenbusControllerOpts {
int maxEventChannels; /* -1 == undef */
};
+struct _virDomainNVMeControllerOpts {
+ char *serial;
+};
+
/* Stores the virtual disk controller configuration */
struct _virDomainControllerDef {
virDomainControllerType type;
@@ -782,6 +788,7 @@ struct _virDomainControllerDef {
virDomainPCIControllerOpts pciopts;
virDomainUSBControllerOpts usbopts;
virDomainXenbusControllerOpts xenbusopts;
+ virDomainNVMeControllerOpts nvmeopts;
} opts;
virDomainDeviceInfo info;
virDomainVirtioOptions *virtio;
diff --git a/src/conf/domain_postparse.c b/src/conf/domain_postparse.c
index bf33f29638..68d99f3c81 100644
--- a/src/conf/domain_postparse.c
+++ b/src/conf/domain_postparse.c
@@ -523,6 +523,8 @@ virDomainDiskDefPostParse(virDomainDiskDef *disk,
disk->bus = VIR_DOMAIN_DISK_BUS_XEN;
else if (STRPREFIX(disk->dst, "ubd"))
disk->bus = VIR_DOMAIN_DISK_BUS_UML;
+ else if (STRPREFIX(disk->dst, "nvmens"))
+ disk->bus = VIR_DOMAIN_DISK_BUS_NVME_NS;
}
}
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:
case VIR_DOMAIN_DISK_BUS_LAST:
return true;
}
@@ -948,7 +949,8 @@ virDomainDiskDefValidate(const virDomainDef *def,
!STRPREFIX(disk->dst, "sd") &&
!STRPREFIX(disk->dst, "vd") &&
!STRPREFIX(disk->dst, "xvd") &&
- !STRPREFIX(disk->dst, "ubd")) {
+ !STRPREFIX(disk->dst, "ubd") &&
+ !STRPREFIX(disk->dst, "nvmens")) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid harddisk device name: %1$s"), disk->dst);
return -1;
diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
index c70437bc05..8c6fcdbeaa 100644
--- a/src/conf/virconftypes.h
+++ b/src/conf/virconftypes.h
@@ -276,6 +276,8 @@ typedef struct _virDomainXMLPrivateDataCallbacks virDomainXMLPrivateDataCallback
typedef struct _virDomainXenbusControllerOpts virDomainXenbusControllerOpts;
+typedef struct _virDomainNVMeControllerOpts virDomainNVMeControllerOpts;
+
typedef enum {
VIR_DOMAIN_DISK_IO_DEFAULT = 0,
VIR_DOMAIN_DISK_IO_NATIVE,
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 0d1e388c08..aefb48923b 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -948,6 +948,7 @@ hypervDomainAttachStorage(virDomainPtr domain, virDomainDef *def, const char *ho
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:
case VIR_DOMAIN_DISK_BUS_LAST:
default:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported controller type"));
@@ -3078,6 +3079,7 @@ hypervDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, unsigned int
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:
case VIR_DOMAIN_DISK_BUS_LAST:
default:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid disk bus in definition"));
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:
+ if (!(controllerAlias = virDomainControllerAliasFind(domainDef,
+ VIR_DOMAIN_CONTROLLER_TYPE_NVME,
+ info->addr.drive.controller)))
+ return -1;
+
+ if (virJSONValueObjectAdd(&props,
+ "s:bus", controllerAlias,
+ NULL) < 0)
+ 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,
+ NULL) < 0)
+ return -1;
+
+ break;
+
case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
@@ -3013,6 +3038,7 @@ qemuBuildControllersCommandLine(virCommand *cmd,
VIR_DOMAIN_CONTROLLER_TYPE_IDE,
VIR_DOMAIN_CONTROLLER_TYPE_SATA,
VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL,
+ VIR_DOMAIN_CONTROLLER_TYPE_NVME,
};
for (i = 0; i < G_N_ELEMENTS(contOrder); i++) {
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index e89cdee487..a4d0c0d0d5 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -616,6 +616,9 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev,
}
break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
+ return pciFlags;
+
case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
@@ -738,6 +741,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev,
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:
case VIR_DOMAIN_DISK_BUS_NONE:
case VIR_DOMAIN_DISK_BUS_LAST:
return 0;
@@ -1919,6 +1923,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDef *def,
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
break;
}
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:
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;
@@ -5856,6 +5859,11 @@ qemuDomainDiskControllerIsBusy(virDomainObj *vm,
continue;
break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
+ if (disk->bus != VIR_DOMAIN_DISK_BUS_NVME_NS)
+ continue;
+ break;
+
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
/* xenbus is not supported by the qemu driver */
continue;
@@ -5905,6 +5913,7 @@ qemuDomainControllerIsBusy(virDomainObj *vm,
case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
return qemuDomainDiskControllerIsBusy(vm, detach);
case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
@@ -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_postparse.c b/src/qemu/qemu_postparse.c
index ed4af9ca8e..8150dffac6 100644
--- a/src/qemu/qemu_postparse.c
+++ b/src/qemu/qemu_postparse.c
@@ -429,6 +429,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
break;
}
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
@@ -2945,6 +2945,7 @@ qemuValidateDomainDeviceDefDiskIOThreads(const virDomainDef *def,
case VIR_DOMAIN_DISK_BUS_SATA:
case VIR_DOMAIN_DISK_BUS_SD:
case VIR_DOMAIN_DISK_BUS_NONE:
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
case VIR_DOMAIN_DISK_BUS_LAST:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("IOThreads not available for bus %1$s target %2$s"),
@@ -3079,6 +3080,7 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
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_CONFIG_UNSUPPORTED,
_("disk device='lun' is not supported for bus='%1$s'"),
virDomainDiskBusTypeToString(disk->bus));
@@ -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"));
+ return -1;
+ }
+ break;
+
case VIR_DOMAIN_DISK_BUS_XEN:
case VIR_DOMAIN_DISK_BUS_SD:
case VIR_DOMAIN_DISK_BUS_NONE:
@@ -3382,6 +3392,7 @@ qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk,
case VIR_DOMAIN_DISK_BUS_USB:
case VIR_DOMAIN_DISK_BUS_VIRTIO:
case VIR_DOMAIN_DISK_BUS_SCSI:
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
break;
case VIR_DOMAIN_DISK_BUS_IDE:
@@ -4399,6 +4410,7 @@ qemuValidateDomainDeviceDefController(const virDomainControllerDef *controller,
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
break;
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 6f18b2b2c8..95ab1cac8f 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -10344,6 +10344,7 @@ testDomainAttachDeviceDiskLiveInternal(testDriver *driver G_GNUC_UNUSED,
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:
case VIR_DOMAIN_DISK_BUS_NONE:
case VIR_DOMAIN_DISK_BUS_LAST:
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
@@ -10792,6 +10793,7 @@ testDomainDetachPrepDisk(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;
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"};
size_t i;
size_t n_digits;
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 349ac832dc..703150d3c6 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -494,6 +494,7 @@ vboxSetStorageController(virDomainControllerDef *controller,
case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
+ case VIR_DOMAIN_CONTROLLER_TYPE_NVME:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
vboxReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("The vbox driver does not support %1$s controller type"),
@@ -1238,6 +1239,7 @@ vboxAttachDrives(virDomainDef *def, struct _vboxDriver *data, IMachine *machine)
case VIR_DOMAIN_DISK_BUS_USB:
case VIR_DOMAIN_DISK_BUS_UML:
case VIR_DOMAIN_DISK_BUS_SD:
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
case VIR_DOMAIN_DISK_BUS_NONE:
case VIR_DOMAIN_DISK_BUS_LAST:
vboxReportError(VIR_ERR_CONFIG_UNSUPPORTED,
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 0dd03c1a88..31878c7399 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2245,6 +2245,7 @@ virVMXGenerateDiskTarget(virDomainDiskDef *def,
case VIR_DOMAIN_DISK_BUS_USB:
case VIR_DOMAIN_DISK_BUS_UML:
case VIR_DOMAIN_DISK_BUS_SD:
+ case VIR_DOMAIN_DISK_BUS_NVME_NS:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported bus type '%1$s' for device type '%2$s'"),
virDomainDiskBusTypeToString(def->bus),
--
2.11.0On Sun, Apr 27, 2025 at 19:48:03 +0800, honglei.wang@smartx.com wrote:
> From: ray <honglei.wang@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@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;
>
Thanks a lot for the detailed review and suggestions.
>
> Reviewing this would likely be simpler if addition of the controller
> was split from the addition of the disk.
>
Okay, I will split the patch into two parts as suggested—one for introducing the controller and another for the disk.
>
> We ususally defer validation of minor infractions such as this to the
> schema rather than having code for it.
>
I will remove the validation logic and keep only the logic for retrieving the 'serial' value.
>
> You have a switch statement checking type right below ...
>
>
> ... so remove all of the above for:
>
> def->opts.nvmeopts.serial = virXPathString("string(./serial)", ctxt);
>
ACK
>
> 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.
>
ACK
>
> I don't see a corresponding change to free this field so it will likely
> be leaked.
>
That was my oversight—I will make sure to free the memory in virDomainControllerDefFree, and I’ll also check if there are other places where this is needed.
>
> 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.
>
Yes, you're right.
>
> Please keep the spacing consistent ...
>
>
> Also the alignment here ..
>
>
> ... and here is broken.
>
ACK
>
> 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.
>
The 'serial' field is required—not optional—because the QEMU implementation mandates its presence.
>
> So what is the story regarding hotplug here? I presume that the
> individual namespace devices need to exist before the controller is
> attached, right?
In QEMU, the NVMe controller device supports hotplug, but nvme-ns devices do not, so the current behavior is aligned with QEMU.
>
> 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.
>
Yes, since nvme-ns devices do not support hot-unplug, controllers with attached namespaces cannot be hot-unplugged either.
>
> 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.
>
Yes, we should use VIR_ERR_CONFIG_UNSUPPORTED for this case.
>
> I'm not sure I like the 'nvmens' prefix. Let's discuss that in the patch
> adding the XML
I agree the nvmens prefix feels a bit odd. Ideally, the naming should resemble something like nvme0n1, but that would introduce more complexity and tightly couple the name to a specific controller. As a compromise, maybe we can use nvme as the prefix.
Also, I’d like to discuss whether nvme-ns is the appropriate choice for the disk bus type.
On Fri, May 09, 2025 at 08:32:42 -0000, ray wang wrote: > Thanks a lot for the detailed review and suggestions. Please for the next time include more context to your replies. I've already forgotten what I was refering to in most contexts that you are replying below so I'll have hard time following up. In general making review harder is detremental to your series because it will take longer. > > Reviewing this would likely be simpler if addition of the controller > > was split from the addition of the disk. > > > Okay, I will split the patch into two parts as suggested—one for introducing the controller and another for the disk. > > > > > We ususally defer validation of minor infractions such as this to the > > schema rather than having code for it. > > > I will remove the validation logic and keep only the logic for retrieving the 'serial' value. [...] > > 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. > > > The 'serial' field is required—not optional—because the QEMU implementation mandates its presence. Okay; you then need a validation check that mandates it.
© 2016 - 2025 Red Hat, Inc.