In near future we will have a list of PCI devices we want to detach (held in virPCIDeviceListPtr) but we don't have virDomainHostdevDefPtr. That's okay because virHostdevPreparePCIDevices() works with virPCIDeviceListPtr mostly anyway. And in very few places where it needs virDomainHostdevDefPtr are not interesting for our case. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virhostdev.c | 48 ++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index a3647a6cf4..88b0828675 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -613,27 +613,22 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } } -int -virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, - const char *drv_name, - const char *dom_name, - const unsigned char *uuid, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs, - unsigned int flags) + +static int +virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + const unsigned char *uuid, + virPCIDeviceListPtr pcidevs, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs, + unsigned int flags) { - VIR_AUTOUNREF(virPCIDeviceListPtr) pcidevs = NULL; int last_processed_hostdev_vf = -1; size_t i; int ret = -1; virPCIDeviceAddressPtr devAddr = NULL; - if (!nhostdevs) - return 0; - - if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) - return -1; - virObjectLock(mgr->activePCIHostdevs); virObjectLock(mgr->inactivePCIHostdevs); @@ -906,10 +901,31 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, cleanup: virObjectUnlock(mgr->activePCIHostdevs); virObjectUnlock(mgr->inactivePCIHostdevs); - return ret; } + +int +virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + const unsigned char *uuid, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs, + unsigned int flags) +{ + VIR_AUTOUNREF(virPCIDeviceListPtr) pcidevs = NULL; + + if (!nhostdevs) + return 0; + + if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) + return -1; + + return virHostdevPreparePCIDevicesImpl(mgr, drv_name, dom_name, uuid, + pcidevs, hostdevs, nhostdevs, flags); +} + /* * Pre-condition: inactivePCIHostdevs & activePCIHostdevs * are locked -- 2.21.0

In near future we will have a list of PCI devices we want to re-attach to the host (held in virPCIDeviceListPtr) but we don't have virDomainHostdevDefPtr. That's okay because virHostdevReAttachPCIDevices() works with virPCIDeviceListPtr mostly anyway. And in very few places where it needs virDomainHostdevDefPtr are not interesting for our case. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virhostdev.c | 58 +++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 88b0828675..07397b9682 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -953,30 +953,18 @@ virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, } } -/* @oldStateDir: - * For upgrade purpose: see virHostdevRestoreNetConfig - */ -void -virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, - const char *drv_name, - const char *dom_name, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs, - const char *oldStateDir) + +static void +virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + virPCIDeviceListPtr pcidevs, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs, + const char *oldStateDir) { - VIR_AUTOUNREF(virPCIDeviceListPtr) pcidevs = NULL; size_t i; - if (!nhostdevs) - return; - - if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { - VIR_ERROR(_("Failed to allocate PCI device list: %s"), - virGetLastErrorMessage()); - virResetLastError(); - return; - } - virObjectLock(mgr->activePCIHostdevs); virObjectLock(mgr->inactivePCIHostdevs); @@ -1100,6 +1088,34 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, virObjectUnref(pcidevs); } + +/* @oldStateDir: + * For upgrade purpose: see virHostdevRestoreNetConfig + */ +void +virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs, + const char *oldStateDir) +{ + VIR_AUTOUNREF(virPCIDeviceListPtr) pcidevs = NULL; + + if (!nhostdevs) + return; + + if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { + VIR_ERROR(_("Failed to allocate PCI device list: %s"), + virGetLastErrorMessage()); + virResetLastError(); + return; + } + + virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs, + hostdevs, nhostdevs, oldStateDir); +} + int virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, virDomainHostdevDefPtr *hostdevs, -- 2.21.0

