[libvirt] [PATCH v3 09/18] qemu: set/use info->pciConnectFlags during qemuDomainAssignDevicePCISlots
Laine Stump
laine at laine.org
Tue Oct 4 15:50:24 UTC 2016
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.
Nah, just shows that I had done it that way in the past, and this time
it seemed to take less typing to do it the other way :-) I'm fine with
either though.
>
>> 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.
Which is correct (for all occasions, except the built-in SATA controller
in Q35, and that is handled with an "override" in the Q35-specific
function (I forget its name right now). In the past I had mistakenly
assumed that the SATA controller could also be PCIe because it was one
of the integrated devices in a Q35 machine, but that's not the case.
>
>> - 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.
Yep. Although libvirt doesn't support hotplugging them, qemu apparently
does, and the recommendation I was given when I posted the "hotpluggable
attribute" patchset was that we should in general consider *all* devices
as potentially hotpluggable unless there was a good reason/precedent for
not doing so.
>
>> - 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.
That will be changed in an upcoming patch to add PCIE_DEVICE, and it
will then be correct. I left it this way here so that behavior wouldn't
change.
>
>> - 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.
See above.
>
>> - 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.
Also correct. I had previously made the same mistaken assumption with
video devices as with the SATA controller. In reality, all of the
emulated video devices supported by qemu are PCI-only except for the
virtio video device (that will be set to PCIE_DEVICE in an upcoming patch.)
(BTW, part of my confusion was caused by the fact that I didn't realize
that a device plugged into pcie-root shows up as a PCI device, not PCIe
- since I'd been told it was okay to plug the video devices into
pcie-root, I thought that meant they were PCIe devices, but that
conclusion *doesn't* follow from the former information).
>
> 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! :)
Yeah, you're missing the fact that these current flag settings are
incorrect, decided on when I had less information/knowledge than I do
now. Fixing them doesn't create any changes in behavior, but it does
set us up to make behavior changes (in the right direction) in upcoming
patches.
>
> 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 might have to create an extra patch so that this function isn't called
until it is "fixed", but I see your point.
>
> 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.
But the PCI controllers each have a different flag (because they're all
slightly different) so an entry for each one is needed anyhow. For the
rest of the devices, most of them are plain "PCI + hotpluggable", with a
relatively small number of exceptions. If we explicitly put in an entry
for every model of every device, we could end up in a situation of
losing the interesting "trees" (exceptions) in the "forest" (all the
nearly identical settings to the default).
>
> 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!
It's because many of these variables aren't defined as enums in the
device object, but as int. I suppose this could be taken care of by
typecasting to the enum in the switches. Again, it seems like a lot of
extra lines when the majority of emulated devices are still legacy PCI
though. Let me think about it...
>
>> - 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 :)
Or better yet, don't name the function, but describe it in another way
that won't be affected by name changes.
>
> [...]
>> @@ -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 :)
Well, that patch changed def->controllers[i] to cont, not def->nets[i] :-P
There were a few places this happened (going over the usage threshold of
def->blahs[i] so that it became cleaner to have a temp variable). The
ones with controllers[i] were just so excessive that I went back and
pulled them out into a separate patch. When a few more came along later,
they didn't seem as significant, and I was more burned out on the whole
"create more patches to make the set look more intimidating to a
reviewer while simultaneously creating merge conflicts when I rebase to
add the new patch" thing.
If that makes it easier for you, I can pull out the rest of these on the
next round though.
>
> [...]
>> @@ -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.
I'm no longer able to figure out when you seriously want me to fix
these, and when you're just existentially complaining about the state of
the universe :-P
More information about the libvir-list
mailing list