On Thu, Jul 11, 2019 at 17:53:59 +0200, Michal Privoznik wrote: > 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@xxxxxxxxxx> > --- > 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/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"); please use a different log domain > +#define VIR_FROM_THIS VIR_FROM_NONE > + > +struct _virNVMeDevice { > + virPCIDeviceAddress address; /* PCI address of controller */ > + unsigned long namespace; /* Namespace ID */ unsinged int/unsigned long long > + bool managed; > + > + char *drvname; > + char *domname; > +}; > + > + > +struct _virNVMeDeviceList { > + virObjectLockable parent; > + > + size_t count; > + virNVMeDevicePtr *devs; > +}; > + [...] > +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) [1] (see below) > +{ > + 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 This function seems to be too unsafe to export as people might want to store the index while not holding the lock and something would then change it. Also [1] has the same issue. > +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, This function is too complex to be without a comment describing it. Especially since it's returning list of pci devices. > + 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); > +} Note that I did not look at the patches using the code, thus comments about some APIs being unnecessary may not be true.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
