Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Thu, Sep 30, 2021 at 18:52 Laine Stump wrote: > On 9/30/21 2:16 AM, Ani Sinha wrote: > > On Fri, Sep 24, 2021 at 2:16 AM Laine Stump wrote: > >> > >> On 9/11/21 11:26 PM, Ani Sinha wrote: > >>> The above two options are only available for qemu driver and that too > for x86 > >>> guests only. Both of them are global options. > >>> > >>> ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug > support for cold > >>> plugged bridges. Examples of cold plugged bridges include PCI-PCI > bridge > >>> (pci-bridge controller) for pc machines and pcie-root-port controller > for q35 > >>> machines. The corresponding commandline options to qemu for x86 guests > are: > >> > >> 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". > > > > I think emphasizing cold plugged bridges is important as Igor (CC'd) > > has clarified in the other email on patch #3 of this series. > > Okay, so the implication in Igor's email is that a) it is possible to > hotplug a pcie controller, but b) any controller that is hotplugged will > not have ACPI enabled. Note though that libvirt doesn't allow > hotplugging *any* PCI controller, since we were told long ago that no OS > will actually rescan the PCI topology when this is done, and so the new > controller wouldn't be usable anyway. (that information may well be > outdated). >From i440fx side all empty ports in the pci root controller are described as hotplug capable from ACPI. So I do not see why we cannot hotplug a pci bridge in one of the pci root ports and OS should be able to detect it without reboot. I have not tried it though. > > I think if you're going to mention that it is specifically for > "cold-plugged bridges" then you should also 1) define what > "cold-plugged" means, i.e. "(PCI controllers that were present in the > domain definition when the guest was first started"), and 2) note that > "ACPI is not enabled for bridges that are hot-plugged (but currently > libvirt doesn't support hotplugging a pci controller anyway)" or > something like that. > >
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On 9/30/21 2:16 AM, Ani Sinha wrote: On Fri, Sep 24, 2021 at 2:16 AM Laine Stump wrote: On 9/11/21 11:26 PM, Ani Sinha wrote: The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options. ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are: 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". I think emphasizing cold plugged bridges is important as Igor (CC'd) has clarified in the other email on patch #3 of this series. Okay, so the implication in Igor's email is that a) it is possible to hotplug a pcie controller, but b) any controller that is hotplugged will not have ACPI enabled. Note though that libvirt doesn't allow hotplugging *any* PCI controller, since we were told long ago that no OS will actually rescan the PCI topology when this is done, and so the new controller wouldn't be usable anyway. (that information may well be outdated). I think if you're going to mention that it is specifically for "cold-plugged bridges" then you should also 1) define what "cold-plugged" means, i.e. "(PCI controllers that were present in the domain definition when the guest was first started"), and 2) note that "ACPI is not enabled for bridges that are hot-plugged (but currently libvirt doesn't support hotplugging a pci controller anyway)" or something like that.
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Fri, Sep 24, 2021 at 2:16 AM 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: > > > > > > > > > > > > (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 > subelement of , 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 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: > > > > > > >... > > > But: > > * currently there are no subelements to . So this isn't "extending > according to an existing pattern". > > * even though the 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 in the domain XML kind of implies that > the associated capability flags should be represented in the > section of the domain capabilities. For example, is listed under > 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 > (seems a bit redundant to me to have it in both places, but > I'm sure there are $reasons). > > > 2) * > > Alternately, there is an subelement of , 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. > > > > /path/to/slic.dat > > > >... > > > My problem with adding these new PCI controller acpi options to os/acpi > is simply that it's in the 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 > that would have separate attributes for the two flags, e.g.: > > > > 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 above two options are only available for qemu driver and that too for > > x86 > > guests only. Both of them are global options. > > > > ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for > > cold > > plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge > > (pci-bridge controller) for pc machines and
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > On Tue, Sep 28, 2021 at 06:14:12PM +0530, Ani Sinha wrote: > > On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé > > wrote: > > > > > On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote: > > > > > > > > > > > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > > > > > > > > > On Thu, Sep 23, 2021 at 04:46:38PM -0400, 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: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (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. > > > > > > > > > > I had the same reaction. Even if QEMU hangs it off a "_PM" device, > > > > > I feel it is a pretty wierd location from libvirt POV to put this. > > > > > > > > > > > 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 > > > > > > subelement of , 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 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: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > > But: > > > > > > > > > > > > * currently there are no subelements to . So this isn't > > > "extending > > > > > > according to an existing pattern". > > > > > > > > > > > > * even though the 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 in the domain XML kind of implies > > > that the > > > > > > associated capability flags should be represented in the > > > section > > > > > > of the domain capabilities. For example, is listed under > > > > > > > > > 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 > > > (seems a > > > > > > bit redundant to me to have it in both places, but I'm sure there > > > > > > are > > > > > > $reasons). > > > > > > > > > > Essentially has become a dumping ground for adhoc global > > > > > properties. So in that sense it probably is the best fit for this. > > > > > > > > > > If we don't want to touch th existing
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Tue, Sep 28, 2021 at 10:21 PM Daniel P. Berrangé wrote: > On Tue, Sep 28, 2021 at 06:14:12PM +0530, Ani Sinha wrote: > > On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé > > wrote: > > > > > On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote: > > > > > > > > > > > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > > > > > > > > > On Thu, Sep 23, 2021 at 04:46:38PM -0400, 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: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (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. > > > > > > > > > > I had the same reaction. Even if QEMU hangs it off a "_PM" device, > > > > > I feel it is a pretty wierd location from libvirt POV to put this. > > > > > > > > > > > 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 > > > > > > subelement of , 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 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: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > > But: > > > > > > > > > > > > * currently there are no subelements to . So this isn't > > > "extending > > > > > > according to an existing pattern". > > > > > > > > > > > > * even though the 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 in the domain XML kind of > implies > > > that the > > > > > > associated capability flags should be represented in the > > > > section > > > > > > of the domain capabilities. For example, is listed under > > > > > > > > > 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 > > > > (seems a > > > > > > bit redundant to me to have it in both places, but I'm sure > there are > > > > > > $reasons). > > > > > > > > > > Essentially has become a dumping ground for adhoc global > > > > > properties. So in that sense it probably is the best fit for this. > > > > > > > > > > If we don't want to touch th existing
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Tue, Sep 28, 2021 at 06:14:12PM +0530, Ani Sinha wrote: > On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé > wrote: > > > On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote: > > > > > > > > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > > > > > > > On Thu, Sep 23, 2021 at 04:46:38PM -0400, 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: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (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. > > > > > > > > I had the same reaction. Even if QEMU hangs it off a "_PM" device, > > > > I feel it is a pretty wierd location from libvirt POV to put this. > > > > > > > > > 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 > > > > > subelement of , 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 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: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > But: > > > > > > > > > > * currently there are no subelements to . So this isn't > > "extending > > > > > according to an existing pattern". > > > > > > > > > > * even though the 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 in the domain XML kind of implies > > that the > > > > > associated capability flags should be represented in the > > section > > > > > of the domain capabilities. For example, is listed under > > > > > > > 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 > > (seems a > > > > > bit redundant to me to have it in both places, but I'm sure there are > > > > > $reasons). > > > > > > > > Essentially has become a dumping ground for adhoc global > > > > properties. So in that sense it probably is the best fit for this. > > > > > > > > If we don't want to touch th existing element for fear of > > > > back compat issues, we could have > > > > > > > > > > > > > > > > for the acpi-pci-hotplug-with-bridge-support setting ? > > > > > > > > > > Since this is pci bridge related setting, maybe we should have: > > > > > > > > > > > > Although in that case, the user should be aware that pcie-root-ports are > > > like bridges.
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé wrote: > On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote: > > > > > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > > > > > On Thu, Sep 23, 2021 at 04:46:38PM -0400, 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: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (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. > > > > > > I had the same reaction. Even if QEMU hangs it off a "_PM" device, > > > I feel it is a pretty wierd location from libvirt POV to put this. > > > > > > > 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 > > > > subelement of , 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 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: > > > > > > > > > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > But: > > > > > > > > * currently there are no subelements to . So this isn't > "extending > > > > according to an existing pattern". > > > > > > > > * even though the 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 in the domain XML kind of implies > that the > > > > associated capability flags should be represented in the > section > > > > of the domain capabilities. For example, is listed under > > > > > 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 > (seems a > > > > bit redundant to me to have it in both places, but I'm sure there are > > > > $reasons). > > > > > > Essentially has become a dumping ground for adhoc global > > > properties. So in that sense it probably is the best fit for this. > > > > > > If we don't want to touch th existing element for fear of > > > back compat issues, we could have > > > > > > > > > > > > for the acpi-pci-hotplug-with-bridge-support setting ? > > > > > > > Since this is pci bridge related setting, maybe we should have: > > > > > > > > Although in that case, the user should be aware that pcie-root-ports are > > like bridges. But if we do not have -bridge, then it does not convey the > > fact that this setting does not apply to pci-root bus on i440fx. :-\ > > I thought without -bridge is better, because we might want to hang > more PCI hotplug options off it later. The docs can clarify the > semantics How about > > > Regards, > Daniel > -- > |:
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote: > > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > > > On Thu, Sep 23, 2021 at 04:46:38PM -0400, 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: > > > > > > > > > > > > > > > > > > > > > > > > > > (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. > > > > I had the same reaction. Even if QEMU hangs it off a "_PM" device, > > I feel it is a pretty wierd location from libvirt POV to put this. > > > > > 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 > > > subelement of , 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 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: > > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > But: > > > > > > * currently there are no subelements to . So this isn't "extending > > > according to an existing pattern". > > > > > > * even though the 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 in the domain XML kind of implies that > > > the > > > associated capability flags should be represented in the > > > section > > > of the domain capabilities. For example, is listed under > > > > > > 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 (seems > > > a > > > bit redundant to me to have it in both places, but I'm sure there are > > > $reasons). > > > > Essentially has become a dumping ground for adhoc global > > properties. So in that sense it probably is the best fit for this. > > > > If we don't want to touch th existing element for fear of > > back compat issues, we could have > > > > > > > > for the acpi-pci-hotplug-with-bridge-support setting ? > > > > Since this is pci bridge related setting, maybe we should have: > > > > Although in that case, the user should be aware that pcie-root-ports are > like bridges. But if we do not have -bridge, then it does not convey the > fact that this setting does not apply to pci-root bus on i440fx. :-\ I thought without -bridge is better, because we might want to hang more PCI hotplug options off it later. The docs can clarify the semantics Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > On Thu, Sep 23, 2021 at 04:46:38PM -0400, 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: > > > > > > > > > > > > > > > > > > > (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. > > I had the same reaction. Even if QEMU hangs it off a "_PM" device, > I feel it is a pretty wierd location from libvirt POV to put this. > > > 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 > > subelement of , 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 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: > > > > > > > > > > > > > > ... > > > > > > But: > > > > * currently there are no subelements to . So this isn't "extending > > according to an existing pattern". > > > > * even though the 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 in the domain XML kind of implies that the > > associated capability flags should be represented in the section > > of the domain capabilities. For example, is listed under > > 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 (seems a > > bit redundant to me to have it in both places, but I'm sure there are > > $reasons). > > Essentially has become a dumping ground for adhoc global > properties. So in that sense it probably is the best fit for this. > > If we don't want to touch th existing element for fear of > back compat issues, we could have > > > > for the acpi-pci-hotplug-with-bridge-support setting ? > Since this is pci bridge related setting, maybe we should have: Although in that case, the user should be aware that pcie-root-ports are like bridges. But if we do not have -bridge, then it does not convey the fact that this setting does not apply to pci-root bus on i440fx. :-\ > > > 2) * > > > > Alternately, there is an subelement of , 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. > > > > > > > > /path/to/slic.dat > > > > > > > > ... > > > > > > My problem with adding these new PCI controller acpi options to os/acpi is > > simply that it's in the 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. > > Yeah, we've kind of abused a little with adding under > that. I can see why we did it, as its another blob kinda like the > loader blob,
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Thu, Sep 23, 2021 at 04:46:38PM -0400, 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: > > > > > > > > > > > > (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. I had the same reaction. Even if QEMU hangs it off a "_PM" device, I feel it is a pretty wierd location from libvirt POV to put this. > 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 > subelement of , 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 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: > > > > > > > ... > > > But: > > * currently there are no subelements to . So this isn't "extending > according to an existing pattern". > > * even though the 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 in the domain XML kind of implies that the > associated capability flags should be represented in the section > of the domain capabilities. For example, is listed under > 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 (seems a > bit redundant to me to have it in both places, but I'm sure there are > $reasons). Essentially has become a dumping ground for adhoc global properties. So in that sense it probably is the best fit for this. If we don't want to touch th existing element for fear of back compat issues, we could have for the acpi-pci-hotplug-with-bridge-support setting ? > 2) * > > Alternately, there is an subelement of , 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. > > > > /path/to/slic.dat > > > > ... > > > My problem with adding these new PCI controller acpi options to os/acpi is > simply that it's in the 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. Yeah, we've kind of abused a little with adding under that. I can see why we did it, as its another blob kinda like the loader blob, but it was probabl a mistake. > > 3) > > A third option, suggested somewhere by Ani, would be to make a completely > new top-level element, called something like that would have > separate attributes for the two flags, e.g.: > > > > 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. Agreed, lets not add more top level pieces. Regards, Daniel -- |: https://berrange.com
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Sat, Sep 25, 2021 at 8:39 PM Ani Sinha 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 ? > > On Sat, Sep 25, 2021 at 8:29 PM Laine Stump 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: >> >> >> >> >> >> >> >> >> >> >> > >> > (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 >> > subelement of , 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 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: >> > >> > >> > >> > >> > >> > >> >... >> > >> > >> > But: >> > >> > * currently there are no subelements to . So this isn't "extending >> > according to an existing pattern". >> > >> > * even though the 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 in the domain XML kind of implies that >> > the associated capability flags should be represented in the >> > section of the domain capabilities. For example, is listed under >> > 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 >> > (seems a bit redundant to me to have it in both places, but >> > I'm sure there are $reasons). >> > >> > >> > 2) * >> > >> > Alternately, there is an subelement of , 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. >> > >> > >> > >> > /path/to/slic.dat >> > >> > >> > >> >... >> > >> > >> > My problem with adding these new PCI controller acpi options to os/acpi >> > is simply that it's in the 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
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
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? On Sat, Sep 25, 2021 at 8:29 PM Laine Stump 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: > >> > >> > >> > >> > >> > > > > (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 > > subelement of , 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 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: > > > > > > > > > > > > > >... > > > > > > But: > > > > * currently there are no subelements to . So this isn't "extending > > according to an existing pattern". > > > > * even though the 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 in the domain XML kind of implies that > > the associated capability flags should be represented in the > > section of the domain capabilities. For example, is listed under > > 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 > > (seems a bit redundant to me to have it in both places, but > > I'm sure there are $reasons). > > > > > > 2) * > > > > Alternately, there is an subelement of , 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. > > > > > > > > /path/to/slic.dat > > > > > > > >... > > > > > > My problem with adding these new PCI controller acpi options to os/acpi > > is simply that it's in the 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 > > that would have separate attributes for the two flags, e.g.: > > > > > > > > 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*
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
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: (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 subelement of , 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 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: ... But: * currently there are no subelements to . So this isn't "extending according to an existing pattern". * even though the 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 in the domain XML kind of implies that the associated capability flags should be represented in the section of the domain capabilities. For example, is listed under 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 (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons). 2) * Alternately, there is an subelement of , 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. /path/to/slic.dat ... My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the 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 that would have separate attributes for the two flags, e.g.: 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) laine: i didn't have time to reply,but was thinking why it isn't a property of the pci(e)-root That makes perfect sense for acpi-hotplug-root: 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
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
DanPB, Could you please reiterate the suggestion regarding the placement of the pci root hotplug xml you made in the irc channel? On Fri, Sep 24, 2021 at 02:16 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: > > > > > > > > > > > > (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 > subelement of , 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 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: > > > > > > >... > > > But: > > * currently there are no subelements to . So this isn't "extending > according to an existing pattern". > > * even though the 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 in the domain XML kind of implies that > the associated capability flags should be represented in the > section of the domain capabilities. For example, is listed under > 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 > (seems a bit redundant to me to have it in both places, but > I'm sure there are $reasons). > > > 2) * > > Alternately, there is an subelement of , 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. > > > > /path/to/slic.dat > > > >... > > > My problem with adding these new PCI controller acpi options to os/acpi > is simply that it's in the 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 > that would have separate attributes for the two flags, e.g.: > > > > 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 above two options are only available for qemu driver and that too > for x86 > > guests only. Both of them are global options. > > > > ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support > for cold > >
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
+Igor +Michael On Thu, 23 Sep 2021, 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: > > > > > > > > > > > > (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"?) yes this is fine. I can swap the two words. > > 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 > subelement of , 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 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: > > > > > > > ... > > > But: > > * currently there are no subelements to . So this isn't "extending > according to an existing pattern". > > * even though the 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 in the domain XML kind of implies that the > associated capability flags should be represented in the section of > the domain capabilities. For example, is listed under 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 (seems a bit redundant to > me to have it in both places, but I'm sure there are $reasons). > > > 2) * > > Alternately, there is an subelement of , 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. > > > > /path/to/slic.dat > > > > ... > > > My problem with adding these new PCI controller acpi options to os/acpi is > simply that it's in the 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 that would have > separate attributes for the two flags, e.g.: > > > > 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? > My preference is obviously option #0 and #3. However, community opinion/perspective is certainly required here. > > The above two options are only available for qemu driver and that too for > > x86 > > guests only. Both of them are global options. > > > > ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug
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: Hi all: This patchset introduces libvirt xml support for the following two pm conf options: (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 subelement of , 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 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: ... But: * currently there are no subelements to . So this isn't "extending according to an existing pattern". * even though the 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 in the domain XML kind of implies that the associated capability flags should be represented in the section of the domain capabilities. For example, is listed under 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 (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons). 2) * Alternately, there is an subelement of , 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. /path/to/slic.dat ... My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the 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 that would have separate attributes for the two flags, e.g.: 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 above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options. ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are: 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
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
Ping again
On Mon, Sep 20, 2021 at 21:25 Ani Sinha wrote:
> Ping
>
> On Mon, Sep 13, 2021 at 1:39 PM Ani Sinha wrote:
>
>>
>>
>> On Sun, 12 Sep 2021, Ani Sinha wrote:
>>
>> > Hi all:
>> >
>> > This patchset introduces libvirt xml support for the following two pm
>> conf
>> > options:
>> >
>> >
>> >
>> >
>> >
>> >
>>
>> Another option is to create a new xml tag and add these under that tag.
>> Something like:
>>
>> +
>> +
>> +
>> +
>>
>> These are not exactly power management options. So putting them under
>> may not be correct.
>> If this is a better approach I will resend the patchset with the changes
>> along with addressing other review comments.
>>
>>
>> > The above two options are only available for qemu driver and that too
>> for x86
>> > guests only. Both of them are global options.
>> >
>> > ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support
>> for cold
>> > plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge
>> > (pci-bridge controller) for pc machines and pcie-root-port controller
>> for q35
>> > machines. The corresponding commandline options to qemu for x86 guests
>> are:
>> >
>> > (pc machines): -global
>> PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
>> > (q35 machines): -global
>> ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
>> >
>> > Being global options, no other bridge specific options for pci-bridge
>> > controller or pcie-root-port controllers are required. For pc machine
>> type in
>> > x86, this option is available in qemu for a long time, from version 2.1.
>> > Please see the changes in qemu.git:
>> >
>> > 9e047b982452c6 ("piix4: add acpi pci hotplug support")
>> > 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI
>> bridge hotplug is disabled")
>> >
>> > For q35 machine type, this was introduced in qemu 6.1 with the following
>> > changes in qemu.git:
>> >
>> > (a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
>> > (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on
>> Q35")
>> >
>> > The reasons for enabling ACPI based hotplug for PCIe (q35) based
>> machines (as
>> > opposed to native hotplug) for bridges are outlined in (b). It is
>> possible that
>> > some users might still want to use native hotplug on PCIe [1].
>> Therefore,
>> > this conf option enables users to choose either ACPI based hotplug or
>> native
>> > hotplug for cold plugged bridges (for example for pcie root port
>> controller
>> > in q35 machines).
>> >
>> > ``acpi-root-hotplug`` option enables or disables ACPI based hotplug for
>> PCI root
>> > bus (pci-root controller). This option is only available for pc machine
>> type.
>> > The corresponding commandline option to qemu for x86 guests is:
>> >
>> > -global PIIX4_PM.acpi-root-pci-hotplug=
>> >
>> > This additional option enables users to disable hotplug for all devices
>> in the
>> > system without adding an additional PCI-PCI bridge, putting the devices
>> behind
>> > the bridge and using the existing ``acpi-hotplug-bridge`` option to
>> disable
>> > hotplug on that bridge. This feature was introduced from qemu version
>> 5.2 with
>> > the following change in qemu.git:
>> >
>> > 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug
>> on the root bus")
>> >
>> > The above qemu commit describes some compelling reasons why users might
>> to
>> > disable hotplug on PCI root buses [2].
>> >
>> > A brief summary of the patches:
>> >
>> > >[PATCH v3 1/5] qemu: capablities: detect presence of
>> > >[PATCH v3 2/5] qemu: capablities: detect presence of
>> > Patches 1 and 2 implement support for qemu capability checks for the
>> above
>> > config options.
>> >
>> > >[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and
>> > Patch 3 actually adds the config option to the schema and adds related
>> unit
>> > tests.
>> >
>> > >[PATCH v3 4/5] qemu: command: add support for qemu options that
>> > Patch 4 adds the backend qemu commandline support for the options. It
>> also adds
>> > relevant unit tests for the same.
>> >
>> > >[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release
>> > Patch 5 adds the release notes for the next libvirt release.
>> >
>> >
>> > Changelog:
>> > v1: initial implementation. Had some bugs and missed some unit tests.
>> > v2: fixed bugs and added additional missing unit tests.
>> > v3: reorganized the patches as per Laine's suggestion. Added more
>> > details in commit messages. Added conf description in
>> formatdomain.rst.
>> > Added changelog for next release.
>> >
>> >
>> > Notes:
>> >
>> > [1] One concrete example of why one might still want to use native
>> hotplug with
>> > pcie-root-port controller is the fact that we are still discovering
>> issues with
>> > acpi hotplug on PCIE. One such issue is:
>> > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html
>> > Another reason is that users have been using native hotplug on pcie
>> root ports
>> > up until now.
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
Ping
On Mon, Sep 13, 2021 at 1:39 PM Ani Sinha wrote:
>
>
> On Sun, 12 Sep 2021, Ani Sinha wrote:
>
> > Hi all:
> >
> > This patchset introduces libvirt xml support for the following two pm
> conf
> > options:
> >
> >
> >
> >
> >
> >
>
> Another option is to create a new xml tag and add these under that tag.
> Something like:
>
> +
> +
> +
> +
>
> These are not exactly power management options. So putting them under
> may not be correct.
> If this is a better approach I will resend the patchset with the changes
> along with addressing other review comments.
>
>
> > The above two options are only available for qemu driver and that too
> for x86
> > guests only. Both of them are global options.
> >
> > ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support
> for cold
> > plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge
> > (pci-bridge controller) for pc machines and pcie-root-port controller
> for q35
> > machines. The corresponding commandline options to qemu for x86 guests
> are:
> >
> > (pc machines): -global
> PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
> > (q35 machines): -global
> ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
> >
> > Being global options, no other bridge specific options for pci-bridge
> > controller or pcie-root-port controllers are required. For pc machine
> type in
> > x86, this option is available in qemu for a long time, from version 2.1.
> > Please see the changes in qemu.git:
> >
> > 9e047b982452c6 ("piix4: add acpi pci hotplug support")
> > 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI
> bridge hotplug is disabled")
> >
> > For q35 machine type, this was introduced in qemu 6.1 with the following
> > changes in qemu.git:
> >
> > (a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> > (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on
> Q35")
> >
> > The reasons for enabling ACPI based hotplug for PCIe (q35) based
> machines (as
> > opposed to native hotplug) for bridges are outlined in (b). It is
> possible that
> > some users might still want to use native hotplug on PCIe [1]. Therefore,
> > this conf option enables users to choose either ACPI based hotplug or
> native
> > hotplug for cold plugged bridges (for example for pcie root port
> controller
> > in q35 machines).
> >
> > ``acpi-root-hotplug`` option enables or disables ACPI based hotplug for
> PCI root
> > bus (pci-root controller). This option is only available for pc machine
> type.
> > The corresponding commandline option to qemu for x86 guests is:
> >
> > -global PIIX4_PM.acpi-root-pci-hotplug=
> >
> > This additional option enables users to disable hotplug for all devices
> in the
> > system without adding an additional PCI-PCI bridge, putting the devices
> behind
> > the bridge and using the existing ``acpi-hotplug-bridge`` option to
> disable
> > hotplug on that bridge. This feature was introduced from qemu version
> 5.2 with
> > the following change in qemu.git:
> >
> > 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug
> on the root bus")
> >
> > The above qemu commit describes some compelling reasons why users might
> to
> > disable hotplug on PCI root buses [2].
> >
> > A brief summary of the patches:
> >
> > >[PATCH v3 1/5] qemu: capablities: detect presence of
> > >[PATCH v3 2/5] qemu: capablities: detect presence of
> > Patches 1 and 2 implement support for qemu capability checks for the
> above
> > config options.
> >
> > >[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and
> > Patch 3 actually adds the config option to the schema and adds related
> unit
> > tests.
> >
> > >[PATCH v3 4/5] qemu: command: add support for qemu options that
> > Patch 4 adds the backend qemu commandline support for the options. It
> also adds
> > relevant unit tests for the same.
> >
> > >[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release
> > Patch 5 adds the release notes for the next libvirt release.
> >
> >
> > Changelog:
> > v1: initial implementation. Had some bugs and missed some unit tests.
> > v2: fixed bugs and added additional missing unit tests.
> > v3: reorganized the patches as per Laine's suggestion. Added more
> > details in commit messages. Added conf description in
> formatdomain.rst.
> > Added changelog for next release.
> >
> >
> > Notes:
> >
> > [1] One concrete example of why one might still want to use native
> hotplug with
> > pcie-root-port controller is the fact that we are still discovering
> issues with
> > acpi hotplug on PCIE. One such issue is:
> > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html
> > Another reason is that users have been using native hotplug on pcie root
> ports
> > up until now. They have built and tested their systems based on native
> hotplug.
> > They may not want to suddenly move to acpi based hotplug just because it
> is now
> > the default in qemu. Supporting the option to chose
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Sun, 12 Sep 2021, Ani Sinha wrote:
> Hi all:
>
> This patchset introduces libvirt xml support for the following two pm conf
> options:
>
>
>
>
>
>
Another option is to create a new xml tag and add these under that tag.
Something like:
+
+
+
+
These are not exactly power management options. So putting them under
may not be correct.
If this is a better approach I will resend the patchset with the changes
along with addressing other review comments.
> The above two options are only available for qemu driver and that too for x86
> guests only. Both of them are global options.
>
> ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for
> cold
> plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge
> (pci-bridge controller) for pc machines and pcie-root-port controller for q35
> machines. The corresponding commandline options to qemu for x86 guests are:
>
> (pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
> (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
>
> Being global options, no other bridge specific options for pci-bridge
> controller or pcie-root-port controllers are required. For pc machine type in
> x86, this option is available in qemu for a long time, from version 2.1.
> Please see the changes in qemu.git:
>
> 9e047b982452c6 ("piix4: add acpi pci hotplug support")
> 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge
> hotplug is disabled")
>
> For q35 machine type, this was introduced in qemu 6.1 with the following
> changes in qemu.git:
>
> (a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
>
> The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as
> opposed to native hotplug) for bridges are outlined in (b). It is possible
> that
> some users might still want to use native hotplug on PCIe [1]. Therefore,
> this conf option enables users to choose either ACPI based hotplug or native
> hotplug for cold plugged bridges (for example for pcie root port controller
> in q35 machines).
>
> ``acpi-root-hotplug`` option enables or disables ACPI based hotplug for PCI
> root
> bus (pci-root controller). This option is only available for pc machine type.
> The corresponding commandline option to qemu for x86 guests is:
>
> -global PIIX4_PM.acpi-root-pci-hotplug=
>
> This additional option enables users to disable hotplug for all devices in the
> system without adding an additional PCI-PCI bridge, putting the devices behind
> the bridge and using the existing ``acpi-hotplug-bridge`` option to disable
> hotplug on that bridge. This feature was introduced from qemu version 5.2 with
> the following change in qemu.git:
>
> 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the
> root bus")
>
> The above qemu commit describes some compelling reasons why users might to
> disable hotplug on PCI root buses [2].
>
> A brief summary of the patches:
>
> >[PATCH v3 1/5] qemu: capablities: detect presence of
> >[PATCH v3 2/5] qemu: capablities: detect presence of
> Patches 1 and 2 implement support for qemu capability checks for the above
> config options.
>
> >[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and
> Patch 3 actually adds the config option to the schema and adds related unit
> tests.
>
> >[PATCH v3 4/5] qemu: command: add support for qemu options that
> Patch 4 adds the backend qemu commandline support for the options. It also
> adds
> relevant unit tests for the same.
>
> >[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release
> Patch 5 adds the release notes for the next libvirt release.
>
>
> Changelog:
> v1: initial implementation. Had some bugs and missed some unit tests.
> v2: fixed bugs and added additional missing unit tests.
> v3: reorganized the patches as per Laine's suggestion. Added more
> details in commit messages. Added conf description in formatdomain.rst.
> Added changelog for next release.
>
>
> Notes:
>
> [1] One concrete example of why one might still want to use native hotplug
> with
> pcie-root-port controller is the fact that we are still discovering issues
> with
> acpi hotplug on PCIE. One such issue is:
> https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html
> Another reason is that users have been using native hotplug on pcie root ports
> up until now. They have built and tested their systems based on native
> hotplug.
> They may not want to suddenly move to acpi based hotplug just because it is
> now
> the default in qemu. Supporting the option to chose one or the other through
> libvirt makes things simpler for end users.
>
> [2] The use case scenario described by Laine in
> https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html
> intentionally does not discuss i440fx and focusses solely on q35. I do realize
> that redhat has moved on