The pci_driver_bind() and pci_driver_unbind() functions are "internal implementation", meaning other parts of the code should be able to call them and get the job done. Checking for actions (PCI_ACTION_BIND and PCI_ACTION_UNBIND) should be done in handlers (pci_driver_handle_bind() and pci_driver_handle_unbind()). Surprisingly, the other two actions (PCI_ACTION_NEW_ID and PCI_ACTION_REMOVE_ID) are checked already at this level. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/virpcimock.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/virpcimock.c b/tests/virpcimock.c index beb5e1490d..6865f992dc 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -551,8 +551,8 @@ pci_driver_bind(struct pciDriver *driver, int ret = -1; char *devpath = NULL, *driverpath = NULL; - if (dev->driver || PCI_ACTION_BIND & driver->fail) { - /* Device already bound or failing driver requested */ + if (dev->driver) { + /* Device already bound */ errno = ENODEV; return ret; } @@ -598,8 +598,8 @@ pci_driver_unbind(struct pciDriver *driver, int ret = -1; char *devpath = NULL, *driverpath = NULL; - if (dev->driver != driver || PCI_ACTION_UNBIND & driver->fail) { - /* Device not bound to the @driver or failing driver used */ + if (dev->driver != driver) { + /* Device not bound to the @driver */ errno = ENODEV; return ret; } @@ -669,8 +669,8 @@ pci_driver_handle_bind(const char *path) struct pciDevice *dev = pci_device_find_by_content(path); struct pciDriver *driver = pci_driver_find_by_path(path); - if (!driver || !dev) { - /* This should never happen (TM) */ + if (!driver || !dev || PCI_ACTION_BIND & driver->fail) { + /* No driver, no device or failing driver requested */ errno = ENODEV; goto cleanup; } @@ -686,8 +686,8 @@ pci_driver_handle_unbind(const char *path) int ret = -1; struct pciDevice *dev = pci_device_find_by_content(path); - if (!dev || !dev->driver) { - /* This should never happen (TM) */ + if (!dev || !dev->driver || PCI_ACTION_UNBIND & dev->driver->fail) { + /* No device, device not binded or failing driver requested */ errno = ENODEV; goto cleanup; } -- 2.21.0

On 7/16/19 2:03 PM, Peter Krempa wrote: This whole PCI_ACTION_BIND mess exists because with RHEL-7 kernel it's not possible to bind a PCI device directly to vfio-pci driver. I mean, with RHEL-7 kernel the following steps fail: 01:00.0 Non-Volatile memory controller: Device 1cc1:8201 (rev 03) Subsystem: Device 1cc1:8201 Kernel driver in use: nvme # echo "1cc1 8201" > /sys/bus/pci/drivers/vfio-pci/new_id # echo "0000:01:00.0" > /sys/bus/pci/devices/0000\:01\:00.0/driver/unbind # echo "0000:01:00.0" > /sys/bus/pci/drivers/vfio-pci/bind 01:00.0 Non-Volatile memory controller: Device 1cc1:8201 (rev 03) Subsystem: Device 1cc1:8201 Kernel driver in use: vfio-pci But on anything else (e.g. my vanilla kernel) this is allowed. Anyway, this is irrelevant because even on RHEL-7 we are using 'driver_override' (which is way simpler to use and doesn't create a window where an unbinded PCI device can be claimed by a different PCI driver). How does this concern pcimock? Well, there are only so many "entry" points to the mock. These are functions which have "handle" in their name and are called from within pci_driver_handle_change(). Every other function is just a helper. For instance, pci_device_autobind() is called from pci_device_new_from_stub() which just creates a PCI device during the mock initialization. Or, it's called on a write to "drivers_probe" which again succeeds (on both RHEL-7 and vanilla kernels). And for pci_driver_handle_new_id it's the same story. If you want, I can add those checks there so that this patch looks complete. But honestly, it doesn't matter because this code path will not be used once 'driver_override' is implemented (patch 05/31). Michal

Newer kernels (v3.16-rc1~29^2~6^4) have 'driver_override' file which simplifies way of binding a PCI device to desired driver. Libvirt has support for this for some time too (v2.3.0-rc1~236), but not our virpcimock. So far we did not care because our code is designed to deal with this situation. Except for one. hypothetical case: binding a device to the vfio-pci driver can be successful only via driver_override. Any attempt to bind a PCI device to vfio-pci driver using old method (new_id + unbind + bind) will fail because of b803b29c1a5. While on vanilla kernel I'm able to use the old method successfully, it's failing on RHEL kernels (not sure why). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/virpcimock.c | 57 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 6 deletions(-) diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 6865f992dc..18d06d11d4 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -87,6 +87,11 @@ char *fakesysfspcidir; * Probe for a driver that handles the specified device. * Data in format "DDDD:BB:DD.F" (Domain:Bus:Device.Function). * + * /sys/bus/pci/devices/<device>/driver_override + * Name of a driver that overrides preferred driver can be written + * here. The device will be attached to it on drivers_probe event. + * Writing an empty string (or "\n") clears the override. + * * As a little hack, we are not mocking write to these files, but close() * instead. The advantage is we don't need any self growing array to hold the * partial writes and construct them back. We can let all the writes finish, @@ -147,6 +152,7 @@ static struct pciDevice *pci_device_find_by_content(const char *path); static void pci_driver_new(const char *name, int fail, ...); static struct pciDriver *pci_driver_find_by_dev(struct pciDevice *dev); static struct pciDriver *pci_driver_find_by_path(const char *path); +static struct pciDriver *pci_driver_find_by_driver_override(struct pciDevice *dev); static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev); static int pci_driver_unbind(struct pciDriver *driver, struct pciDevice *dev); static int pci_driver_handle_change(int fd, const char *path); @@ -202,7 +208,8 @@ make_symlink(const char *path, static int pci_read_file(const char *path, char *buf, - size_t buf_size) + size_t buf_size, + bool truncate) { int ret = -1; int fd = -1; @@ -224,7 +231,8 @@ pci_read_file(const char *path, goto cleanup; } - if (ftruncate(fd, 0) < 0) + if (truncate && + ftruncate(fd, 0) < 0) goto cleanup; ret = 0; @@ -398,6 +406,8 @@ pci_device_new_from_stub(const struct pciDevice *data) ABORT("@tmp overflow"); make_file(devpath, "class", tmp, -1); + make_file(devpath, "driver_override", NULL, -1); + if (snprintf(tmp, sizeof(tmp), "%s/../../../kernel/iommu_groups/%d", devpath, dev->iommuGroup) < 0) { @@ -441,7 +451,7 @@ pci_device_find_by_content(const char *path) { char tmp[32]; - if (pci_read_file(path, tmp, sizeof(tmp)) < 0) + if (pci_read_file(path, tmp, sizeof(tmp), true) < 0) return NULL; return pci_device_find_by_id(tmp); @@ -450,7 +460,10 @@ pci_device_find_by_content(const char *path) static int pci_device_autobind(struct pciDevice *dev) { - struct pciDriver *driver = pci_driver_find_by_dev(dev); + struct pciDriver *driver = pci_driver_find_by_driver_override(dev); + + if (!driver) + driver = pci_driver_find_by_dev(dev); if (!driver) { /* No driver found. Nothing to do */ @@ -544,6 +557,36 @@ pci_driver_find_by_path(const char *path) return NULL; } +static struct pciDriver * +pci_driver_find_by_driver_override(struct pciDevice *dev) +{ + struct pciDriver *ret = NULL; + char *path = NULL; + char tmp[32]; + size_t i; + + if (virAsprintfQuiet(&path, + SYSFS_PCI_PREFIX "devices/%s/driver_override", + dev->id) < 0) + return NULL; + + if (pci_read_file(path, tmp, sizeof(tmp), false) < 0) + goto cleanup; + + for (i = 0; i < nPCIDrivers; i++) { + struct pciDriver *driver = pciDrivers[i]; + + if (STREQ(tmp, driver->name)) { + ret = driver; + break; + } + } + + cleanup: + VIR_FREE(path); + return ret; +} + static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev) @@ -657,6 +700,8 @@ pci_driver_handle_change(int fd ATTRIBUTE_UNUSED, const char *path) ret = pci_driver_handle_remove_id(path); else if (STREQ(file, "drivers_probe")) ret = pci_driver_handle_drivers_probe(path); + else if (STREQ(file, "driver_override")) + ret = 0; /* nada */ else ABORT("Not handled write to: %s", path); return ret; @@ -711,7 +756,7 @@ pci_driver_handle_new_id(const char *path) goto cleanup; } - if (pci_read_file(path, buf, sizeof(buf)) < 0) + if (pci_read_file(path, buf, sizeof(buf), true) < 0) goto cleanup; if (sscanf(buf, "%x %x", &vendor, &device) < 2) { @@ -766,7 +811,7 @@ pci_driver_handle_remove_id(const char *path) goto cleanup; } - if (pci_read_file(path, buf, sizeof(buf)) < 0) + if (pci_read_file(path, buf, sizeof(buf), true) < 0) goto cleanup; if (sscanf(buf, "%x %x", &vendor, &device) < 2) { -- 2.21.0

On 7/16/19 2:13 PM, Peter Krempa wrote: Yep, I needed to do that s I've asked Alex Williamson about this and he confirmed that he can see this behaviour but nor he could understand why RHEL-7 kernel behaves that way. This could explain why the check from 03/31 can't be here. If it was, then nor 'driver_override' would be able to bind a PCI device to the vfio-pci driver (which obviously is not the case). Ah, good point. Consider fixed. Michal

On Thu, Jul 11, 2019 at 17:53:55 +0200, Michal Privoznik wrote: [...] Please add a comment stating that this is a deep copy and also note with the definitions of 'struct _virZPCIDeviceAddress' and 'struct _virPCIDeviceAddress' that there is a deep-copy function which needs to be fixed when adding new members. ACK with the above addressed.

On Thu, Jul 11, 2019 at 17:53:57 +0200, Michal Privoznik wrote: s/NVDIMM memory/NVMe device/ The 'type' filed may get confusing a bit as it is supposed to be stored in virStorageSource->nvme->type, while virStorageSource has it's own type. Also I'm pondering whether managed='yes' belongs as a top-level attribute under 'source' as it's possibly specific to the 'pci' setting of type. [...] You specifically forbid startup policy in the next commit, so what's the point of documenting it here? [...] Make at heast one of them use qcow2 as format. I'm also missing any form of documentation describing the caveats (e.g. users should not pass in a NVMe disk the host is using) or any advantages/reasons for using this.

To simplify implementation, some restrictions are added. For instance, an NVMe disk can't go to any bus but virtio and has to be type of 'disk' and can't have startupPolicy set. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/domain_conf.c | 129 +++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_block.c | 1 + src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 4 + src/qemu/qemu_migration.c | 1 + src/util/virstoragefile.c | 59 +++++++++++ src/util/virstoragefile.h | 15 +++ src/xenconfig/xen_xl.c | 1 + tests/qemuxml2argvdata/disk-nvme.xml | 12 ++- tests/qemuxml2xmloutdata/disk-nvme.xml | 1 + tests/qemuxml2xmltest.c | 1 + 12 files changed, 224 insertions(+), 2 deletions(-) create mode 120000 tests/qemuxml2xmloutdata/disk-nvme.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3323c9a5b1..73f5e1fa0f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5088,6 +5088,11 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk, return -1; } + if (disk->src->type == VIR_STORAGE_TYPE_NVME) { + if (disk->src->nvme->managed == VIR_TRISTATE_BOOL_ABSENT) + disk->src->nvme->managed = VIR_TRISTATE_BOOL_YES; + } + if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) { return -1; @@ -5938,6 +5943,38 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) return -1; } + if (disk->src->type == VIR_STORAGE_TYPE_NVME) { + /* These might not be valid for all hypervisors, but be + * strict now and possibly refine in the future. */ + if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported disk type '%s' for NVMe disk"), + virDomainDiskDeviceTypeToString(disk->device)); + return -1; + } + + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported bus '%s' for NVMe disk"), + virDomainDiskBusTypeToString(disk->bus)); + return -1; + } + + if (disk->startupPolicy != VIR_DOMAIN_STARTUP_POLICY_DEFAULT && + disk->startupPolicy != VIR_DOMAIN_STARTUP_POLICY_MANDATORY) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported startup policy '%s' for NVMe disk"), + virDomainStartupPolicyTypeToString(disk->startupPolicy)); + return -1; + } + + if (disk->src->shared) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported <shareable/> for NVMe disk")); + return -1; + } + } + return 0; } @@ -9184,6 +9221,76 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, } +static int +virDomainDiskSourceNVMeParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virStorageSourcePtr src) +{ + VIR_AUTOPTR(virStorageSourceNVMeDef) nvme = NULL; + VIR_AUTOFREE(char *) type = NULL; + VIR_AUTOFREE(char *) namespace = NULL; + VIR_AUTOFREE(char *) managed = NULL; + xmlNodePtr address; + + if (VIR_ALLOC(nvme) < 0) + return -1; + + if (!(type = virXMLPropString(node, "type"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing 'type' attribute to disk source")); + return -1; + } + + if (STRNEQ(type, "pci")) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported source type '%s'"), + type); + return -1; + } + + if (!(namespace = virXMLPropString(node, "namespace"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing 'namespace' attribute to disk source")); + return -1; + } + + if (virStrToLong_ul(namespace, NULL, 10, &nvme->namespace) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("malformed namespace '%s'"), + namespace); + return -1; + } + + /* NVMe namespaces start from 1 */ + if (nvme->namespace == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NVMe namespace can't be zero")); + return -1; + } + + if ((managed = virXMLPropString(node, "managed"))) { + if ((nvme->managed = virTristateBoolTypeFromString(managed)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("malformed managed value '%s'"), + managed); + return -1; + } + } + + if (!(address = virXPathNode("./address", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NVMe disk source is missing address")); + return -1; + } + + if (virPCIDeviceAddressParseXML(address, &nvme->pciAddr) < 0) + return -1; + + VIR_STEAL_PTR(src->nvme, nvme); + return 0; +} + + static int virDomainDiskSourcePRParse(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -9284,6 +9391,10 @@ virDomainStorageSourceParse(xmlNodePtr node, if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) return -1; break; + case VIR_STORAGE_TYPE_NVME: + if (virDomainDiskSourceNVMeParse(node, ctxt, src) < 0) + return -1; + break; case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -23922,6 +24033,19 @@ virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf, } +static void +virDomainDiskSourceNVMeFormat(virBufferPtr attrBuf, + virBufferPtr childBuf, + const virStorageSourceNVMeDef *nvme) +{ + virBufferAddLit(attrBuf, " type='pci'"); + virBufferAsprintf(attrBuf, " managed='%s'", + virTristateBoolTypeToString(nvme->managed)); + virBufferAsprintf(attrBuf, " namespace='%ld'", nvme->namespace); + virPCIDeviceAddressFormat(childBuf, nvme->pciAddr, false); +} + + static int virDomainDiskSourceFormatPrivateData(virBufferPtr buf, virStorageSourcePtr src, @@ -24008,6 +24132,11 @@ virDomainDiskSourceFormat(virBufferPtr buf, break; + case VIR_STORAGE_TYPE_NVME: + if (src->nvme) + virDomainDiskSourceNVMeFormat(&attrBuf, &childBuf, src->nvme); + break; + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6cef8d20fe..350b638193 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2994,6 +2994,7 @@ virStorageSourceNetworkAssignDefaultPorts; virStorageSourceNew; virStorageSourceNewFromBacking; virStorageSourceNewFromBackingAbsolute; +virStorageSourceNVMeDefFree; virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 0a6522577d..5eeb3757f1 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1050,6 +1050,7 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, break; case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: return NULL; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 688dc324c6..927641cf46 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1184,6 +1184,7 @@ qemuGetDriveSourceString(virStorageSourcePtr src, break; case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: break; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a52b54b9d8..9bf12ea20d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14637,6 +14637,7 @@ qemuDomainSnapshotPrepareDiskExternalInactive(virDomainSnapshotDiskDefPtr snapdi case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -14653,6 +14654,7 @@ qemuDomainSnapshotPrepareDiskExternalInactive(virDomainSnapshotDiskDefPtr snapdi case VIR_STORAGE_TYPE_NETWORK: case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -14715,6 +14717,7 @@ qemuDomainSnapshotPrepareDiskExternalActive(virDomainSnapshotDiskDefPtr snapdisk case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -14837,6 +14840,7 @@ qemuDomainSnapshotPrepareDiskInternal(virDomainDiskDefPtr disk, case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2436f5051b..87adccab3d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -227,6 +227,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn, case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 269d0050fd..18aa33fe05 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -57,6 +57,7 @@ VIR_ENUM_IMPL(virStorage, "dir", "network", "volume", + "nvme", ); VIR_ENUM_IMPL(virStorageFileFormat, @@ -2114,6 +2115,48 @@ virStoragePRDefCopy(virStoragePRDefPtr src) } +static virStorageSourceNVMeDefPtr +virStorageSourceNVMeDefCopy(const virStorageSourceNVMeDef *src) +{ + VIR_AUTOPTR(virStorageSourceNVMeDef) ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + *ret = *src; + VIR_RETURN_PTR(ret); +} + + +static bool +virStorageSourceNVMeDefIsEqual(const virStorageSourceNVMeDef *a, + const virStorageSourceNVMeDef *b) +{ + if (!a && !b) + return true; + + if (!a || !b) + return false; + + if (a->namespace != b->namespace || + a->managed != b->managed || + !virPCIDeviceAddressEqual(&a->pciAddr, &b->pciAddr)) + return false; + + return true; +} + + +void +virStorageSourceNVMeDefFree(virStorageSourceNVMeDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def); +} + + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) @@ -2323,6 +2366,10 @@ virStorageSourceCopy(const virStorageSource *src, !(def->pr = virStoragePRDefCopy(src->pr))) return NULL; + if (src->nvme && + !(def->nvme = virStorageSourceNVMeDefCopy(src->nvme))) + return NULL; + if (virStorageSourceInitiatorCopy(&def->initiator, &src->initiator)) return NULL; @@ -2376,6 +2423,10 @@ virStorageSourceIsSameLocation(virStorageSourcePtr a, } } + if (a->type == VIR_STORAGE_TYPE_NVME && + !virStorageSourceNVMeDefIsEqual(a->nvme, b->nvme)) + return false; + return true; } @@ -2463,6 +2514,7 @@ virStorageSourceIsLocalStorage(const virStorageSource *src) case VIR_STORAGE_TYPE_NETWORK: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_LAST: case VIR_STORAGE_TYPE_NONE: return false; @@ -2493,6 +2545,10 @@ virStorageSourceIsEmpty(virStorageSourcePtr src) src->protocol == VIR_STORAGE_NET_PROTOCOL_NONE) return true; + if (src->type == VIR_STORAGE_TYPE_NVME && + !src->nvme) + return true; + return false; } @@ -2548,6 +2604,7 @@ virStorageSourceClear(virStorageSourcePtr def) VIR_FREE(def->compat); virStorageEncryptionFree(def->encryption); virStoragePRDefFree(def->pr); + virStorageSourceNVMeDefFree(def->nvme); virStorageSourceSeclabelsClear(def); virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); @@ -3776,6 +3833,7 @@ virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, /* We shouldn't get VOLUME, but the switch requires all cases */ case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -4242,6 +4300,7 @@ virStorageSourceIsRelative(virStorageSourcePtr src) case VIR_STORAGE_TYPE_NETWORK: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: return false; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 38ba901858..a1294ea608 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -31,6 +31,7 @@ #include "virsecret.h" #include "virautoclean.h" #include "virenum.h" +#include "virpci.h" /* Minimum header size required to probe all known formats with * virStorageFileProbeFormat, or obtain metadata from a known format. @@ -52,6 +53,7 @@ typedef enum { VIR_STORAGE_TYPE_DIR, VIR_STORAGE_TYPE_NETWORK, VIR_STORAGE_TYPE_VOLUME, + VIR_STORAGE_TYPE_NVME, VIR_STORAGE_TYPE_LAST } virStorageType; @@ -231,6 +233,14 @@ struct _virStorageSourceInitiatorDef { char *iqn; /* Initiator IQN */ }; +typedef struct _virStorageSourceNVMeDef virStorageSourceNVMeDef; +typedef virStorageSourceNVMeDef *virStorageSourceNVMeDefPtr; +struct _virStorageSourceNVMeDef { + unsigned long namespace; + int managed; /* enum virTristateBool */ + virPCIDeviceAddress pciAddr; +}; + typedef struct _virStorageDriverData virStorageDriverData; typedef virStorageDriverData *virStorageDriverDataPtr; @@ -262,6 +272,8 @@ struct _virStorageSource { bool encryptionInherited; virStoragePRDefPtr pr; + virStorageSourceNVMeDefPtr nvme; /* type == VIR_STORAGE_TYPE_NVME */ + virStorageSourceInitiatorDef initiator; virObjectPtr privateData; @@ -416,6 +428,9 @@ bool virStoragePRDefIsManaged(virStoragePRDefPtr prd); bool virStorageSourceChainHasManagedPR(virStorageSourcePtr src); +void virStorageSourceNVMeDefFree(virStorageSourceNVMeDefPtr def); +VIR_DEFINE_AUTOPTR_FUNC(virStorageSourceNVMeDef, virStorageSourceNVMeDefFree); + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model); diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index ca094d30c2..fa15e4e2a5 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -1662,6 +1662,7 @@ xenFormatXLDiskSrc(virStorageSourcePtr src, char **srcstr) break; case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: break; diff --git a/tests/qemuxml2argvdata/disk-nvme.xml b/tests/qemuxml2argvdata/disk-nvme.xml index 0b3dbad4eb..fe956d5ab6 100644 --- a/tests/qemuxml2argvdata/disk-nvme.xml +++ b/tests/qemuxml2argvdata/disk-nvme.xml @@ -20,6 +20,7 @@ <address domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> </source> <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> <disk type='nvme' device='disk'> <driver name='qemu' type='raw'/> @@ -27,6 +28,7 @@ <address domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> </source> <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </disk> <disk type='nvme' device='disk'> <driver name='qemu' type='raw'/> @@ -34,6 +36,7 @@ <address domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> </source> <target dev='vdc' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> </disk> <disk type='nvme' device='disk'> <driver name='qemu' type='raw'/> @@ -44,10 +47,15 @@ </encryption> </source> <target dev='vdd' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> </disk> - <controller type='usb' index='0'/> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> <controller type='pci' index='0' model='pci-root'/> - <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <memballoon model='none'/> diff --git a/tests/qemuxml2xmloutdata/disk-nvme.xml b/tests/qemuxml2xmloutdata/disk-nvme.xml new file mode 120000 index 0000000000..ea9eb267ac --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-nvme.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/disk-nvme.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 6d808e172f..c9f3a8dbfa 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -336,6 +336,7 @@ mymain(void) DO_TEST("disk-network-sheepdog", NONE); DO_TEST("disk-network-vxhs", NONE); DO_TEST("disk-network-tlsx509", NONE); + DO_TEST("disk-nvme", QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-scsi", QEMU_CAPS_SCSI_LSI, QEMU_CAPS_SCSI_MEGASAS, QEMU_CAPS_SCSI_MPTSAS1068, QEMU_CAPS_SCSI_DISK_WWN); DO_TEST("disk-virtio-scsi-reservations", -- 2.21.0

On 7/11/19 6:05 PM, Peter Krempa wrote: Is that so? I mean, other checks done in this funtion check only 'top level' disk->src too. Possibly. But since other drivers would still use virNVMeDevice module I'm adding later in this series, and since the module is build on these assumptions I figured the best place to check for them is in driver agnostic callback. Michal

On Thu, Jul 11, 2019 at 17:53:58 +0200, Michal Privoznik wrote: [...] As noted in the other thread, this really should be extracted, placed in the validation callback rather than post parse and must iterate the backing chain if you want this to keep working with -blockdev. I'm displeased that this is yet another function adding validation in the parser. You don't make the status quo any worse though so this is not a request to change it. [...] Missing change to 'qemuMigrationSrcIsSafe' to reject VMs containing NVMe disks not having shared source. Plus that function should probably check the full backing chain rather than the top element only. [...] You opted to use memcpy for the pci address few patches ago. [...] Welp. While I agree that virStorageSourceIsLocalStorage should return false you should really add a documentation comment explaining why NVMe is different. (e.g. regular code accessing src->path would not work). Formating a disk type='nvme' without any data would not be parseable in our parser, so this will never happen. [...] [...] Add a comment noting that the copy function needs to be fixed if this is ever being updated 'long' is either 32 or 64 bit depending on the architecture, please use unsigned int or unsigned long long. [...] Do these need to be exposed? [...] All of these belong to the previous patch adding the test file in the first place.

On Wed, Jul 17, 2019 at 17:05:08 +0200, Michal Privoznik wrote: Sorry I thought it was in post-parse. This is okay. Yes, but that stuff still does not have to be intermixed in the parser code. It's a pre-existing mess though. I meant that the NVMe disk is NOT available on the destination of the migration. That means that 'qemuMigrationSrcIsSafe' must reject it as not having a shared storage (note that "shared" here has a different conotation as <shareable/>, just look at he named function).

On Thu, Jul 11, 2019 at 17:53:59 +0200, Michal Privoznik wrote: [...] please use a different log domain unsinged int/unsigned long long [...] [1] (see below) This function seems to be too unsafe to export as people might want to store the index while not holding the lock and something would then change it. Also [1] has the same issue. This function is too complex to be without a comment describing it. Especially since it's returning list of pci devices. Note that I did not look at the patches using the code, thus comments about some APIs being unnecessary may not be true.

Now that we have virNVMeDevice module (introduced in previous commit), let's use it int virHostdev to track which NVMe devices are free to be used by a domain and which are taken. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 3 + src/util/virhostdev.c | 244 +++++++++++++++++++++++++++++++++++++++ src/util/virhostdev.h | 25 ++++ 3 files changed, 272 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 856b770e57..bc6583562a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2077,18 +2077,21 @@ virHostdevPCINodeDeviceReAttach; virHostdevPCINodeDeviceReset; virHostdevPrepareDomainDevices; virHostdevPrepareMediatedDevices; +virHostdevPrepareNVMeDevices; virHostdevPreparePCIDevices; virHostdevPrepareSCSIDevices; virHostdevPrepareSCSIVHostDevices; virHostdevPrepareUSBDevices; virHostdevReAttachDomainDevices; virHostdevReAttachMediatedDevices; +virHostdevReAttachNVMeDevices; virHostdevReAttachPCIDevices; virHostdevReAttachSCSIDevices; virHostdevReAttachSCSIVHostDevices; virHostdevReAttachUSBDevices; virHostdevUpdateActiveDomainDevices; virHostdevUpdateActiveMediatedDevices; +virHostdevUpdateActiveNVMeDevices; virHostdevUpdateActivePCIDevices; virHostdevUpdateActiveSCSIDevices; virHostdevUpdateActiveUSBDevices; diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 07397b9682..90d94b0a92 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -137,6 +137,7 @@ virHostdevManagerDispose(void *obj) virObjectUnref(hostdevMgr->activeSCSIHostdevs); virObjectUnref(hostdevMgr->activeSCSIVHostHostdevs); virObjectUnref(hostdevMgr->activeMediatedHostdevs); + virObjectUnref(hostdevMgr->activeNVMeHostdevs); VIR_FREE(hostdevMgr->stateDir); } @@ -167,6 +168,9 @@ virHostdevManagerNew(void) if (!(hostdevMgr->activeMediatedHostdevs = virMediatedDeviceListNew())) return NULL; + if (!(hostdevMgr->activeNVMeHostdevs = virNVMeDeviceListNew())) + return NULL; + if (privileged) { if (VIR_STRDUP(hostdevMgr->stateDir, HOSTDEV_STATE_DIR) < 0) return NULL; @@ -2229,3 +2233,243 @@ virHostdevUpdateActiveDomainDevices(virHostdevManagerPtr mgr, return 0; } + + +static virNVMeDeviceListPtr +virHostdevGetNVMeDeviceList(virDomainDiskDefPtr *disks, + size_t ndisks, + const char *drv_name, + const char *dom_name) +{ + VIR_AUTOUNREF(virNVMeDeviceListPtr) nvmeDevices = NULL; + size_t i; + + if (!(nvmeDevices = virNVMeDeviceListNew())) + return NULL; + + for (i = 0; i < ndisks; i++) { + virDomainDiskDefPtr disk = disks[i]; + virStorageSourcePtr n; + + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { + VIR_AUTOPTR(virNVMeDevice) dev = NULL; + const virStorageSourceNVMeDef *srcNVMe = n->nvme; + + if (n->type != VIR_STORAGE_TYPE_NVME) + continue; + + if (!(dev = virNVMeDeviceNew(&srcNVMe->pciAddr, + srcNVMe->namespace, + srcNVMe->managed))) + return NULL; + + if (virNVMeDeviceUsedBySet(dev, drv_name, dom_name) < 0) + return NULL; + + if (virNVMeDeviceListAdd(nvmeDevices, dev) < 0) + return NULL; + } + } + + VIR_RETURN_PTR(nvmeDevices); +} + + +int +virHostdevPrepareNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks) +{ + VIR_AUTOUNREF(virNVMeDeviceListPtr) nvmeDevices = NULL; + VIR_AUTOUNREF(virPCIDeviceListPtr) pciDevices = NULL; + const unsigned int pciFlags = 0; + virNVMeDevicePtr temp = NULL; + size_t i; + ssize_t lastGoodNVMeIdx = -1; + int ret = -1; + + if (!(nvmeDevices = virHostdevGetNVMeDeviceList(disks, ndisks, drv_name, dom_name))) + return -1; + + if (virNVMeDeviceListCount(nvmeDevices) == 0) + return 0; + + virObjectLock(hostdev_mgr->activeNVMeHostdevs); + + /* Firstly, let's check if all devices are free */ + for (i = 0; i < virNVMeDeviceListCount(nvmeDevices); i++) { + const virNVMeDevice *dev = virNVMeDeviceListGet(nvmeDevices, i); + const virPCIDeviceAddress *addr = NULL; + VIR_AUTOFREE(char *) addrStr = NULL; + const char *actual_drvname = NULL; + const char *actual_domname = NULL; + + temp = virNVMeDeviceListLookup(hostdev_mgr->activeNVMeHostdevs, dev); + + /* Not on the list means not used */ + if (!temp) + continue; + + virNVMeDeviceUsedByGet(temp, &actual_drvname, &actual_domname); + addr = virNVMeDeviceAddressGet(dev); + addrStr = virPCIDeviceAddressAsString(addr); + + virReportError(VIR_ERR_OPERATION_INVALID, + _("NVMe device %s already in use by driver %s domain %s"), + NULLSTR(addrStr), actual_drvname, actual_domname); + goto cleanup; + } + + if (!(pciDevices = virNVMeDeviceListCreateDetachList(hostdev_mgr->activeNVMeHostdevs, + nvmeDevices))) + goto cleanup; + + /* This looks like a good opportunity to merge inactive NVMe devices onto + * the active list. This, however, means that if something goes wrong we + * have to perform a rollback before returning.*/ + for (i = 0; i < virNVMeDeviceListCount(nvmeDevices); i++) { + temp = virNVMeDeviceListGet(nvmeDevices, i); + + if (virNVMeDeviceListAdd(hostdev_mgr->activeNVMeHostdevs, temp) < 0) + goto rollback; + + lastGoodNVMeIdx = i; + } + + if (virHostdevPreparePCIDevicesImpl(hostdev_mgr, + drv_name, dom_name, NULL, + pciDevices, NULL, 0, pciFlags) < 0) + goto rollback; + + ret = 0; + cleanup: + virObjectUnlock(hostdev_mgr->activeNVMeHostdevs); + return ret; + + rollback: + while (lastGoodNVMeIdx >= 0) { + temp = virNVMeDeviceListGet(nvmeDevices, lastGoodNVMeIdx); + + virNVMeDeviceListDel(hostdev_mgr->activeNVMeHostdevs, temp); + + lastGoodNVMeIdx--; + } + goto cleanup; +} + + +int +virHostdevReAttachNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks) +{ + VIR_AUTOUNREF(virNVMeDeviceListPtr) nvmeDevices = NULL; + VIR_AUTOUNREF(virPCIDeviceListPtr) pciDevices = NULL; + size_t i; + int ret = -1; + + if (!(nvmeDevices = virHostdevGetNVMeDeviceList(disks, ndisks, drv_name, dom_name))) + return -1; + + if (virNVMeDeviceListCount(nvmeDevices) == 0) + return 0; + + virObjectLock(hostdev_mgr->activeNVMeHostdevs); + + if (!(pciDevices = virNVMeDeviceListCreateReAttachList(hostdev_mgr->activeNVMeHostdevs, + nvmeDevices))) + goto cleanup; + + virHostdevReAttachPCIDevicesImpl(hostdev_mgr, + drv_name, dom_name, pciDevices, + NULL, 0, NULL); + + for (i = 0; i < virNVMeDeviceListCount(nvmeDevices); i++) { + virNVMeDevicePtr temp = virNVMeDeviceListGet(nvmeDevices, i); + + if (virNVMeDeviceListDel(hostdev_mgr->activeNVMeHostdevs, temp) < 0) + goto cleanup; + } + + ret = 0; + cleanup: + virObjectUnlock(hostdev_mgr->activeNVMeHostdevs); + return ret; +} + + +int +virHostdevUpdateActiveNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks) +{ + VIR_AUTOUNREF(virNVMeDeviceListPtr) nvmeDevices = NULL; + VIR_AUTOUNREF(virPCIDeviceListPtr) pciDevices = NULL; + virNVMeDevicePtr temp = NULL; + size_t i; + ssize_t lastGoodNVMeIdx = -1; + ssize_t lastGoodPCIIdx = -1; + int ret = -1; + + if (!(nvmeDevices = virHostdevGetNVMeDeviceList(disks, ndisks, drv_name, dom_name))) + return -1; + + if (virNVMeDeviceListCount(nvmeDevices) == 0) + return 0; + + virObjectLock(hostdev_mgr->activeNVMeHostdevs); + virObjectLock(hostdev_mgr->activePCIHostdevs); + virObjectLock(hostdev_mgr->inactivePCIHostdevs); + + if (!(pciDevices = virNVMeDeviceListCreateDetachList(hostdev_mgr->activeNVMeHostdevs, + nvmeDevices))) + goto cleanup; + + for (i = 0; i < virNVMeDeviceListCount(nvmeDevices); i++) { + temp = virNVMeDeviceListGet(nvmeDevices, i); + + if (virNVMeDeviceListAdd(hostdev_mgr->activeNVMeHostdevs, temp) < 0) + goto rollback; + + lastGoodNVMeIdx = i; + } + + for (i = 0; i < virPCIDeviceListCount(pciDevices); i++) { + virPCIDevicePtr actual = virPCIDeviceListGet(pciDevices, i); + + if (virPCIDeviceListAddCopy(hostdev_mgr->activePCIHostdevs, actual) < 0) + goto rollback; + + lastGoodPCIIdx = i; + } + + ret = 0; + cleanup: + virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); + virObjectUnlock(hostdev_mgr->activePCIHostdevs); + virObjectUnlock(hostdev_mgr->activeNVMeHostdevs); + return ret; + + rollback: + while (lastGoodNVMeIdx >= 0) { + temp = virNVMeDeviceListGet(nvmeDevices, lastGoodNVMeIdx); + + virNVMeDeviceListDel(hostdev_mgr->activeNVMeHostdevs, temp); + + lastGoodNVMeIdx--; + } + while (lastGoodPCIIdx >= 0) { + virPCIDevicePtr actual = virPCIDeviceListGet(pciDevices, i); + + virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, actual); + + lastGoodPCIIdx--; + } + goto cleanup; +} diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 88501e2743..98dc226631 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -29,6 +29,7 @@ #include "virscsivhost.h" #include "conf/domain_conf.h" #include "virmdev.h" +#include "virnvme.h" typedef enum { VIR_HOSTDEV_STRICT_ACS_CHECK = (1 << 0), /* strict acs check */ @@ -53,6 +54,9 @@ struct _virHostdevManager { virSCSIDeviceListPtr activeSCSIHostdevs; virSCSIVHostDeviceListPtr activeSCSIVHostHostdevs; virMediatedDeviceListPtr activeMediatedHostdevs; + /* NVMe devices are PCI devices really, but one NVMe disk can + * have multiple namespaces. */ + virNVMeDeviceListPtr activeNVMeHostdevs; }; virHostdevManagerPtr virHostdevManagerGetDefault(void); @@ -201,3 +205,24 @@ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr, int virHostdevPCINodeDeviceReset(virHostdevManagerPtr mgr, virPCIDevicePtr pci) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int +virHostdevPrepareNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks); + +int +virHostdevReAttachNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks); + +int +virHostdevUpdateActiveNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks); -- 2.21.0

