On Mon, 2016-12-19 at 10:25 -0500, Laine Stump wrote:
> Although setting virDomainPCIAddressReserveAddr()'s fromConfig=true is
> correct when a PCI addres is coming from a domain's config, the *true*
> purpose of the fromConfig argument is to lower restrictions on what
> kind of device can plug into what kind of controller - if fromConfig
> is true, then a PCIE_DEVICE can plug into a slot that is marked as
> only compatible with PCI_DEVICE (and vice versa), and the HOTPLUG flag
> is ignored.
>
> For a long time there have been several calls to
> virDomainPCIAddressReserveAddr() that have fromConfig incorrectly set
> to false - it's correct that the addresses aren't coming from user
> config, but they are coming from hardcoded exceptions in libvirt that
> should, if anything, pay *even less* attention to following the
> pciConnectFlags (under the assumption that the libvirt programmer knew
> what they were doing).
It seems to me that many issues with incorrect usage of the
fromConfig parameter can be traced back to the fact that it's
being assigned two related but not overlapping meanings:
* the address being assigned or validated comes from the
user configuration and error reporting should be worded
accordingly
* use more relaxed rules when assigning or validating the
address, eg. allow legacy PCI devices to be plugged into
PCIe slots and whatnot
The former basically implies the latter, because we assume
the user[1] knows what they are doing; however, the spots
you're touching with this patch only call for the latter
behavior.
[...]
> @@ -591,7 +591,7 @@ virDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr addr,
> virDomainPCIConnectFlags flags)
> {
> - return virDomainPCIAddressReserveAddr(addrs, addr, flags, false);
> + return virDomainPCIAddressReserveAddr(addrs, addr, flags, true);
> }
All uses of virDomainPCIAddressReserveSlot() in the QEMU
driver are for devices we're adding ourselves with explicit
PCI addresses, and those in the bhyve driver are for devices
that come from the guest configuration, so this change
should be safe.
That said, it leads to a weird situation in which
ReserveSlot() implies fromConfig=true and ReserveNextSlot()
implies fromConfig=false, which is very confusing.
I know you're going to rename and change both functions
further down the series so it's not a big issue for now.
ACK
[1] Just like libvirt programmers ;)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
