Re: [PATCH v1 12/31] util: Introduce virNVMeDevice module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux