This reverts commit b70c093ffa00cd87c8d39d3652b798f033a81faf. In next commit the virpcimock is going to be extended and thus binding a PCI device to vfio-pci driver will finally succeed. Remove this test as it will no longer make sense. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcitest.c | 32 -------------------------------- 1 file changed, 32 deletions(-) diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 961a7eff1a..9ecd1b7d27 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -256,36 +256,6 @@ testVirPCIDeviceDetachSingle(const void *opaque) return ret; } -static int -testVirPCIDeviceDetachFail(const void *opaque) -{ - const struct testPCIDevData *data = opaque; - int ret = -1; - virPCIDevicePtr dev; - - dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); - if (!dev) - goto cleanup; - - virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO); - - if (virPCIDeviceDetach(dev, NULL, NULL) < 0) { - if (virTestGetVerbose() || virTestGetDebug()) - virDispatchError(NULL); - virResetLastError(); - ret = 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Attaching device %s to %s should have failed", - virPCIDeviceGetName(dev), - virPCIStubDriverTypeToString(VIR_PCI_STUB_DRIVER_VFIO)); - } - - cleanup: - virPCIDeviceFree(dev); - return ret; -} - static int testVirPCIDeviceReattachSingle(const void *opaque) { @@ -421,8 +391,6 @@ mymain(void) DO_TEST_PCI(testVirPCIDeviceIsAssignable, 5, 0x90, 1, 0); DO_TEST_PCI(testVirPCIDeviceIsAssignable, 1, 1, 0, 0); - DO_TEST_PCI(testVirPCIDeviceDetachFail, 0, 0x0a, 1, 0); - /* Reattach a device already bound to non-stub a driver */ DO_TEST_PCI_DRIVER(0, 0x0a, 1, 0, "i915"); DO_TEST_PCI(testVirPCIDeviceReattachSingle, 0, 0x0a, 1, 0); -- 2.21.0

This function does not change any of the passed addresses. It just reads them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 4 ++-- src/util/virpci.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 75e8daadd5..5392d62406 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1718,8 +1718,8 @@ virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr) } bool -virPCIDeviceAddressEqual(virPCIDeviceAddress *addr1, - virPCIDeviceAddress *addr2) +virPCIDeviceAddressEqual(const virPCIDeviceAddress *addr1, + const virPCIDeviceAddress *addr2) { if (addr1->domain == addr2->domain && addr1->bus == addr2->bus && diff --git a/src/util/virpci.h b/src/util/virpci.h index 457be3c929..a940608701 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -231,8 +231,8 @@ bool virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr, bool report); bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr); -bool virPCIDeviceAddressEqual(virPCIDeviceAddress *addr1, - virPCIDeviceAddress *addr2); +bool virPCIDeviceAddressEqual(const virPCIDeviceAddress *addr1, + const virPCIDeviceAddress *addr2); char *virPCIDeviceAddressAsString(virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1); -- 2.21.0

This function does not change any of the passed addresses. It just reads them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 2 +- src/util/virpci.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 5392d62406..59f478dd41 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1731,7 +1731,7 @@ virPCIDeviceAddressEqual(const virPCIDeviceAddress *addr1, } char * -virPCIDeviceAddressAsString(virPCIDeviceAddressPtr addr) +virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) { char *str; diff --git a/src/util/virpci.h b/src/util/virpci.h index a940608701..1efd8b77ed 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -234,7 +234,7 @@ bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr); bool virPCIDeviceAddressEqual(const virPCIDeviceAddress *addr1, const virPCIDeviceAddress *addr2); -char *virPCIDeviceAddressAsString(virPCIDeviceAddressPtr addr) +char *virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) ATTRIBUTE_NONNULL(1); int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf); -- 2.21.0

This helper is cleaner than plain memcpy() because one doesn't have to look into virPCIDeviceAddress struct to see if it contains any strings / pointers. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virpci.c | 6 ++++++ src/util/virpci.h | 2 ++ 3 files changed, 9 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 02d5b7acce..6cef8d20fe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2612,6 +2612,7 @@ virObjectUnref; # util/virpci.h virPCIDeviceAddressAsString; +virPCIDeviceAddressCopy; virPCIDeviceAddressEqual; virPCIDeviceAddressGetIOMMUGroupAddresses; virPCIDeviceAddressGetIOMMUGroupNum; diff --git a/src/util/virpci.c b/src/util/virpci.c index 59f478dd41..03ce651f40 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1730,6 +1730,12 @@ virPCIDeviceAddressEqual(const virPCIDeviceAddress *addr1, return false; } +void virPCIDeviceAddressCopy(virPCIDeviceAddressPtr dst, + const virPCIDeviceAddress *src) +{ + memcpy(dst, src, sizeof(*src)); +} + char * virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) { diff --git a/src/util/virpci.h b/src/util/virpci.h index 1efd8b77ed..72e90a1ef3 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -233,6 +233,8 @@ bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr); bool virPCIDeviceAddressEqual(const virPCIDeviceAddress *addr1, const virPCIDeviceAddress *addr2); +void virPCIDeviceAddressCopy(virPCIDeviceAddressPtr dst, + const virPCIDeviceAddress *src); char *virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) ATTRIBUTE_NONNULL(1); -- 2.21.0

I find this function more readable if checks for passed storage source are done first and backing chain is done last. Mixing them together does not hurt, but is less readable. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0f1fda2384..f09abc8a73 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5178,11 +5178,6 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk, return -1; } - for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { - if (qemuDomainValidateStorageSource(n, qemuCaps) < 0) - return -1; - } - if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -5191,6 +5186,11 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk, return -1; } + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (qemuDomainValidateStorageSource(n, qemuCaps) < 0) + return -1; + } + return 0; } -- 2.21.0

