Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug

On 9/11/21 11:26 PM, Ani Sinha wrote: (before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?) I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided. My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly. So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities: 1) **** One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors). Possibly the new flags could be put in something like this: <features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features> But: * currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern". * even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".) * Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons). 2) ***** Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver. <os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os> My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from. 3) **** A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.: <acpiHotplug bridge='yes' root='yes'/> I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing. **** Since I always seem to spend *way too much time* worrying about naming, only to have it come out wrong in the end anyway, I'm looking for some other opinions. Counting the version that is in Ani's patch currently as option "0", which option do you all think is the best? Or is it completely unimportant? The "cold plugged bridges" term here throws me for a loop - it implies that hotplugging bridges is something that's supported, and I think it still isn't. Of course this is just the cover letter, so it won't go into git anywhere, but I think it should be enough to say "enables ACPI hotplug into non-root bus PCI bridges/ports". So I'm curious - if the QEMU commandline also included "-no-acpi" along with these, what would happen? Would it be silently ignored? Generate an error? Or does -no-acpi only control the suspend support, and acpi hotplug is still available? Interesting. So how was hotplug handled before this? With SHPC? I know there must be *some* kind of hotplug support in older QEMU, because RHEL6 QEMU supported hotplug, and it was based on qemu 0.12 or something ancient like that... Yes, sigh. I recall someone saying something like "if we switch to ACPI hotplug then all these bugs just go away and everything works" or something like that. Reality never matches the ideal picture we put in our brains. At least ACPI hotplug is only the default on new machinetypes (doesn't help much for management platforms that always just use "q35" every time they start a guest). And it can also cause problems with distro-specific machinetypes in downstream distros when they are rebased: https://bugzilla.redhat.com/2006409 To be fair, aside from "support for Win2000/WinXP", none of the items on the "limitations" page of that slide deck is something that's impossible to do with a Q35 machinetype; it's just that accomplishing some things may be more complicated. But I understand your point. Mainly I brought it up because I wanted to be sure that we're adding these to fulfill an actual need, rather than just adding bulk for the sake of completeness, or to satisfy curiosity. Since I don't follow qemu-devel closely, I didn't have prior knowledge of exactly what the options did, and it was unclear in the earlier versions of your patches that what <acpi-hotplug-bridge enabled='no'/> did was to disable ACPI hotplug for the entire guest (which on Q35 means that native PCIe hotplug will be found/used, and on 440fx means that hotplug won't be possible (unless SHPC hotplugged is enabled)). Your exaplanation and documentation in this spin of the patches makes that all clear though, so I'm beyond the "what does this do and do we need it?" stage to the "are there any problems with the code?" stage, and that's what I'll try to address in my review of the patches.