On 12/2/19 9:26 AM, Michal Privoznik wrote: This last line should be dropped, it is still called like that via qemuSetupDevicesCgroup Maybe just add a comment here explaining that, at VM startup, this can lead to /dev/vfio/vfio being added to the cgroup multiple times, but that has never shown to be a problem. Reviewed-by: Cole Robinson <crobinso(a)redhat.com&gt; - Cole

On 12/2/19 9:26 AM, Michal Privoznik wrote: 6.0.0 or whatever version this will land in Same Stray change? Also, tn the test XML you need to "s/qemu-system-i686/qemu-system-i386/" or you'll hit a weird error. And VIR_TEST_REGENERATE_OUTPUT is also busted, see my patches elsewhere on this list. Reviewed-by: Cole Robinson <crobinso(a)redhat.com&gt; - Cole

This function will return true if there's a storage source of type VIR_STORAGE_TYPE_NVME, or false otherwise. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ACKed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 14 ++++++++++++++ src/util/virstoragefile.h | 2 ++ 3 files changed, 17 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d37d3867a6..a7f94feb0a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3057,6 +3057,7 @@ virStoragePRDefIsManaged; virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceChainHasManagedPR; +virStorageSourceChainHasNVMe; virStorageSourceClear; virStorageSourceCopy; virStorageSourceFindByNodeName; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b4ec9624be..5a5a16aef7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2138,6 +2138,20 @@ virStorageSourceNVMeDefFree(virStorageSourceNVMeDefPtr def) } +bool +virStorageSourceChainHasNVMe(const virStorageSource *src) +{ + const virStorageSource *n; + + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (n->type == VIR_STORAGE_TYPE_NVME) + return true; + } + + return false; +} + + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 42ca49bc64..39e50a989d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -433,6 +433,8 @@ virStorageSourceChainHasManagedPR(virStorageSourcePtr src); void virStorageSourceNVMeDefFree(virStorageSourceNVMeDefPtr def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSourceNVMeDef, virStorageSourceNVMeDefFree); +bool virStorageSourceChainHasNVMe(const virStorageSource *src); + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model); -- 2.23.0

