Re: [PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller

On 9/26/21 10:35 AM, Ani Sinha wrote: It would make more sense to include this in the commandline generator for PCI controllers, since that's where all other XML for PCI controllers is converted into a qemu commandline argument. That's a bit convoluted though, unfortunately. The loop going through all the controllers is in qemuBuildControllersByTypeCommandLine() which then calls qemuBuildControllerDevStr() and *that* is the function that builds the argument to the -device for each controller. For two reasons, we can't add the code for this new option alongside the other PCI controller commandline generation code in qemuBuildControllerDevStr() though: 1) the output of qemuBuildControllerDevStr() is assumed to be following a "-device" argument on the commandline, so we would end up with: "-device -global PIIX4_PM.acpi-root-pci-hotplug=off" which is *not* what we want. 2) the top of the loop in qemuBuildControllersByTypeCommandLine() calls qemuBuildSkipController(), which tells the loop to skip generating any commandline for a pci-root controller (unless it's a P-series guest and the index is non-0). So the only way to make this work is to add a special case to qemuBuildControllersByTypeCommandLine() *before* the call to qemuBuildSkipController() - if the type is pci, model is pci-root, and index is 0 then conditionally add "-global", "PIIX4...." and continue (skip the rest of the body of the loop). As with what you've already done here, this is also not 100% clean and consistent with the rest of the code, but since neither method is perfect I think putting it in the function that generates all the rest of the commandline args associated with PCI controllers is more logical and easier to follow. (If anyone disagrees and thinks that the commandline for this should be generated in the place this patch already does it, please speak up!) Are these three caps ^^^ actually needed? I hope not, because that would indicate something seriously wrong with the code (those caps are related to PCIe controllers, and this option only applies to machinetypes that use conventional pci-root). Ah, now I understand the origin of pc-i440fx-acpi-root-hotplug-enable.xml in the previous patch - you had created it to be test case for this negative test. Actually I think the negative test should be done using the ...-disable test case with no caps; in the case that someone has "hotplug='on'" and the option is missing, I think we should just ignore it, since "hotplug='on'" is what they're going to get anyway (that could be added into the validation that was added in the previous patch). You still you add the -enable test case to qemuxml2xmltest.c as I suggested in the previous patch.