On Thu, Jul 11, 2019 at 17:54:05 +0200, Michal Privoznik wrote: Please include "Disks" in the function name co clarify it's not a hostdev from the domain point. ACK to the rest

On Thu, Jul 11, 2019 at 17:54:06 +0200, Michal Privoznik wrote: preceeding hunk for (i = 0; i < def->nhostdevs; i++) { virDomainHostdevSubsysPtr subsys = &def->hostdevs[i]->source.subsys; if (def->hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV || (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && subsys->u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO))) { memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024; goto done; } } Please set a booleand such as 'needVFIO' in the above hunk and here and do the calculation once based on that boolean. This implementation creates two instancess needing fixing in case when we'd need to ever change the number. ACK with that change

On Thu, Jul 11, 2019 at 17:54:07 +0200, Michal Privoznik wrote: [...] It occurs to me that if you introduce this function earlier you will be able to save some code in the previous patches. ACK to this though.

On Thu, Jul 11, 2019 at 17:54:08 +0200, Michal Privoznik wrote: Same comment as in previous patch. ACK.

On Thu, Jul 11, 2019 at 17:54:09 +0200, Michal Privoznik wrote: If you plan to add some NVMe disk stuff to this function later please add a comment explaining what and why is happening here. ACK

On Thu, Jul 11, 2019 at 17:54:10 +0200, Michal Privoznik wrote: Name of this function is VERY misleading. It returns path of the IOMMU group associated with the host portion of the NVMe device, but the name implies smething completely different. Include IOMMUGroup in the name and add a comment. Also this name looks troubling. I don't like this. It's hiding the stuff necessary to detach VFIO groups in random function and the hostdev code will require exactly the same treatment. Additionally any further possible VFIO based device would require 3 places. Can't we consolidate that somehow?

