1:42 p.m.
On 10/04/2016 10:11 AM, Andrea Bolognani wrote:
On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> Set pciConnectFlags in each device's DeviceInfo prior to assigning PCI
> addresses, and then use those flags later when actually assigning the
> addresses with qemuDomainPCIAddressReserveNextAddr() (rather than
> scattering the logic about which devices need which type of slot all
> over the place).
> ---
> src/qemu/qemu_domain_address.c | 234 ++++++++++++++++++-----------------------
> 1 file changed, 104 insertions(+), 130 deletions(-)
[...]
> @@ -686,9 +685,6 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
> int ret = -1;
> virPCIDeviceAddressPtr addr = &info->addr.pci;
> bool entireSlot;
> - /* flags may be changed from default below */
> - virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
> - VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
This hunk you're removing seems to support my comment in
patch 4, the one arguing against adding HOTPLUGGABLE at the
end of the function.
> if (!virDeviceInfoPCIAddressPresent(info) ||
> ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) &&
> @@ -700,69 +696,25 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
> return 0;
> }
>
> - /* Change flags according to differing requirements of different
> - * devices.
> + /* If we get to here, the device has a PCI address assigned in the
> + * config and we should mark it as in-use. But if the
> + * pciConnectFlags are 0, then this device shouldn't have a PCI
> + * address associated with it. *BUT* since there are cases in the
> + * past where we've apparently allowed that, we need to pretend
> + * for now that it's okay, otherwise an existing domain could
> + * "disappear" from the list of domains due to a parse failure. We
> + * can fix this by just forcing the pciConnectFlags to be
> + * PCI_DEVICE (and then relying on validation functions to report
> + * inappropriate address types.
> */
> - switch (device->type) {
> - case VIR_DOMAIN_DEVICE_CONTROLLER:
> - switch (device->data.controller->type) {
> - case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> - flags =
virDomainPCIControllerModelToConnectType(device->data.controller->model);
> - break;
> -
> - case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
> - /* SATA controllers aren't hot-plugged, and can be put in
> - * either a PCI or PCIe slot
> - */
> - flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
> - | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
The new code doesn't seem to have any special handling of
CONTROLLER_TYPE_SATA, which means it will end up as
PCI_DEVICE|HOTPLUGGABLE instead of PCI_DEVICE|PCIE_DEVICE.
All of the flag settings in this function are essentially pointless, and
*not* what is used when actually assigning an address to a device (it
took me a while to realize this). There is a fuller explanation below,
but in short, when we are examining device addresses that were manually
set (or auto-assigned at some earlier time), any combination of the
following flags is considered equivalent:
VIR_PCI_CONNECT_TYPE_PCI_DEVICE
VIR_PCI_CONNECT_TYPE_PCIE_DEVICE
VIR_PCI_CONNECT_HOTPLUGGABLE
so all of the flags settings in this function are equivalent to
PCI_DEVICE | HOTPLUGGABLE.
> - break;
> -
> - case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> - /* allow UHCI and EHCI controllers to be manually placed on
> - * the PCIe bus (but don't put them there automatically)
> - */
> - switch (device->data.controller->model) {
> - 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:
> - flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
All these will be HOTPLUGGABLE.
^^ e.g. this
> - break;
> - case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
> - /* should this be PCIE-only? Or do we need to allow PCI
> - * for backward compatibility?
> - */
> - flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
> - | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
This will be PCI_DEVICE|HOTPLUGGABLE instead of
PCI_DEVICE|PCIE_DEVICE.
^^ and this. etc.
> - break;
> - case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
> - case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
> - case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
> - /* Allow these for PCI only */
> - break;
> - }
> - }
> - break;
> -
> - case VIR_DOMAIN_DEVICE_SOUND:
> - switch (device->data.sound->model) {
> - case VIR_DOMAIN_SOUND_MODEL_ICH6:
> - case VIR_DOMAIN_SOUND_MODEL_ICH9:
> - flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
All these will be HOTPLUGGABLE.
> - break;
> - }
> - break;
> -
> - case VIR_DOMAIN_DEVICE_VIDEO:
> - /* video cards aren't hot-plugged, and can be put in either a
> - * PCI or PCIe slot
> - */
> - flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
> - | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
These will be hotpluggable and PCI only.
All these differences between the code you're removing and
the code that's supposed to replace it make me wonder whether
I'm missing something. Feel free to point out my mistake
right about... Now! :)
I believed you were onto something, so I modified the new function
(qemuDomainDeviceCalculatePCIConnectFlags()) to exactly reflect the
flags as they're set in this function (qemuDomainCollectPCIAddress()).
But that caused some unit tests to fail. That's when I realized that the
flag settings in qemuDomainCollectPCIAddress()) are only used when
checking manually set addresses ("fromConfig") as we're setting in-use
bits for all the addresses already used, and in this case the setting of
HOTPLUGGABLE, or PCI vs. PCIE doesn't is ignored. What is actually
important is how the flags are set in qemuDomainAssignDevicePCISlots(),
which uses "PCI_DEVICE | HOTPLUGGABLE" for *everything* except PCI
controllers.
In the case of qemuDomainCollectPCIAddress(), since the addresses are
coming "fromConfig", validation will succeed regardless of any mismatch
in hotpluggable flag, or PCI vs PCIE (the address validation function
allows you to break those rules when manually addressing, because qemu
allows it).
So the proper thing for the new
qemuDomainDeviceCalculatePCIConnectFlags() function (I've renamed it,
but haven't yet reposted) is to set PCI_DEVICE | HOTPLUGGABLE for
everything (until the patches that make exceptions for specific
device/type/model combinations).
Note that one very nice side effect of this patch is that there is now
only a single place where the PCI connect flags are calculated, rather
than 2, so such confusion won't happen in the future.
On a related note, reviewing this would be *much* easier if
you could start off qemuDomainDeviceConnectFlagsInternal()
as (mostly) a straight copy of this chunk of code, and then
stray from it after switching over, instead of starting with
a completely different implementation.
Actually, not a copy of this chunk of code, because this chunk of code
has settings that would break the other use of
qemuDomainDeviceCalculatePCIConnectFlags() (see above).
I have rewritten (and renamed) the function, but its initial version now
always sets the flags to PCI_DEVICE | HOTPLUGGABLE.
I also believe the original code is easier to follow, because
it looks more like a direct mapping from model to flags.
In all honesty, the original code is just plain pointless. It *tried* to
be right, but got some things wrong, and then it ended up not mattering
anyway because the hotpluggable flag, and PCI vs. PCIE, is ignored when
validating already-assigned addresses anyway.
Ideally we would just list all devices and models, grouped by
purpose, and explicitly assign flags to them, in a similar
way we get flags for PCI controllers.
That's what I've done now. You'll see the results in a day or two.
And shouldn't the compiler give us at least a warning when
we're not considering all the possible values of an enum as
part of a switch()? That would be super-neat!
The jury is out.