[PATCH v7 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines

Ani Sinha posted 4 patches 4 years, 4 months ago
[PATCH v7 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
Posted by Ani Sinha 4 years, 4 months ago
The following change in qemu added support for a global boolean flag specific
to i440fx machines that would turn off or on acpi based hotplug for pci root
bus:

3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus")

The option is passed as "-global PIIX4_PM.acpi-root-pci-hotplug=on" etc in qemu
commandline. It is enabled by default. This patch adds the corresponding qemu
capabilities in libvirt as QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG.

Please note that the test specific qemu capabilities .replies files has already
been updated as a part of regular refreshing them when a new qemu version is
released. Hence, no updates to those files are required.

Signed-off-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Laine Stump <laine@redhat.com>
---
 src/qemu/qemu_capabilities.c                     | 4 ++++
 src/qemu/qemu_capabilities.h                     | 3 +++
 tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
 5 files changed, 10 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a1be0cb74e..9d0c96a20c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -639,6 +639,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
               "s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */
               "set-action", /* QEMU_CAPS_SET_ACTION */
               "virtio-blk.queue-size", /* QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE */
+
+              /* 410 */
+              "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
     );
 
 
@@ -1465,6 +1468,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsIDEDrive[] = {
 static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = {
     { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL },
     { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL },
+    { "acpi-root-pci-hotplug", QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, NULL },
 };
 
 static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index b0fa1eec35..3460daac00 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -620,6 +620,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
     QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */
     QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, /* virtio-blk-*.queue-size */
 
+    /* 410 */
+    QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, /* -M pc PIIX4_PM.acpi-root-pci-hotplug */
+
     QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
index 515a970f28..1e5833a9f0 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
@@ -230,6 +230,7 @@
   <flag name='input-linux'/>
   <flag name='query-display-options'/>
   <flag name='virtio-blk.queue-size'/>
+  <flag name='piix4-acpi-root-hotplug-en'/>
   <version>5002000</version>
   <kvmVersion>0</kvmVersion>
   <microcodeVersion>43100243</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index 02eb9a15bb..b54dd8a22e 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -238,6 +238,7 @@
   <flag name='query-display-options'/>
   <flag name='set-action'/>
   <flag name='virtio-blk.queue-size'/>
+  <flag name='piix4-acpi-root-hotplug-en'/>
   <version>6000000</version>
   <kvmVersion>0</kvmVersion>
   <microcodeVersion>43100242</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 678b440d92..0ad493191d 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -240,6 +240,7 @@
   <flag name='query-display-options'/>
   <flag name='set-action'/>
   <flag name='virtio-blk.queue-size'/>
+  <flag name='piix4-acpi-root-hotplug-en'/>
   <version>6001000</version>
   <kvmVersion>0</kvmVersion>
   <microcodeVersion>43100243</microcodeVersion>
-- 
2.25.1

Re: [PATCH v7 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
Posted by Andrea Bolognani 4 years, 4 months ago
On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
> +++ b/src/qemu/qemu_capabilities.c
> +              /* 410 */
> +              "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */

Sorry if this is a silly question, but can you please explain where
the "-en" suffix comes from?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v7 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
Posted by Ani Sinha 4 years, 4 months ago

On Mon, 4 Oct 2021, Andrea Bolognani wrote:

> On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
> > +++ b/src/qemu/qemu_capabilities.c
> > +              /* 410 */
> > +              "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
>
> Sorry if this is a silly question, but can you please explain where
> the "-en" suffix comes from?

"en" stands for enable.

Re: [PATCH v7 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
Posted by Andrea Bolognani 4 years, 4 months ago
On Mon, Oct 04, 2021 at 10:19:17PM +0530, Ani Sinha wrote:
> On Mon, 4 Oct 2021, Andrea Bolognani wrote:
> > On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
> > > +++ b/src/qemu/qemu_capabilities.c
> > > +              /* 410 */
> > > +              "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
> >
> > Sorry if this is a silly question, but can you please explain where
> > the "-en" suffix comes from?
>
> "en" stands for enable.

AFAICT there are no other capabilities that use this convention.
Moreover, the option can be used both to enable *and* disable
hotplug, so the name doesn't seem accurate either...

The string is not exposed to users, so ultimately none of this really
matters, but I would still suggest changing it to

  piix4.acpi-root-pci-hotplug

I'd be happy to either review a patch of yours that does that, or
preparing such a patch myself.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v7 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
Posted by Ani Sinha 4 years, 4 months ago

On Mon, 4 Oct 2021, Andrea Bolognani wrote:

> On Mon, Oct 04, 2021 at 10:19:17PM +0530, Ani Sinha wrote:
> > On Mon, 4 Oct 2021, Andrea Bolognani wrote:
> > > On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
> > > > +++ b/src/qemu/qemu_capabilities.c
> > > > +              /* 410 */
> > > > +              "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
> > >
> > > Sorry if this is a silly question, but can you please explain where
> > > the "-en" suffix comes from?
> >
> > "en" stands for enable.
>
> AFAICT there are no other capabilities that use this convention.
> Moreover, the option can be used both to enable *and* disable
> hotplug, so the name doesn't seem accurate either...
>
> The string is not exposed to users, so ultimately none of this really
> matters, but I would still suggest changing it to
>
>   piix4.acpi-root-pci-hotplug
>
> I'd be happy to either review a patch of yours that does that, or
> preparing such a patch myself.

Sent a patch to rename it.

Re: [PATCH v7 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
Posted by Laine Stump 4 years, 4 months ago
On 10/4/21 1:05 PM, Andrea Bolognani wrote:
> On Mon, Oct 04, 2021 at 10:19:17PM +0530, Ani Sinha wrote:
>> On Mon, 4 Oct 2021, Andrea Bolognani wrote:
>>> On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
>>>> +++ b/src/qemu/qemu_capabilities.c
>>>> +              /* 410 */
>>>> +              "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
>>>
>>> Sorry if this is a silly question, but can you please explain where
>>> the "-en" suffix comes from?
>>
>> "en" stands for enable.
> 
> AFAICT there are no other capabilities that use this convention.
> Moreover, the option can be used both to enable *and* disable
> hotplug, so the name doesn't seem accurate either...
> 
> The string is not exposed to users, so ultimately none of this really
> matters, but I would still suggest changing it to
> 
>    piix4.acpi-root-pci-hotplug
> 
> I'd be happy to either review a patch of yours that does that, or
> preparing such a patch myself.
> 

BAH!!! Someone else mentioned that in an earlier iteration of the 
patches (and may have even suggested the "." after piix4 instead of 
"-"), and I had meant to check/ask about it in the respin, and then 
forgot. :-/ (somehow my brain skipped right over those 3 characters)

I agree with Andrea's suggested change, and apologize for not catching 
it in review (fortunately it was pushed just *after* a release instead 
of before, so we can still make such a change). I'll gladly review 
and/or push any such patch that arrives.