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.
> - 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.
> - 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.
> - 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! :)
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.
I also believe the original code is easier to follow, because
it looks more like a direct mapping from model to flags.
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.
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!
> - break;
> + if (info->pciConnectFlags == 0) {
> + char *addrStr = virDomainPCIAddressAsString(&info->addr.pci);
Double space.
> + VIR_WARN("qemuDomainDeviceConnectFlagsInternal() thinks that the "
Adjust if you change the name of the function due to my
comments on patch 4 :)
[...]
> @@ -1325,27 +1264,31 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
> * in hostdevs list anyway, so handle them with other hostdevs
> * instead of here.
> */
> - if ((def->nets[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) ||
> - !virDeviceInfoPCIAddressWanted(&def->nets[i]->info)) {
> + virDomainNetDefPtr net = def->nets[i];
> +
> + if ((net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) ||
> + !virDeviceInfoPCIAddressWanted(&net->info)) {
> continue;
> }
> - if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->nets[i]->info,
> - flags) < 0)
> +
> + if (qemuDomainPCIAddressReserveNextSlot(addrs, &net->info) < 0)
The change from def->nets[i] to net is a good one, but it
should have been done in a separate commit earlier in the
series. In fact, patch 2/18 is basically the same change
in a different part of libvirt :)
[...]
> @@ -1412,7 +1355,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
>
> if (foundAddr) {
> /* Reserve this function on the slot we found */
> - if (virDomainPCIAddressReserveAddr(addrs, &addr, flags,
> + if (virDomainPCIAddressReserveAddr(addrs, &addr,
> + def->controllers[i]->info.pciConnectFlags,
Long line is long.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list