[libvirt] [PATCH 0/2] replacement for Patches 5/8 & 6/8 of "aggregate multiple pcie-root-ports onto a single slot"
During review of the aforementioned series, Andrea expressed misgivings about patch 6, which automatically set the multifunction=on option transiently at the time the qemu commandline was generated. Dan agreed. These two patches replace patches 5 and 6 of that series with two patches that will set multi='on' in the domain's config rather than just temporarily in the commandline. This way it will remain set even if all the devices on the non-0 function are later removed (unless it's explicitly changed by the user/management app, that is). The rest of the series is ACKed and ready to push, which I'd like to do before the freeze on Wednesday; these two patche are all that's holding it up. Laine Stump (2): conf: new function virDomainPCIAddressSetAllMulti() qemu: use virDomainPCIAddressSetAllMulti() to set multi when needed src/conf/domain_addr.c | 75 +++++++++++++ src/conf/domain_addr.h | 4 + src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 5 + .../qemuxml2argv-q35-multifunction.args | 42 ++++++++ .../qemuxml2argv-q35-multifunction.xml | 51 +++++++++ tests/qemuxml2argvtest.c | 23 ++++ .../qemuxml2xmlout-q35-multifunction.xml | 120 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 23 ++++ 9 files changed, 344 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-multifunction.xml -- 2.7.4
This utility function looks through a virPCIAddressSet for any slot of any bus that has multiple functions in use, and sets the "multi" flag in the virDomainDeviceInfo for the device that is assigned to function 0 of that slot (as long as it hasn't already been set explicitly by someone who presumably has better information than we do). It isn't yet called from anywhere, so will have no functional effect. --- src/conf/domain_addr.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 4 +++ src/libvirt_private.syms | 1 + 3 files changed, 80 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 4d14ac4..0364e95 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -867,6 +867,81 @@ virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs, } +static int +virDomainPCIAddressSetMultiIter(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *data) +{ + virPCIDeviceAddressPtr testAddr = data; + virPCIDeviceAddressPtr thisAddr; + + if (!info || info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + return 0; + + thisAddr = &info->addr.pci; + + if (thisAddr->domain == testAddr->domain && + thisAddr->bus == testAddr->bus && + thisAddr->slot == testAddr->slot && + thisAddr->function == 0) { + + /* only set to ON if it wasn't previously set + * (assuming that the user must have better information + * than us if they explicitly set it OFF) + */ + if (thisAddr->multi == VIR_TRISTATE_SWITCH_ABSENT) + thisAddr->multi = VIR_TRISTATE_SWITCH_ON; + + return -1; /* finish early, *NOT* an error */ + } + + return 0; +} + + +/** + * virDomainPCIAddressSetAllMulti(): + * + * @def: the domain definition whose devices may need adjusting + * @addrs: address set keeping track of all addresses in use. + * + * Look for any PCI slots that have multiple functions assigned, and + * set multi to YES in the address for the device at function 0 + * (unless it has been explicitly set to NO). + * + * No return code, since there is no possibility of failure. + */ +void +virDomainPCIAddressSetAllMulti(virDomainDefPtr def, + virDomainPCIAddressSetPtr addrs) +{ + /* Scan through all the slots in @addrs looking for any that have + * more than just function 0 marked as in use, then use an + * iterator to find the DeviceInfo that uses function 0 on that + * slot and mark it as multi = YES + */ + size_t busIdx, slotIdx; + + for (busIdx = 0; busIdx < addrs->nbuses; busIdx++) { + virDomainPCIAddressBusPtr bus = &addrs->buses[busIdx]; + + for (slotIdx = bus->minSlot; slotIdx <= bus->maxSlot; slotIdx++) { + if (bus->slot[slotIdx].functions > 1) { + virPCIDeviceAddress addr = { .domain = 0, + .bus = busIdx, + .slot = slotIdx, + .function = 0 }; + + ignore_value(virDomainDeviceInfoIterate(def, + virDomainPCIAddressSetMultiIter, + &addr)); + } + } + } +} + + static char* virDomainCCWAddressAsString(virDomainDeviceCCWAddressPtr addr) { diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 5f0924e..2263114 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -173,6 +173,10 @@ int virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs, virDomainPCIConnectFlags flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void virDomainPCIAddressSetAllMulti(virDomainDefPtr def, + virDomainPCIAddressSetPtr addrs) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + struct _virDomainCCWAddressSet { virHashTablePtr defined; virDomainDeviceCCWAddress next; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a928e90..a0db583 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -104,6 +104,7 @@ virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextAddr; virDomainPCIAddressReserveNextSlot; virDomainPCIAddressReserveSlot; +virDomainPCIAddressSetAllMulti; virDomainPCIAddressSetAlloc; virDomainPCIAddressSetFree; virDomainPCIAddressSetGrow; -- 2.7.4
On Tue, 2017-01-10 at 03:23 -0500, Laine Stump wrote:
This utility function looks through a virPCIAddressSet for any slot of any bus that has multiple functions in use, and sets the "multi" flag in the virDomainDeviceInfo for the device that is assigned to function 0 of that slot (as long as it hasn't already been set explicitly by someone who presumably has better information than we do). It isn't yet called from anywhere, so will have no functional effect.
[...]
+static int +virDomainPCIAddressSetMultiIter(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *data)
virDomainPCIAddressSetAllMultiIter()? Or not :) [...]
+/** + * virDomainPCIAddressSetAllMulti(): + * + * @def: the domain definition whose devices may need adjusting + * @addrs: address set keeping track of all addresses in use. + * + * Look for any PCI slots that have multiple functions assigned, and + * set multi to YES in the address for the device at function 0
s/YES/ON/
+ * (unless it has been explicitly set to NO).
s/NO/OFF/
+ * + * No return code, since there is no possibility of failure. + */ +void +virDomainPCIAddressSetAllMulti(virDomainDefPtr def, + virDomainPCIAddressSetPtr addrs)
I also just realized that virDomainPCIAddressSetAllMulti() might be mistaken for vir::DomainPCIAddressSet::AllMulti() instead of vir::Domain::PCIAddressSetAllMulti(). The joys of pretending to use an object-oriented language when actually all you have is plain old C :)
+{ + /* Scan through all the slots in @addrs looking for any that have + * more than just function 0 marked as in use, then use an + * iterator to find the DeviceInfo that uses function 0 on that + * slot and mark it as multi = YES
s/YES/ON/
+ */ + size_t busIdx, slotIdx; + + for (busIdx = 0; busIdx < addrs->nbuses; busIdx++) { + virDomainPCIAddressBusPtr bus = &addrs->buses[busIdx]; + + for (slotIdx = bus->minSlot; slotIdx <= bus->maxSlot; slotIdx++) { + if (bus->slot[slotIdx].functions > 1) { + virPCIDeviceAddress addr = { .domain = 0,
This doesn't look right. What happens if the multifunction device is at eg. 0001:00:04.0? ... Oh, never mind, apparently virDomainPCIAddressSet has no concept of PCI domains anyways... So that's an existing limitation - one that I assume we're going to have to address, especially now that ppc64 guests are going to want to use *lots* of separate PCI domains. On the other hand, at the point where you're going to call this function haven't PCI addresses already been assigned and stored in @def? Why not iterate over it twice to make the comparison future-proof? The computational cost would be the same AFAICT. -- Andrea Bolognani / Red Hat / Virtualization
This utility function iterates through all devices looking for any with a PCI address that has function != 0 (which implies that multiple functions are in use on that slot), then uses an inner iterator to find the device that's on function 0 of that same slot and sets the "multi" in its virDomainDeviceInfo (as long as it hasn't already been set explicitly by someone who presumably has better information than we do). It isn't yet called from anywhere, so will have no functional effect. --- Change from V1: implement as two nested deviceinfo iterators as suggested by Andrea. src/conf/domain_addr.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 3 ++ src/libvirt_private.syms | 1 + 3 files changed, 86 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 4d14ac4..453f287 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -867,6 +867,88 @@ virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs, } +static int +virDomainPCIAddressSetMultiIter(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *data) +{ + virPCIDeviceAddressPtr testAddr = data; + virPCIDeviceAddressPtr thisAddr; + + if (!info || info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + return 0; + + thisAddr = &info->addr.pci; + + if (thisAddr->domain == testAddr->domain && + thisAddr->bus == testAddr->bus && + thisAddr->slot == testAddr->slot && + thisAddr->function == 0) { + + /* only set to ON if it wasn't previously set + * (assuming that the user must have better information + * than us if they explicitly set it OFF) + */ + if (thisAddr->multi == VIR_TRISTATE_SWITCH_ABSENT) + thisAddr->multi = VIR_TRISTATE_SWITCH_ON; + + return -1; /* finish early, *NOT* an error */ + } + + return 0; +} + + +static int +virDomainPCIAddressSetAllMultiIter(virDomainDefPtr def, + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *data ATTRIBUTE_UNUSED) +{ + virPCIDeviceAddressPtr testAddr; + + if (!info || info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + return 0; + + testAddr = &info->addr.pci; + if (testAddr->function != 0) { + ignore_value(virDomainDeviceInfoIterate(def, + virDomainPCIAddressSetMultiIter, + testAddr)); + } + + return 0; +} + + +/** + * virDomainPCIAddressSetAllMulti(): + * + * @def: the domain definition whose devices may need adjusting + * @addrs: address set keeping track of all addresses in use. + * + * Look for any PCI slots that have multiple functions assigned, and + * set multi to ON in the address for the device at function 0 + * (unless it has been explicitly set to OFF). + * + * No return code, since there is no possibility of failure. + */ +void +virDomainPCIAddressSetAllMulti(virDomainDefPtr def) +{ + /* Use nested iterators over all the devices - the outer iterator + * scans through all the devices looking for those whose address + * has a non-0 function; when one is found, the inner iterator looks + * for the device that uses function 0 on the same slot and marks + * it as multi = ON + */ + ignore_value(virDomainDeviceInfoIterate(def, + virDomainPCIAddressSetAllMultiIter, + NULL)); +} + + static char* virDomainCCWAddressAsString(virDomainDeviceCCWAddressPtr addr) { diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 5f0924e..c728c00 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -173,6 +173,9 @@ int virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs, virDomainPCIConnectFlags flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void virDomainPCIAddressSetAllMulti(virDomainDefPtr def) + ATTRIBUTE_NONNULL(1); + struct _virDomainCCWAddressSet { virHashTablePtr defined; virDomainDeviceCCWAddress next; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 82adf7e..bdc0272 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -104,6 +104,7 @@ virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextAddr; virDomainPCIAddressReserveNextSlot; virDomainPCIAddressReserveSlot; +virDomainPCIAddressSetAllMulti; virDomainPCIAddressSetAlloc; virDomainPCIAddressSetFree; virDomainPCIAddressSetGrow; -- 2.7.4
On Tue, 2017-01-10 at 11:55 -0500, Laine Stump wrote:
This utility function iterates through all devices looking for any with a PCI address that has function != 0 (which implies that multiple functions are in use on that slot), then uses an inner iterator to find the device that's on function 0 of that same slot and sets the "multi" in its virDomainDeviceInfo (as long as it hasn't already been set explicitly by someone who presumably has better information than we do). It isn't yet called from anywhere, so will have no functional effect.
[...]
+static int +virDomainPCIAddressSetMultiIter(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *data)
virDomainPCIAddressSetAllMultiIterInner()? Or maybe virDomainPCIAddressSetAllMultiInnerIter()? Just kidding ;) [...]
+static int +virDomainPCIAddressSetAllMultiIter(virDomainDefPtr def, + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *data ATTRIBUTE_UNUSED) +{ + virPCIDeviceAddressPtr testAddr; + + if (!info || info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + return 0; + + testAddr = &info->addr.pci;
Empty line here.
+ if (testAddr->function != 0) { + ignore_value(virDomainDeviceInfoIterate(def, + virDomainPCIAddressSetMultiIter, + testAddr)); + } + + return 0; +}
ACK -- Andrea Bolognani / Red Hat / Virtualization
If there are multiple devices assigned to the different functions of a single PCI slot, they will not work properly if the device at function 0 doesn't have its "multi" attribute turned on, so it makes sense for libvirt to turn it on during PCI address assignment. Just in case a future change in the configuration eliminates the devices on all non-0 functions, and in case some guest OS would be confused by having the multi flag of function 0 modified to "no" at the next boot of the guest (it may seem like an inconsequential guest ABI change, but it *is* a guest ABI change). We are set multi during PCI address assignment (which happens in the domain post-parse callback) rather than at qemu runtime so that the setting will be stored in config and persist even in the case that the devices in the non-0 functions of the slot are later removed from the config - this is done to prevent a "silent" guest ABI change (although it can only happen after shutting down and restarting the guest, and although it may seem like an inconsequential guest ABI change, it *is* a guest ABI change to turn off the multi bit). --- src/qemu/qemu_domain_address.c | 5 + .../qemuxml2argv-q35-multifunction.args | 42 ++++++++ .../qemuxml2argv-q35-multifunction.xml | 51 +++++++++ tests/qemuxml2argvtest.c | 23 ++++ .../qemuxml2xmlout-q35-multifunction.xml | 120 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 23 ++++ 6 files changed, 264 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-multifunction.xml diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 4a2a489..d350921 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -2088,6 +2088,11 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; + /* set multi attribute for devices at function 0 of + * any slot that has multiple functions in use + */ + virDomainPCIAddressSetAllMulti(def, addrs); + for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args b/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args new file mode 100644 index 0000000..6c4e9b2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args @@ -0,0 +1,42 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm \ +-name q35-test \ +-S \ +-M q35 \ +-m 2048 \ +-smp 2,sockets=2,cores=1,threads=1 \ +-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-q35-test/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device ioh3420,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\ +addr=0x2 \ +-device ioh3420,port=0x11,chassis=2,id=pci.2,bus=pcie.0,addr=0x2.0x1 \ +-device ioh3420,port=0x12,chassis=3,id=pci.3,bus=pcie.0,addr=0x2.0x2 \ +-device ioh3420,port=0x18,chassis=4,id=pci.4,bus=pcie.0,multifunction=on,\ +addr=0x3 \ +-device ioh3420,port=0x19,chassis=5,id=pci.5,bus=pcie.0,multifunction=on,\ +addr=0x3.0x1 \ +-device ioh3420,port=0x20,chassis=6,id=pci.6,bus=pcie.0,multifunction=off,\ +addr=0x4 \ +-device ioh3420,port=0x21,chassis=7,id=pci.7,bus=pcie.0,addr=0x4.0x1 \ +-device ioh3420,port=0x8,chassis=8,id=pci.8,bus=pcie.0,addr=0x1 \ +-device ioh3420,port=0x28,chassis=9,id=pci.9,bus=pcie.0,addr=0x5 \ +-device ioh3420,port=0x30,chassis=10,id=pci.10,bus=pcie.0,addr=0x6 \ +-device ioh3420,port=0x38,chassis=11,id=pci.11,bus=pcie.0,addr=0x7 \ +-device ioh3420,port=0x40,chassis=12,id=pci.12,bus=pcie.0,addr=0x8 \ +-device ioh3420,port=0x48,chassis=13,id=pci.13,bus=pcie.0,addr=0x9 \ +-device ioh3420,port=0x50,chassis=14,id=pci.14,bus=pcie.0,addr=0xa \ +-device ioh3420,port=0x58,chassis=15,id=pci.15,bus=pcie.0,addr=0xb \ +-device ioh3420,port=0x60,chassis=16,id=pci.16,bus=pcie.0,addr=0xc \ +-device ioh3420,port=0x68,chassis=17,id=pci.17,bus=pcie.0,addr=0xd \ +-device ioh3420,port=0x70,chassis=18,id=pci.18,bus=pcie.0,addr=0xe \ +-device nec-usb-xhci,id=usb,bus=pci.1,addr=0x0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml new file mode 100644 index 0000000..c1edca1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml @@ -0,0 +1,51 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>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/libexec/qemu-kvm</emulator> + <controller type='pci' model='pcie-root'/> + <controller type='pci' model='pcie-root-port'> + <address type='pci' slot='2' function='0'/> + </controller> + <controller type='pci' model='pcie-root-port'> + <address type='pci' slot='2' function='1'/> + </controller> + <controller type='pci' model='pcie-root-port'> + <address type='pci' slot='2' function='2'/> + </controller> + <controller type='pci' model='pcie-root-port'> + <address type='pci' slot='3' function='0'/> + </controller> + <controller type='pci' model='pcie-root-port'> + <address type='pci' slot='3' function='1' multifunction='on'/> + </controller> + <controller type='pci' model='pcie-root-port'> + <address type='pci' slot='4' function='0' multifunction='off'/> + </controller> + <controller type='pci' model='pcie-root-port'> + <address type='pci' slot='4' function='1'/> + </controller> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + <controller type='pci' model='pcie-root-port'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ab3ad08..307caa8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1924,6 +1924,29 @@ mymain(void) QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_NEC_USB_XHCI, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + DO_TEST("q35-multifunction", + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, + QEMU_CAPS_DEVICE_VIRTIO_RNG, + QEMU_CAPS_OBJECT_RNG_RANDOM, + QEMU_CAPS_NETDEV, + QEMU_CAPS_DEVICE_VIRTIO_NET, + QEMU_CAPS_DEVICE_VIRTIO_GPU, + QEMU_CAPS_VIRTIO_GPU_VIRGL, + QEMU_CAPS_VIRTIO_KEYBOARD, + QEMU_CAPS_VIRTIO_MOUSE, + QEMU_CAPS_VIRTIO_TABLET, + QEMU_CAPS_VIRTIO_INPUT_HOST, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_FSDEV, + QEMU_CAPS_FSDEV_WRITEOUT, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, + QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_NEC_USB_XHCI, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY); DO_TEST("q35-virt-manager-basic", QEMU_CAPS_KVM, QEMU_CAPS_RTC, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-multifunction.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-multifunction.xml new file mode 100644 index 0000000..7da78a5 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-multifunction.xml @@ -0,0 +1,120 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>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/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='1' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='2' port='0x11'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x1'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x12'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x2'/> + </controller> + <controller type='pci' index='4' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='4' port='0x18'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='5' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='5' port='0x19'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x1' multifunction='on'/> + </controller> + <controller type='pci' index='6' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='6' port='0x20'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='off'/> + </controller> + <controller type='pci' index='7' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='7' port='0x21'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x1'/> + </controller> + <controller type='pci' index='8' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='8' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='9' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='9' port='0x28'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </controller> + <controller type='pci' index='10' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='10' port='0x30'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </controller> + <controller type='pci' index='11' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='11' port='0x38'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </controller> + <controller type='pci' index='12' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='12' port='0x40'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </controller> + <controller type='pci' index='13' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='13' port='0x48'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> + </controller> + <controller type='pci' index='14' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='14' port='0x50'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </controller> + <controller type='pci' index='15' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='15' port='0x58'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> + </controller> + <controller type='pci' index='16' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='16' port='0x60'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> + </controller> + <controller type='pci' index='17' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='17' port='0x68'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/> + </controller> + <controller type='pci' index='18' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='18' port='0x70'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> + </controller> + <controller type='usb' index='0' model='nec-xhci'> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index ddd17cb..569a375 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -795,6 +795,29 @@ mymain(void) QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_NEC_USB_XHCI, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + DO_TEST("q35-multifunction", + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, + QEMU_CAPS_DEVICE_VIRTIO_RNG, + QEMU_CAPS_OBJECT_RNG_RANDOM, + QEMU_CAPS_NETDEV, + QEMU_CAPS_DEVICE_VIRTIO_NET, + QEMU_CAPS_DEVICE_VIRTIO_GPU, + QEMU_CAPS_VIRTIO_GPU_VIRGL, + QEMU_CAPS_VIRTIO_KEYBOARD, + QEMU_CAPS_VIRTIO_MOUSE, + QEMU_CAPS_VIRTIO_TABLET, + QEMU_CAPS_VIRTIO_INPUT_HOST, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_FSDEV, + QEMU_CAPS_FSDEV_WRITEOUT, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, + QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_NEC_USB_XHCI, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY); DO_TEST("q35-virt-manager-basic", QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, QEMU_CAPS_DEVICE_VIRTIO_RNG, -- 2.7.4
On Tue, 2017-01-10 at 03:23 -0500, Laine Stump wrote:
If there are multiple devices assigned to the different functions of a single PCI slot, they will not work properly if the device at function 0 doesn't have its "multi" attribute turned on, so it makes sense for libvirt to turn it on during PCI address assignment. Just in case a future change in the configuration eliminates the devices on all non-0 functions, and in case some guest OS would be confused by having the multi flag of function 0 modified to "no" at the next boot of the guest (it may seem like an inconsequential guest ABI change, but it *is* a guest ABI change). We are set multi during PCI address assignment (which happens in the domain post-parse callback) rather than at qemu runtime so that the setting will be stored in config and persist even in the case that the devices in the non-0 functions of the slot are later removed from the config - this is done to prevent a "silent" guest ABI change (although it can only happen after shutting down and restarting the guest, and although it may seem like an inconsequential guest ABI change, it *is* a guest ABI change to turn off the multi bit).
The commit messages seems to contain editing artifacts that I trust you'll get rid of before pushing. [...]
+ /* set multi attribute for devices at function 0 of + * any slot that has multiple functions in use + */ + virDomainPCIAddressSetAllMulti(def, addrs);
This will of course have to be changed now that virDomainPCIAddressSetAllMulti() takes a single argument. ACK -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Laine Stump