Re: [PATCH v4 12/25] qemu: new functions to calculate/set device pciConnectFlags

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

 



On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:
> The lowest level function of this trio
> (qemuDomainDeviceCalculatePCIConnectFlags()) aims to be the single
> authority for the virDomainPCIConnectFlags to use for any given device
> using a particular arch/machinetype/qemu-binary.
> 
> qemuDomainFillDevicePCIConnectFlags() sets info->pciConnectFlags in a
> single device (unless it has no virDomainDeviceInfo, in which case
> it's a NOP).
> 
> qemuDomainFillAllPCIConnectFlags() sets info->pciConnectFlags in all
> devices that have a virDomainDeviceInfo
> 
> The latter two functions aren't called anywhere yet. This commit is
> just making them available. Later patches will replace all the current
> hodge-podge of flag settings with calls to this single authority.
> ---
> 
> Change: re-implemented as a giant two-level compound switch statement
>         with no default cases, as suggested by Andrea. Also start off
>         with the function setting the flags for *all* devices to
>         PCI_DEVICE|HOTPLUGGABLE, which is what's currently used when
>         assigning addresses (as opposed to merely validating addresses,
>         which is much less strict).
> 
>  src/conf/device_conf.h         |   5 +
>  src/qemu/qemu_domain_address.c | 379 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 384 insertions(+)
> 
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index 8443de6..f435fb5 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -142,6 +142,11 @@ typedef struct _virDomainDeviceInfo {
>      /* bootIndex is only used for disk, network interface, hostdev
>       * and redirdev devices */
>      unsigned int bootIndex;
> +
> +    /* pciConnectFlags is only used internally during address
> +     * assignment, never saved and never reported.
> +     */
> +    int pciConnectFlags; /* enum virDomainPCIConnectFlags */
>  } virDomainDeviceInfo, *virDomainDeviceInfoPtr;
>  
>  
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index b4ea906..457eae6 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -405,6 +405,385 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
>  }
>  
>  
> +/**
> + * qemuDomainDeviceCalculatePCIConnectFlags:
> + *
> + * Lowest level function to determine PCI connectFlags for a
> + * device. This function relies on the next higher-level function
> + * determining the value for pcieFlags and virtioFlags in advance -
> + * this is to make it more efficient to call multiple times.
> + *
> + * @dev: The device to be checked
> + *
> + * @pcieFlags: flags to use for a known PCI Express device
> + *
> + * @virtioFlags: flags to use for a virtio device (properly vetted
> + *       for the current qemu binary and arch/machinetype)
> + *
> + * returns appropriate virDomainPCIConnectFlags for this device in
> + * this domain, or 0 if the device doesn't connect using PCI. There
> + * is no failure.
> + */

Please adjust the formatting of this and later comments to
match the guidelines we discussed for earlier patches.