If a domain has an NVMe disk configured, then we need to create /dev/vfio/* paths in domain's namespace so that qemu can open them. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 949bbace88..cd3205a588 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11831,7 +11831,8 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, perm = VIR_CGROUP_DEVICE_RW; if (teardown) { - if (!virDomainDefHasVFIOHostdev(def)) + if (!virDomainDefHasVFIOHostdev(def) && + !virDomainDefHasNVMeDisk(def)) includeVFIO = true; } else { includeVFIO = true; @@ -12415,6 +12416,22 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, int ret = -1; for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) { + /* NVMe disks must be checked before virStorageSourceIsLocalStorage() + * is called. This is because while NVMe disks are local, they don't + * have next->path set. */ + if (next->type == VIR_STORAGE_TYPE_NVME) { + VIR_AUTOSTRINGLIST nvmePaths = NULL; + size_t i; + + if (!(nvmePaths = qemuDomainGetDiskNVMePaths(NULL, next, false))) + goto cleanup; + + for (i = 0; nvmePaths[i]; i++) { + if (qemuDomainCreateDevice(nvmePaths[i], data, false) < 0) + goto cleanup; + } + } + if (!next->path || !virStorageSourceIsLocalStorage(next)) { /* Not creating device. Just continue. */ continue; @@ -13462,12 +13479,28 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, virStorageSourcePtr src) { virStorageSourcePtr next; + VIR_AUTOSTRINGLIST nvmePaths = NULL; const char **paths = NULL; size_t npaths = 0; char *dmPath = NULL; int ret = -1; for (next = src; virStorageSourceIsBacking(next); next = next->backingStore) { + /* NVMe disks must be checked before virStorageSourceIsLocalStorage() + * is called. This is because while NVMe disks are local, they don't + * have next->path set. */ + if (next->type == VIR_STORAGE_TYPE_NVME) { + size_t i; + + if (!(nvmePaths = qemuDomainGetDiskNVMePaths(NULL, next, false))) + goto cleanup; + + for (i = 0; nvmePaths[i]; i++) { + if (VIR_APPEND_ELEMENT_COPY(paths, npaths, nvmePaths[i]) < 0) + goto cleanup; + } + } + if (virStorageSourceIsEmpty(next) || !virStorageSourceIsLocalStorage(next)) { /* Not creating device. Just continue. */ -- 2.21.0

On 7/16/19 4:37 PM, Peter Krempa wrote: The thing is that NVMe disks are both hostdevs and disks. So whenever we deal with /dev/vfio/* we have to consider both. One solution that comes to my mind is to take /dev/vfio/vfio completely out of the picture on qemuDomainGetHostdevPath() and qemuDomainGetNVMeDiskIOMMUGroupPaths() levels, have them return a single path that device is associated with (/dev/vfio/N) and let caller do checks then if /dev/vfio/vfio must also be included in whatever it is they want to do. Michal

On Thu, Jul 11, 2019 at 17:54:12 +0200, Michal Privoznik wrote: Yeah, that should be fine. We tell qemu to open it R/O afterwards and we can't do better here. [...] What if the IOMMU group is shared by another disk? or perhaps even an hostdev?

The summary is misleading. Mention NVNe and since you are fixing two functions don't mention the name. On Thu, Jul 11, 2019 at 17:54:14 +0200, Michal Privoznik wrote: This is not a virObject. Same as above. same as above. same as above.

Now, that we have everything prepared, we can generate command line for NVMe disks. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_block.c | 25 ++++++++- src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_process.c | 7 +++ .../disk-nvme.x86_64-latest.args | 52 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 5 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/disk-nvme.x86_64-latest.args diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 5eeb3757f1..bf49b7bc7c 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -992,6 +992,25 @@ qemuBlockStorageSourceGetVvfatProps(virStorageSourcePtr src) } +static virJSONValuePtr +qemuBlockStorageSourceGetNVMeProps(virStorageSourcePtr src) +{ + const virStorageSourceNVMeDef *nvme = src->nvme; + VIR_AUTOFREE(char *) pciAddr = NULL; + virJSONValuePtr ret = NULL; + + if (!(pciAddr = virPCIDeviceAddressAsString(&nvme->pciAddr))) + return NULL; + + ignore_value(virJSONValueObjectCreate(&ret, + "s:driver", "nvme", + "s:device", pciAddr, + "U:namespace", nvme->namespace, + NULL)); + return ret; +} + + static int qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSourcePtr src, virJSONValuePtr props) @@ -1049,8 +1068,12 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, return NULL; break; - case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NVME: + if (!(fileprops = qemuBlockStorageSourceGetNVMeProps(src))) + return NULL; + break; + + case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: return NULL; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 927641cf46..e42377927e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1594,6 +1594,9 @@ qemuDiskSourceNeedsProps(virStorageSourcePtr src, src->haveTLS == VIR_TRISTATE_BOOL_YES) return true; + if (actualType == VIR_STORAGE_TYPE_NVME) + return true; + return false; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index aa09ef175a..ebe35e6363 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5294,6 +5294,13 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm, _("PowerPC pseries machines do not support floppy device")); return -1; } + + if (src->type == VIR_STORAGE_TYPE_NVME && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_NVME)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("NVMe disks are not supported with this QEMU binary")); + return -1; + } } return 0; diff --git a/tests/qemuxml2argvdata/disk-nvme.x86_64-latest.args b/tests/qemuxml2argvdata/disk-nvme.x86_64-latest.args new file mode 100644 index 0000000000..5ff41de7b9 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-nvme.x86_64-latest.args @@ -0,0 +1,52 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ +-drive file.driver=nvme,file.device=0000:01:00.0,file.namespace=1,format=raw,\ +if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ +id=virtio-disk0,bootindex=1 \ +-drive file.driver=nvme,file.device=0000:01:00.0,file.namespace=2,format=raw,\ +if=none,id=drive-virtio-disk1 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\ +id=virtio-disk1 \ +-drive file.driver=nvme,file.device=0000:02:00.0,file.namespace=1,format=raw,\ +if=none,id=drive-virtio-disk2 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk2,\ +id=virtio-disk2 \ +-object secret,id=virtio-disk3-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file.driver=nvme,file.device=0001:02:00.0,file.namespace=2,\ +key-secret=virtio-disk3-luks-secret0,format=luks,if=none,id=drive-virtio-disk3 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio-disk3,\ +id=virtio-disk3 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0ac128be00..d4a16c35a6 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1068,6 +1068,7 @@ mymain(void) driver.config->vxhsTLS = 0; VIR_FREE(driver.config->vxhsTLSx509certdir); DO_TEST("disk-no-boot", NONE); + DO_TEST_CAPS_LATEST("disk-nvme"); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", QEMU_CAPS_VIRTIO_SCSI); DO_TEST_FAILURE("disk-usb-nosupport", NONE); -- 2.21.0

On Thu, Jul 11, 2019 at 17:54:18 +0200, Michal Privoznik wrote: What's the point of this ... ... if this just exits the function? [...] I'd prefer if you could base this on top of the new disk source tracking for new blockjobs I've done for -blockdev support: git fetch https://github.com/pipo/libvirt.git job-tracking-send That code tentatively moves this out if the disk backend is still required while the frontend was unplugged.