On Sat, Sep 25, 2021 at 8:39 PM Ani Sinha <ani@xxxxxxxxxxx> wrote: > > Ok then let me do this. I will split up this patch set and send out a separate patch set just for acpi-hotplug-root with the conf change as suggested by danpb. It makes sense for me too to go down the path of his suggestion. > > Meanwhile let's figure out what we wanted to do for acpi-hotplug-bridge. That could be posted as a separate patch set. > > Sounds good? OK, while we have split this up and are making progress with the pci-root-hotplug patchset, have we decided where to put the conf for pci-hotplug-bridge? Should we leave it as it is under <pm>? > > On Sat, Sep 25, 2021 at 8:29 PM Laine Stump <laine@xxxxxxxxxx> wrote: >> >> On 9/23/21 4:46 PM, Laine Stump wrote: >> > On 9/11/21 11:26 PM, Ani Sinha wrote: >> >> Hi all: >> >> >> >> This patchset introduces libvirt xml support for the following two pm >> >> conf >> >> options: >> >> >> >> <pm> >> >> <acpi-hotplug-bridge enabled='no'/> >> >> <acpi-root-hotplug enabled='yes'/> >> >> </pm> >> > >> > (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? >> >> In an IRC discussion, danpb suggested what I'll label as option (4): >> >> 4) **** >> >> <danpb1> laine: i didn't have time to reply,but was thinking why it >> isn't a property of the pci(e)-root <controller> >> >> >> That makes perfect sense for acpi-hotplug-root: >> >> <controller type='pci' index='0' model='pci-root'> >> <target hotplug='off'/> >> </controller> >> >> This is the same attribute used to disable all hotplug for >> pcie-root-ports (and downstream ports, I guess - I never use those and >> recall a bug being filed about failures to hotplug devices on a >> downstream port; but I digress...). So it fits logically. >> >> (I will point out though that normally the options within a device's XML >> element are added to the qemu commandline as a part of that devices >> "-device", not as a separate global option as happens with this (for >> that matter there is no -device added to the commandline for pci-root or >> pcie-root at all - they're just implicit in the base machine).) >> >> >> But the "acpi-hotplug-bridge" option doesn't really fit as an attribute >> of pci-root/pcie-root - imagine for a moment that you had this option in >> a pcie-root controller, it would look something like this: >> >> <controller type='pci' index='0' model='pcie-root'> >> <target acpiHotplugBridge='on'/> >> </controller> >> >> Note that in this case, the option turns on *ACPI* hotplug support *for >> other PCI controllers*, **NOT** this controller (and as a side effect, >> also turns *off* PCIe native hotplug for all controllers, which can then >> be turned on/off on a per controller basis using QEMU's native_hotplug >> commandline option, which isn't supported by libvirt). >> >> Another issue - with this - putting this option into the XML of the >> pcie-root controller and saying that it applies to "the other >> controllers in the PCI hierarchy" implies that if there was a separate >> pcie-root (for example a pxb-pcie controller) then the option wouldn't >> apply to bridges that were a part of that separate hierarchy, and I >> believe that is *not* the case. >> >> So while I like controlling acpi-hotplug-root with <target >> hotplug='on|off'/> inside the pci-root's element, I don't really like >> putting acpi-hotplug-bridge in the pci(e)-root controller's element. >>