> +static virDomainPCIConnectFlags
> +qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
> +                                         virDomainPCIConnectFlags pcieFlags
> +                                         ATTRIBUTE_UNUSED,
> +                                         virDomainPCIConnectFlags virtioFlags
> +                                         ATTRIBUTE_UNUSED)
> +{
> +    virDomainPCIConnectFlags pciFlags =  (VIR_PCI_CONNECT_TYPE_PCI_DEVICE |

Double space.

> +                                          VIR_PCI_CONNECT_HOTPLUGGABLE);
> +
> +    switch ((virDomainDeviceType)dev->type) {

We use both '(type)expr' and '(type) expr', but the latter
seems to be more popular. Up to you whether or not you want
to conform :)

> +    case VIR_DOMAIN_DEVICE_CONTROLLER: {
> +        virDomainControllerDefPtr cont = dev->data.controller;
> +
> +        switch ((virDomainControllerType)cont->type) {
> +        case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> +            return virDomainPCIControllerModelToConnectType(cont->model);
> +
> +        case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
> +            return pciFlags;
> +
> +        case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> +            switch ((virDomainControllerModelUSB)cont->model) {
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
> +                return pciFlags;
> +
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
> +                return pciFlags;
> +
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1: /* xen only */
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2: /* xen only */
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
> +            case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
> +                /* should be 0 */
> +                return pciFlags;
> +            }
> +
> +        case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> +        case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> +        case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
> +            return pciFlags;
> +
> +        case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
> +        case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
> +        case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
> +            /* should be 0 */
> +            return pciFlags;
> +        }
> +    }
> +
> +    case VIR_DOMAIN_DEVICE_FS:
> +        /* the only type of filesystem so far is virtio-9p-pci */
> +        return pciFlags;
> +
> +    case VIR_DOMAIN_DEVICE_NET: {
> +        virDomainNetDefPtr net = dev->data.net;
> +
> +        /* NB: a type='hostdev' will use PCI, but its
> +         * address is assigned when we're assigning the
> +         * addresses for other hostdev devices.
> +         */
> +        if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
> +            STREQ(net->model, "usb-net")) {
> +            /* should be 0 */
> +            return pciFlags;
> +        }
> +        return pciFlags;
> +    }
> +
> +    case VIR_DOMAIN_DEVICE_SOUND:
> +        switch ((virDomainSoundModel)dev->data.sound->model) {
> +        case VIR_DOMAIN_SOUND_MODEL_ES1370:
> +        case VIR_DOMAIN_SOUND_MODEL_AC97:
> +        case VIR_DOMAIN_SOUND_MODEL_ICH6:
> +        case VIR_DOMAIN_SOUND_MODEL_ICH9:
> +            return pciFlags;
> +
> +        case VIR_DOMAIN_SOUND_MODEL_SB16:
> +        case VIR_DOMAIN_SOUND_MODEL_PCSPK:
> +        case VIR_DOMAIN_SOUND_MODEL_USB:
> +        case VIR_DOMAIN_SOUND_MODEL_LAST:
> +            /* should be 0 */
> +            return pciFlags;
> +        }
> +
> +    case VIR_DOMAIN_DEVICE_DISK:
> +        switch ((virDomainDiskBus)dev->data.disk->bus) {
> +        case VIR_DOMAIN_DISK_BUS_VIRTIO:
> +            return pciFlags; /* only virtio disks use PCI */
> +
> +        case VIR_DOMAIN_DISK_BUS_IDE:
> +        case VIR_DOMAIN_DISK_BUS_FDC:
> +        case VIR_DOMAIN_DISK_BUS_SCSI:
> +        case VIR_DOMAIN_DISK_BUS_XEN:
> +        case VIR_DOMAIN_DISK_BUS_USB:
> +        case VIR_DOMAIN_DISK_BUS_UML:
> +        case VIR_DOMAIN_DISK_BUS_SATA:
> +        case VIR_DOMAIN_DISK_BUS_SD:
> +        case VIR_DOMAIN_DISK_BUS_LAST:
> +            /* should be 0 */
> +            return pciFlags;
> +        }
> +
> +    case VIR_DOMAIN_DEVICE_HOSTDEV:
> +        return pciFlags;
> +
> +    case VIR_DOMAIN_DEVICE_MEMBALLOON:
> +        switch ((virDomainMemBalloonModel)dev->data.memballoon->model) {

You will of course need to s/MemBalloon/Memballoon/ here now.

> +        case VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO:
> +            return pciFlags;
> +
> +        case VIR_DOMAIN_MEMBALLOON_MODEL_XEN:
> +        case VIR_DOMAIN_MEMBALLOON_MODEL_NONE:
> +        case VIR_DOMAIN_MEMBALLOON_MODEL_LAST:
> +            /* should be 0 (not PCI) */
> +            return pciFlags;
> +        }
> +
> +    case VIR_DOMAIN_DEVICE_RNG:
> +        switch ((virDomainRNGModel)dev->data.rng->model) {
> +        case VIR_DOMAIN_RNG_MODEL_VIRTIO:
> +            return pciFlags;
> +
> +        case VIR_DOMAIN_RNG_MODEL_LAST:
> +            /* should be 0 */
> +            return pciFlags;
> +        }
> +
> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
> +        /* only one model connects using PCI */
> +        switch ((virDomainWatchdogModel)dev->data.watchdog->model) {
> +        case VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB:
> +            return pciFlags;
> +
> +        case VIR_DOMAIN_WATCHDOG_MODEL_IB700:
> +        case VIR_DOMAIN_WATCHDOG_MODEL_DIAG288:
> +        case VIR_DOMAIN_WATCHDOG_MODEL_LAST:
> +            /* should be 0 */
> +            return pciFlags;
> +        }
> +
> +    case VIR_DOMAIN_DEVICE_VIDEO:
> +        switch ((virDomainVideoType)dev->data.video->type) {
> +        case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
> +        case VIR_DOMAIN_VIDEO_TYPE_VGA:
> +        case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> +        case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
> +        case VIR_DOMAIN_VIDEO_TYPE_XEN:
> +        case VIR_DOMAIN_VIDEO_TYPE_VBOX:
> +        case VIR_DOMAIN_VIDEO_TYPE_QXL:
> +        case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
> +            return pciFlags;
> +
> +        case VIR_DOMAIN_VIDEO_TYPE_LAST:
> +            /* should be 0 */
> +            return pciFlags;
> +        }
> +
> +
> +    case VIR_DOMAIN_DEVICE_SHMEM:
> +        return pciFlags;
> +
> +    case VIR_DOMAIN_DEVICE_INPUT:
> +        switch ((virDomainInputBus)dev->data.input->bus) {
> +        case VIR_DOMAIN_INPUT_BUS_VIRTIO:
> +            return pciFlags;
> +
> +        case VIR_DOMAIN_INPUT_BUS_PS2:
> +        case VIR_DOMAIN_INPUT_BUS_USB:
> +        case VIR_DOMAIN_INPUT_BUS_XEN:
> +        case VIR_DOMAIN_INPUT_BUS_PARALLELS:
> +        case VIR_DOMAIN_INPUT_BUS_LAST:
> +            /* should be 0 */
> +            return pciFlags;
> +        }
> +
> +    case VIR_DOMAIN_DEVICE_CHR:
> +        switch ((virDomainChrSerialTargetType)dev->data.chr->targetType) {
> +        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
> +            return pciFlags;
> +
> +        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
> +        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
> +        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
> +            /* should be 0 */
> +            return pciFlags;
> +        }
> +
> +    /* These devices don't ever connect with PCI */
> +    case VIR_DOMAIN_DEVICE_NVRAM:
> +    case VIR_DOMAIN_DEVICE_TPM:
> +    case VIR_DOMAIN_DEVICE_PANIC:
> +    case VIR_DOMAIN_DEVICE_MEMORY:
> +    case VIR_DOMAIN_DEVICE_HUB:
> +    case VIR_DOMAIN_DEVICE_REDIRDEV:
> +    case VIR_DOMAIN_DEVICE_SMARTCARD:
> +    /* These devices don't even have a DeviceInfo */
> +    case VIR_DOMAIN_DEVICE_LEASE:
> +    case VIR_DOMAIN_DEVICE_GRAPHICS:
> +    case VIR_DOMAIN_DEVICE_IOMMU:
> +    case VIR_DOMAIN_DEVICE_LAST:
> +    case VIR_DOMAIN_DEVICE_NONE:
> +        /* should be 0 */
> +        return pciFlags;
> +    }
> +
> +    /* We can never get here, because all cases are covered in the
> +     * switch, and they all return, but the compiler will still
> +     * complain "control reaches end of non-void function" unless
> +     * we add the following return.
> +     */
> +    return 0;
> +}
> +
> +
> +typedef struct {
> +    virDomainPCIConnectFlags virtioFlags;
> +    virDomainPCIConnectFlags pcieFlags;
> +} qemuDomainFillDevicePCIConnectFlagsIterData;
> +