There is this class of PCI devices that act like disks: NVMe. Therefore, they are both PCI devices and disks. While we already have <hostdev/> (and can assign a NVMe device to a domain successfully) we don't have disk representation. There are three problems with PCI assignment in case of a NVMe device: 1) domains with <hostdev/> can't be migrated 2) NVMe device is assigned whole, there's no way to assign only a namespace 3) Because hypervisors see <hostdev/> they don't put block layer on top of it - users don't get all the fancy features like snapshots NVMe namespaces are way of splitting one continuous NVDIMM memory into smaller ones, effectively creating smaller NVMe-s (which can then be partitioned, LVMed, etc.) Because of all of this the following XML was chosen to model a NVMe device: <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'/> </disk> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 45 +++++++++++++++++++++-- docs/schemas/domaincommon.rng | 32 ++++++++++++++++ tests/qemuxml2argvdata/disk-nvme.xml | 55 ++++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-nvme.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a7a6ec32a5..545578076d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2922,6 +2922,13 @@ </backingStore> <target dev='vdd' bus='virtio'/> </disk> + <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='vde' bus='virtio'/> + </disk> </devices> ...</pre> @@ -2935,7 +2942,8 @@ Valid values are "file", "block", "dir" (<span class="since">since 0.7.5</span>), "network" (<span class="since">since 0.8.7</span>), or - "volume" (<span class="since">since 1.0.5</span>) + "volume" (<span class="since">since 1.0.5</span>), or + "nvme" (<span class="since">since 5.5.0</span>) and refer to the underlying source for the disk. <span class="since">Since 0.0.3</span> </dd> @@ -3118,6 +3126,31 @@ <span class="since">Since 1.0.5</span> </p> </dd> + <dt><code>nvme</code></dt> + <dd> + To specify disk source for NVMe disk the <code>source</code> + element has the following attributes: + <dl> + <dt><code>type</code></dt> + <dd>The type of address specified in <code>address</code> + sub-element. Currently, only <code>pci</code> value is + accepted. + </dd> + + <dt><code>managed</code></dt> + <dd>This attribute instructs libvirt to detach NVMe + controller automatically on domain startup (<code>yes</code>) + or expect the controller to be detached by system + administrator (<code>no</code>). + </dd> + + <dt><code>namespace</code></dt> + <dd>The namespace ID which should be assigned to the domain. + According to NVMe standard, namespace numbers start from 1, + including. + </dd> + </dl> + </dd> </dl> With "file", "block", and "volume", one or more optional sub-elements <code>seclabel</code>, <a href="#seclabel">described @@ -3280,11 +3313,17 @@ initiator IQN needed to access the source via mandatory attribute <code>name</code>. </dd> + <dt><code>address</code></dt> + <dd>For disk of type <code>nvme</code> this element + specifies the PCI address of the host NVMe + controller. + <span class="since">Since 5.5.0</span> + </dd> </dl> <p> - For a "file" or "volume" disk type which represents a cdrom or floppy - (the <code>device</code> attribute), it is possible to define + For a "file", "volume" or "nvme" disk type which represents a cdrom or + floppy (the <code>device</code> attribute), it is possible to define policy what to do with the disk if the source file is not accessible. (NB, <code>startupPolicy</code> is not valid for "volume" disk unless the specified storage volume is of "file" type). This is done by the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 31db599ab9..f367e8f6fd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1603,6 +1603,7 @@ <ref name="diskSourceDir"/> <ref name="diskSourceNetwork"/> <ref name="diskSourceVolume"/> + <ref name="diskSourceNvme"/> </choice> </define> @@ -1918,6 +1919,37 @@ </optional> </define> + <define name="diskSourceNvme"> + <attribute name="type"> + <value>nvme</value> + </attribute> + <optional> + <element name="source"> + <attribute name="type"> + <value>pci</value> + </attribute> + <attribute name="namespace"> + <ref name="uint32"/> + </attribute> + <optional> + <attribute name="managed"> + <ref name="virYesNo"/> + </attribute> + </optional> + <element name="address"> + <ref name="pciaddress"/> + </element> + <ref name="diskSourceCommon"/> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <optional> + <ref name="encryption"/> + </optional> + </element> + </optional> + </define> + <define name="diskTarget"> <data type="string"> <param name="pattern">(ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+</param> diff --git a/tests/qemuxml2argvdata/disk-nvme.xml b/tests/qemuxml2argvdata/disk-nvme.xml new file mode 100644 index 0000000000..0b3dbad4eb --- /dev/null +++ b/tests/qemuxml2argvdata/disk-nvme.xml @@ -0,0 +1,55 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <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'/> + </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'/> + </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'/> + </disk> + <disk type='nvme' device='disk'> + <driver name='qemu' type='raw'/> + <source type='pci' managed='no' namespace='2'> + <address domain='0x0001' bus='0x02' slot='0x00' function='0x0'/> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> + <target dev='vdd' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <controller type='scsi' index='0' model='virtio-scsi'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> -- 2.21.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@redhat.com> --- src/libvirt_private.syms | 18 ++ src/util/Makefile.inc.am | 2 + src/util/virnvme.c | 412 +++++++++++++++++++++++++++++++++++++++ src/util/virnvme.h | 89 +++++++++ 4 files changed, 521 insertions(+) create mode 100644 src/util/virnvme.c create mode 100644 src/util/virnvme.h diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 350b638193..856b770e57 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2585,6 +2585,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 a47f333a98..998bec741e 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -143,6 +143,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..53724b63f7 --- /dev/null +++ b/src/util/virnvme.c @@ -0,0 +1,412 @@ +/* + * 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.pci"); +#define VIR_FROM_THIS VIR_FROM_NONE + +struct _virNVMeDevice { + virPCIDeviceAddress address; /* PCI address of controller */ + unsigned long 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) +{ + VIR_AUTOPTR(virNVMeDevice) dev = NULL; + + if (VIR_ALLOC(dev) < 0) + return NULL; + + virPCIDeviceAddressCopy(&dev->address, address); + dev->namespace = namespace; + dev->managed = managed; + + VIR_RETURN_PTR(dev); +} + + +void +virNVMeDeviceFree(virNVMeDevicePtr dev) +{ + if (!dev) + return; + + virNVMeDeviceUsedByClear(dev); + VIR_FREE(dev); +} + + +virNVMeDevicePtr +virNVMeDeviceCopy(const virNVMeDevice *dev) +{ + VIR_AUTOPTR(virNVMeDevice) copy = NULL; + + if (VIR_ALLOC(copy) < 0 || + VIR_STRDUP(copy->drvname, dev->drvname) < 0 || + VIR_STRDUP(copy->domname, dev->domname) < 0) + return NULL; + + virPCIDeviceAddressCopy(©->address, &dev->address); + copy->namespace = dev->namespace; + copy->managed = dev->managed; + + VIR_RETURN_PTR(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; +} + + +int +virNVMeDeviceUsedBySet(virNVMeDevicePtr dev, + const char *drv, + const char *dom) +{ + if (VIR_STRDUP(dev->drvname, drv) < 0 || + VIR_STRDUP(dev->domname, dom) < 0) { + virNVMeDeviceUsedByClear(dev); + return -1; + } + + return 0; +} + + +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))) { + VIR_AUTOFREE(char *) addrStr = virPCIDeviceAddressAsString(&tmp->address); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NVMe device %s namespace %lu 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) { + VIR_AUTOFREE(char *) addrStr = virPCIDeviceAddressAsString(&dev->address); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NVMe device %s namespace %lu 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; +} + + +virPCIDeviceListPtr +virNVMeDeviceListCreateDetachList(virNVMeDeviceListPtr activeList, + virNVMeDeviceListPtr toDetachList) +{ + VIR_AUTOUNREF(virPCIDeviceListPtr) pciDevices = NULL; + size_t i; + + if (!(pciDevices = virPCIDeviceListNew())) + return NULL; + + for (i = 0; i < toDetachList->count; i++) { + const virNVMeDevice *d = toDetachList->devs[i]; + VIR_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 = virPCIDeviceNew(d->address.domain, + d->address.bus, + d->address.slot, + d->address.function))) + return NULL; + + /* NVMe devices must be bound to vfio */ + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); + virPCIDeviceSetManaged(pci, d->managed); + + if (virPCIDeviceListAdd(pciDevices, pci) < 0) + return NULL; + + /* avoid freeing the device */ + pci = NULL; + } + + VIR_RETURN_PTR(pciDevices); +} + + +virPCIDeviceListPtr +virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList, + virNVMeDeviceListPtr toReAttachList) +{ + VIR_AUTOUNREF(virPCIDeviceListPtr) pciDevices = NULL; + size_t i; + + if (!(pciDevices = virPCIDeviceListNew())) + return NULL; + + for (i = 0; i < toReAttachList->count; i++) { + const virNVMeDevice *d = toReAttachList->devs[i]; + VIR_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) */ + VIR_AUTOFREE(char *) addrStr = virPCIDeviceAddressAsString(&d->address); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NVMe device %s namespace %lu 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 = virPCIDeviceNew(d->address.domain, + d->address.bus, + d->address.slot, + d->address.function))) + return NULL; + + /* NVMe devices must be bound to vfio */ + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); + virPCIDeviceSetManaged(pci, d->managed); + + if (virPCIDeviceListAdd(pciDevices, pci) < 0) + return NULL; + + /* avoid freeing the device */ + pci = NULL; + } + + VIR_RETURN_PTR(pciDevices); +} diff --git a/src/util/virnvme.h b/src/util/virnvme.h new file mode 100644 index 0000000000..edf5fe58ab --- /dev/null +++ b/src/util/virnvme.h @@ -0,0 +1,89 @@ +/* + * 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; +typedef struct _virNVMeDeviceList virNVMeDeviceList; +typedef virNVMeDeviceList *virNVMeDeviceListPtr; + +virNVMeDevicePtr +virNVMeDeviceNew(const virPCIDeviceAddress *address, + unsigned long namespace, + bool managed); + +void +virNVMeDeviceFree(virNVMeDevicePtr dev); + +VIR_DEFINE_AUTOPTR_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); + +int +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.21.0

The myInit() function is called before any of the test cases because it prepares all internal structures for individual cases. Well, if it fails there's no point in proceeding with testing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 20eaca82e0..cf39c83c76 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -574,8 +574,11 @@ mymain(void) ret = -1; \ } while (0) - if (myInit() < 0) + if (myInit() < 0) { fprintf(stderr, "Init data structures failed."); + virFileDeleteTree(fakerootdir); + return EXIT_FAILURE; + } DO_TEST(testVirHostdevRoundtripNoGuest); DO_TEST(testVirHostdevRoundtripUnmanaged); -- 2.21.0

In near future we will need to check for number of members of two different types of lists: PCI and NVMe. Rename CHECK_LIST_COUNT to CHECK_PCI_LIST_COUNT to mark explicitly what type of list it is working with. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 83 ++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 40 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index cf39c83c76..7d15a87797 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -34,10 +34,10 @@ VIR_LOG_INIT("tests.hostdevtest"); -# define CHECK_LIST_COUNT(list, cnt) \ +# define CHECK_LIST_COUNT(list, cnt, cb) \ do { \ size_t actualCount; \ - if ((actualCount = virPCIDeviceListCount(list)) != cnt) { \ + if ((actualCount = cb(list)) != cnt) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ "Unexpected count of items in " #list ": %zu, " \ "expecting %zu", actualCount, (size_t) cnt); \ @@ -45,6 +45,9 @@ VIR_LOG_INIT("tests.hostdevtest"); } \ } while (0) +# define CHECK_PCI_LIST_COUNT(list, cnt) \ + CHECK_LIST_COUNT(list, cnt, virPCIDeviceListCount) + # define TEST_STATE_DIR abs_builddir "/hostdevmgr" static const char *drv_name = "test_driver"; static const char *dom_name = "test_domain"; @@ -143,16 +146,16 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, NULL, 0, 0) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); /* Test unmanaged hostdevs */ VIR_DEBUG("Test >=1 unmanaged hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - nhostdevs); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - nhostdevs); /* Test conflict */ active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); @@ -161,22 +164,22 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) if (!virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); if (!virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); if (!virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); ret = 0; @@ -203,14 +206,14 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(void) VIR_DEBUG("Test 0 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, NULL, 0, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test >=1 unmanaged hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + nhostdevs); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + nhostdevs); ret = 0; @@ -236,14 +239,14 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); /* If testing a mixed roundtrip, devices are already in the inactive list * before we start and are removed from it as soon as we attach them to * the guest */ if (mixed) - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - nhostdevs); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - nhostdevs); else - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); /* Test conflict */ active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); @@ -252,22 +255,22 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) if (!virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); if (!virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); if (!virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); ret = 0; @@ -294,19 +297,19 @@ testVirHostdevReAttachPCIHostdevs_managed(bool mixed) VIR_DEBUG("Test 0 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, NULL, 0, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test >=1 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); /* If testing a mixed roundtrip, devices are added back to the inactive * list as soon as we detach from the guest */ if (mixed) - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + nhostdevs); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + nhostdevs); else - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); ret = 0; @@ -326,8 +329,8 @@ testVirHostdevDetachPCINodeDevice(void) inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceDetach(mgr, dev[i]) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + 1); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + 1); } ret = 0; @@ -347,8 +350,8 @@ testVirHostdevResetPCINodeDevice(void) inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceReset(mgr, dev[i]) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); } ret = 0; @@ -369,8 +372,8 @@ testVirHostdevReAttachPCINodeDevice(void) inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceReAttach(mgr, dev[i]) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - 1); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - 1); } ret = 0; @@ -393,15 +396,15 @@ testVirHostdevUpdateActivePCIHostdevs(void) if (virHostdevUpdateActivePCIDevices(mgr, NULL, 0, drv_name, dom_name) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test >=1 hostdevs"); if (virHostdevUpdateActivePCIDevices(mgr, hostdevs, nhostdevs, drv_name, dom_name) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); + CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); ret = 0; -- 2.21.0

The device configs (which are actually the same one config) come from a NVMe disk of mine. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 18d06d11d4..a26b7c1b2e 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -880,6 +880,7 @@ init_env(void) MAKE_PCI_DRIVER("i915", 0x8086, 0x0046, 0x8086, 0x0047); MAKE_PCI_DRIVER("pci-stub", -1, -1); pci_driver_new("vfio-pci", PCI_ACTION_BIND, -1, -1); + MAKE_PCI_DRIVER("nvme", 0x1cc1, 0x8201); # define MAKE_PCI_DEVICE(Id, Vendor, Device, ...) \ do { \ @@ -902,6 +903,8 @@ init_env(void) MAKE_PCI_DEVICE("0000:0a:01.0", 0x8086, 0x0047); MAKE_PCI_DEVICE("0000:0a:02.0", 0x8286, 0x0048); MAKE_PCI_DEVICE("0000:0a:03.0", 0x8386, 0x0048); + MAKE_PCI_DEVICE("0000:01:00.0", 0x1cc1, 0x8201, .iommuGroup = 8, .klass = 0x010802); + MAKE_PCI_DEVICE("0000:02:00.0", 0x1cc1, 0x8201, .iommuGroup = 9, .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.21.0

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 97 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 7d15a87797..e34959c4f1 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_KVM); } + 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; if (VIR_STRDUP(mgr->stateDir, TEST_STATE_DIR) < 0) goto cleanup; if (virFileMakePath(mgr->stateDir) < 0) @@ -550,6 +594,58 @@ testVirHostdevOther(const void *opaque ATTRIBUTE_UNUSED) return ret; } +static int +testNVMeDiskRoundtrip(const void *opaque ATTRIBUTE_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 @@ -588,6 +684,7 @@ mymain(void) DO_TEST(testVirHostdevRoundtripManaged); DO_TEST(testVirHostdevRoundtripMixed); DO_TEST(testVirHostdevOther); + DO_TEST(testNVMeDiskRoundtrip); myCleanup(); -- 2.21.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@redhat.com> --- 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 92b037e1ed..efa4d62f1f 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -96,13 +96,28 @@ qemuHostdevUpdateActiveMediatedDevices(virQEMUDriverPtr driver, } +int +qemuHostdevUpdateActiveNVMeDevices(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 (qemuHostdevUpdateActiveNVMeDevices(driver, def) < 0) + return -1; + if (qemuHostdevUpdateActivePCIDevices(driver, def) < 0) return -1; @@ -226,6 +241,17 @@ qemuHostdevPreparePCIDevicesCheckSupport(virDomainHostdevDefPtr *hostdevs, return true; } +int +qemuHostdevPrepareNVMeDevices(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, @@ -342,9 +368,12 @@ qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, virQEMUCapsPtr qemuCaps, unsigned int flags) { - if (!def->nhostdevs) + if (!def->nhostdevs && !def->ndisks) return 0; + if (qemuHostdevPrepareNVMeDevices(driver, def->name, def->disks, def->ndisks) < 0) + return -1; + if (qemuHostdevPreparePCIDevices(driver, def->name, def->uuid, def->hostdevs, def->nhostdevs, qemuCaps, flags) < 0) @@ -369,6 +398,17 @@ qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, return 0; } +void +qemuHostdevReAttachNVMeDevices(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, @@ -448,9 +488,12 @@ void qemuHostdevReAttachDomainDevices(virQEMUDriverPtr driver, virDomainDefPtr def) { - if (!def->nhostdevs) + if (!def->nhostdevs && !def->ndisks) return; + qemuHostdevReAttachNVMeDevices(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 f6d76c1c2a..4afb103354 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -27,6 +27,8 @@ bool qemuHostdevHostSupportsPassthroughLegacy(void); bool qemuHostdevHostSupportsPassthroughVFIO(void); +int qemuHostdevUpdateActiveNVMeDevices(virQEMUDriverPtr driver, + virDomainDefPtr def); int qemuHostdevUpdateActiveMediatedDevices(virQEMUDriverPtr driver, virDomainDefPtr def); int qemuHostdevUpdateActivePCIDevices(virQEMUDriverPtr driver, @@ -38,6 +40,10 @@ int qemuHostdevUpdateActiveSCSIDevices(virQEMUDriverPtr driver, int qemuHostdevUpdateActiveDomainDevices(virQEMUDriverPtr driver, virDomainDefPtr def); +int qemuHostdevPrepareNVMeDevices(virQEMUDriverPtr driver, + const char *name, + virDomainDiskDefPtr *disks, + size_t ndisks); int qemuHostdevPreparePCIDevices(virQEMUDriverPtr driver, const char *name, const unsigned char *uuid, @@ -67,6 +73,10 @@ int qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, virQEMUCapsPtr qemuCaps, unsigned int flags); +void qemuHostdevReAttachNVMeDevices(virQEMUDriverPtr driver, + const char *name, + virDomainDiskDefPtr *disks, + size_t ndisks); void qemuHostdevReAttachPCIDevices(virQEMUDriverPtr driver, const char *name, virDomainHostdevDefPtr *hostdevs, -- 2.21.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@redhat.com> --- src/qemu/qemu_domain.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f09abc8a73..09e5ee37f4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10950,6 +10950,21 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def) } } + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + virStorageSourcePtr n; + + if (!disk->src) + continue; + + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (n->type == VIR_STORAGE_TYPE_NVME) { + memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024; + goto done; + } + } + } + done: return memKB << 10; } -- 2.21.0

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@redhat.com> --- 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 bc6583562a..5b7aa58dd8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2994,6 +2994,7 @@ virStoragePRDefIsManaged; virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceChainHasManagedPR; +virStorageSourceChainHasNVMe; virStorageSourceClear; virStorageSourceCopy; virStorageSourceFindByNodeName; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 18aa33fe05..a9ceb697cf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2157,6 +2157,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 a1294ea608..8afd5d60cb 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -431,6 +431,8 @@ virStorageSourceChainHasManagedPR(virStorageSourcePtr src); void virStorageSourceNVMeDefFree(virStorageSourceNVMeDefPtr def); VIR_DEFINE_AUTOPTR_FUNC(virStorageSourceNVMeDef, virStorageSourceNVMeDefFree); +bool virStorageSourceChainHasNVMe(const virStorageSource *src); + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model); -- 2.21.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@redhat.com> --- 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 73f5e1fa0f..1b6ee3bfa6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31431,6 +31431,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; +} + + /** * virDomainGraphicsDefHasOpenGL: * @def: domain definition diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c1b5fc1337..2a067633bd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3611,6 +3611,9 @@ virDomainDiskGetDetectZeroesMode(virDomainDiskDiscard discard, bool virDomainDefHasManagedPR(const virDomainDef *def); +bool +virDomainDefHasNVMeDisk(const virDomainDef *def); + bool virDomainGraphicsDefHasOpenGL(const virDomainDef *def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5b7aa58dd8..15bfae115f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -284,6 +284,7 @@ virDomainDefHasDeviceAddress; virDomainDefHasManagedPR; virDomainDefHasMemballoon; virDomainDefHasMemoryHotplug; +virDomainDefHasNVMeDisk; virDomainDefHasUSB; virDomainDefHasVcpusOffline; virDomainDefLifecycleActionAllowed; -- 2.21.0

This piece of code will be re-used later. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 17 +++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 13 ++----------- 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1b6ee3bfa6..e71e484a6f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31445,6 +31445,23 @@ virDomainDefHasNVMeDisk(const virDomainDef *def) } +bool +virDomainDefHasVFIOHostdev(const virDomainDef *def) +{ + size_t i; + + for (i = 0; i < def->nhostdevs; i++) { + const virDomainHostdevDef *tmp = def->hostdevs[i]; + if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + tmp->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && + tmp->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) + return true; + } + + return false; +} + + /** * virDomainGraphicsDefHasOpenGL: * @def: domain definition diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2a067633bd..5c6c5b7a33 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3614,6 +3614,9 @@ virDomainDefHasManagedPR(const virDomainDef *def); bool virDomainDefHasNVMeDisk(const virDomainDef *def); +bool +virDomainDefHasVFIOHostdev(const virDomainDef *def); + bool virDomainGraphicsDefHasOpenGL(const virDomainDef *def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 15bfae115f..4fda747fb3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -287,6 +287,7 @@ virDomainDefHasMemoryHotplug; virDomainDefHasNVMeDisk; virDomainDefHasUSB; virDomainDefHasVcpusOffline; +virDomainDefHasVFIOHostdev; virDomainDefLifecycleActionAllowed; virDomainDefMaybeAddController; virDomainDefMaybeAddInput; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 09e5ee37f4..2a7f09ce24 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11764,7 +11764,7 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, bool includeVFIO = false; char **tmpPaths = NULL; int *tmpPerms = NULL; - size_t i, tmpNpaths = 0; + size_t tmpNpaths = 0; int perm = 0; *npaths = 0; @@ -11787,16 +11787,7 @@ qemuDomainGetHostdevPath(virDomainDefPtr def, perm = VIR_CGROUP_DEVICE_RW; if (teardown) { - size_t nvfios = 0; - for (i = 0; i < def->nhostdevs; i++) { - virDomainHostdevDefPtr tmp = def->hostdevs[i]; - if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - tmp->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && - tmp->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) - nvfios++; - } - - if (nvfios == 0) + if (!virDomainDefHasVFIOHostdev(def)) includeVFIO = true; } else { includeVFIO = true; -- 2.21.0

Couple of places in the QEMU driver will want to know what paths are associated with NVMe disks (for instance CGroup code or namespaces code). Introduce helpers which return desired paths (for instance /dev/vfio/vfio and /dev/vfio/N). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 44 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 6 ++++++ 2 files changed, 50 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2a7f09ce24..949bbace88 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11723,6 +11723,50 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, } +char * +qemuDomainGetNVMeDiskPath(const virStorageSourceNVMeDef *nvme) +{ + VIR_AUTOPTR(virPCIDevice) pci = NULL; + + /* All NVMe devices are VFIO PCI devices */ + if (!(pci = virPCIDeviceNew(nvme->pciAddr.domain, + nvme->pciAddr.bus, + nvme->pciAddr.slot, + nvme->pciAddr.function))) + return NULL; + + return virPCIDeviceGetIOMMUGroupDev(pci); +} + + +char ** +qemuDomainGetDiskNVMePaths(const virDomainDef *def, + const virStorageSource *src, + bool teardown) +{ + VIR_AUTOFREE(char *) iommuGroup = NULL; + VIR_AUTOSTRINGLIST paths = NULL; + bool includeVFIO = !teardown; + + if (!(iommuGroup = qemuDomainGetNVMeDiskPath(src->nvme))) + return NULL; + + if (virStringListAdd(&paths, iommuGroup) < 0) + return NULL; + + if (teardown && def && + !virDomainDefHasNVMeDisk(def) && + !virDomainDefHasVFIOHostdev(def)) + includeVFIO = true; + + if (includeVFIO && + virStringListAdd(&paths, QEMU_DEV_VFIO) < 0) + return NULL; + + VIR_RETURN_PTR(paths); +} + + /** * qemuDomainGetHostdevPath: * @def: domain definition diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 3eea8b0f96..82e225088d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1011,6 +1011,12 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver, bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, virQEMUCapsPtr qemuCaps); +char * qemuDomainGetNVMeDiskPath(const virStorageSourceNVMeDef *nvme); + +char ** qemuDomainGetDiskNVMePaths(const virDomainDef *def, + const virStorageSource *src, + bool teardown); + int qemuDomainGetHostdevPath(virDomainDefPtr def, virDomainHostdevDefPtr dev, bool teardown, -- 2.21.0

If a domain has an NVMe disk configured, then we need to allow it on devices CGroup so that qemu can access it. There is one caveat though - if an NVMe disk is read only we need CGroup to allow write too. This is because when opening the device, qemu does couple of ioctl()-s which are considered as write. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 59 +++++++++++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 19ca60905a..2a7fc07ac7 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -118,10 +118,29 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm, virStorageSourcePtr src, bool forceReadonly) { - if (!src->path || !virStorageSourceIsLocalStorage(src)) { - VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", - NULLSTR(src->path), virStorageTypeToString(src->type)); - return 0; + VIR_AUTOFREE(char *) path = NULL; + bool readonly = src->readonly || forceReadonly; + + if (src->type == VIR_STORAGE_TYPE_NVME) { + /* Even though disk is R/O we can't make it so in + * CGroups. QEMU will try to do some ioctl()-s over the + * device and such operations are R/W. */ + readonly = false; + + if (!(path = qemuDomainGetNVMeDiskPath(src->nvme))) + return -1; + + if (qemuSetupImagePathCgroup(vm, QEMU_DEV_VFIO, false) < 0) + return -1; + } else { + if (!src->path || !virStorageSourceIsLocalStorage(src)) { + VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", + NULLSTR(src->path), virStorageTypeToString(src->type)); + return 0; + } + + if (VIR_STRDUP(path, src->path) < 0) + return -1; } if (virStoragePRDefIsManaged(src->pr) && @@ -129,7 +148,7 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm, qemuSetupImagePathCgroup(vm, QEMU_DEVICE_MAPPER_CONTROL_PATH, false) < 0) return -1; - return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly); + return qemuSetupImagePathCgroup(vm, path, readonly); } @@ -146,6 +165,7 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) { qemuDomainObjPrivatePtr priv = vm->privateData; + VIR_AUTOFREE(char *path) = NULL; int perms = VIR_CGROUP_DEVICE_RWM; size_t i; int ret; @@ -154,10 +174,25 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - if (!src->path || !virStorageSourceIsLocalStorage(src)) { - VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", - NULLSTR(src->path), virStorageTypeToString(src->type)); - return 0; + if (src->type == VIR_STORAGE_TYPE_NVME) { + if (!(path = qemuDomainGetNVMeDiskPath(src->nvme))) + return -1; + + ret = virCgroupDenyDevicePath(priv->cgroup, QEMU_DEV_VFIO, perms, true); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", + QEMU_DEV_VFIO, + virCgroupGetDevicePermsString(perms), ret); + if (ret < 0) + return -1; + } else { + if (!src->path || !virStorageSourceIsLocalStorage(src)) { + VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", + NULLSTR(src->path), virStorageTypeToString(src->type)); + return 0; + } + + if (VIR_STRDUP(path, src->path) < 0) + return -1; } if (virFileExists(QEMU_DEVICE_MAPPER_CONTROL_PATH)) { @@ -184,11 +219,11 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, } } - VIR_DEBUG("Deny path %s", src->path); + VIR_DEBUG("Deny path %s", path); - ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); + ret = virCgroupDenyDevicePath(priv->cgroup, path, perms, true); - virDomainAuditCgroupPath(vm, priv->cgroup, "deny", src->path, + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, virCgroupGetDevicePermsString(perms), ret); /* If you're looking for a counter part to -- 2.21.0

This function calls virSecuritySELinuxSetFilecon() or virSecuritySELinuxSetFileconOptional() from a lot of places. It works, because in all places we're passing src->path which is what we wanted. But not anymore. We will want to be able to pass a different path and thus the function must be reworked a bit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 39 ++++++++++++++------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ea20373a90..99cef3f212 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1820,7 +1820,10 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityDeviceLabelDefPtr disk_seclabel; virSecurityDeviceLabelDefPtr parent_seclabel = NULL; bool remember; - int ret; + const char *path = src->path; + const char *tcon = NULL; + bool optional = false; + int ret = -1; if (!src->path || !virStorageSourceIsLocalStorage(src)) return 0; @@ -1853,40 +1856,32 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, if (!disk_seclabel->relabel) return 0; - ret = virSecuritySELinuxSetFilecon(mgr, src->path, - disk_seclabel->label, remember); + tcon = disk_seclabel->label; } else if (parent_seclabel && (!parent_seclabel->relabel || parent_seclabel->label)) { if (!parent_seclabel->relabel) return 0; - ret = virSecuritySELinuxSetFilecon(mgr, src->path, - parent_seclabel->label, remember); + tcon = parent_seclabel->label; } else if (!parent || parent == src) { if (src->shared) { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - data->file_context, - remember); + tcon = data->file_context; + optional = true; } else if (src->readonly) { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - data->content_context, - remember); + tcon = data->content_context; + optional = true; } else if (secdef->imagelabel) { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - secdef->imagelabel, - remember); + tcon = secdef->imagelabel; + optional = true; } else { - ret = 0; + return 0; } } else { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - data->content_context, - remember); + optional = true; + tcon = data->content_context; } + ret = virSecuritySELinuxSetFileconHelper(mgr, path, tcon, optional, remember); + if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us * proceed anyway, then we don't need to relabel later. */ -- 2.21.0

