Re: [libvirt] [PATCH v3 09/18] qemu: set/use info->pciConnectFlags during qemuDomainAssignDevicePCISlots

On 10/04/2016 10:11 AM, Andrea Bolognani wrote: 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. 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. 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. 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. See above. 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). 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. I might have to create an extra patch so that this function isn't called until it is "fixed", but I see your point. 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). 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... Or better yet, don't name the function, but describe it in another way that won't be affected by name changes. 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. 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