Add one more empty line here.

> +/**
> + * qemuDomainFillDevicePCIConnectIterInit:

Should be qemuDomainFillDevicePCIConnectFlagsIterInit(), even
though it's already quite a mouthful as it is.

> + *
> + * Initialize the iterator data that is used when calling
> + * qemuDomainCalculateDevicePCIConnectFlags().
> + *

Remove this empty line.

> + */
> +static void
> +qemuDomainFillDevicePCIConnectIterInit(virDomainDefPtr def,
> +                                       virQEMUCapsPtr qemuCaps,
> +                                       qemuDomainFillDevicePCIConnectFlagsIterData *data)
> +{
> +    if (qemuDomainMachineHasPCIeRoot(def)) {
> +        data->pcieFlags =  (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE |
> +                            VIR_PCI_CONNECT_HOTPLUGGABLE);

Double space here...

> +    } else {
> +        data->pcieFlags =  (VIR_PCI_CONNECT_TYPE_PCI_DEVICE |
> +                            VIR_PCI_CONNECT_HOTPLUGGABLE);

... and again here.

> +    }
> +
> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {
> +        data->virtioFlags = data->pcieFlags;
> +    } else {
> +        data->virtioFlags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE |
> +                             VIR_PCI_CONNECT_HOTPLUGGABLE);
> +    }
> +}
> +
> +
> +/**
> + * qemuDomainFillDevicePCIConnectFlagsIter:
> + *
> + * The version of the function to call with
> + * virDomainDeviceInfoIterate()

Please change this to something along the lines of

  Sets PCI connect flags for @dev. For use with
  virDomainDeviceInfoIterate().

> + *
> + * @def: the entire DomainDef
> + *
> + * @dev: The device to be checked
> + *
> + * @info: virDomainDeviceInfo within the device
> + *
> + * @opaque: points to iterator data setup beforehand.
> + *
> + * Sets @info->pciConnectFlags. Always returns 0 - there
> + * is no failure.
> + *
> + */
> +static int
> +qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
> +                                        virDomainDeviceDefPtr dev,
> +                                        virDomainDeviceInfoPtr info,
> +                                        void *opaque)
> +{
> +    qemuDomainFillDevicePCIConnectFlagsIterData *data = opaque;
> +
> +    info->pciConnectFlags
> +        = qemuDomainDeviceCalculatePCIConnectFlags(dev, data->pcieFlags,
> +                                                   data->virtioFlags);
> +    return 0;
> +}
> +
> +
> +/**
> + * qemuDomainFillAllPCIConnectFlags:
> + *
> + * Set the info->pciConnectFlags for all devices in the domain.
> + *
> + * @def: the entire DomainDef
> + *
> + * @qemuCaps: as you'd expect
> + *
> + * sets info->pciConnectFlags for all devices as appropriate. returns
> + * 0 on success or -1 on failure (the only possibility of failure would
> + * be some internal problem with virDomainDeviceInfoIterate())
> + */
> +static int ATTRIBUTE_UNUSED
> +qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def,
> +                                 virQEMUCapsPtr qemuCaps)
> +{
> +    qemuDomainFillDevicePCIConnectFlagsIterData data;
> +
> +    qemuDomainFillDevicePCIConnectIterInit(def, qemuCaps, &data);
> +
> +    return virDomainDeviceInfoIterate(def,
> +                                      qemuDomainFillDevicePCIConnectFlagsIter,
> +                                      &data);
> +}
> +
> +
> +/**
> + * qemuDomainFillDevicePCIConnectFlags:
> + *
> + * The version of the function to call if it will be called just
> + * once.

The description for qemuDomainFillAllPCIConnectFlags() is
pretty good, please change this one to be along those lines.

> + *
> + * @def: the entire DomainDef
> + *
> + * @dev: The device to be checked
> + *
> + * @qemuCaps: as you'd expect
> + *
> + * sets device's info->pciConnectFlags when appropriate.
> + *
> + */
> +static void ATTRIBUTE_UNUSED
> +qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def,
> +                                    virDomainDeviceDefPtr dev,
> +                                    virQEMUCapsPtr qemuCaps)
> +{
> +    virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
> +
> +    if (info) {
> +        /* qemuDomainDeviceCalculatePCIConnectFlags() is called with
> +         * the data setup in the ...IterData by ...IterInit() rather
> +         * than setting the values directly here.  It may seem like
> +         * pointless posturing, but it's done this way to eliminate
> +         * duplicated setup code while allowing more efficient
> +         * operation when it's being done repeatedly with the device
> +         * iterator (since qemuDomainFillAllPCIConnectFlags() only
> +         * calls ...IterInit() once for all devices).
> +         */

I feel like this comment would be a better fit for
qemuDomainFillDevicePCIConnectIterInit().

> +        qemuDomainFillDevicePCIConnectFlagsIterData data;
> +
> +        qemuDomainFillDevicePCIConnectIterInit(def, qemuCaps, &data);
> +
> +        info->pciConnectFlags
> +            = qemuDomainDeviceCalculatePCIConnectFlags(dev, data.pcieFlags,
> +                                                       data.virtioFlags);
> +    }
> +}
> +
> +
>  static int
>  qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
>                                      virDomainDeviceInfoPtr dev,

ACK after you take care of the stuff mentioned.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
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]