This function is currently not called for any type of storage source that is not considered 'local' (as defined by virStorageSourceIsLocalStorage()). Well, NVMe disks are not 'local' from that point of view and therefore we will need to call this function more frequently. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 38 ++++++++++++++++++++++ src/security/security_selinux.c | 56 ++++++++++++++++++++++++++++----- 2 files changed, 87 insertions(+), 7 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 137daf5d28..d0b84c99b0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -912,6 +912,23 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; } + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + VIR_AUTOFREE(char *) vfioGroupDev = NULL; + VIR_AUTOUNREF(virPCIDevicePtr) pci = virPCIDeviceNew(nvme->pciAddr.domain, + nvme->pciAddr.bus, + nvme->pciAddr.slot, + nvme->pciAddr.function); + if (!pci || + !(vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci))) + return -1; + + return virSecurityDACSetOwnership(mgr, NULL, vfioGroupDev, user, group, false); + } + /* We can't do restore on shared resources safely. Not even * with refcounting implemented in XATTRs because if there * was a domain running with the feature turned off the @@ -1001,6 +1018,27 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, } } + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + VIR_AUTOFREE(char *) vfioGroupDev = NULL; + VIR_AUTOUNREF(virPCIDevicePtr) pci = virPCIDeviceNew(nvme->pciAddr.domain, + nvme->pciAddr.bus, + nvme->pciAddr.slot, + nvme->pciAddr.function); + if (!pci || + !(vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci))) + return -1; + + /* Ideally, we would check if there is not another PCI + * device within domain def that is in the same IOMMU + * group. But we're not doing that for hostdevs yet. */ + + return virSecurityDACRestoreFileLabelInternal(mgr, NULL, vfioGroupDev, false); + } + return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, true); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 99cef3f212..a2e4dcb6da 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1751,9 +1751,8 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, { virSecurityLabelDefPtr seclabel; virSecurityDeviceLabelDefPtr disk_seclabel; - - if (!src->path || !virStorageSourceIsLocalStorage(src)) - return 0; + VIR_AUTOFREE(char *) vfioGroupDev = NULL; + const char *path = src->path; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) @@ -1785,9 +1784,16 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, * ownership, because that kills access on the destination host which is * sub-optimal for the guest VM's I/O attempts :-) */ if (migrated) { - int rc = virFileIsSharedFS(src->path); - if (rc < 0) - return -1; + int rc = 1; + + if (virStorageSourceIsLocalStorage(src)) { + if (!src->path) + return 0; + + if ((rc = virFileIsSharedFS(src->path)) < 0) + return -1; + } + if (rc == 1) { VIR_DEBUG("Skipping image label restore on %s because FS is shared", src->path); @@ -1795,7 +1801,26 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecuritySELinuxRestoreFileLabel(mgr, src->path, true); + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + VIR_AUTOUNREF(virPCIDevicePtr) pci = virPCIDeviceNew(nvme->pciAddr.domain, + nvme->pciAddr.bus, + nvme->pciAddr.slot, + nvme->pciAddr.function); + if (!pci || + !(vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci))) + return -1; + + /* Ideally, we would check if there is not another PCI + * device within domain def that is in the same IOMMU + * group. But we're not doing that for hostdevs yet. */ + path = vfioGroupDev; + } + + return virSecuritySELinuxRestoreFileLabel(mgr, path, true); } @@ -1820,6 +1845,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityDeviceLabelDefPtr disk_seclabel; virSecurityDeviceLabelDefPtr parent_seclabel = NULL; bool remember; + VIR_AUTOFREE(char *) vfioGroupDev = NULL; const char *path = src->path; const char *tcon = NULL; bool optional = false; @@ -1880,6 +1906,22 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, tcon = data->content_context; } + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + VIR_AUTOUNREF(virPCIDevicePtr) pci = virPCIDeviceNew(nvme->pciAddr.domain, + nvme->pciAddr.bus, + nvme->pciAddr.slot, + nvme->pciAddr.function); + if (!pci || + !(vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci))) + return -1; + + path = vfioGroupDev; + } + ret = virSecuritySELinuxSetFileconHelper(mgr, path, tcon, optional, remember); if (ret == 1 && !disk_seclabel) { -- 2.21.0

This capability tracks if qemu is capable of: -drive file.driver=nvme The feature was added in QEMU's commit of v2.12.0-rc0~104^2~2. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 3 +++ tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 + 20 files changed, 25 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 02e84edc15..87f506b09e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -533,6 +533,9 @@ VIR_ENUM_IMPL(virQEMUCaps, "x86-max-cpu", "cpu-unavailable-features", "canonical-cpu-features", + + /* 335 */ + "drive-nvme", ); @@ -1274,6 +1277,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "query-iothreads/ret-type/poll-max-ns", QEMU_CAPS_IOTHREAD_POLLING }, { "query-display-options/ret-type/+egl-headless/rendernode", QEMU_CAPS_EGL_HEADLESS_RENDERNODE }, { "nbd-server-add/arg-type/bitmap", QEMU_CAPS_NBD_BITMAP }, + { "blockdev-add/arg-type/+nvme", QEMU_CAPS_DRIVE_NVME }, }; typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 915ba6cb2e..fc288b023c 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -515,6 +515,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_CPU_UNAVAILABLE_FEATURES, /* "unavailable-features" CPU property */ QEMU_CAPS_CANONICAL_CPU_FEATURES, /* avoid CPU feature aliases */ + /* 335 */ + QEMU_CAPS_DRIVE_NVME, /* -drive file.driver=nvme */ + QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index 140da91b86..3b72024216 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -153,6 +153,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='drive-nvme'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700807</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index fd9ae0bcb8..d4ed7cea49 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -151,6 +151,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='drive-nvme'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900807</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 2930381068..91aee9f838 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -121,6 +121,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='drive-nvme'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100807</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 61b3602c48..96fa7c43c9 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -195,6 +195,7 @@ <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> <flag name='x86-max-cpu'/> + <flag name='drive-nvme'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100807</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml index 40718981a8..d848e55fc1 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml @@ -151,6 +151,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='drive-nvme'/> <version>2012050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900757</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml index 865becc179..d50789711a 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml @@ -92,6 +92,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='drive-nvme'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml index eb54aeaff3..06a2e98b90 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml @@ -92,6 +92,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='drive-nvme'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml index d511377262..a8fb99c9fd 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml @@ -123,6 +123,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='drive-nvme'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100757</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml index c6394db602..1c6fd4dd83 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml @@ -198,6 +198,7 @@ <flag name='memory-backend-file.align'/> <flag name='nvdimm.unarmed'/> <flag name='x86-max-cpu'/> + <flag name='drive-nvme'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100757</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml index ee6921ff92..dc340838a8 100644 --- a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml @@ -156,6 +156,7 @@ <flag name='memory-backend-file.align'/> <flag name='memory-backend-file.pmem'/> <flag name='overcommit'/> + <flag name='drive-nvme'/> <version>3000091</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900758</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml index a8cb061bf3..f1517fa365 100644 --- a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml @@ -201,6 +201,7 @@ <flag name='nvdimm.unarmed'/> <flag name='overcommit'/> <flag name='x86-max-cpu'/> + <flag name='drive-nvme'/> <version>3000092</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100758</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml index 250b7edd52..d2966b6045 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml @@ -163,6 +163,7 @@ <flag name='machine.virt.iommu'/> <flag name='bitmap-merge'/> <flag name='nbd-bitmap'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700758</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml index 24b55002a6..4afbc543cc 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml @@ -168,6 +168,7 @@ <flag name='query-current-machine'/> <flag name='bitmap-merge'/> <flag name='nbd-bitmap'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900758</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml index 230e1e7c99..1b8137612a 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml @@ -166,6 +166,7 @@ <flag name='query-current-machine'/> <flag name='bitmap-merge'/> <flag name='nbd-bitmap'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml index 4b2f4cf628..bd06a816d8 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml @@ -166,6 +166,7 @@ <flag name='query-current-machine'/> <flag name='bitmap-merge'/> <flag name='nbd-bitmap'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml index a1ac2587a0..82326dec79 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml @@ -131,6 +131,7 @@ <flag name='query-current-machine'/> <flag name='bitmap-merge'/> <flag name='nbd-bitmap'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100758</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index 716b756979..b8b46a7fa7 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -205,6 +205,7 @@ <flag name='bitmap-merge'/> <flag name='nbd-bitmap'/> <flag name='x86-max-cpu'/> + <flag name='drive-nvme'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100758</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml index 9cbf65b405..6bbe0603d3 100644 --- a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml @@ -207,6 +207,7 @@ <flag name='x86-max-cpu'/> <flag name='cpu-unavailable-features'/> <flag name='canonical-cpu-features'/> + <flag name='drive-nvme'/> <version>4000050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100759</microcodeVersion> -- 2.21.0

At the very beginning of the attach function the qemuDomainStorageSourceChainAccessAllow() is called which modifies CGroups, locks and seclabels for new disk and its backing chain. This must be followed by a counterpart which reverts back all the changes if something goes wrong. This boils down to calling qemuDomainStorageSourceChainAccessRevoke() which is done under 'error' label. But not all failure branches jump there. They just jump onto 'cleanup' label where no revoke is done. Such mistake is easy to do because 'cleanup' label does exist. Therefore, dissolve 'error' block in 'cleanup' and have everything jump onto 'cleanup' label. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7e9c1a1649..3c6c0da3a0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -624,13 +624,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, VIR_AUTOFREE(char *) corAlias = NULL; if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) - goto cleanup; + return -1; if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) - goto error; + goto cleanup; if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) - goto error; + goto cleanup; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) { if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON && @@ -647,13 +647,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, } if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, 0, priv->qemuCaps))) - goto error; + goto cleanup; if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0) - goto error; + goto cleanup; if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) - goto error; + goto cleanup; qemuDomainObjEnterMonitor(driver, vm); @@ -674,7 +674,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -2; - goto error; + goto cleanup; } virDomainAuditDisk(vm, NULL, disk->src, "attach", true); @@ -683,6 +683,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ret = 0; cleanup: + if (ret < 0) + ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); return ret; @@ -700,9 +702,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ret = -2; virDomainAuditDisk(vm, NULL, disk->src, "attach", false); - - error: - ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); goto cleanup; } -- 2.21.0

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 65 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3c6c0da3a0..6dbff23aa0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -605,6 +605,54 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } +static int +qemuDomainStorageSourcePrepareDisk(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + bool teardown) +{ + int rc; + bool adjustMemlock = false; + bool reattach = false; + + if (!virDomainDefHasNVMeDisk(vm->def) && + !virStorageSourceChainHasNVMe(disk->src)) + return 0; + + if (teardown) { + adjustMemlock = true; + reattach = true; + goto rollback; + } + + /* Tentatively add disk to domain def so that memlock limit can be computed. */ + vm->def->disks[vm->def->ndisks++] = disk; + rc = qemuDomainAdjustMaxMemLock(vm); + vm->def->disks[--vm->def->ndisks] = NULL; + + if (rc < 0) + return -1; + + adjustMemlock = true; + + if (qemuHostdevPrepareNVMeDevices(driver, vm->def->name, &disk, 1) < 0) + return -1; + + reattach = true; + + return 0; + + rollback: + if (reattach) + qemuHostdevReAttachNVMeDevices(driver, vm->def->name, &disk, 1); + + if (adjustMemlock) + qemuDomainAdjustMaxMemLock(vm); + + return 0; +} + + /** * qemuDomainAttachDiskGeneric: * @@ -623,8 +671,14 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, VIR_AUTOPTR(virJSONValue) corProps = NULL; VIR_AUTOFREE(char *) corAlias = NULL; + if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0) + return -1; + + if (qemuDomainStorageSourcePrepareDisk(driver, vm, disk, false) < 0) + return -1; + if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) - return -1; + goto cleanup; if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto cleanup; @@ -649,9 +703,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, 0, priv->qemuCaps))) goto cleanup; - if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0) - goto cleanup; - if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; @@ -683,8 +734,10 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ret = 0; cleanup: - if (ret < 0) + if (ret < 0) { ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); + qemuDomainStorageSourcePrepareDisk(driver, vm, disk, true); + } qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); return ret; @@ -4267,6 +4320,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, dev.data.disk = disk; ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name)); + qemuDomainStorageSourcePrepareDisk(driver, vm, disk, true); + if (virStorageSourceChainHasManagedPR(disk->src) && qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; -- 2.21.0