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