This function will return true if any of disks (or their backing chain) for given domain contains an NVMe disk. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ACKed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 14 ++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 18 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 58879a66ba..b6e04db0f7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31467,6 +31467,20 @@ virDomainDefHasManagedPR(const virDomainDef *def) } +bool +virDomainDefHasNVMeDisk(const virDomainDef *def) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + if (virStorageSourceChainHasNVMe(def->disks[i]->src)) + return true; + } + + return false; +} + + bool virDomainDefHasVFIOHostdev(const virDomainDef *def) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 49cb6970e6..520d2da0d2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3674,6 +3674,9 @@ virDomainDiskGetDetectZeroesMode(virDomainDiskDiscard discard, bool virDomainDefHasManagedPR(const virDomainDef *def); +bool +virDomainDefHasNVMeDisk(const virDomainDef *def); + bool virDomainDefHasVFIOHostdev(const virDomainDef *def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7f94feb0a..86ee9cbc53 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -297,6 +297,7 @@ virDomainDefHasManagedPR; virDomainDefHasMdevHostdev; virDomainDefHasMemballoon; virDomainDefHasMemoryHotplug; +virDomainDefHasNVMeDisk; virDomainDefHasUSB; virDomainDefHasVcpusOffline; virDomainDefHasVFIOHostdev; -- 2.23.0

This module will be used by virHostdevManager and it's inspired by virPCIDevice module. They are very similar except instead of what makes a NVMe device: PCI address AND namespace ID. This means that a NVMe device can appear in a domain multiple times, each time with a different namespace. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- po/POTFILES.in | 1 + src/libvirt_private.syms | 18 ++ src/util/Makefile.inc.am | 2 + src/util/virnvme.c | 447 +++++++++++++++++++++++++++++++++++++++ src/util/virnvme.h | 95 +++++++++ 5 files changed, 563 insertions(+) create mode 100644 src/util/virnvme.c create mode 100644 src/util/virnvme.h diff --git a/po/POTFILES.in b/po/POTFILES.in index debb51cd70..3d075201c4 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -277,6 +277,7 @@ @SRCDIR(a)/src/util/virnetlink.c @SRCDIR(a)/src/util/virnodesuspend.c @SRCDIR(a)/src/util/virnuma.c +@SRCDIR(a)/src/util/virnvme.c @SRCDIR(a)/src/util/virobject.c @SRCDIR(a)/src/util/virpci.c @SRCDIR(a)/src/util/virperf.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 86ee9cbc53..aab2b49bdf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2664,6 +2664,24 @@ virNumaSetPagePoolSize; virNumaSetupMemoryPolicy; +# util/virnvme.h +virNVMeDeviceAddressGet; +virNVMeDeviceCopy; +virNVMeDeviceFree; +virNVMeDeviceListAdd; +virNVMeDeviceListCount; +virNVMeDeviceListCreateDetachList; +virNVMeDeviceListDel; +virNVMeDeviceListGet; +virNVMeDeviceListLookup; +virNVMeDeviceListLookupIndex; +virNVMeDeviceListNew; +virNVMeDeviceNew; +virNVMeDeviceUsedByClear; +virNVMeDeviceUsedByGet; +virNVMeDeviceUsedBySet; + + # util/virobject.h virClassForObject; virClassForObjectLockable; diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index 0855f152fd..838e9479a9 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -150,6 +150,8 @@ UTIL_SOURCES = \ util/virnetlink.h \ util/virnodesuspend.c \ util/virnodesuspend.h \ + util/virnvme.c \ + util/virnvme.h \ util/virkmod.c \ util/virkmod.h \ util/virnuma.c \ diff --git a/src/util/virnvme.c b/src/util/virnvme.c new file mode 100644 index 0000000000..b8179aa431 --- /dev/null +++ b/src/util/virnvme.c @@ -0,0 +1,447 @@ +/* + * virnvme.c: helper APIs for managing NVMe devices + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + */ + +#include <config.h> + +#include "virnvme.h" +#include "virobject.h" +#include "virpci.h" +#include "viralloc.h" +#include "virlog.h" +#include "virstring.h" + +VIR_LOG_INIT("util.nvme"); +#define VIR_FROM_THIS VIR_FROM_NONE + +struct _virNVMeDevice { + virPCIDeviceAddress address; /* PCI address of controller */ + unsigned int namespace; /* Namespace ID */ + bool managed; + + char *drvname; + char *domname; +}; + + +struct _virNVMeDeviceList { + virObjectLockable parent; + + size_t count; + virNVMeDevicePtr *devs; +}; + + +static virClassPtr virNVMeDeviceListClass; + +static void virNVMeDeviceListDispose(void *obj); + +static int +virNVMeOnceInit(void) +{ + if (!VIR_CLASS_NEW(virNVMeDeviceList, virClassForObjectLockable())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNVMe); + + +virNVMeDevicePtr +virNVMeDeviceNew(const virPCIDeviceAddress *address, + unsigned long namespace, + bool managed) +{ + virNVMeDevicePtr dev = NULL; + + dev = g_new0(virNVMeDevice, 1); + + virPCIDeviceAddressCopy(&dev->address, address); + dev->namespace = namespace; + dev->managed = managed; + + return dev; +} + + +void +virNVMeDeviceFree(virNVMeDevicePtr dev) +{ + if (!dev) + return; + + virNVMeDeviceUsedByClear(dev); + VIR_FREE(dev); +} + + +virNVMeDevicePtr +virNVMeDeviceCopy(const virNVMeDevice *dev) +{ + virNVMeDevicePtr copy = NULL; + + copy = g_new0(virNVMeDevice, 1); + copy->drvname = g_strdup(dev->drvname); + copy->domname = g_strdup(dev->domname); + + virPCIDeviceAddressCopy(&copy->address, &dev->address); + copy->namespace = dev->namespace; + copy->managed = dev->managed; + + return copy; +} + + +const virPCIDeviceAddress * +virNVMeDeviceAddressGet(const virNVMeDevice *dev) +{ + return &dev->address; +} + + +void +virNVMeDeviceUsedByClear(virNVMeDevicePtr dev) +{ + VIR_FREE(dev->drvname); + VIR_FREE(dev->domname); +} + + +void +virNVMeDeviceUsedByGet(const virNVMeDevice *dev, + const char **drv, + const char **dom) +{ + *drv = dev->drvname; + *dom = dev->domname; +} + + +void +virNVMeDeviceUsedBySet(virNVMeDevicePtr dev, + const char *drv, + const char *dom) +{ + dev->drvname = g_strdup(drv); + dev->domname = g_strdup(dom); +} + + +virNVMeDeviceListPtr +virNVMeDeviceListNew(void) +{ + virNVMeDeviceListPtr list; + + if (virNVMeInitialize() < 0) + return NULL; + + if (!(list = virObjectLockableNew(virNVMeDeviceListClass))) + return NULL; + + return list; +} + + +static void +virNVMeDeviceListDispose(void *obj) +{ + virNVMeDeviceListPtr list = obj; + size_t i; + + for (i = 0; i < list->count; i++) + virNVMeDeviceFree(list->devs[i]); + + VIR_FREE(list->devs); +} + + +size_t +virNVMeDeviceListCount(const virNVMeDeviceList *list) +{ + return list->count; +} + + +int +virNVMeDeviceListAdd(virNVMeDeviceListPtr list, + const virNVMeDevice *dev) +{ + virNVMeDevicePtr tmp; + + if ((tmp = virNVMeDeviceListLookup(list, dev))) { + g_autofree char *addrStr = virPCIDeviceAddressAsString(&tmp->address); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NVMe device %s namespace %u is already on the list"), + NULLSTR(addrStr), tmp->namespace); + return -1; + } + + if (!(tmp = virNVMeDeviceCopy(dev)) || + VIR_APPEND_ELEMENT(list->devs, list->count, tmp) < 0) { + virNVMeDeviceFree(tmp); + return -1; + } + + return 0; +} + + +int +virNVMeDeviceListDel(virNVMeDeviceListPtr list, + const virNVMeDevice *dev) +{ + ssize_t idx; + virNVMeDevicePtr tmp = NULL; + + if ((idx = virNVMeDeviceListLookupIndex(list, dev)) < 0) { + g_autofree char *addrStr = virPCIDeviceAddressAsString(&dev->address); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NVMe device %s namespace %u not found"), + NULLSTR(addrStr), dev->namespace); + return -1; + } + + tmp = list->devs[idx]; + VIR_DELETE_ELEMENT(list->devs, idx, list->count); + virNVMeDeviceFree(tmp); + return 0; +} + + +virNVMeDevicePtr +virNVMeDeviceListGet(virNVMeDeviceListPtr list, + size_t i) +{ + return i < list->count ? list->devs[i] : NULL; +} + + +virNVMeDevicePtr +virNVMeDeviceListLookup(virNVMeDeviceListPtr list, + const virNVMeDevice *dev) +{ + ssize_t idx; + + if ((idx = virNVMeDeviceListLookupIndex(list, dev)) < 0) + return NULL; + + return list->devs[idx]; +} + + +ssize_t +virNVMeDeviceListLookupIndex(virNVMeDeviceListPtr list, + const virNVMeDevice *dev) +{ + size_t i; + + if (!list) + return -1; + + for (i = 0; i < list->count; i++) { + virNVMeDevicePtr other = list->devs[i]; + + if (virPCIDeviceAddressEqual(&dev->address, &other->address) && + dev->namespace == other->namespace) + return i; + } + + return -1; +} + + +static virNVMeDevicePtr +virNVMeDeviceListLookupByPCIAddress(virNVMeDeviceListPtr list, + const virPCIDeviceAddress *address) +{ + size_t i; + + if (!list) + return NULL; + + for (i = 0; i < list->count; i++) { + virNVMeDevicePtr other = list->devs[i]; + + if (virPCIDeviceAddressEqual(address, &other->address)) + return other; + } + + return NULL; +} + + +static virPCIDevicePtr +virNVMeDeviceCreatePCIDevice(const virNVMeDevice *nvme) +{ + g_autoptr(virPCIDevice) pci = NULL; + + if (!(pci = virPCIDeviceNew(nvme->address.domain, + nvme->address.bus, + nvme->address.slot, + nvme->address.function))) + return NULL; + + /* NVMe devices must be bound to vfio */ + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); + virPCIDeviceSetManaged(pci, nvme->managed); + + return g_steal_pointer(&pci); +} + + +/** + * virNVMeDeviceListCreateDetachList: + * @activeList: list of active NVMe devices + * @toDetachList: list of NVMe devices to detach from the host + * + * This function creates a list of PCI devices which can then be + * reused by PCI device detach functions (e.g. + * virHostdevPreparePCIDevicesImpl()) as each PCI device from the + * returned list is initialized properly for detach. + * + * Basically, this just blindly collects unique PCI addresses + * from @toDetachList that don't appear on @activeList. + * + * Returns: a list on success, + * NULL otherwise. + */ +virPCIDeviceListPtr +virNVMeDeviceListCreateDetachList(virNVMeDeviceListPtr activeList, + virNVMeDeviceListPtr toDetachList) +{ + g_autoptr(virPCIDeviceList) pciDevices = NULL; + size_t i; + + if (!(pciDevices = virPCIDeviceListNew())) + return NULL; + + for (i = 0; i < toDetachList->count; i++) { + const virNVMeDevice *d = toDetachList->devs[i]; + g_autoptr(virPCIDevice) pci = NULL; + + /* If there is a NVMe device with the same PCI address on + * the activeList, the device is already detached. */ + if (virNVMeDeviceListLookupByPCIAddress(activeList, &d->address)) + continue; + + /* It may happen that we want to detach two namespaces + * from the same NVMe device. This will be represented as + * two different instances of virNVMeDevice, but + * obviously we want to put the PCI device on the detach + * list only once. */ + if (virPCIDeviceListFindByIDs(pciDevices, + d->address.domain, + d->address.bus, + d->address.slot, + d->address.function)) + continue; + + if (!(pci = virNVMeDeviceCreatePCIDevice(d))) + return NULL; + + if (virPCIDeviceListAdd(pciDevices, pci) < 0) + return NULL; + + /* avoid freeing the device */ + pci = NULL; + } + + return g_steal_pointer(&pciDevices); +} + + +/** + * virNVMeDeviceListCreateReAttachList: + * @activeList: list of active NVMe devices + * @toReAttachList: list of devices to reattach to the host + * + * This is a counterpart to virNVMeDeviceListCreateDetachList. + * + * This function creates a list of PCI devices which can then be + * reused by PCI device reattach functions (e.g. + * virHostdevReAttachPCIDevicesImpl()) as each PCI device from + * the returned list is initialized properly for reattach. + * + * Basically, this just collects unique PCI addresses + * of devices that appear on @toReAttachList and are used + * exactly once (i.e. no other namespaces are used from the same + * NVMe device). For that purpose, this function needs to know + * list of active NVMe devices (@activeList). + * + * Returns: a list on success, + * NULL otherwise. + */ +virPCIDeviceListPtr +virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList, + virNVMeDeviceListPtr toReAttachList) +{ + g_autoptr(virPCIDeviceList) pciDevices = NULL; + size_t i; + + if (!(pciDevices = virPCIDeviceListNew())) + return NULL; + + for (i = 0; i < toReAttachList->count; i++) { + const virNVMeDevice *d = toReAttachList->devs[i]; + g_autoptr(virPCIDevice) pci = NULL; + size_t nused = 0; + + /* Check if there is any other NVMe device with the same PCI address as + * @d. To simplify this, let's just count how many NVMe devices with + * the same PCI address there are on the @activeList. */ + for (i = 0; i < activeList->count; i++) { + virNVMeDevicePtr other = activeList->devs[i]; + + if (!virPCIDeviceAddressEqual(&d->address, &other->address)) + continue; + + nused++; + } + + /* Now, the following cases can happen: + * nused > 1 -> there are other NVMe device active, do NOT detach it + * nused == 1 -> we've found only @d on the @activeList, detach it + * nused == 0 -> huh, wait, what? @d is NOT on the @active list, how can + * we reattach it? + */ + + if (nused == 0) { + /* Shouldn't happen (TM) */ + g_autofree char *addrStr = virPCIDeviceAddressAsString(&d->address); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NVMe device %s namespace %u not found"), + NULLSTR(addrStr), d->namespace); + return NULL; + } else if (nused > 1) { + /* NVMe device is still in use */ + continue; + } + + /* nused == 1 -> detach the device */ + if (!(pci = virNVMeDeviceCreatePCIDevice(d))) + return NULL; + + if (virPCIDeviceListAdd(pciDevices, pci) < 0) + return NULL; + + /* avoid freeing the device */ + pci = NULL; + } + + return g_steal_pointer(&pciDevices); +} diff --git a/src/util/virnvme.h b/src/util/virnvme.h new file mode 100644 index 0000000000..911a9d29f3 --- /dev/null +++ b/src/util/virnvme.h @@ -0,0 +1,95 @@ +/* + * virnvme.h: helper APIs for managing NVMe devices + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + */ + +#pragma once + +#include "virpci.h" + +typedef struct _virNVMeDevice virNVMeDevice; +typedef virNVMeDevice *virNVMeDevicePtr; + +/* Note that this list is lockable, and in fact, it is caller's + * responsibility to acquire the lock and release it. The reason + * is that in a lot of cases the list must be locked between two + * API calls and therefore only caller knows when it is safe to + * finally release the lock. */ +typedef struct _virNVMeDeviceList virNVMeDeviceList; +typedef virNVMeDeviceList *virNVMeDeviceListPtr; + +virNVMeDevicePtr +virNVMeDeviceNew(const virPCIDeviceAddress *address, + unsigned long namespace, + bool managed); + +void +virNVMeDeviceFree(virNVMeDevicePtr dev); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNVMeDevice, virNVMeDeviceFree); + +virNVMeDevicePtr +virNVMeDeviceCopy(const virNVMeDevice *dev); + +const virPCIDeviceAddress * +virNVMeDeviceAddressGet(const virNVMeDevice *dev); + +void +virNVMeDeviceUsedByClear(virNVMeDevicePtr dev); + +void +virNVMeDeviceUsedByGet(const virNVMeDevice *dev, + const char **drv, + const char **dom); + +void +virNVMeDeviceUsedBySet(virNVMeDevicePtr dev, + const char *drv, + const char *dom); + +virNVMeDeviceListPtr +virNVMeDeviceListNew(void); + +size_t +virNVMeDeviceListCount(const virNVMeDeviceList *list); + +int +virNVMeDeviceListAdd(virNVMeDeviceListPtr list, + const virNVMeDevice *dev); + +int +virNVMeDeviceListDel(virNVMeDeviceListPtr list, + const virNVMeDevice *dev); + +virNVMeDevicePtr +virNVMeDeviceListGet(virNVMeDeviceListPtr list, + size_t i); + +virNVMeDevicePtr +virNVMeDeviceListLookup(virNVMeDeviceListPtr list, + const virNVMeDevice *dev); + +ssize_t +virNVMeDeviceListLookupIndex(virNVMeDeviceListPtr list, + const virNVMeDevice *dev); + +virPCIDeviceListPtr +virNVMeDeviceListCreateDetachList(virNVMeDeviceListPtr activeList, + virNVMeDeviceListPtr toDetachList); + +virPCIDeviceListPtr +virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList, + virNVMeDeviceListPtr toReAttachList); -- 2.23.0

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 | 5 + src/util/virhostdev.c | 331 +++++++++++++++++++++++++++++++++++++++ src/util/virhostdev.h | 37 +++++ src/util/virnvme.h | 2 + 4 files changed, 375 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aab2b49bdf..398f8c21ac 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2146,18 +2146,23 @@ virHostdevPCINodeDeviceReAttach; virHostdevPCINodeDeviceReset; virHostdevPrepareDomainDevices; virHostdevPrepareMediatedDevices; +virHostdevPrepareNVMeDevices; +virHostdevPrepareOneNVMeDevice; virHostdevPreparePCIDevices; virHostdevPrepareSCSIDevices; virHostdevPrepareSCSIVHostDevices; virHostdevPrepareUSBDevices; virHostdevReAttachDomainDevices; virHostdevReAttachMediatedDevices; +virHostdevReAttachNVMeDevices; +virHostdevReAttachOneNVMeDevice; virHostdevReAttachPCIDevices; virHostdevReAttachSCSIDevices; virHostdevReAttachSCSIVHostDevices; virHostdevReAttachUSBDevices; virHostdevUpdateActiveDomainDevices; virHostdevUpdateActiveMediatedDevices; +virHostdevUpdateActiveNVMeDevices; virHostdevUpdateActivePCIDevices; virHostdevUpdateActiveSCSIDevices; virHostdevUpdateActiveUSBDevices; diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 94a0185597..78e409732a 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -135,6 +135,7 @@ virHostdevManagerDispose(void *obj) virObjectUnref(hostdevMgr->activeSCSIHostdevs); virObjectUnref(hostdevMgr->activeSCSIVHostHostdevs); virObjectUnref(hostdevMgr->activeMediatedHostdevs); + virObjectUnref(hostdevMgr->activeNVMeHostdevs); VIR_FREE(hostdevMgr->stateDir); } @@ -165,6 +166,9 @@ virHostdevManagerNew(void) if (!(hostdevMgr->activeMediatedHostdevs = virMediatedDeviceListNew())) return NULL; + if (!(hostdevMgr->activeNVMeHostdevs = virNVMeDeviceListNew())) + return NULL; + if (privileged) { hostdevMgr->stateDir = g_strdup(HOSTDEV_STATE_DIR); @@ -2235,3 +2239,330 @@ virHostdevUpdateActiveDomainDevices(virHostdevManagerPtr mgr, return 0; } + + +static int +virHostdevGetNVMeDeviceList(virNVMeDeviceListPtr nvmeDevices, + virStorageSourcePtr src, + const char *drv_name, + const char *dom_name) +{ + virStorageSourcePtr n; + + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { + g_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 -1; + + virNVMeDeviceUsedBySet(dev, drv_name, dom_name); + + if (virNVMeDeviceListAdd(nvmeDevices, dev) < 0) + return -1; + } + + return 0; +} + + +int +virHostdevPrepareOneNVMeDevice(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virStorageSourcePtr src) +{ + g_autoptr(virNVMeDeviceList) nvmeDevices = NULL; + g_autoptr(virPCIDeviceList) pciDevices = NULL; + const unsigned int pciFlags = 0; + virNVMeDevicePtr temp = NULL; + size_t i; + ssize_t lastGoodNVMeIdx = -1; + int ret = -1; + + if (!(nvmeDevices = virNVMeDeviceListNew())) + return -1; + + if (virHostdevGetNVMeDeviceList(nvmeDevices, src, drv_name, dom_name) < 0) + 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; + g_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; + + /* Let's check if all PCI devices are NVMe disks. */ + for (i = 0; i < virPCIDeviceListCount(pciDevices); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pciDevices, i); + g_autofree char *drvPath = NULL; + g_autofree char *drvName = NULL; + int stub = VIR_PCI_STUB_DRIVER_NONE; + + if (virPCIDeviceGetDriverPathAndName(pci, &drvPath, &drvName) < 0) + goto cleanup; + + if (drvName) + stub = virPCIStubDriverTypeFromString(drvName); + + if (stub == VIR_PCI_STUB_DRIVER_VFIO || + STREQ_NULLABLE(drvName, "nvme")) + continue; + + VIR_WARN("Suspicious NVMe disk assignment. PCI device " + "%s is not an NVMe disk, it has %s driver", + virPCIDeviceGetName(pci), NULLSTR(drvName)); + } + + /* 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 +virHostdevPrepareNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks) +{ + size_t i; + ssize_t lastGoodDiskIdx = -1; + + for (i = 0; i < ndisks; i++) { + if (virHostdevPrepareOneNVMeDevice(hostdev_mgr, drv_name, + dom_name, disks[i]->src) < 0) + goto rollback; + + lastGoodDiskIdx = i; + } + + return 0; + + rollback: + while (lastGoodDiskIdx >= 0) { + if (virHostdevReAttachOneNVMeDevice(hostdev_mgr, drv_name, dom_name, + disks[lastGoodDiskIdx]->src) < 0) { + VIR_ERROR(_("Failed to reattach NVMe for disk target: %s"), + disks[lastGoodDiskIdx]->dst); + } + + lastGoodDiskIdx--; + } + + return -1; +} + + +int +virHostdevReAttachOneNVMeDevice(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virStorageSourcePtr src) +{ + g_autoptr(virNVMeDeviceList) nvmeDevices = NULL; + g_autoptr(virPCIDeviceList) pciDevices = NULL; + size_t i; + int ret = -1; + + if (!(nvmeDevices = virNVMeDeviceListNew())) + return -1; + + if (virHostdevGetNVMeDeviceList(nvmeDevices, src, drv_name, dom_name) < 0) + 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 +virHostdevReAttachNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks) +{ + size_t i; + int ret = 0; + + /* Contrary to virHostdevPrepareNVMeDevices, this is a best + * effort approach. Just iterate over all disks and try to + * reattach them. Don't stop at the first failure. */ + for (i = 0; i < ndisks; i++) { + if (virHostdevReAttachOneNVMeDevice(hostdev_mgr, drv_name, + dom_name, disks[i]->src) < 0) { + VIR_ERROR(_("Failed to reattach NVMe for disk target: %s"), + disks[i]->dst); + ret = -1; + } + } + + return ret; +} + + +int +virHostdevUpdateActiveNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks) +{ + g_autoptr(virNVMeDeviceList) nvmeDevices = NULL; + g_autoptr(virPCIDeviceList) pciDevices = NULL; + virNVMeDevicePtr temp = NULL; + size_t i; + ssize_t lastGoodNVMeIdx = -1; + ssize_t lastGoodPCIIdx = -1; + int ret = -1; + + if (!(nvmeDevices = virNVMeDeviceListNew())) + return -1; + + for (i = 0; i < ndisks; i++) { + if (virHostdevGetNVMeDeviceList(nvmeDevices, disks[i]->src, drv_name, dom_name) < 0) + 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); + + /* We must restore some attributes that were lost on daemon restart. */ + virPCIDeviceSetUnbindFromStub(actual, true); + if (virPCIDeviceSetUsedBy(actual, drv_name, dom_name) < 0) + goto rollback; + + 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 2d61c21e9d..ae84ed3d20 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; }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virHostdevManager, virObjectUnref); @@ -207,3 +211,36 @@ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr, int virHostdevPCINodeDeviceReset(virHostdevManagerPtr mgr, virPCIDevicePtr pci) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int +virHostdevPrepareOneNVMeDevice(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virStorageSourcePtr src); + +int +virHostdevPrepareNVMeDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainDiskDefPtr *disks, + size_t ndisks); + +int +virHostdevReAttachOneNVMeDevice(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virStorageSourcePtr src); + +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); diff --git a/src/util/virnvme.h b/src/util/virnvme.h index 911a9d29f3..db4d72ab9a 100644 --- a/src/util/virnvme.h +++ b/src/util/virnvme.h @@ -31,6 +31,8 @@ typedef virNVMeDevice *virNVMeDevicePtr; typedef struct _virNVMeDeviceList virNVMeDeviceList; typedef virNVMeDeviceList *virNVMeDeviceListPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNVMeDeviceList, virObjectUnref); + virNVMeDevicePtr virNVMeDeviceNew(const virPCIDeviceAddress *address, unsigned long namespace, -- 2.23.0

The device configs (which are actually the same one config) come from a NVMe disk of mine. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ACKed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/virpcimock.c | 3 +++ tests/virpcitestdata/0000-01-00.0.config | Bin 0 -> 4096 bytes tests/virpcitestdata/0000-02-00.0.config | Bin 0 -> 4096 bytes 3 files changed, 3 insertions(+) create mode 100644 tests/virpcitestdata/0000-01-00.0.config create mode 100644 tests/virpcitestdata/0000-02-00.0.config diff --git a/tests/virpcimock.c b/tests/virpcimock.c index cd6ae1cff6..6048118d5c 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -980,6 +980,7 @@ init_env(void) MAKE_PCI_DRIVER("iwlwifi", 0x8086, 0x0044); MAKE_PCI_DRIVER("i915", 0x8086, 0x0046, 0x8086, 0x0047); MAKE_PCI_DRIVER("vfio-pci", -1, -1); + MAKE_PCI_DRIVER("nvme", 0x1cc1, 0x8201); # define MAKE_PCI_DEVICE(Id, Vendor, Device, IommuGroup, ...) \ do { \ @@ -1015,6 +1016,8 @@ init_env(void) MAKE_PCI_DEVICE("0021:de:1f.1", 0x8086, 0x0047, 13, .physfn = "0021:de:1f.0"); /* Virtual Function */ + MAKE_PCI_DEVICE("0000:01:00.0", 0x1cc1, 0x8201, 14, .klass = 0x010802); + MAKE_PCI_DEVICE("0000:02:00.0", 0x1cc1, 0x8201, 15, .klass = 0x010802); } diff --git a/tests/virpcitestdata/0000-01-00.0.config b/tests/virpcitestdata/0000-01-00.0.config new file mode 100644 index 0000000000000000000000000000000000000000..f92455e2ac5701ce60a51ae19828658b80744399 GIT binary patch literal 4096 zcmeHIF;Buk6#lMP3WC_80!0h0vM?|ZHYb~)#L>jXKcF*<t0@ew#(&`84`6g`<KpZu zaP%LTxZu0C6dH+PAO@3rm)v{zz3*Nx-?guS#YUQHfGar$G8Om~evt*l6}THG3$%ls zbL8T+aGAkfSZ5AOg~nJxaQ{&~b`11hPvNqjks{E-76s`bTjV$zsdNdt2Zx}86r3!0 z5-k@njLH$yMaSt!;XCjcMN7{rhH)Lzgm%?1tR|ap5RC)?OfZuh+-MNn&bPwMnC1$Y zWtY532v8x%Zq%*)y_#9Aly`TwONPEx+$`iba#<~-a)litu!!pkegQ#S0so<=$gQVo zbjxR?KpRD$uKLK=emkc^$-Zgf<*MZ@;uWY8!!UdWrXoze;QR3q_ajXzAQg}b)NzRk z87VP9U%z%(HWJ0B|6JTCgnRh9Wz<V@F5}c=IPK9xUNln!?lk>Mr%9L>;{S2-*!p%x w=b5pKIZ+ec2|JnLT|5EZ*;+<Yfz>MDr9bc*RexLU6J#~1fK)&#@Fxm<1DBgMR{#J2 literal 0 HcmV?d00001 diff --git a/tests/virpcitestdata/0000-02-00.0.config b/tests/virpcitestdata/0000-02-00.0.config new file mode 100644 index 0000000000000000000000000000000000000000..ebb44d8f69c91809b82e8d2669026dfff42c3100 GIT binary patch literal 4096 zcmeHIJ5Iwu5Pj>-$HXLdfc(IT4QW!Oh|*DEDG*U2(QpB%)6hF9Xc0G{-~cHpZ9zfJ z322bGLCP>|J5DSjlp;bw+F5C5_RZVz>a9KYO*YD;3~)tdAWH!g;g^|DT!A}LQllO0 zf<ukg!legyL7fFC5gKC!{{2_w#5T}-JA=b|MuI>KOBAGo6v%Nj66qpz7dAnM2{>Nx zI9e@W7?nb%gO1$~!w=vwj8>jg7)EtS6WUe7uo7>+ML1#rsDf3w!Hov7tz0X}jA<@| znO4!A1^^YZtw!BE*soP9<<j2nPSMZ{`E4z?rDikf6j#_0e3Q7Y;A`;P3iuB_MQ$@K zL$`cR3bc`bbvTZ_%x~vZDA})?c)4!b%Xk`9Vi*Rmz)Xah7kn=o;(nw_1*8H}fjX`* zB_kyU=<7E&%Z8$O^q-3wg>Vm(Pe#2&br`1}!)cEm@WPoIaHr{&J59pe0RNAZ%Qm+& w+Ruz#E{GcIPT1)j@8SvQ&et-M3anQFH~E3rsQUYQpCGGA1*8H}fj?2;8=mzw{{R30 literal 0 HcmV?d00001 -- 2.23.0

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ACKed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/virhostdevtest.c | 97 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index ed8e7e8fc1..39216edc67 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -48,6 +48,9 @@ VIR_LOG_INIT("tests.hostdevtest"); # define CHECK_PCI_LIST_COUNT(list, cnt) \ CHECK_LIST_COUNT(list, cnt, virPCIDeviceListCount) +# define CHECK_NVME_LIST_COUNT(list, cnt) \ + CHECK_LIST_COUNT(list, cnt, virNVMeDeviceListCount) + # define TEST_STATE_DIR abs_builddir "/hostdevmgr" static const char *drv_name = "test_driver"; static const char *dom_name = "test_domain"; @@ -57,6 +60,36 @@ static int nhostdevs = 3; static virDomainHostdevDefPtr hostdevs[] = {NULL, NULL, NULL}; static virPCIDevicePtr dev[] = {NULL, NULL, NULL}; static virHostdevManagerPtr mgr; +static const size_t ndisks = 3; +static virDomainDiskDefPtr disks[] = {NULL, NULL, NULL}; +static const char *diskXML[] = { + "<disk type='nvme' device='disk'>" + " <driver name='qemu' type='raw'/>" + " <source type='pci' managed='yes' namespace='1'>" + " <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'/>" + " <source type='pci' managed='yes' namespace='2'>" + " <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'/>" + " <source type='pci' managed='no' namespace='1'>" + " <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>" +}; static void myCleanup(void) @@ -67,6 +100,9 @@ myCleanup(void) virDomainHostdevDefFree(hostdevs[i]); } + for (i = 0; i < ndisks; i++) + virDomainDiskDefFree(disks[i]); + if (mgr) { if (!getenv("LIBVIRT_SKIP_CLEANUP")) virFileDeleteTree(mgr->stateDir); @@ -75,6 +111,7 @@ myCleanup(void) virObjectUnref(mgr->activeUSBHostdevs); virObjectUnref(mgr->inactivePCIHostdevs); virObjectUnref(mgr->activeSCSIHostdevs); + virObjectUnref(mgr->activeNVMeHostdevs); VIR_FREE(mgr->stateDir); VIR_FREE(mgr); } @@ -107,6 +144,11 @@ myInit(void) virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO); } + for (i = 0; i < ndisks; i++) { + if (!(disks[i] = virDomainDiskDefParse(diskXML[i], NULL, NULL, 0))) + goto cleanup; + } + if (VIR_ALLOC(mgr) < 0) goto cleanup; if ((mgr->activePCIHostdevs = virPCIDeviceListNew()) == NULL) @@ -117,6 +159,8 @@ myInit(void) goto cleanup; if ((mgr->activeSCSIHostdevs = virSCSIDeviceListNew()) == NULL) goto cleanup; + if ((mgr->activeNVMeHostdevs = virNVMeDeviceListNew()) == NULL) + goto cleanup; mgr->stateDir = g_strdup(TEST_STATE_DIR); if (virFileMakePath(mgr->stateDir) < 0) goto cleanup; @@ -492,6 +536,58 @@ testVirHostdevOther(const void *opaque G_GNUC_UNUSED) return 0; } +static int +testNVMeDiskRoundtrip(const void *opaque G_GNUC_UNUSED) +{ + int ret = -1; + + /* Don't rely on a state that previous test cases might have + * left the manager in. Start with a clean slate. */ + virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, + hostdevs, nhostdevs, NULL); + + CHECK_NVME_LIST_COUNT(mgr->activeNVMeHostdevs, 0); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, 0); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, 0); + + /* Firstly, attach all NVMe disks */ + if (virHostdevPrepareNVMeDevices(mgr, drv_name, dom_name, disks, ndisks) < 0) + goto cleanup; + + CHECK_NVME_LIST_COUNT(mgr->activeNVMeHostdevs, 3); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, 2); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, 0); + + /* Now, try to detach the first one. */ + if (virHostdevReAttachNVMeDevices(mgr, drv_name, dom_name, disks, 1) < 0) + goto cleanup; + + CHECK_NVME_LIST_COUNT(mgr->activeNVMeHostdevs, 2); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, 2); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, 0); + + /* And the last one */ + if (virHostdevReAttachNVMeDevices(mgr, drv_name, dom_name, &disks[2], 1) < 0) + goto cleanup; + + CHECK_NVME_LIST_COUNT(mgr->activeNVMeHostdevs, 1); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, 1); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, 0); + + /* Finally, detach the middle one */ + if (virHostdevReAttachNVMeDevices(mgr, drv_name, dom_name, &disks[1], 1) < 0) + goto cleanup; + + CHECK_NVME_LIST_COUNT(mgr->activeNVMeHostdevs, 0); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, 0); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, 0); + + ret = 0; + cleanup: + return ret; +} + + # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" static int @@ -526,6 +622,7 @@ mymain(void) DO_TEST(testVirHostdevRoundtripManaged); DO_TEST(testVirHostdevRoundtripMixed); DO_TEST(testVirHostdevOther); + DO_TEST(testNVMeDiskRoundtrip); myCleanup(); -- 2.23.0

The qemu driver has its own wrappers around virHostdev module (so that some arguments are filled in automatically). Extend these to include NVMe devices too. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ACKed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_hostdev.c | 49 ++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_hostdev.h | 10 +++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index ebbca817b8..5ab0217858 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -96,13 +96,28 @@ qemuHostdevUpdateActiveMediatedDevices(virQEMUDriverPtr driver, } +int +qemuHostdevUpdateActiveNVMeDisks(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + return virHostdevUpdateActiveNVMeDevices(driver->hostdevMgr, + QEMU_DRIVER_NAME, + def->name, + def->disks, + def->ndisks); +} + + int qemuHostdevUpdateActiveDomainDevices(virQEMUDriverPtr driver, virDomainDefPtr def) { - if (!def->nhostdevs) + if (!def->nhostdevs && !def->ndisks) return 0; + if (qemuHostdevUpdateActiveNVMeDisks(driver, def) < 0) + return -1; + if (qemuHostdevUpdateActivePCIDevices(driver, def) < 0) return -1; @@ -197,6 +212,17 @@ qemuHostdevPreparePCIDevicesCheckSupport(virDomainHostdevDefPtr *hostdevs, return true; } +int +qemuHostdevPrepareNVMeDisks(virQEMUDriverPtr driver, + const char *name, + virDomainDiskDefPtr *disks, + size_t ndisks) +{ + return virHostdevPrepareNVMeDevices(driver->hostdevMgr, + QEMU_DRIVER_NAME, + name, disks, ndisks); +} + int qemuHostdevPreparePCIDevices(virQEMUDriverPtr driver, const char *name, @@ -313,9 +339,12 @@ qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, virQEMUCapsPtr qemuCaps, unsigned int flags) { - if (!def->nhostdevs) + if (!def->nhostdevs && !def->ndisks) return 0; + if (qemuHostdevPrepareNVMeDisks(driver, def->name, def->disks, def->ndisks) < 0) + return -1; + if (qemuHostdevPreparePCIDevices(driver, def->name, def->uuid, def->hostdevs, def->nhostdevs, qemuCaps, flags) < 0) @@ -340,6 +369,17 @@ qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, return 0; } +void +qemuHostdevReAttachNVMeDisks(virQEMUDriverPtr driver, + const char *name, + virDomainDiskDefPtr *disks, + size_t ndisks) +{ + virHostdevReAttachNVMeDevices(driver->hostdevMgr, + QEMU_DRIVER_NAME, + name, disks, ndisks); +} + void qemuHostdevReAttachPCIDevices(virQEMUDriverPtr driver, const char *name, @@ -419,9 +459,12 @@ void qemuHostdevReAttachDomainDevices(virQEMUDriverPtr driver, virDomainDefPtr def) { - if (!def->nhostdevs) + if (!def->nhostdevs && !def->ndisks) return; + qemuHostdevReAttachNVMeDisks(driver, def->name, def->disks, + def->ndisks); + qemuHostdevReAttachPCIDevices(driver, def->name, def->hostdevs, def->nhostdevs); diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index 536069fe8a..735414b6aa 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -28,6 +28,8 @@ bool qemuHostdevNeedsVFIO(const virDomainHostdevDef *hostdev); bool qemuHostdevHostSupportsPassthroughVFIO(void); +int qemuHostdevUpdateActiveNVMeDisks(virQEMUDriverPtr driver, + virDomainDefPtr def); int qemuHostdevUpdateActiveMediatedDevices(virQEMUDriverPtr driver, virDomainDefPtr def); int qemuHostdevUpdateActivePCIDevices(virQEMUDriverPtr driver, @@ -39,6 +41,10 @@ int qemuHostdevUpdateActiveSCSIDevices(virQEMUDriverPtr driver, int qemuHostdevUpdateActiveDomainDevices(virQEMUDriverPtr driver, virDomainDefPtr def); +int qemuHostdevPrepareNVMeDisks(virQEMUDriverPtr driver, + const char *name, + virDomainDiskDefPtr *disks, + size_t ndisks); int qemuHostdevPreparePCIDevices(virQEMUDriverPtr driver, const char *name, const unsigned char *uuid, @@ -68,6 +74,10 @@ int qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, virQEMUCapsPtr qemuCaps, unsigned int flags); +void qemuHostdevReAttachNVMeDisks(virQEMUDriverPtr driver, + const char *name, + virDomainDiskDefPtr *disks, + size_t ndisks); void qemuHostdevReAttachPCIDevices(virQEMUDriverPtr driver, const char *name, virDomainHostdevDefPtr *hostdevs, -- 2.23.0

We have this beautiful function that does crystal ball divination. The function is named qemuDomainGetMemLockLimitBytes() and it calculates the upper limit of how much locked memory is given guest going to need. The function bases its guess on devices defined for a domain. For instance, if there is a VFIO hostdev defined then it adds 1GiB to the guessed maximum. Since NVMe disks are pretty much VFIO hostdevs (but not quite), we have to do the same sorcery. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ACKed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 59e98de2d2..7b515b9520 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11920,6 +11920,9 @@ getPPC64MemLockLimitBytes(virDomainDefPtr def) } } + if (virDomainDefHasNVMeDisk(def)) + usesVFIO = true; + memory = virDomainDefGetMemoryTotal(def); if (def->mem.max_memory) @@ -12018,6 +12021,7 @@ unsigned long long qemuDomainGetMemLockLimitBytes(virDomainDefPtr def) { unsigned long long memKB = 0; + bool usesVFIO = false; size_t i; /* prefer the hard limit */ @@ -12058,11 +12062,17 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def) for (i = 0; i < def->nhostdevs; i++) { if (virHostdevIsVFIODevice(def->hostdevs[i]) || virHostdevIsMdevDevice(def->hostdevs[i])) { - memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024; - goto done; + usesVFIO = true; + break; } } + if (virDomainDefHasNVMeDisk(def)) + usesVFIO = true; + + if (usesVFIO) + memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024; + done: return memKB << 10; } -- 2.23.0

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 | 67 ++++++++++++++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7b515b9520..70c4ee8e65 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -13430,16 +13430,29 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED, { virStorageSourcePtr next; char *dst = NULL; + bool hasNVMe = false; int ret = -1; for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) { - if (!next->path || !virStorageSourceIsLocalStorage(next)) { - /* Not creating device. Just continue. */ - continue; - } + if (next->type == VIR_STORAGE_TYPE_NVME) { + g_autofree char *nvmePath = NULL; - if (qemuDomainCreateDevice(next->path, data, false) < 0) - goto cleanup; + hasNVMe = true; + + if (!(nvmePath = virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr))) + goto cleanup; + + if (qemuDomainCreateDevice(nvmePath, data, false) < 0) + goto cleanup; + } else { + if (!next->path || !virStorageSourceIsLocalStorage(next)) { + /* Not creating device. Just continue. */ + continue; + } + + if (qemuDomainCreateDevice(next->path, data, false) < 0) + goto cleanup; + } } /* qemu-pr-helper might require access to /dev/mapper/control. */ @@ -13447,6 +13460,10 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED, qemuDomainCreateDevice(QEMU_DEVICE_MAPPER_CONTROL_PATH, data, true) < 0) goto cleanup; + if (hasNVMe && + qemuDomainCreateDevice(QEMU_DEV_VFIO, data, false) < 0) + goto cleanup; + ret = 0; cleanup: VIR_FREE(dst); @@ -14461,19 +14478,32 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, virStorageSourcePtr src) { virStorageSourcePtr next; - const char **paths = NULL; + char **paths = NULL; size_t npaths = 0; - char *dmPath = NULL; + bool hasNVMe = false; + g_autofree char *dmPath = NULL; + g_autofree char *vfioPath = NULL; int ret = -1; for (next = src; virStorageSourceIsBacking(next); next = next->backingStore) { - if (virStorageSourceIsEmpty(next) || - !virStorageSourceIsLocalStorage(next)) { - /* Not creating device. Just continue. */ - continue; + g_autofree char *tmpPath = NULL; + + if (next->type == VIR_STORAGE_TYPE_NVME) { + hasNVMe = true; + + if (!(tmpPath = virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr))) + goto cleanup; + } else { + if (virStorageSourceIsEmpty(next) || + !virStorageSourceIsLocalStorage(next)) { + /* Not creating device. Just continue. */ + continue; + } + + tmpPath = g_strdup(next->path); } - if (VIR_APPEND_ELEMENT_COPY(paths, npaths, next->path) < 0) + if (VIR_APPEND_ELEMENT(paths, npaths, tmpPath) < 0) goto cleanup; } @@ -14484,13 +14514,18 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, goto cleanup; } - if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0) + if (hasNVMe) { + vfioPath = g_strdup(QEMU_DEV_VFIO); + if (VIR_APPEND_ELEMENT(paths, npaths, vfioPath) < 0) + goto cleanup; + } + + if (qemuDomainNamespaceMknodPaths(vm, (const char **) paths, npaths) < 0) goto cleanup; ret = 0; cleanup: - VIR_FREE(dmPath); - VIR_FREE(paths); + virStringListFreeCount(paths, npaths); return ret; } -- 2.23.0

On 12/10/19 9:16 PM, Cole Robinson wrote: I think you're right. I will save that to a follow up series. Michal

On 12/2/19 9:26 AM, Michal Privoznik wrote: I think my suggested helpers will simplify selinux and apparmor a bit. Not sure about DAC but I don't fully understand the chownCallback + transaction stuff. Looks mechanically sound AFAICT Reviewed-by: Cole Robinson <crobinso(a)redhat.com&gt; - Cole

On 12/2/19 9:26 AM, Michal Privoznik wrote: For the code: Reviewed-by: Cole Robinson <crobinso(a)redhat.com&gt; Does the blockdev infrastructure have magic that turns the Props into a drive string? I don't see any -drive examples in the test output - Cole

On 12/12/19 4:19 AM, Michal Privoznik wrote: Right. I think my confusion is that this is a pre-blockdev/-drive code path, but I didn't see any explicit -drive handling in the series, just -blockdev. So either I 1) missed the -drive changes but in that case there should probably be test output demoing -drive output here, or 2) this feature is -blockdev only in which case this is dead code. Or some other case that I'm missing :) - Cole