* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
[not found] ` <399ca3a9-8b95-39af-8376-85f2edf00c7e@redhat.com>
@ 2022-03-08 15:51 ` Laine Stump
2022-03-08 16:27 ` Ani Sinha
2022-03-08 16:45 ` Ani Sinha
0 siblings, 2 replies; 18+ messages in thread
From: Laine Stump @ 2022-03-08 15:51 UTC (permalink / raw)
To: libvir-list; +Cc: Ani Sinha, imammedo, jusual, qemu list, Michael S. Tsirkin
Aha! the domain of qemu-devel@nongnu.org was incorrect in the original
send (it was "nognu.org"), so none of this thread was making it to that
list. I've corrected it in this message, but interested parties from
qemu-devel will need to look on the libvir-list archives for the actual
patch mails:
https://listman.redhat.com/archives/libvir-list/2022-March/229089.html
Anyone else who responds to any of the mail on this thread should fix
the qemu-devel address accordingly.
On 3/8/22 10:33 AM, Laine Stump wrote:
> On 3/8/22 1:39 AM, Ani Sinha wrote:
>> Changelog:
>> v2 - rebased the patch series to latest master.
>>
>> I am re-introducing the patchset for <acpi-hotplug-bridge> which got
>> reverted here few months back:
>>
>> https://www.spinics.net/linux/fedora/libvir/msg224089.html
>>
>> The reason for the reversal was that there seemed to be some
>> instability/issues around the use of the qemu commandline which this
>> patchset tries to support. In particular, some guest operating systems
>> did not like the way QEMU was trying to disable native hotplug on pcie
>> root ports.
>
> My memory isn't completely clear, but I think there was also the issue
> that the option claims to enable ACPI hotplug when set to on, but
> instead what it actually does (in the Q35 case at least) is to enable
> native PCI hotplug when set to off (without actually disabling ACPI
> hotplug) and disable native PCI hotplug when set to on, or something
> like that. This ends up leaving it up to the guest OS to decide which
> type of hotplug to use, meaning its decision could override what's in
> the libvirt config, thus confusing everyone. Again, I probably have the
> details mixed up, but it was something like this.
>
> I asked mst about this this morning, and he suggested something that
> you've already done - Cc'ing the series to qemu-devel and the relevant
> maintainers so we can have a discussion with all involved parties about
> their opinions on whether we really should expose this existing option
> in libvirt, or if we should instead have two new options that are more
> orthogonal about enabling/disabling the two types of hotplug, so that
> libvirt config can more accurately represent what is being presented to
> the guest rather than a "best guess" of what we think the guest is going
> to do with what is presented.
>
> (Michael did also say that, with the current flurry of bug reports for
> the QEMU rc's, this discusion may not happen until closer to release
> when the bug reports die down. I know this doesn't mesh with your desire
> to "push now to allow for testing" (which in general would be a good
> thing if we were certain that we wanted the option like this and were
> just expecting some minor bugs that could be fixed), but my opinion is
> that 1) it's possible for anyone interested to test the functionality
> using <qemu:commandline>, and 2) we should avoid turning libvirt git
> into a revolving door of experiments. The only practical difference
> between using <qemu:commandline> and having a dedicated option is that
> the use of <qemu:commandline> causes the domain to be tainted, and the
> XML is a bit more complicated. But since the people we're talking about
> here will already have built their own libvirt binaries, the tainted
> status of any guests is irrelevant and the extra complexity of using
> <qemu:commandline> is probably trivial to them :-).
>
>> Subsequently, in QEMU 6.2, we have changed our mechanism
>> using which we disable native hotplug. As I understand, we do not have
>> any reported issues so far in 6.2 around this area. QEMU will enter a
>> soft feature freeze in the first week of march in prep for 7.0 release.
>> Libvirt is also entering a new release cycle phaze. Hence, I am
>> introducing this patchset early enough in the release cycles so that if
>> we do see any issues on the qemu side during the rc0, rc1 cycles and if
>> reversal of this patchset is again required, it can be done in time
>> before the next libvirt release end of March.
>>
>> All the patches in this series had been previously reviewed. Some
>> subsequent fixes were made after my initial patches were pushed. I have
>> squashed all those fixes and consolidated them into four patches. I have
>> also updated the documentation to reflect the new changes from the QEMU
>> side and rebased my changes fixing the tests in the process.
>>
>> What changed in QEMU post version 6.1 ?
>> =========================================
>>
>> We have made basically two major changes in QEMU. First is this change:
>>
>> (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
>> Author: Julia Suvorova <jusual@redhat.com>
>> Date: Fri Nov 12 06:08:56 2021 -0500
>>
>> hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
>> There are two ways to enable ACPI PCI Hot-plug:
>> * Disable the Hot-plug Capable bit on PCIe slots.
>> This was the first approach which led to regression [1-2], as
>> I/O space for a port is allocated only when it is hot-pluggable,
>> which is determined by HPC bit.
>> * Leave the HPC bit on and disable PCIe Native Hot-plug
>> in _OSC
>> method.
>> This removes the (future) ability of hot-plugging switches with PCIe
>> Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
>> bridges. If the user wants to explicitely use this feature, they can
>> disable ACPI PCI Hot-plug with:
>> --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
>> Change the bit in _OSC method so that the OS selects ACPI PCI
>> Hot-plug
>> instead of PCIe Native.
>> [1] https://gitlab.com/qemu-project/qemu/-/issues/641
>> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
>> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>>
>> The patch description says it all. Instead of masking out the HPC bit in
>> pcie slots, we keep them turned on. Instead, we do not advertize native
>> hotplug capability for PCIE using _OSC control method. See section
>> 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
>> these slots so now the guest OS can select ACPI hotplug instead.
>>
>> The second change is introduction of a property with which we keep the
>> existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
>> and ACPI hotplug is enabled by default for pcie root ports.
>> The QEMU commit is:
>>
>> (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
>> Author: Julia Suvorova <jusual@redhat.com>
>> Date: Fri Nov 12 06:08:54 2021 -0500
>>
>> hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine
>> type
>> To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
>> turned on, while the switch to ACPI Hot-plug will be done in the
>> DSDT table.
>> Introducing 'x-keep-native-hpc' property disables the HPC bit only
>> in 6.1 and as a result keeps the forced 'reserve-io' on
>> pcie-root-ports in 6.1 too.
>> [1] https://gitlab.com/qemu-project/qemu/-/issues/641
>> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
>> mask out HPC bit in PCIE, the work done by this patch is no longer
>> needed:
>>
>> (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
>> Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Date: Mon Aug 2 12:00:57 2021 +0300
>>
>> hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
>> Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
>> As opposed to native PCIe hotplug, guests like Fedora 34
>> will not assign IO range to pcie-root-ports not supporting
>> native hotplug, resulting into a regression.
>> Reproduce by:
>> qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
>> device_add e1000,bus=p1
>> In the Guest OS the respective pcie-root-port will have the IO range
>> disabled.
>> Fix it by setting the "reserve-io" hint capability of the
>> pcie-root-ports so the firmware will allocate the IO range instead.
>> Acked-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
>> patch (3) since we mask out HPC bit from pcie ports.
>>
>>
>> I know this is convoluted mess. In fairness I am trying all I can in my
>> spare time to help from the QEMU side. I am determined to see this
>> patchset through into libvirt.
>>
>> Thanks
>>
>>
>> Ani Sinha (4):
>> qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
>> conf: introduce support for acpi-bridge-hotplug feature
>> qemu: command: add support for acpi-bridge-hotplug feature
>> NEWS: document new acpi pci hotplug config option
>>
>> NEWS.rst | 8 ++
>> docs/formatdomain.rst | 32 +++++++
>> docs/schemas/domaincommon.rng | 15 ++++
>> src/conf/domain_conf.c | 89 ++++++++++++++++++-
>> src/conf/domain_conf.h | 9 ++
>> src/qemu/qemu_capabilities.c | 4 +
>> src/qemu/qemu_capabilities.h | 3 +
>> src/qemu/qemu_command.c | 19 ++++
>> src/qemu/qemu_validate.c | 42 +++++++++
>> .../caps_6.1.0.x86_64.xml | 1 +
>> .../caps_6.2.0.x86_64.xml | 1 +
>> .../caps_7.0.0.x86_64.xml | 1 +
>> ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
>> .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
>> ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
>> .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
>> .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
>> ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
>> ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
>> .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
>> .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
>> tests/qemuxml2argvtest.c | 7 ++
>> ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
>> ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
>> ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
>> ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
>> tests/qemuxml2xmltest.c | 4 +
>> 27 files changed, 504 insertions(+), 1 deletion(-)
>> create mode 100644
>> tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
>>
>> create mode 100644
>> tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
>> create mode 100644
>> tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
>>
>> create mode 100644
>> tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
>> create mode 100644
>> tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
>> create mode 100644
>> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
>> create mode 100644
>> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
>> create mode 100644
>> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
>> create mode 100644
>> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
>> create mode 120000
>> tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
>>
>> create mode 120000
>> tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
>>
>> create mode 120000
>> tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
>>
>> create mode 120000
>> tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 15:51 ` [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge> Laine Stump
@ 2022-03-08 16:27 ` Ani Sinha
2022-03-08 16:30 ` Michael S. Tsirkin
2022-03-08 16:45 ` Ani Sinha
1 sibling, 1 reply; 18+ messages in thread
From: Ani Sinha @ 2022-03-08 16:27 UTC (permalink / raw)
To: Laine Stump; +Cc: libvir-list, imammedo, jusual, qemu list, Michael S. Tsirkin
[-- Attachment #1: Type: text/plain, Size: 13610 bytes --]
On Tue, Mar 8, 2022 at 21:21 Laine Stump <laine@redhat.com> wrote:
> Aha! the domain of qemu-devel@nongnu.org was incorrect in the original
> send (it was "nognu.org"), so none of this thread was making it to that
> list. I've corrected it in this message, but interested parties from
> qemu-devel will need to look on the libvir-list archives for the actual
> patch mails:
>
> https://listman.redhat.com/archives/libvir-list/2022-March/229089.html
>
> Anyone else who responds to any of the mail on this thread should fix
> the qemu-devel address accordingly.
This patch set has been a true test of my diligence and perseverance 🙂
>
> On 3/8/22 10:33 AM, Laine Stump wrote:
> > On 3/8/22 1:39 AM, Ani Sinha wrote:
> >> Changelog:
> >> v2 - rebased the patch series to latest master.
> >>
> >> I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> >> reverted here few months back:
> >>
> >> https://www.spinics.net/linux/fedora/libvir/msg224089.html
> >>
> >> The reason for the reversal was that there seemed to be some
> >> instability/issues around the use of the qemu commandline which this
> >> patchset tries to support. In particular, some guest operating systems
> >> did not like the way QEMU was trying to disable native hotplug on pcie
> >> root ports.
> >
> > My memory isn't completely clear, but I think there was also the issue
> > that the option claims to enable ACPI hotplug when set to on, but
> > instead what it actually does (in the Q35 case at least) is to enable
> > native PCI hotplug when set to off (without actually disabling ACPI
> > hotplug) and disable native PCI hotplug when set to on, or something
> > like that. This ends up leaving it up to the guest OS to decide which
> > type of hotplug to use, meaning its decision could override what's in
> > the libvirt config, thus confusing everyone. Again, I probably have the
> > details mixed up, but it was something like this.
> >
> > I asked mst about this this morning, and he suggested something that
> > you've already done - Cc'ing the series to qemu-devel and the relevant
> > maintainers so we can have a discussion with all involved parties about
> > their opinions on whether we really should expose this existing option
> > in libvirt, or if we should instead have two new options that are more
> > orthogonal about enabling/disabling the two types of hotplug, so that
> > libvirt config can more accurately represent what is being presented to
> > the guest rather than a "best guess" of what we think the guest is going
> > to do with what is presented.
> >
> > (Michael did also say that, with the current flurry of bug reports for
> > the QEMU rc's, this discusion may not happen until closer to release
> > when the bug reports die down. I know this doesn't mesh with your desire
> > to "push now to allow for testing" (which in general would be a good
> > thing if we were certain that we wanted the option like this and were
> > just expecting some minor bugs that could be fixed), but my opinion is
> > that 1) it's possible for anyone interested to test the functionality
> > using <qemu:commandline>, and 2) we should avoid turning libvirt git
> > into a revolving door of experiments. The only practical difference
> > between using <qemu:commandline> and having a dedicated option is that
> > the use of <qemu:commandline> causes the domain to be tainted, and the
> > XML is a bit more complicated. But since the people we're talking about
> > here will already have built their own libvirt binaries, the tainted
> > status of any guests is irrelevant and the extra complexity of using
> > <qemu:commandline> is probably trivial to them :-).
> >
> >> Subsequently, in QEMU 6.2, we have changed our mechanism
> >> using which we disable native hotplug. As I understand, we do not have
> >> any reported issues so far in 6.2 around this area. QEMU will enter a
> >> soft feature freeze in the first week of march in prep for 7.0 release.
> >> Libvirt is also entering a new release cycle phaze. Hence, I am
> >> introducing this patchset early enough in the release cycles so that if
> >> we do see any issues on the qemu side during the rc0, rc1 cycles and if
> >> reversal of this patchset is again required, it can be done in time
> >> before the next libvirt release end of March.
> >>
> >> All the patches in this series had been previously reviewed. Some
> >> subsequent fixes were made after my initial patches were pushed. I have
> >> squashed all those fixes and consolidated them into four patches. I have
> >> also updated the documentation to reflect the new changes from the QEMU
> >> side and rebased my changes fixing the tests in the process.
> >>
> >> What changed in QEMU post version 6.1 ?
> >> =========================================
> >>
> >> We have made basically two major changes in QEMU. First is this change:
> >>
> >> (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> >> Author: Julia Suvorova <jusual@redhat.com>
> >> Date: Fri Nov 12 06:08:56 2021 -0500
> >>
> >> hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
> >> There are two ways to enable ACPI PCI Hot-plug:
> >> * Disable the Hot-plug Capable bit on PCIe slots.
> >> This was the first approach which led to regression [1-2], as
> >> I/O space for a port is allocated only when it is hot-pluggable,
> >> which is determined by HPC bit.
> >> * Leave the HPC bit on and disable PCIe Native Hot-plug
> >> in _OSC
> >> method.
> >> This removes the (future) ability of hot-plugging switches with
> PCIe
> >> Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> >> bridges. If the user wants to explicitely use this feature, they
> can
> >> disable ACPI PCI Hot-plug with:
> >> --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> >> Change the bit in _OSC method so that the OS selects ACPI PCI
> >> Hot-plug
> >> instead of PCIe Native.
> >> [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> >> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> >> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> >> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >>
> >> The patch description says it all. Instead of masking out the HPC bit in
> >> pcie slots, we keep them turned on. Instead, we do not advertize native
> >> hotplug capability for PCIE using _OSC control method. See section
> >> 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> >> these slots so now the guest OS can select ACPI hotplug instead.
> >>
> >> The second change is introduction of a property with which we keep the
> >> existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> >> and ACPI hotplug is enabled by default for pcie root ports.
> >> The QEMU commit is:
> >>
> >> (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> >> Author: Julia Suvorova <jusual@redhat.com>
> >> Date: Fri Nov 12 06:08:54 2021 -0500
> >>
> >> hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine
> >> type
> >> To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will
> be
> >> turned on, while the switch to ACPI Hot-plug will be done in the
> >> DSDT table.
> >> Introducing 'x-keep-native-hpc' property disables the HPC bit only
> >> in 6.1 and as a result keeps the forced 'reserve-io' on
> >> pcie-root-ports in 6.1 too.
> >> [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> >> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> >> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> >> mask out HPC bit in PCIE, the work done by this patch is no longer
> >> needed:
> >>
> >> (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> >> Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> >> Date: Mon Aug 2 12:00:57 2021 +0300
> >>
> >> hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
> >> Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> >> As opposed to native PCIe hotplug, guests like Fedora 34
> >> will not assign IO range to pcie-root-ports not supporting
> >> native hotplug, resulting into a regression.
> >> Reproduce by:
> >> qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> >> device_add e1000,bus=p1
> >> In the Guest OS the respective pcie-root-port will have the IO
> range
> >> disabled.
> >> Fix it by setting the "reserve-io" hint capability of the
> >> pcie-root-ports so the firmware will allocate the IO range instead.
> >> Acked-by: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >> Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> >> patch (3) since we mask out HPC bit from pcie ports.
> >>
> >>
> >> I know this is convoluted mess. In fairness I am trying all I can in my
> >> spare time to help from the QEMU side. I am determined to see this
> >> patchset through into libvirt.
> >>
> >> Thanks
> >>
> >>
> >> Ani Sinha (4):
> >> qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> >> conf: introduce support for acpi-bridge-hotplug feature
> >> qemu: command: add support for acpi-bridge-hotplug feature
> >> NEWS: document new acpi pci hotplug config option
> >>
> >> NEWS.rst | 8 ++
> >> docs/formatdomain.rst | 32 +++++++
> >> docs/schemas/domaincommon.rng | 15 ++++
> >> src/conf/domain_conf.c | 89 ++++++++++++++++++-
> >> src/conf/domain_conf.h | 9 ++
> >> src/qemu/qemu_capabilities.c | 4 +
> >> src/qemu/qemu_capabilities.h | 3 +
> >> src/qemu/qemu_command.c | 19 ++++
> >> src/qemu/qemu_validate.c | 42 +++++++++
> >> .../caps_6.1.0.x86_64.xml | 1 +
> >> .../caps_6.2.0.x86_64.xml | 1 +
> >> .../caps_7.0.0.x86_64.xml | 1 +
> >> ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> >> .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> >> ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> >> .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> >> .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> >> ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> >> ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> >> .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> >> .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> >> tests/qemuxml2argvtest.c | 7 ++
> >> ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> >> ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> >> ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> >> ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> >> tests/qemuxml2xmltest.c | 4 +
> >> 27 files changed, 504 insertions(+), 1 deletion(-)
> >> create mode 100644
> >>
> tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
>
> >>
> >> create mode 100644
> >> tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> >> create mode 100644
> >>
> tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
>
> >>
> >> create mode 100644
> >> tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> >> create mode 100644
> >> tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> >> create mode 100644
> >> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> >> create mode 100644
> >>
> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> >> create mode 100644
> >> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> >> create mode 100644
> >> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> >> create mode 120000
> >>
> tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
>
> >>
> >> create mode 120000
> >>
> tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
>
> >>
> >> create mode 120000
> >>
> tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> >>
> >> create mode 120000
> >>
> tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
> >>
> >
>
>
[-- Attachment #2: Type: text/html, Size: 18766 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 16:27 ` Ani Sinha
@ 2022-03-08 16:30 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2022-03-08 16:30 UTC (permalink / raw)
To: Ani Sinha; +Cc: libvir-list, imammedo, jusual, qemu list, Laine Stump
On Tue, Mar 08, 2022 at 09:57:04PM +0530, Ani Sinha wrote:
>
>
> On Tue, Mar 8, 2022 at 21:21 Laine Stump <laine@redhat.com> wrote:
>
> Aha! the domain of qemu-devel@nongnu.org was incorrect in the original
> send (it was "nognu.org"), so none of this thread was making it to that
> list. I've corrected it in this message, but interested parties from
> qemu-devel will need to look on the libvir-list archives for the actual
> patch mails:
>
> https://listman.redhat.com/archives/libvir-list/2022-March/229089.html
>
> Anyone else who responds to any of the mail on this thread should fix
> the qemu-devel address accordingly.
>
>
> This patch set has been a true test of my diligence and perseverance 🙂
Right. But could you maybe repost to the proper qemu-devel?
I'd like to be able to pull the whole email not just the
summary listman deems safe to show me.
>
>
>
> On 3/8/22 10:33 AM, Laine Stump wrote:
> > On 3/8/22 1:39 AM, Ani Sinha wrote:
> >> Changelog:
> >> v2 - rebased the patch series to latest master.
> >>
> >> I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> >> reverted here few months back:
> >>
> >> https://www.spinics.net/linux/fedora/libvir/msg224089.html
> >>
> >> The reason for the reversal was that there seemed to be some
> >> instability/issues around the use of the qemu commandline which this
> >> patchset tries to support. In particular, some guest operating systems
> >> did not like the way QEMU was trying to disable native hotplug on pcie
> >> root ports.
> >
> > My memory isn't completely clear, but I think there was also the issue
> > that the option claims to enable ACPI hotplug when set to on, but
> > instead what it actually does (in the Q35 case at least) is to enable
> > native PCI hotplug when set to off (without actually disabling ACPI
> > hotplug) and disable native PCI hotplug when set to on, or something
> > like that. This ends up leaving it up to the guest OS to decide which
> > type of hotplug to use, meaning its decision could override what's in
> > the libvirt config, thus confusing everyone. Again, I probably have the
> > details mixed up, but it was something like this.
> >
> > I asked mst about this this morning, and he suggested something that
> > you've already done - Cc'ing the series to qemu-devel and the relevant
> > maintainers so we can have a discussion with all involved parties about
> > their opinions on whether we really should expose this existing option
> > in libvirt, or if we should instead have two new options that are more
> > orthogonal about enabling/disabling the two types of hotplug, so that
> > libvirt config can more accurately represent what is being presented to
> > the guest rather than a "best guess" of what we think the guest is going
> > to do with what is presented.
> >
> > (Michael did also say that, with the current flurry of bug reports for
> > the QEMU rc's, this discusion may not happen until closer to release
> > when the bug reports die down. I know this doesn't mesh with your desire
> > to "push now to allow for testing" (which in general would be a good
> > thing if we were certain that we wanted the option like this and were
> > just expecting some minor bugs that could be fixed), but my opinion is
> > that 1) it's possible for anyone interested to test the functionality
> > using <qemu:commandline>, and 2) we should avoid turning libvirt git
> > into a revolving door of experiments. The only practical difference
> > between using <qemu:commandline> and having a dedicated option is that
> > the use of <qemu:commandline> causes the domain to be tainted, and the
> > XML is a bit more complicated. But since the people we're talking about
> > here will already have built their own libvirt binaries, the tainted
> > status of any guests is irrelevant and the extra complexity of using
> > <qemu:commandline> is probably trivial to them :-).
> >
> >> Subsequently, in QEMU 6.2, we have changed our mechanism
> >> using which we disable native hotplug. As I understand, we do not have
> >> any reported issues so far in 6.2 around this area. QEMU will enter a
> >> soft feature freeze in the first week of march in prep for 7.0 release.
> >> Libvirt is also entering a new release cycle phaze. Hence, I am
> >> introducing this patchset early enough in the release cycles so that if
> >> we do see any issues on the qemu side during the rc0, rc1 cycles and if
> >> reversal of this patchset is again required, it can be done in time
> >> before the next libvirt release end of March.
> >>
> >> All the patches in this series had been previously reviewed. Some
> >> subsequent fixes were made after my initial patches were pushed. I have
> >> squashed all those fixes and consolidated them into four patches. I have
> >> also updated the documentation to reflect the new changes from the QEMU
> >> side and rebased my changes fixing the tests in the process.
> >>
> >> What changed in QEMU post version 6.1 ?
> >> =========================================
> >>
> >> We have made basically two major changes in QEMU. First is this change:
> >>
> >> (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> >> Author: Julia Suvorova <jusual@redhat.com>
> >> Date: Fri Nov 12 06:08:56 2021 -0500
> >>
> >> hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
> >> There are two ways to enable ACPI PCI Hot-plug:
> >> * Disable the Hot-plug Capable bit on PCIe slots.
> >> This was the first approach which led to regression [1-2], as
> >> I/O space for a port is allocated only when it is hot-pluggable,
> >> which is determined by HPC bit.
> >> * Leave the HPC bit on and disable PCIe Native Hot-plug
> >> in _OSC
> >> method.
> >> This removes the (future) ability of hot-plugging switches with
> PCIe
> >> Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> >> bridges. If the user wants to explicitely use this feature, they
> can
> >> disable ACPI PCI Hot-plug with:
> >> --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> >> Change the bit in _OSC method so that the OS selects ACPI PCI
> >> Hot-plug
> >> instead of PCIe Native.
> >> [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> >> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> >> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> >> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >>
> >> The patch description says it all. Instead of masking out the HPC bit in
> >> pcie slots, we keep them turned on. Instead, we do not advertize native
> >> hotplug capability for PCIE using _OSC control method. See section
> >> 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> >> these slots so now the guest OS can select ACPI hotplug instead.
> >>
> >> The second change is introduction of a property with which we keep the
> >> existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> >> and ACPI hotplug is enabled by default for pcie root ports.
> >> The QEMU commit is:
> >>
> >> (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> >> Author: Julia Suvorova <jusual@redhat.com>
> >> Date: Fri Nov 12 06:08:54 2021 -0500
> >>
> >> hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine
> >> type
> >> To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will
> be
> >> turned on, while the switch to ACPI Hot-plug will be done in the
> >> DSDT table.
> >> Introducing 'x-keep-native-hpc' property disables the HPC bit only
> >> in 6.1 and as a result keeps the forced 'reserve-io' on
> >> pcie-root-ports in 6.1 too.
> >> [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> >> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> >> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> >> mask out HPC bit in PCIE, the work done by this patch is no longer
> >> needed:
> >>
> >> (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> >> Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> >> Date: Mon Aug 2 12:00:57 2021 +0300
> >>
> >> hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
> >> Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> >> As opposed to native PCIe hotplug, guests like Fedora 34
> >> will not assign IO range to pcie-root-ports not supporting
> >> native hotplug, resulting into a regression.
> >> Reproduce by:
> >> qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> >> device_add e1000,bus=p1
> >> In the Guest OS the respective pcie-root-port will have the IO
> range
> >> disabled.
> >> Fix it by setting the "reserve-io" hint capability of the
> >> pcie-root-ports so the firmware will allocate the IO range instead.
> >> Acked-by: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >> Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> >> patch (3) since we mask out HPC bit from pcie ports.
> >>
> >>
> >> I know this is convoluted mess. In fairness I am trying all I can in my
> >> spare time to help from the QEMU side. I am determined to see this
> >> patchset through into libvirt.
> >>
> >> Thanks
> >>
> >>
> >> Ani Sinha (4):
> >> qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> >> conf: introduce support for acpi-bridge-hotplug feature
> >> qemu: command: add support for acpi-bridge-hotplug feature
> >> NEWS: document new acpi pci hotplug config option
> >>
> >> NEWS.rst | 8 ++
> >> docs/formatdomain.rst | 32 +++++++
> >> docs/schemas/domaincommon.rng | 15 ++++
> >> src/conf/domain_conf.c | 89 ++++++++++++++++++-
> >> src/conf/domain_conf.h | 9 ++
> >> src/qemu/qemu_capabilities.c | 4 +
> >> src/qemu/qemu_capabilities.h | 3 +
> >> src/qemu/qemu_command.c | 19 ++++
> >> src/qemu/qemu_validate.c | 42 +++++++++
> >> .../caps_6.1.0.x86_64.xml | 1 +
> >> .../caps_6.2.0.x86_64.xml | 1 +
> >> .../caps_7.0.0.x86_64.xml | 1 +
> >> ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> >> .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> >> ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> >> .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> >> .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> >> ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> >> ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> >> .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> >> .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> >> tests/qemuxml2argvtest.c | 7 ++
> >> ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> >> ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> >> ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> >> ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> >> tests/qemuxml2xmltest.c | 4 +
> >> 27 files changed, 504 insertions(+), 1 deletion(-)
> >> create mode 100644
> >> tests/qemuxml2argvdata/
> aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
> >>
> >> create mode 100644
> >> tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> >> create mode 100644
> >> tests/qemuxml2argvdata/
> pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
> >>
> >> create mode 100644
> >> tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> >> create mode 100644
> >> tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> >> create mode 100644
> >> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> >> create mode 100644
> >> tests/qemuxml2argvdata/
> q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> >> create mode 100644
> >> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> >> create mode 100644
> >> tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> >> create mode 120000
> >> tests/qemuxml2xmloutdata/
> pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
> >>
> >> create mode 120000
> >> tests/qemuxml2xmloutdata/
> pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
> >>
> >> create mode 120000
> >> tests/qemuxml2xmloutdata/
> q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> >>
> >> create mode 120000
> >> tests/qemuxml2xmloutdata/
> q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
> >>
> >
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 15:51 ` [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge> Laine Stump
2022-03-08 16:27 ` Ani Sinha
@ 2022-03-08 16:45 ` Ani Sinha
2022-03-08 16:47 ` Michael S. Tsirkin
1 sibling, 1 reply; 18+ messages in thread
From: Ani Sinha @ 2022-03-08 16:45 UTC (permalink / raw)
To: Laine Stump
Cc: Michael S. Tsirkin, libvir-list, jusual, qemu list, Ani Sinha,
imammedo
On Tue, 8 Mar 2022, Laine Stump wrote:
> Aha! the domain of qemu-devel@nongnu.org was incorrect in the original send
> (it was "nognu.org"), so none of this thread was making it to that list.
Not to give any excuses but this happened because on Qemu side I never
have to type this manually. My git config is set up so that
the cc in send-email is filled up automatically using
scripts/get_maintainer.pl. On libvirt side also the domain and mailing
list is easy to remember. Its only when I have to manually type stuff that
shit happens :-)
^ permalink raw reply [flat|nested] 18+ messages in thread
* [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
@ 2022-03-08 16:45 Ani Sinha
2022-03-08 16:57 ` Michael S. Tsirkin
0 siblings, 1 reply; 18+ messages in thread
From: Ani Sinha @ 2022-03-08 16:45 UTC (permalink / raw)
To: libvir-list; +Cc: mst, jusual, qemu-devel, laine, Ani Sinha, imammedo
Change log:
v2: rebased the patchset. Laine's response is appended at the end.
I am re-introducing the patchset for <acpi-hotplug-bridge> which got
reverted here few months back:
https://www.spinics.net/linux/fedora/libvir/msg224089.html
The reason for the reversal was that there seemed to be some
instability/issues around the use of the qemu commandline which this
patchset tries to support. In particular, some guest operating systems
did not like the way QEMU was trying to disable native hotplug on pcie
root ports. Subsequently, in QEMU 6.2, we have changed our mechanism
using which we disable native hotplug. As I understand, we do not have
any reported issues so far in 6.2 around this area. QEMU will enter a
soft feature freeze in the first week of march in prep for 7.0 release.
Libvirt is also entering a new release cycle phaze. Hence, I am
introducing this patchset early enough in the release cycles so that if
we do see any issues on the qemu side during the rc0, rc1 cycles and if
reversal of this patchset is again required, it can be done in time
before the next libvirt release end of March.
All the patches in this series had been previously reviewed. Some
subsequent fixes were made after my initial patches were pushed. I have
squashed all those fixes and consolidated them into four patches. I have
also updated the documentation to reflect the new changes from the QEMU
side and rebased my changes fixing the tests in the process.
What changed in QEMU post version 6.1 ?
=========================================
We have made basically two major changes in QEMU. First is this change:
(1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
Author: Julia Suvorova <jusual@redhat.com>
Date: Fri Nov 12 06:08:56 2021 -0500
hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
There are two ways to enable ACPI PCI Hot-plug:
* Disable the Hot-plug Capable bit on PCIe slots.
This was the first approach which led to regression [1-2], as
I/O space for a port is allocated only when it is hot-pluggable,
which is determined by HPC bit.
* Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
method.
This removes the (future) ability of hot-plugging switches with PCIe
Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
bridges. If the user wants to explicitely use this feature, they can
disable ACPI PCI Hot-plug with:
--global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
instead of PCIe Native.
[1] https://gitlab.com/qemu-project/qemu/-/issues/641
[2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
Signed-off-by: Julia Suvorova <jusual@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
The patch description says it all. Instead of masking out the HPC bit in
pcie slots, we keep them turned on. Instead, we do not advertize native
hotplug capability for PCIE using _OSC control method. See section
6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
these slots so now the guest OS can select ACPI hotplug instead.
The second change is introduction of a property with which we keep the
existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
and ACPI hotplug is enabled by default for pcie root ports.
The QEMU commit is:
(2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
Author: Julia Suvorova <jusual@redhat.com>
Date: Fri Nov 12 06:08:54 2021 -0500
hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type
To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
turned on, while the switch to ACPI Hot-plug will be done in the
DSDT table.
Introducing 'x-keep-native-hpc' property disables the HPC bit only
in 6.1 and as a result keeps the forced 'reserve-io' on
pcie-root-ports in 6.1 too.
[1] https://gitlab.com/qemu-project/qemu/-/issues/641
[2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
Signed-off-by: Julia Suvorova <jusual@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
mask out HPC bit in PCIE, the work done by this patch is no longer
needed:
(3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Date: Mon Aug 2 12:00:57 2021 +0300
hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
As opposed to native PCIe hotplug, guests like Fedora 34
will not assign IO range to pcie-root-ports not supporting
native hotplug, resulting into a regression.
Reproduce by:
qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
device_add e1000,bus=p1
In the Guest OS the respective pcie-root-port will have the IO range
disabled.
Fix it by setting the "reserve-io" hint capability of the
pcie-root-ports so the firmware will allocate the IO range instead.
Acked-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
patch (3) since we mask out HPC bit from pcie ports.
I know this is convoluted mess. In fairness I am trying all I can in my
spare time to help from the QEMU side. I am determined to see this
patchset through into libvirt.
Thanks
Laine's comments ...
My memory isn't completely clear, but I think there was also the issue
that the option claims to enable ACPI hotplug when set to on, but
instead what it actually does (in the Q35 case at least) is to enable
native PCI hotplug when set to off (without actually disabling ACPI
hotplug) and disable native PCI hotplug when set to on, or something
like that. This ends up leaving it up to the guest OS to decide which
type of hotplug to use, meaning its decision could override what's in
the libvirt config, thus confusing everyone. Again, I probably have the
details mixed up, but it was something like this.
I asked mst about this this morning, and he suggested something that
you've already done - Cc'ing the series to qemu-devel and the relevant
maintainers so we can have a discussion with all involved parties about
their opinions on whether we really should expose this existing option
in libvirt, or if we should instead have two new options that are more
orthogonal about enabling/disabling the two types of hotplug, so that
libvirt config can more accurately represent what is being presented to
the guest rather than a "best guess" of what we think the guest is going
to do with what is presented.
(Michael did also say that, with the current flurry of bug reports for
the QEMU rc's, this discusion may not happen until closer to release
when the bug reports die down. I know this doesn't mesh with your desire
to "push now to allow for testing" (which in general would be a good
thing if we were certain that we wanted the option like this and were
just expecting some minor bugs that could be fixed), but my opinion is
that 1) it's possible for anyone interested to test the functionality
using <qemu:commandline>, and 2) we should avoid turning libvirt git
into a revolving door of experiments. The only practical difference
between using <qemu:commandline> and having a dedicated option is that
the use of <qemu:commandline> causes the domain to be tainted, and the
XML is a bit more complicated. But since the people we're talking about
here will already have built their own libvirt binaries, the tainted
status of any guests is irrelevant and the extra complexity of using
<qemu:commandline> is probably trivial to them :-).
Ani Sinha (4):
qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
conf: introduce support for acpi-bridge-hotplug feature
qemu: command: add support for acpi-bridge-hotplug feature
NEWS: document new acpi pci hotplug config option
NEWS.rst | 8 ++
docs/formatdomain.rst | 32 +++++++
docs/schemas/domaincommon.rng | 15 ++++
src/conf/domain_conf.c | 89 ++++++++++++++++++-
src/conf/domain_conf.h | 9 ++
src/qemu/qemu_capabilities.c | 4 +
src/qemu/qemu_capabilities.h | 3 +
src/qemu/qemu_command.c | 19 ++++
src/qemu/qemu_validate.c | 42 +++++++++
.../caps_6.1.0.x86_64.xml | 1 +
.../caps_6.2.0.x86_64.xml | 1 +
.../caps_7.0.0.x86_64.xml | 1 +
...-hotplug-bridge-disable.aarch64-latest.err | 1 +
.../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
.../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
.../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
.../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
.../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
tests/qemuxml2argvtest.c | 7 ++
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
27 files changed, 504 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 16:45 ` Ani Sinha
@ 2022-03-08 16:47 ` Michael S. Tsirkin
2022-03-08 16:53 ` Ani Sinha
0 siblings, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2022-03-08 16:47 UTC (permalink / raw)
To: Ani Sinha; +Cc: libvir-list, imammedo, jusual, qemu list, Laine Stump
On Tue, Mar 08, 2022 at 10:15:11PM +0530, Ani Sinha wrote:
>
>
> On Tue, 8 Mar 2022, Laine Stump wrote:
>
> > Aha! the domain of qemu-devel@nongnu.org was incorrect in the original send
> > (it was "nognu.org"), so none of this thread was making it to that list.
>
>
> Not to give any excuses but this happened because on Qemu side I never
> have to type this manually. My git config is set up so that
> the cc in send-email is filled up automatically using
> scripts/get_maintainer.pl. On libvirt side also the domain and mailing
> list is easy to remember. Its only when I have to manually type stuff that
> shit happens :-)
Donnu about alpine, but with mutt you can easily set up
and alias and then it expands for you.
--
MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 16:47 ` Michael S. Tsirkin
@ 2022-03-08 16:53 ` Ani Sinha
2022-03-08 17:18 ` Michael S. Tsirkin
0 siblings, 1 reply; 18+ messages in thread
From: Ani Sinha @ 2022-03-08 16:53 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: libvir-list, imammedo, jusual, qemu list, Laine Stump
On Tue, Mar 8, 2022 at 10:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Mar 08, 2022 at 10:15:11PM +0530, Ani Sinha wrote:
> >
> >
> > On Tue, 8 Mar 2022, Laine Stump wrote:
> >
> > > Aha! the domain of qemu-devel@nongnu.org was incorrect in the original send
> > > (it was "nognu.org"), so none of this thread was making it to that list.
> >
> >
> > Not to give any excuses but this happened because on Qemu side I never
> > have to type this manually. My git config is set up so that
> > the cc in send-email is filled up automatically using
> > scripts/get_maintainer.pl. On libvirt side also the domain and mailing
> > list is easy to remember. Its only when I have to manually type stuff that
> > shit happens :-)
>
> Donnu about alpine, but with mutt you can easily set up
> and alias and then it expands for you.
I use alpine to only reply/review patches. I use git send-email to
actually send the patch. There I am not sure the best way to avoid
manually typing in the mailing list address.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 16:45 Ani Sinha
@ 2022-03-08 16:57 ` Michael S. Tsirkin
2022-04-12 4:20 ` Ani Sinha
2022-04-22 5:25 ` Ani Sinha
0 siblings, 2 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2022-03-08 16:57 UTC (permalink / raw)
To: Ani Sinha; +Cc: libvir-list, imammedo, jusual, qemu-devel, laine
On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote:
>
> Change log:
> v2: rebased the patchset. Laine's response is appended at the end.
>
> I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> reverted here few months back:
>
> https://www.spinics.net/linux/fedora/libvir/msg224089.html
>
> The reason for the reversal was that there seemed to be some
> instability/issues around the use of the qemu commandline which this
> patchset tries to support. In particular, some guest operating systems
> did not like the way QEMU was trying to disable native hotplug on pcie
> root ports. Subsequently, in QEMU 6.2, we have changed our mechanism
> using which we disable native hotplug. As I understand, we do not have
> any reported issues so far in 6.2 around this area. QEMU will enter a
> soft feature freeze in the first week of march in prep for 7.0 release.
Right. But unfortunately we did not yet really work on
a sane interface for this.
The way I see it, at high level we thinkably need two flags
- disable ACPI hotplug
- enable native hotplug (maybe separately for pci and pcie?)
and with both enabled guests actually can switch between
the two.
This will at least reflect the hardware, so has a chance to be
stable.
The big question however would be what is the actual use-case.
Without that this begs the question of why do we bother at all.
To allow hotplug of bridges? If it is really necessary for us then
we should think hard about questions that surround this:
- how does one hotplug a pcie switch?
- any way to use e.g. dynamic ACPI to support hotplug of bridges?
- do we want to bite the bullet and create an option for management
to fully control guest memory layout including all pci devices?
> Libvirt is also entering a new release cycle phaze. Hence, I am
> introducing this patchset early enough in the release cycles so that if
> we do see any issues on the qemu side during the rc0, rc1 cycles and if
> reversal of this patchset is again required, it can be done in time
> before the next libvirt release end of March.
>
> All the patches in this series had been previously reviewed. Some
> subsequent fixes were made after my initial patches were pushed. I have
> squashed all those fixes and consolidated them into four patches. I have
> also updated the documentation to reflect the new changes from the QEMU
> side and rebased my changes fixing the tests in the process.
>
> What changed in QEMU post version 6.1 ?
> =========================================
>
> We have made basically two major changes in QEMU. First is this change:
>
> (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> Author: Julia Suvorova <jusual@redhat.com>
> Date: Fri Nov 12 06:08:56 2021 -0500
>
> hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
>
> There are two ways to enable ACPI PCI Hot-plug:
>
> * Disable the Hot-plug Capable bit on PCIe slots.
>
> This was the first approach which led to regression [1-2], as
> I/O space for a port is allocated only when it is hot-pluggable,
> which is determined by HPC bit.
>
> * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
> method.
>
> This removes the (future) ability of hot-plugging switches with PCIe
> Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> bridges. If the user wants to explicitely use this feature, they can
> disable ACPI PCI Hot-plug with:
> --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
>
> Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
> instead of PCIe Native.
>
> [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> The patch description says it all. Instead of masking out the HPC bit in
> pcie slots, we keep them turned on. Instead, we do not advertize native
> hotplug capability for PCIE using _OSC control method. See section
> 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> these slots so now the guest OS can select ACPI hotplug instead.
>
> The second change is introduction of a property with which we keep the
> existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> and ACPI hotplug is enabled by default for pcie root ports.
> The QEMU commit is:
>
> (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> Author: Julia Suvorova <jusual@redhat.com>
> Date: Fri Nov 12 06:08:54 2021 -0500
>
> hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type
>
> To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
> turned on, while the switch to ACPI Hot-plug will be done in the
> DSDT table.
>
> Introducing 'x-keep-native-hpc' property disables the HPC bit only
> in 6.1 and as a result keeps the forced 'reserve-io' on
> pcie-root-ports in 6.1 too.
>
> [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> mask out HPC bit in PCIE, the work done by this patch is no longer
> needed:
>
> (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Date: Mon Aug 2 12:00:57 2021 +0300
>
> hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
>
> Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> As opposed to native PCIe hotplug, guests like Fedora 34
> will not assign IO range to pcie-root-ports not supporting
> native hotplug, resulting into a regression.
>
> Reproduce by:
> qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> device_add e1000,bus=p1
> In the Guest OS the respective pcie-root-port will have the IO range
> disabled.
>
> Fix it by setting the "reserve-io" hint capability of the
> pcie-root-ports so the firmware will allocate the IO range instead.
>
> Acked-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> patch (3) since we mask out HPC bit from pcie ports.
>
>
> I know this is convoluted mess. In fairness I am trying all I can in my
> spare time to help from the QEMU side. I am determined to see this
> patchset through into libvirt.
>
> Thanks
>
> Laine's comments ...
>
> My memory isn't completely clear, but I think there was also the issue
> that the option claims to enable ACPI hotplug when set to on, but
> instead what it actually does (in the Q35 case at least) is to enable
> native PCI hotplug when set to off (without actually disabling ACPI
> hotplug) and disable native PCI hotplug when set to on, or something
> like that. This ends up leaving it up to the guest OS to decide which
> type of hotplug to use, meaning its decision could override what's in
> the libvirt config, thus confusing everyone. Again, I probably have the
> details mixed up, but it was something like this.
>
> I asked mst about this this morning, and he suggested something that
> you've already done - Cc'ing the series to qemu-devel and the relevant
> maintainers so we can have a discussion with all involved parties about
> their opinions on whether we really should expose this existing option
> in libvirt, or if we should instead have two new options that are more
> orthogonal about enabling/disabling the two types of hotplug, so that
> libvirt config can more accurately represent what is being presented to
> the guest rather than a "best guess" of what we think the guest is going
> to do with what is presented.
>
> (Michael did also say that, with the current flurry of bug reports for
> the QEMU rc's, this discusion may not happen until closer to release
> when the bug reports die down. I know this doesn't mesh with your desire
> to "push now to allow for testing" (which in general would be a good
> thing if we were certain that we wanted the option like this and were
> just expecting some minor bugs that could be fixed), but my opinion is
> that 1) it's possible for anyone interested to test the functionality
> using <qemu:commandline>, and 2) we should avoid turning libvirt git
> into a revolving door of experiments. The only practical difference
> between using <qemu:commandline> and having a dedicated option is that
> the use of <qemu:commandline> causes the domain to be tainted, and the
> XML is a bit more complicated. But since the people we're talking about
> here will already have built their own libvirt binaries, the tainted
> status of any guests is irrelevant and the extra complexity of using
> <qemu:commandline> is probably trivial to them :-).
>
>
> Ani Sinha (4):
> qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> conf: introduce support for acpi-bridge-hotplug feature
> qemu: command: add support for acpi-bridge-hotplug feature
> NEWS: document new acpi pci hotplug config option
>
> NEWS.rst | 8 ++
> docs/formatdomain.rst | 32 +++++++
> docs/schemas/domaincommon.rng | 15 ++++
> src/conf/domain_conf.c | 89 ++++++++++++++++++-
> src/conf/domain_conf.h | 9 ++
> src/qemu/qemu_capabilities.c | 4 +
> src/qemu/qemu_capabilities.h | 3 +
> src/qemu/qemu_command.c | 19 ++++
> src/qemu/qemu_validate.c | 42 +++++++++
> .../caps_6.1.0.x86_64.xml | 1 +
> .../caps_6.2.0.x86_64.xml | 1 +
> .../caps_7.0.0.x86_64.xml | 1 +
> ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> tests/qemuxml2argvtest.c | 7 ++
> ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> tests/qemuxml2xmltest.c | 4 +
> 27 files changed, 504 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
> create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
> create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
> create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
>
> --
> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 16:53 ` Ani Sinha
@ 2022-03-08 17:18 ` Michael S. Tsirkin
2022-03-09 7:17 ` Ani Sinha
0 siblings, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2022-03-08 17:18 UTC (permalink / raw)
To: Ani Sinha; +Cc: libvir-list, imammedo, jusual, qemu list, Laine Stump
On Tue, Mar 08, 2022 at 10:23:20PM +0530, Ani Sinha wrote:
> On Tue, Mar 8, 2022 at 10:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Mar 08, 2022 at 10:15:11PM +0530, Ani Sinha wrote:
> > >
> > >
> > > On Tue, 8 Mar 2022, Laine Stump wrote:
> > >
> > > > Aha! the domain of qemu-devel@nongnu.org was incorrect in the original send
> > > > (it was "nognu.org"), so none of this thread was making it to that list.
> > >
> > >
> > > Not to give any excuses but this happened because on Qemu side I never
> > > have to type this manually. My git config is set up so that
> > > the cc in send-email is filled up automatically using
> > > scripts/get_maintainer.pl. On libvirt side also the domain and mailing
> > > list is easy to remember. Its only when I have to manually type stuff that
> > > shit happens :-)
> >
> > Donnu about alpine, but with mutt you can easily set up
> > and alias and then it expands for you.
>
> I use alpine to only reply/review patches. I use git send-email to
> actually send the patch. There I am not sure the best way to avoid
> manually typing in the mailing list address.
send-email supports aliases too.
--
MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 17:18 ` Michael S. Tsirkin
@ 2022-03-09 7:17 ` Ani Sinha
2022-03-11 7:12 ` Erik Skultety
0 siblings, 1 reply; 18+ messages in thread
From: Ani Sinha @ 2022-03-09 7:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: libvir-list, jusual, qemu list, Laine Stump, Ani Sinha, imammedo
On Tue, 8 Mar 2022, Michael S. Tsirkin wrote:
> On Tue, Mar 08, 2022 at 10:23:20PM +0530, Ani Sinha wrote:
> > On Tue, Mar 8, 2022 at 10:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Mar 08, 2022 at 10:15:11PM +0530, Ani Sinha wrote:
> > > >
> > > >
> > > > On Tue, 8 Mar 2022, Laine Stump wrote:
> > > >
> > > > > Aha! the domain of qemu-devel@nongnu.org was incorrect in the original send
> > > > > (it was "nognu.org"), so none of this thread was making it to that list.
> > > >
> > > >
> > > > Not to give any excuses but this happened because on Qemu side I never
> > > > have to type this manually. My git config is set up so that
> > > > the cc in send-email is filled up automatically using
> > > > scripts/get_maintainer.pl. On libvirt side also the domain and mailing
> > > > list is easy to remember. Its only when I have to manually type stuff that
> > > > shit happens :-)
> > >
> > > Donnu about alpine, but with mutt you can easily set up
> > > and alias and then it expands for you.
> >
> > I use alpine to only reply/review patches. I use git send-email to
> > actually send the patch. There I am not sure the best way to avoid
> > manually typing in the mailing list address.
>
> send-email supports aliases too.
Ah cool. I just set this up with some help from
https://felipec.wordpress.com/2009/10/25/git-send-email-tricks/ . Now I
can simply say
$ git send-email --to=qemu-list <patch>
without worrying about typo :-) Thanks for the pointer.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-09 7:17 ` Ani Sinha
@ 2022-03-11 7:12 ` Erik Skultety
0 siblings, 0 replies; 18+ messages in thread
From: Erik Skultety @ 2022-03-11 7:12 UTC (permalink / raw)
To: Ani Sinha; +Cc: libvir-list, qemu list, Michael S. Tsirkin
On Wed, Mar 09, 2022 at 12:47:26PM +0530, Ani Sinha wrote:
>
>
> On Tue, 8 Mar 2022, Michael S. Tsirkin wrote:
>
> > On Tue, Mar 08, 2022 at 10:23:20PM +0530, Ani Sinha wrote:
> > > On Tue, Mar 8, 2022 at 10:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Mar 08, 2022 at 10:15:11PM +0530, Ani Sinha wrote:
> > > > >
> > > > >
> > > > > On Tue, 8 Mar 2022, Laine Stump wrote:
> > > > >
> > > > > > Aha! the domain of qemu-devel@nongnu.org was incorrect in the original send
> > > > > > (it was "nognu.org"), so none of this thread was making it to that list.
> > > > >
> > > > >
> > > > > Not to give any excuses but this happened because on Qemu side I never
> > > > > have to type this manually. My git config is set up so that
> > > > > the cc in send-email is filled up automatically using
> > > > > scripts/get_maintainer.pl. On libvirt side also the domain and mailing
> > > > > list is easy to remember. Its only when I have to manually type stuff that
> > > > > shit happens :-)
> > > >
> > > > Donnu about alpine, but with mutt you can easily set up
> > > > and alias and then it expands for you.
> > >
> > > I use alpine to only reply/review patches. I use git send-email to
> > > actually send the patch. There I am not sure the best way to avoid
> > > manually typing in the mailing list address.
> >
> > send-email supports aliases too.
>
> Ah cool. I just set this up with some help from
> https://felipec.wordpress.com/2009/10/25/git-send-email-tricks/ . Now I
> can simply say
>
> $ git send-email --to=qemu-list <patch>
>
> without worrying about typo :-) Thanks for the pointer.
>
So, in context of sending patches to mailing lists, especially with libvirt and
QEMU, you could utilize 'git publish'. Both QEMU and libvirt have a .gitpublish
config in the repo which sets some git-send email options, especially the
QEMU's one is nice listing a bunch of common profile templates as well.
I'd say that if you're contributing to a project which doesn't have a
.gitpublish config yet, then rather than not using 'git publish', the project
should adopt the config instead :).
Regards,
Erik
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 16:57 ` Michael S. Tsirkin
@ 2022-04-12 4:20 ` Ani Sinha
2022-04-12 4:22 ` Ani Sinha
2022-04-12 7:04 ` Michael S. Tsirkin
2022-04-22 5:25 ` Ani Sinha
1 sibling, 2 replies; 18+ messages in thread
From: Ani Sinha @ 2022-04-12 4:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: libvir-list, imammedo, jusual, qemu-devel, laine
On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote:
> >
> > Change log:
> > v2: rebased the patchset. Laine's response is appended at the end.
> >
> > I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> > reverted here few months back:
> >
> > https://www.spinics.net/linux/fedora/libvir/msg224089.html
> >
> > The reason for the reversal was that there seemed to be some
> > instability/issues around the use of the qemu commandline which this
> > patchset tries to support. In particular, some guest operating systems
> > did not like the way QEMU was trying to disable native hotplug on pcie
> > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism
> > using which we disable native hotplug. As I understand, we do not have
> > any reported issues so far in 6.2 around this area. QEMU will enter a
> > soft feature freeze in the first week of march in prep for 7.0 release.
>
> Right. But unfortunately we did not yet really work on
> a sane interface for this.
>
> The way I see it, at high level we thinkably need two flags
> - disable ACPI hotplug
> - enable native hotplug (maybe separately for pci and pcie?)
pci does not have native hotplug. so this would be applicable only for
q35. For i440fx we have two separate flags already to disable acpi
hotplug, one for root bus and another for bridges.
>
> and with both enabled guests actually can switch between
> the two.
>
> This will at least reflect the hardware, so has a chance to be
> stable.
>
> The big question however would be what is the actual use-case.
> Without that this begs the question of why do we bother at all.
To me the main motivation is as I have described here:
https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html
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.
> To allow hotplug of bridges? If it is really necessary for us then
> we should think hard about questions that surround this:
>
> - how does one hotplug a pcie switch?
> - any way to use e.g. dynamic ACPI to support hotplug of bridges?
> - do we want to bite the bullet and create an option for management
> to fully control guest memory layout including all pci devices?
>
>
>
> > Libvirt is also entering a new release cycle phaze. Hence, I am
> > introducing this patchset early enough in the release cycles so that if
> > we do see any issues on the qemu side during the rc0, rc1 cycles and if
> > reversal of this patchset is again required, it can be done in time
> > before the next libvirt release end of March.
> >
> > All the patches in this series had been previously reviewed. Some
> > subsequent fixes were made after my initial patches were pushed. I have
> > squashed all those fixes and consolidated them into four patches. I have
> > also updated the documentation to reflect the new changes from the QEMU
> > side and rebased my changes fixing the tests in the process.
> >
> > What changed in QEMU post version 6.1 ?
> > =========================================
> >
> > We have made basically two major changes in QEMU. First is this change:
> >
> > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> > Author: Julia Suvorova <jusual@redhat.com>
> > Date: Fri Nov 12 06:08:56 2021 -0500
> >
> > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
> >
> > There are two ways to enable ACPI PCI Hot-plug:
> >
> > * Disable the Hot-plug Capable bit on PCIe slots.
> >
> > This was the first approach which led to regression [1-2], as
> > I/O space for a port is allocated only when it is hot-pluggable,
> > which is determined by HPC bit.
> >
> > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
> > method.
> >
> > This removes the (future) ability of hot-plugging switches with PCIe
> > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> > bridges. If the user wants to explicitely use this feature, they can
> > disable ACPI PCI Hot-plug with:
> > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> >
> > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
> > instead of PCIe Native.
> >
> > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > The patch description says it all. Instead of masking out the HPC bit in
> > pcie slots, we keep them turned on. Instead, we do not advertize native
> > hotplug capability for PCIE using _OSC control method. See section
> > 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> > these slots so now the guest OS can select ACPI hotplug instead.
> >
> > The second change is introduction of a property with which we keep the
> > existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> > and ACPI hotplug is enabled by default for pcie root ports.
> > The QEMU commit is:
> >
> > (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> > Author: Julia Suvorova <jusual@redhat.com>
> > Date: Fri Nov 12 06:08:54 2021 -0500
> >
> > hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type
> >
> > To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
> > turned on, while the switch to ACPI Hot-plug will be done in the
> > DSDT table.
> >
> > Introducing 'x-keep-native-hpc' property disables the HPC bit only
> > in 6.1 and as a result keeps the forced 'reserve-io' on
> > pcie-root-ports in 6.1 too.
> >
> > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> > mask out HPC bit in PCIE, the work done by this patch is no longer
> > needed:
> >
> > (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> > Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > Date: Mon Aug 2 12:00:57 2021 +0300
> >
> > hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
> >
> > Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> > As opposed to native PCIe hotplug, guests like Fedora 34
> > will not assign IO range to pcie-root-ports not supporting
> > native hotplug, resulting into a regression.
> >
> > Reproduce by:
> > qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> > device_add e1000,bus=p1
> > In the Guest OS the respective pcie-root-port will have the IO range
> > disabled.
> >
> > Fix it by setting the "reserve-io" hint capability of the
> > pcie-root-ports so the firmware will allocate the IO range instead.
> >
> > Acked-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> > patch (3) since we mask out HPC bit from pcie ports.
> >
> >
> > I know this is convoluted mess. In fairness I am trying all I can in my
> > spare time to help from the QEMU side. I am determined to see this
> > patchset through into libvirt.
> >
> > Thanks
> >
> > Laine's comments ...
> >
> > My memory isn't completely clear, but I think there was also the issue
> > that the option claims to enable ACPI hotplug when set to on, but
> > instead what it actually does (in the Q35 case at least) is to enable
> > native PCI hotplug when set to off (without actually disabling ACPI
> > hotplug) and disable native PCI hotplug when set to on, or something
> > like that. This ends up leaving it up to the guest OS to decide which
> > type of hotplug to use, meaning its decision could override what's in
> > the libvirt config, thus confusing everyone. Again, I probably have the
> > details mixed up, but it was something like this.
> >
> > I asked mst about this this morning, and he suggested something that
> > you've already done - Cc'ing the series to qemu-devel and the relevant
> > maintainers so we can have a discussion with all involved parties about
> > their opinions on whether we really should expose this existing option
> > in libvirt, or if we should instead have two new options that are more
> > orthogonal about enabling/disabling the two types of hotplug, so that
> > libvirt config can more accurately represent what is being presented to
> > the guest rather than a "best guess" of what we think the guest is going
> > to do with what is presented.
> >
> > (Michael did also say that, with the current flurry of bug reports for
> > the QEMU rc's, this discusion may not happen until closer to release
> > when the bug reports die down. I know this doesn't mesh with your desire
> > to "push now to allow for testing" (which in general would be a good
> > thing if we were certain that we wanted the option like this and were
> > just expecting some minor bugs that could be fixed), but my opinion is
> > that 1) it's possible for anyone interested to test the functionality
> > using <qemu:commandline>, and 2) we should avoid turning libvirt git
> > into a revolving door of experiments. The only practical difference
> > between using <qemu:commandline> and having a dedicated option is that
> > the use of <qemu:commandline> causes the domain to be tainted, and the
> > XML is a bit more complicated. But since the people we're talking about
> > here will already have built their own libvirt binaries, the tainted
> > status of any guests is irrelevant and the extra complexity of using
> > <qemu:commandline> is probably trivial to them :-).
> >
> >
> > Ani Sinha (4):
> > qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> > conf: introduce support for acpi-bridge-hotplug feature
> > qemu: command: add support for acpi-bridge-hotplug feature
> > NEWS: document new acpi pci hotplug config option
> >
> > NEWS.rst | 8 ++
> > docs/formatdomain.rst | 32 +++++++
> > docs/schemas/domaincommon.rng | 15 ++++
> > src/conf/domain_conf.c | 89 ++++++++++++++++++-
> > src/conf/domain_conf.h | 9 ++
> > src/qemu/qemu_capabilities.c | 4 +
> > src/qemu/qemu_capabilities.h | 3 +
> > src/qemu/qemu_command.c | 19 ++++
> > src/qemu/qemu_validate.c | 42 +++++++++
> > .../caps_6.1.0.x86_64.xml | 1 +
> > .../caps_6.2.0.x86_64.xml | 1 +
> > .../caps_7.0.0.x86_64.xml | 1 +
> > ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> > .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> > ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> > .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> > .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> > ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> > ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> > .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> > .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> > tests/qemuxml2argvtest.c | 7 ++
> > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > tests/qemuxml2xmltest.c | 4 +
> > 27 files changed, 504 insertions(+), 1 deletion(-)
> > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
> > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
> > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
> >
> > --
> > 2.25.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-04-12 4:20 ` Ani Sinha
@ 2022-04-12 4:22 ` Ani Sinha
2022-04-12 7:11 ` Michael S. Tsirkin
2022-04-12 7:04 ` Michael S. Tsirkin
1 sibling, 1 reply; 18+ messages in thread
From: Ani Sinha @ 2022-04-12 4:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: libvir-list, imammedo, jusual, qemu-devel, laine
On Tue, Apr 12, 2022 at 9:50 AM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote:
> > >
> > > Change log:
> > > v2: rebased the patchset. Laine's response is appended at the end.
> > >
> > > I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> > > reverted here few months back:
> > >
> > > https://www.spinics.net/linux/fedora/libvir/msg224089.html
> > >
> > > The reason for the reversal was that there seemed to be some
> > > instability/issues around the use of the qemu commandline which this
> > > patchset tries to support. In particular, some guest operating systems
> > > did not like the way QEMU was trying to disable native hotplug on pcie
> > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism
> > > using which we disable native hotplug. As I understand, we do not have
> > > any reported issues so far in 6.2 around this area. QEMU will enter a
> > > soft feature freeze in the first week of march in prep for 7.0 release.
> >
> > Right. But unfortunately we did not yet really work on
> > a sane interface for this.
> >
> > The way I see it, at high level we thinkably need two flags
> > - disable ACPI hotplug
> > - enable native hotplug (maybe separately for pci and pcie?)
>
> pci does not have native hotplug. so this would be applicable only for
> q35. For i440fx we have two separate flags already to disable acpi
> hotplug, one for root bus and another for bridges.
>
> >
> > and with both enabled guests actually can switch between
> > the two.
> >
> > This will at least reflect the hardware, so has a chance to be
> > stable.
> >
> > The big question however would be what is the actual use-case.
> > Without that this begs the question of why do we bother at all.
>
> To me the main motivation is as I have described here:
> https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html
>
> 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.
Essentially what I do not like is that we are imposing acpi hotplug on
q35 for the entire community without giving them a choice to revert
back to native hotplug though libvirt.
>
> > To allow hotplug of bridges? If it is really necessary for us then
> > we should think hard about questions that surround this:
> >
> > - how does one hotplug a pcie switch?
> > - any way to use e.g. dynamic ACPI to support hotplug of bridges?
> > - do we want to bite the bullet and create an option for management
> > to fully control guest memory layout including all pci devices?
> >
> >
> >
> > > Libvirt is also entering a new release cycle phaze. Hence, I am
> > > introducing this patchset early enough in the release cycles so that if
> > > we do see any issues on the qemu side during the rc0, rc1 cycles and if
> > > reversal of this patchset is again required, it can be done in time
> > > before the next libvirt release end of March.
> > >
> > > All the patches in this series had been previously reviewed. Some
> > > subsequent fixes were made after my initial patches were pushed. I have
> > > squashed all those fixes and consolidated them into four patches. I have
> > > also updated the documentation to reflect the new changes from the QEMU
> > > side and rebased my changes fixing the tests in the process.
> > >
> > > What changed in QEMU post version 6.1 ?
> > > =========================================
> > >
> > > We have made basically two major changes in QEMU. First is this change:
> > >
> > > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> > > Author: Julia Suvorova <jusual@redhat.com>
> > > Date: Fri Nov 12 06:08:56 2021 -0500
> > >
> > > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
> > >
> > > There are two ways to enable ACPI PCI Hot-plug:
> > >
> > > * Disable the Hot-plug Capable bit on PCIe slots.
> > >
> > > This was the first approach which led to regression [1-2], as
> > > I/O space for a port is allocated only when it is hot-pluggable,
> > > which is determined by HPC bit.
> > >
> > > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
> > > method.
> > >
> > > This removes the (future) ability of hot-plugging switches with PCIe
> > > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> > > bridges. If the user wants to explicitely use this feature, they can
> > > disable ACPI PCI Hot-plug with:
> > > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> > >
> > > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
> > > instead of PCIe Native.
> > >
> > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > >
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > > The patch description says it all. Instead of masking out the HPC bit in
> > > pcie slots, we keep them turned on. Instead, we do not advertize native
> > > hotplug capability for PCIE using _OSC control method. See section
> > > 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> > > these slots so now the guest OS can select ACPI hotplug instead.
> > >
> > > The second change is introduction of a property with which we keep the
> > > existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> > > and ACPI hotplug is enabled by default for pcie root ports.
> > > The QEMU commit is:
> > >
> > > (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> > > Author: Julia Suvorova <jusual@redhat.com>
> > > Date: Fri Nov 12 06:08:54 2021 -0500
> > >
> > > hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type
> > >
> > > To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
> > > turned on, while the switch to ACPI Hot-plug will be done in the
> > > DSDT table.
> > >
> > > Introducing 'x-keep-native-hpc' property disables the HPC bit only
> > > in 6.1 and as a result keeps the forced 'reserve-io' on
> > > pcie-root-ports in 6.1 too.
> > >
> > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > >
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> > > mask out HPC bit in PCIE, the work done by this patch is no longer
> > > needed:
> > >
> > > (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> > > Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > Date: Mon Aug 2 12:00:57 2021 +0300
> > >
> > > hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
> > >
> > > Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> > > As opposed to native PCIe hotplug, guests like Fedora 34
> > > will not assign IO range to pcie-root-ports not supporting
> > > native hotplug, resulting into a regression.
> > >
> > > Reproduce by:
> > > qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> > > device_add e1000,bus=p1
> > > In the Guest OS the respective pcie-root-port will have the IO range
> > > disabled.
> > >
> > > Fix it by setting the "reserve-io" hint capability of the
> > > pcie-root-ports so the firmware will allocate the IO range instead.
> > >
> > > Acked-by: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > > This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> > > patch (3) since we mask out HPC bit from pcie ports.
> > >
> > >
> > > I know this is convoluted mess. In fairness I am trying all I can in my
> > > spare time to help from the QEMU side. I am determined to see this
> > > patchset through into libvirt.
> > >
> > > Thanks
> > >
> > > Laine's comments ...
> > >
> > > My memory isn't completely clear, but I think there was also the issue
> > > that the option claims to enable ACPI hotplug when set to on, but
> > > instead what it actually does (in the Q35 case at least) is to enable
> > > native PCI hotplug when set to off (without actually disabling ACPI
> > > hotplug) and disable native PCI hotplug when set to on, or something
> > > like that. This ends up leaving it up to the guest OS to decide which
> > > type of hotplug to use, meaning its decision could override what's in
> > > the libvirt config, thus confusing everyone. Again, I probably have the
> > > details mixed up, but it was something like this.
> > >
> > > I asked mst about this this morning, and he suggested something that
> > > you've already done - Cc'ing the series to qemu-devel and the relevant
> > > maintainers so we can have a discussion with all involved parties about
> > > their opinions on whether we really should expose this existing option
> > > in libvirt, or if we should instead have two new options that are more
> > > orthogonal about enabling/disabling the two types of hotplug, so that
> > > libvirt config can more accurately represent what is being presented to
> > > the guest rather than a "best guess" of what we think the guest is going
> > > to do with what is presented.
> > >
> > > (Michael did also say that, with the current flurry of bug reports for
> > > the QEMU rc's, this discusion may not happen until closer to release
> > > when the bug reports die down. I know this doesn't mesh with your desire
> > > to "push now to allow for testing" (which in general would be a good
> > > thing if we were certain that we wanted the option like this and were
> > > just expecting some minor bugs that could be fixed), but my opinion is
> > > that 1) it's possible for anyone interested to test the functionality
> > > using <qemu:commandline>, and 2) we should avoid turning libvirt git
> > > into a revolving door of experiments. The only practical difference
> > > between using <qemu:commandline> and having a dedicated option is that
> > > the use of <qemu:commandline> causes the domain to be tainted, and the
> > > XML is a bit more complicated. But since the people we're talking about
> > > here will already have built their own libvirt binaries, the tainted
> > > status of any guests is irrelevant and the extra complexity of using
> > > <qemu:commandline> is probably trivial to them :-).
> > >
> > >
> > > Ani Sinha (4):
> > > qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> > > conf: introduce support for acpi-bridge-hotplug feature
> > > qemu: command: add support for acpi-bridge-hotplug feature
> > > NEWS: document new acpi pci hotplug config option
> > >
> > > NEWS.rst | 8 ++
> > > docs/formatdomain.rst | 32 +++++++
> > > docs/schemas/domaincommon.rng | 15 ++++
> > > src/conf/domain_conf.c | 89 ++++++++++++++++++-
> > > src/conf/domain_conf.h | 9 ++
> > > src/qemu/qemu_capabilities.c | 4 +
> > > src/qemu/qemu_capabilities.h | 3 +
> > > src/qemu/qemu_command.c | 19 ++++
> > > src/qemu/qemu_validate.c | 42 +++++++++
> > > .../caps_6.1.0.x86_64.xml | 1 +
> > > .../caps_6.2.0.x86_64.xml | 1 +
> > > .../caps_7.0.0.x86_64.xml | 1 +
> > > ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> > > .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> > > ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> > > .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> > > .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> > > ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> > > ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> > > .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> > > .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> > > tests/qemuxml2argvtest.c | 7 ++
> > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > tests/qemuxml2xmltest.c | 4 +
> > > 27 files changed, 504 insertions(+), 1 deletion(-)
> > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
> > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > >
> > > --
> > > 2.25.1
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-04-12 4:20 ` Ani Sinha
2022-04-12 4:22 ` Ani Sinha
@ 2022-04-12 7:04 ` Michael S. Tsirkin
2022-04-13 11:16 ` Ani Sinha
1 sibling, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2022-04-12 7:04 UTC (permalink / raw)
To: Ani Sinha; +Cc: libvir-list, imammedo, jusual, qemu-devel, laine
On Tue, Apr 12, 2022 at 09:50:15AM +0530, Ani Sinha wrote:
> On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote:
> > >
> > > Change log:
> > > v2: rebased the patchset. Laine's response is appended at the end.
> > >
> > > I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> > > reverted here few months back:
> > >
> > > https://www.spinics.net/linux/fedora/libvir/msg224089.html
> > >
> > > The reason for the reversal was that there seemed to be some
> > > instability/issues around the use of the qemu commandline which this
> > > patchset tries to support. In particular, some guest operating systems
> > > did not like the way QEMU was trying to disable native hotplug on pcie
> > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism
> > > using which we disable native hotplug. As I understand, we do not have
> > > any reported issues so far in 6.2 around this area. QEMU will enter a
> > > soft feature freeze in the first week of march in prep for 7.0 release.
> >
> > Right. But unfortunately we did not yet really work on
> > a sane interface for this.
> >
> > The way I see it, at high level we thinkably need two flags
> > - disable ACPI hotplug
> > - enable native hotplug (maybe separately for pci and pcie?)
>
> pci does not have native hotplug.
shpc?
> so this would be applicable only for
> q35. For i440fx we have two separate flags already to disable acpi
> hotplug, one for root bus and another for bridges.
>
> >
> > and with both enabled guests actually can switch between
> > the two.
> >
> > This will at least reflect the hardware, so has a chance to be
> > stable.
> >
> > The big question however would be what is the actual use-case.
> > Without that this begs the question of why do we bother at all.
>
> To me the main motivation is as I have described here:
> https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html
>
> 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.
To work around bugs then.
> > To allow hotplug of bridges? If it is really necessary for us then
> > we should think hard about questions that surround this:
> >
> > - how does one hotplug a pcie switch?
> > - any way to use e.g. dynamic ACPI to support hotplug of bridges?
> > - do we want to bite the bullet and create an option for management
> > to fully control guest memory layout including all pci devices?
> >
> >
> >
> > > Libvirt is also entering a new release cycle phaze. Hence, I am
> > > introducing this patchset early enough in the release cycles so that if
> > > we do see any issues on the qemu side during the rc0, rc1 cycles and if
> > > reversal of this patchset is again required, it can be done in time
> > > before the next libvirt release end of March.
> > >
> > > All the patches in this series had been previously reviewed. Some
> > > subsequent fixes were made after my initial patches were pushed. I have
> > > squashed all those fixes and consolidated them into four patches. I have
> > > also updated the documentation to reflect the new changes from the QEMU
> > > side and rebased my changes fixing the tests in the process.
> > >
> > > What changed in QEMU post version 6.1 ?
> > > =========================================
> > >
> > > We have made basically two major changes in QEMU. First is this change:
> > >
> > > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> > > Author: Julia Suvorova <jusual@redhat.com>
> > > Date: Fri Nov 12 06:08:56 2021 -0500
> > >
> > > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
> > >
> > > There are two ways to enable ACPI PCI Hot-plug:
> > >
> > > * Disable the Hot-plug Capable bit on PCIe slots.
> > >
> > > This was the first approach which led to regression [1-2], as
> > > I/O space for a port is allocated only when it is hot-pluggable,
> > > which is determined by HPC bit.
> > >
> > > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
> > > method.
> > >
> > > This removes the (future) ability of hot-plugging switches with PCIe
> > > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> > > bridges. If the user wants to explicitely use this feature, they can
> > > disable ACPI PCI Hot-plug with:
> > > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> > >
> > > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
> > > instead of PCIe Native.
> > >
> > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > >
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > > The patch description says it all. Instead of masking out the HPC bit in
> > > pcie slots, we keep them turned on. Instead, we do not advertize native
> > > hotplug capability for PCIE using _OSC control method. See section
> > > 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> > > these slots so now the guest OS can select ACPI hotplug instead.
> > >
> > > The second change is introduction of a property with which we keep the
> > > existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> > > and ACPI hotplug is enabled by default for pcie root ports.
> > > The QEMU commit is:
> > >
> > > (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> > > Author: Julia Suvorova <jusual@redhat.com>
> > > Date: Fri Nov 12 06:08:54 2021 -0500
> > >
> > > hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type
> > >
> > > To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
> > > turned on, while the switch to ACPI Hot-plug will be done in the
> > > DSDT table.
> > >
> > > Introducing 'x-keep-native-hpc' property disables the HPC bit only
> > > in 6.1 and as a result keeps the forced 'reserve-io' on
> > > pcie-root-ports in 6.1 too.
> > >
> > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > >
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> > > mask out HPC bit in PCIE, the work done by this patch is no longer
> > > needed:
> > >
> > > (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> > > Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > Date: Mon Aug 2 12:00:57 2021 +0300
> > >
> > > hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
> > >
> > > Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> > > As opposed to native PCIe hotplug, guests like Fedora 34
> > > will not assign IO range to pcie-root-ports not supporting
> > > native hotplug, resulting into a regression.
> > >
> > > Reproduce by:
> > > qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> > > device_add e1000,bus=p1
> > > In the Guest OS the respective pcie-root-port will have the IO range
> > > disabled.
> > >
> > > Fix it by setting the "reserve-io" hint capability of the
> > > pcie-root-ports so the firmware will allocate the IO range instead.
> > >
> > > Acked-by: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > > This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> > > patch (3) since we mask out HPC bit from pcie ports.
> > >
> > >
> > > I know this is convoluted mess. In fairness I am trying all I can in my
> > > spare time to help from the QEMU side. I am determined to see this
> > > patchset through into libvirt.
> > >
> > > Thanks
> > >
> > > Laine's comments ...
> > >
> > > My memory isn't completely clear, but I think there was also the issue
> > > that the option claims to enable ACPI hotplug when set to on, but
> > > instead what it actually does (in the Q35 case at least) is to enable
> > > native PCI hotplug when set to off (without actually disabling ACPI
> > > hotplug) and disable native PCI hotplug when set to on, or something
> > > like that. This ends up leaving it up to the guest OS to decide which
> > > type of hotplug to use, meaning its decision could override what's in
> > > the libvirt config, thus confusing everyone. Again, I probably have the
> > > details mixed up, but it was something like this.
> > >
> > > I asked mst about this this morning, and he suggested something that
> > > you've already done - Cc'ing the series to qemu-devel and the relevant
> > > maintainers so we can have a discussion with all involved parties about
> > > their opinions on whether we really should expose this existing option
> > > in libvirt, or if we should instead have two new options that are more
> > > orthogonal about enabling/disabling the two types of hotplug, so that
> > > libvirt config can more accurately represent what is being presented to
> > > the guest rather than a "best guess" of what we think the guest is going
> > > to do with what is presented.
> > >
> > > (Michael did also say that, with the current flurry of bug reports for
> > > the QEMU rc's, this discusion may not happen until closer to release
> > > when the bug reports die down. I know this doesn't mesh with your desire
> > > to "push now to allow for testing" (which in general would be a good
> > > thing if we were certain that we wanted the option like this and were
> > > just expecting some minor bugs that could be fixed), but my opinion is
> > > that 1) it's possible for anyone interested to test the functionality
> > > using <qemu:commandline>, and 2) we should avoid turning libvirt git
> > > into a revolving door of experiments. The only practical difference
> > > between using <qemu:commandline> and having a dedicated option is that
> > > the use of <qemu:commandline> causes the domain to be tainted, and the
> > > XML is a bit more complicated. But since the people we're talking about
> > > here will already have built their own libvirt binaries, the tainted
> > > status of any guests is irrelevant and the extra complexity of using
> > > <qemu:commandline> is probably trivial to them :-).
> > >
> > >
> > > Ani Sinha (4):
> > > qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> > > conf: introduce support for acpi-bridge-hotplug feature
> > > qemu: command: add support for acpi-bridge-hotplug feature
> > > NEWS: document new acpi pci hotplug config option
> > >
> > > NEWS.rst | 8 ++
> > > docs/formatdomain.rst | 32 +++++++
> > > docs/schemas/domaincommon.rng | 15 ++++
> > > src/conf/domain_conf.c | 89 ++++++++++++++++++-
> > > src/conf/domain_conf.h | 9 ++
> > > src/qemu/qemu_capabilities.c | 4 +
> > > src/qemu/qemu_capabilities.h | 3 +
> > > src/qemu/qemu_command.c | 19 ++++
> > > src/qemu/qemu_validate.c | 42 +++++++++
> > > .../caps_6.1.0.x86_64.xml | 1 +
> > > .../caps_6.2.0.x86_64.xml | 1 +
> > > .../caps_7.0.0.x86_64.xml | 1 +
> > > ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> > > .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> > > ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> > > .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> > > .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> > > ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> > > ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> > > .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> > > .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> > > tests/qemuxml2argvtest.c | 7 ++
> > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > tests/qemuxml2xmltest.c | 4 +
> > > 27 files changed, 504 insertions(+), 1 deletion(-)
> > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
> > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > >
> > > --
> > > 2.25.1
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-04-12 4:22 ` Ani Sinha
@ 2022-04-12 7:11 ` Michael S. Tsirkin
2022-04-13 11:18 ` Ani Sinha
0 siblings, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2022-04-12 7:11 UTC (permalink / raw)
To: Ani Sinha; +Cc: libvir-list, imammedo, jusual, qemu-devel, laine
On Tue, Apr 12, 2022 at 09:52:26AM +0530, Ani Sinha wrote:
> On Tue, Apr 12, 2022 at 9:50 AM Ani Sinha <ani@anisinha.ca> wrote:
> >
> > On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote:
> > > >
> > > > Change log:
> > > > v2: rebased the patchset. Laine's response is appended at the end.
> > > >
> > > > I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> > > > reverted here few months back:
> > > >
> > > > https://www.spinics.net/linux/fedora/libvir/msg224089.html
> > > >
> > > > The reason for the reversal was that there seemed to be some
> > > > instability/issues around the use of the qemu commandline which this
> > > > patchset tries to support. In particular, some guest operating systems
> > > > did not like the way QEMU was trying to disable native hotplug on pcie
> > > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism
> > > > using which we disable native hotplug. As I understand, we do not have
> > > > any reported issues so far in 6.2 around this area. QEMU will enter a
> > > > soft feature freeze in the first week of march in prep for 7.0 release.
> > >
> > > Right. But unfortunately we did not yet really work on
> > > a sane interface for this.
> > >
> > > The way I see it, at high level we thinkably need two flags
> > > - disable ACPI hotplug
> > > - enable native hotplug (maybe separately for pci and pcie?)
I still think this is the case.
> > pci does not have native hotplug. so this would be applicable only for
> > q35. For i440fx we have two separate flags already to disable acpi
> > hotplug, one for root bus and another for bridges.
> >
> > >
> > > and with both enabled guests actually can switch between
> > > the two.
> > >
> > > This will at least reflect the hardware, so has a chance to be
> > > stable.
> > >
> > > The big question however would be what is the actual use-case.
> > > Without that this begs the question of why do we bother at all.
> >
> > To me the main motivation is as I have described here:
> > https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html
> >
> > 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
This one was fixed, right?
> > 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.
>
> Essentially what I do not like is that we are imposing acpi hotplug on
> q35 for the entire community without giving them a choice to revert
> back to native hotplug though libvirt.
The reason qemu did it is because it was expected it's more or less
transparent. Barring bugs bug hey, there's always bugs with any change.
> >
> > > To allow hotplug of bridges? If it is really necessary for us then
> > > we should think hard about questions that surround this:
> > >
> > > - how does one hotplug a pcie switch?
> > > - any way to use e.g. dynamic ACPI to support hotplug of bridges?
> > > - do we want to bite the bullet and create an option for management
> > > to fully control guest memory layout including all pci devices?
> > >
> > >
> > >
> > > > Libvirt is also entering a new release cycle phaze. Hence, I am
> > > > introducing this patchset early enough in the release cycles so that if
> > > > we do see any issues on the qemu side during the rc0, rc1 cycles and if
> > > > reversal of this patchset is again required, it can be done in time
> > > > before the next libvirt release end of March.
> > > >
> > > > All the patches in this series had been previously reviewed. Some
> > > > subsequent fixes were made after my initial patches were pushed. I have
> > > > squashed all those fixes and consolidated them into four patches. I have
> > > > also updated the documentation to reflect the new changes from the QEMU
> > > > side and rebased my changes fixing the tests in the process.
> > > >
> > > > What changed in QEMU post version 6.1 ?
> > > > =========================================
> > > >
> > > > We have made basically two major changes in QEMU. First is this change:
> > > >
> > > > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> > > > Author: Julia Suvorova <jusual@redhat.com>
> > > > Date: Fri Nov 12 06:08:56 2021 -0500
> > > >
> > > > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
> > > >
> > > > There are two ways to enable ACPI PCI Hot-plug:
> > > >
> > > > * Disable the Hot-plug Capable bit on PCIe slots.
> > > >
> > > > This was the first approach which led to regression [1-2], as
> > > > I/O space for a port is allocated only when it is hot-pluggable,
> > > > which is determined by HPC bit.
> > > >
> > > > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
> > > > method.
> > > >
> > > > This removes the (future) ability of hot-plugging switches with PCIe
> > > > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> > > > bridges. If the user wants to explicitely use this feature, they can
> > > > disable ACPI PCI Hot-plug with:
> > > > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> > > >
> > > > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
> > > > instead of PCIe Native.
> > > >
> > > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > > >
> > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > >
> > > > The patch description says it all. Instead of masking out the HPC bit in
> > > > pcie slots, we keep them turned on. Instead, we do not advertize native
> > > > hotplug capability for PCIE using _OSC control method. See section
> > > > 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> > > > these slots so now the guest OS can select ACPI hotplug instead.
> > > >
> > > > The second change is introduction of a property with which we keep the
> > > > existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> > > > and ACPI hotplug is enabled by default for pcie root ports.
> > > > The QEMU commit is:
> > > >
> > > > (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> > > > Author: Julia Suvorova <jusual@redhat.com>
> > > > Date: Fri Nov 12 06:08:54 2021 -0500
> > > >
> > > > hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type
> > > >
> > > > To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
> > > > turned on, while the switch to ACPI Hot-plug will be done in the
> > > > DSDT table.
> > > >
> > > > Introducing 'x-keep-native-hpc' property disables the HPC bit only
> > > > in 6.1 and as a result keeps the forced 'reserve-io' on
> > > > pcie-root-ports in 6.1 too.
> > > >
> > > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > > >
> > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > > Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> > > > mask out HPC bit in PCIE, the work done by this patch is no longer
> > > > needed:
> > > >
> > > > (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> > > > Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > > Date: Mon Aug 2 12:00:57 2021 +0300
> > > >
> > > > hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
> > > >
> > > > Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> > > > As opposed to native PCIe hotplug, guests like Fedora 34
> > > > will not assign IO range to pcie-root-ports not supporting
> > > > native hotplug, resulting into a regression.
> > > >
> > > > Reproduce by:
> > > > qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> > > > device_add e1000,bus=p1
> > > > In the Guest OS the respective pcie-root-port will have the IO range
> > > > disabled.
> > > >
> > > > Fix it by setting the "reserve-io" hint capability of the
> > > > pcie-root-ports so the firmware will allocate the IO range instead.
> > > >
> > > > Acked-by: Igor Mammedov <imammedo@redhat.com>
> > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > > Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > >
> > > > This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> > > > patch (3) since we mask out HPC bit from pcie ports.
> > > >
> > > >
> > > > I know this is convoluted mess. In fairness I am trying all I can in my
> > > > spare time to help from the QEMU side. I am determined to see this
> > > > patchset through into libvirt.
> > > >
> > > > Thanks
> > > >
> > > > Laine's comments ...
> > > >
> > > > My memory isn't completely clear, but I think there was also the issue
> > > > that the option claims to enable ACPI hotplug when set to on, but
> > > > instead what it actually does (in the Q35 case at least) is to enable
> > > > native PCI hotplug when set to off (without actually disabling ACPI
> > > > hotplug) and disable native PCI hotplug when set to on, or something
> > > > like that. This ends up leaving it up to the guest OS to decide which
> > > > type of hotplug to use, meaning its decision could override what's in
> > > > the libvirt config, thus confusing everyone. Again, I probably have the
> > > > details mixed up, but it was something like this.
> > > >
> > > > I asked mst about this this morning, and he suggested something that
> > > > you've already done - Cc'ing the series to qemu-devel and the relevant
> > > > maintainers so we can have a discussion with all involved parties about
> > > > their opinions on whether we really should expose this existing option
> > > > in libvirt, or if we should instead have two new options that are more
> > > > orthogonal about enabling/disabling the two types of hotplug, so that
> > > > libvirt config can more accurately represent what is being presented to
> > > > the guest rather than a "best guess" of what we think the guest is going
> > > > to do with what is presented.
> > > >
> > > > (Michael did also say that, with the current flurry of bug reports for
> > > > the QEMU rc's, this discusion may not happen until closer to release
> > > > when the bug reports die down. I know this doesn't mesh with your desire
> > > > to "push now to allow for testing" (which in general would be a good
> > > > thing if we were certain that we wanted the option like this and were
> > > > just expecting some minor bugs that could be fixed), but my opinion is
> > > > that 1) it's possible for anyone interested to test the functionality
> > > > using <qemu:commandline>, and 2) we should avoid turning libvirt git
> > > > into a revolving door of experiments. The only practical difference
> > > > between using <qemu:commandline> and having a dedicated option is that
> > > > the use of <qemu:commandline> causes the domain to be tainted, and the
> > > > XML is a bit more complicated. But since the people we're talking about
> > > > here will already have built their own libvirt binaries, the tainted
> > > > status of any guests is irrelevant and the extra complexity of using
> > > > <qemu:commandline> is probably trivial to them :-).
> > > >
> > > >
> > > > Ani Sinha (4):
> > > > qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> > > > conf: introduce support for acpi-bridge-hotplug feature
> > > > qemu: command: add support for acpi-bridge-hotplug feature
> > > > NEWS: document new acpi pci hotplug config option
> > > >
> > > > NEWS.rst | 8 ++
> > > > docs/formatdomain.rst | 32 +++++++
> > > > docs/schemas/domaincommon.rng | 15 ++++
> > > > src/conf/domain_conf.c | 89 ++++++++++++++++++-
> > > > src/conf/domain_conf.h | 9 ++
> > > > src/qemu/qemu_capabilities.c | 4 +
> > > > src/qemu/qemu_capabilities.h | 3 +
> > > > src/qemu/qemu_command.c | 19 ++++
> > > > src/qemu/qemu_validate.c | 42 +++++++++
> > > > .../caps_6.1.0.x86_64.xml | 1 +
> > > > .../caps_6.2.0.x86_64.xml | 1 +
> > > > .../caps_7.0.0.x86_64.xml | 1 +
> > > > ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> > > > .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> > > > ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> > > > .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> > > > .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> > > > ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> > > > ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> > > > .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> > > > .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> > > > tests/qemuxml2argvtest.c | 7 ++
> > > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > > tests/qemuxml2xmltest.c | 4 +
> > > > 27 files changed, 504 insertions(+), 1 deletion(-)
> > > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
> > > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> > > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> > > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> > > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > > >
> > > > --
> > > > 2.25.1
> > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-04-12 7:04 ` Michael S. Tsirkin
@ 2022-04-13 11:16 ` Ani Sinha
0 siblings, 0 replies; 18+ messages in thread
From: Ani Sinha @ 2022-04-13 11:16 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: libvir-list, imammedo, jusual, qemu-devel, laine
On Tue, Apr 12, 2022 at 12:34 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 12, 2022 at 09:50:15AM +0530, Ani Sinha wrote:
> > On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote:
> > > >
> > > > Change log:
> > > > v2: rebased the patchset. Laine's response is appended at the end.
> > > >
> > > > I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> > > > reverted here few months back:
> > > >
> > > > https://www.spinics.net/linux/fedora/libvir/msg224089.html
> > > >
> > > > The reason for the reversal was that there seemed to be some
> > > > instability/issues around the use of the qemu commandline which this
> > > > patchset tries to support. In particular, some guest operating systems
> > > > did not like the way QEMU was trying to disable native hotplug on pcie
> > > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism
> > > > using which we disable native hotplug. As I understand, we do not have
> > > > any reported issues so far in 6.2 around this area. QEMU will enter a
> > > > soft feature freeze in the first week of march in prep for 7.0 release.
> > >
> > > Right. But unfortunately we did not yet really work on
> > > a sane interface for this.
> > >
> > > The way I see it, at high level we thinkably need two flags
> > > - disable ACPI hotplug
> > > - enable native hotplug (maybe separately for pci and pcie?)
> >
> > pci does not have native hotplug.
>
> shpc?
>
> > so this would be applicable only for
> > q35. For i440fx we have two separate flags already to disable acpi
> > hotplug, one for root bus and another for bridges.
> >
> > >
> > > and with both enabled guests actually can switch between
> > > the two.
> > >
> > > This will at least reflect the hardware, so has a chance to be
> > > stable.
> > >
> > > The big question however would be what is the actual use-case.
> > > Without that this begs the question of why do we bother at all.
> >
> > To me the main motivation is as I have described here:
> > https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html
> >
> > 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.
>
> To work around bugs then.
Bugs that we might have not discovered yet. Let's look at end user
scenario. Users might have spent enormous QA time to stabilize and
test hotplug using pcie native. pcie native has been around for a
while and has been thus getting tested widely. acpi was recently
introduced. I think we should at least give end users an option to opt
out of acpi hotplug if they wanted to. Also opt out gracefully,
through a mechanism from libvirt other than passthrough qemu
commandline.
>
> > > To allow hotplug of bridges? If it is really necessary for us then
> > > we should think hard about questions that surround this:
> > >
> > > - how does one hotplug a pcie switch?
> > > - any way to use e.g. dynamic ACPI to support hotplug of bridges?
> > > - do we want to bite the bullet and create an option for management
> > > to fully control guest memory layout including all pci devices?
> > >
> > >
> > >
> > > > Libvirt is also entering a new release cycle phaze. Hence, I am
> > > > introducing this patchset early enough in the release cycles so that if
> > > > we do see any issues on the qemu side during the rc0, rc1 cycles and if
> > > > reversal of this patchset is again required, it can be done in time
> > > > before the next libvirt release end of March.
> > > >
> > > > All the patches in this series had been previously reviewed. Some
> > > > subsequent fixes were made after my initial patches were pushed. I have
> > > > squashed all those fixes and consolidated them into four patches. I have
> > > > also updated the documentation to reflect the new changes from the QEMU
> > > > side and rebased my changes fixing the tests in the process.
> > > >
> > > > What changed in QEMU post version 6.1 ?
> > > > =========================================
> > > >
> > > > We have made basically two major changes in QEMU. First is this change:
> > > >
> > > > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> > > > Author: Julia Suvorova <jusual@redhat.com>
> > > > Date: Fri Nov 12 06:08:56 2021 -0500
> > > >
> > > > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
> > > >
> > > > There are two ways to enable ACPI PCI Hot-plug:
> > > >
> > > > * Disable the Hot-plug Capable bit on PCIe slots.
> > > >
> > > > This was the first approach which led to regression [1-2], as
> > > > I/O space for a port is allocated only when it is hot-pluggable,
> > > > which is determined by HPC bit.
> > > >
> > > > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
> > > > method.
> > > >
> > > > This removes the (future) ability of hot-plugging switches with PCIe
> > > > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> > > > bridges. If the user wants to explicitely use this feature, they can
> > > > disable ACPI PCI Hot-plug with:
> > > > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> > > >
> > > > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
> > > > instead of PCIe Native.
> > > >
> > > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > > >
> > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > >
> > > > The patch description says it all. Instead of masking out the HPC bit in
> > > > pcie slots, we keep them turned on. Instead, we do not advertize native
> > > > hotplug capability for PCIE using _OSC control method. See section
> > > > 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> > > > these slots so now the guest OS can select ACPI hotplug instead.
> > > >
> > > > The second change is introduction of a property with which we keep the
> > > > existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> > > > and ACPI hotplug is enabled by default for pcie root ports.
> > > > The QEMU commit is:
> > > >
> > > > (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> > > > Author: Julia Suvorova <jusual@redhat.com>
> > > > Date: Fri Nov 12 06:08:54 2021 -0500
> > > >
> > > > hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type
> > > >
> > > > To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
> > > > turned on, while the switch to ACPI Hot-plug will be done in the
> > > > DSDT table.
> > > >
> > > > Introducing 'x-keep-native-hpc' property disables the HPC bit only
> > > > in 6.1 and as a result keeps the forced 'reserve-io' on
> > > > pcie-root-ports in 6.1 too.
> > > >
> > > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > > >
> > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > > Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> > > > mask out HPC bit in PCIE, the work done by this patch is no longer
> > > > needed:
> > > >
> > > > (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> > > > Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > > Date: Mon Aug 2 12:00:57 2021 +0300
> > > >
> > > > hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
> > > >
> > > > Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> > > > As opposed to native PCIe hotplug, guests like Fedora 34
> > > > will not assign IO range to pcie-root-ports not supporting
> > > > native hotplug, resulting into a regression.
> > > >
> > > > Reproduce by:
> > > > qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> > > > device_add e1000,bus=p1
> > > > In the Guest OS the respective pcie-root-port will have the IO range
> > > > disabled.
> > > >
> > > > Fix it by setting the "reserve-io" hint capability of the
> > > > pcie-root-ports so the firmware will allocate the IO range instead.
> > > >
> > > > Acked-by: Igor Mammedov <imammedo@redhat.com>
> > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > > Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > >
> > > > This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> > > > patch (3) since we mask out HPC bit from pcie ports.
> > > >
> > > >
> > > > I know this is convoluted mess. In fairness I am trying all I can in my
> > > > spare time to help from the QEMU side. I am determined to see this
> > > > patchset through into libvirt.
> > > >
> > > > Thanks
> > > >
> > > > Laine's comments ...
> > > >
> > > > My memory isn't completely clear, but I think there was also the issue
> > > > that the option claims to enable ACPI hotplug when set to on, but
> > > > instead what it actually does (in the Q35 case at least) is to enable
> > > > native PCI hotplug when set to off (without actually disabling ACPI
> > > > hotplug) and disable native PCI hotplug when set to on, or something
> > > > like that. This ends up leaving it up to the guest OS to decide which
> > > > type of hotplug to use, meaning its decision could override what's in
> > > > the libvirt config, thus confusing everyone. Again, I probably have the
> > > > details mixed up, but it was something like this.
> > > >
> > > > I asked mst about this this morning, and he suggested something that
> > > > you've already done - Cc'ing the series to qemu-devel and the relevant
> > > > maintainers so we can have a discussion with all involved parties about
> > > > their opinions on whether we really should expose this existing option
> > > > in libvirt, or if we should instead have two new options that are more
> > > > orthogonal about enabling/disabling the two types of hotplug, so that
> > > > libvirt config can more accurately represent what is being presented to
> > > > the guest rather than a "best guess" of what we think the guest is going
> > > > to do with what is presented.
> > > >
> > > > (Michael did also say that, with the current flurry of bug reports for
> > > > the QEMU rc's, this discusion may not happen until closer to release
> > > > when the bug reports die down. I know this doesn't mesh with your desire
> > > > to "push now to allow for testing" (which in general would be a good
> > > > thing if we were certain that we wanted the option like this and were
> > > > just expecting some minor bugs that could be fixed), but my opinion is
> > > > that 1) it's possible for anyone interested to test the functionality
> > > > using <qemu:commandline>, and 2) we should avoid turning libvirt git
> > > > into a revolving door of experiments. The only practical difference
> > > > between using <qemu:commandline> and having a dedicated option is that
> > > > the use of <qemu:commandline> causes the domain to be tainted, and the
> > > > XML is a bit more complicated. But since the people we're talking about
> > > > here will already have built their own libvirt binaries, the tainted
> > > > status of any guests is irrelevant and the extra complexity of using
> > > > <qemu:commandline> is probably trivial to them :-).
> > > >
> > > >
> > > > Ani Sinha (4):
> > > > qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> > > > conf: introduce support for acpi-bridge-hotplug feature
> > > > qemu: command: add support for acpi-bridge-hotplug feature
> > > > NEWS: document new acpi pci hotplug config option
> > > >
> > > > NEWS.rst | 8 ++
> > > > docs/formatdomain.rst | 32 +++++++
> > > > docs/schemas/domaincommon.rng | 15 ++++
> > > > src/conf/domain_conf.c | 89 ++++++++++++++++++-
> > > > src/conf/domain_conf.h | 9 ++
> > > > src/qemu/qemu_capabilities.c | 4 +
> > > > src/qemu/qemu_capabilities.h | 3 +
> > > > src/qemu/qemu_command.c | 19 ++++
> > > > src/qemu/qemu_validate.c | 42 +++++++++
> > > > .../caps_6.1.0.x86_64.xml | 1 +
> > > > .../caps_6.2.0.x86_64.xml | 1 +
> > > > .../caps_7.0.0.x86_64.xml | 1 +
> > > > ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> > > > .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> > > > ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> > > > .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> > > > .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> > > > ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> > > > ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> > > > .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> > > > .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> > > > tests/qemuxml2argvtest.c | 7 ++
> > > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > > tests/qemuxml2xmltest.c | 4 +
> > > > 27 files changed, 504 insertions(+), 1 deletion(-)
> > > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
> > > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> > > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> > > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> > > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > > >
> > > > --
> > > > 2.25.1
> > >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-04-12 7:11 ` Michael S. Tsirkin
@ 2022-04-13 11:18 ` Ani Sinha
0 siblings, 0 replies; 18+ messages in thread
From: Ani Sinha @ 2022-04-13 11:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: libvir-list, imammedo, jusual, qemu-devel, laine
On Tue, Apr 12, 2022 at 12:41 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 12, 2022 at 09:52:26AM +0530, Ani Sinha wrote:
> > On Tue, Apr 12, 2022 at 9:50 AM Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > > On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote:
> > > > >
> > > > > Change log:
> > > > > v2: rebased the patchset. Laine's response is appended at the end.
> > > > >
> > > > > I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> > > > > reverted here few months back:
> > > > >
> > > > > https://www.spinics.net/linux/fedora/libvir/msg224089.html
> > > > >
> > > > > The reason for the reversal was that there seemed to be some
> > > > > instability/issues around the use of the qemu commandline which this
> > > > > patchset tries to support. In particular, some guest operating systems
> > > > > did not like the way QEMU was trying to disable native hotplug on pcie
> > > > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism
> > > > > using which we disable native hotplug. As I understand, we do not have
> > > > > any reported issues so far in 6.2 around this area. QEMU will enter a
> > > > > soft feature freeze in the first week of march in prep for 7.0 release.
> > > >
> > > > Right. But unfortunately we did not yet really work on
> > > > a sane interface for this.
> > > >
> > > > The way I see it, at high level we thinkably need two flags
> > > > - disable ACPI hotplug
> > > > - enable native hotplug (maybe separately for pci and pcie?)
>
> I still think this is the case.
>
> > > pci does not have native hotplug. so this would be applicable only for
> > > q35. For i440fx we have two separate flags already to disable acpi
> > > hotplug, one for root bus and another for bridges.
> > >
> > > >
> > > > and with both enabled guests actually can switch between
> > > > the two.
> > > >
> > > > This will at least reflect the hardware, so has a chance to be
> > > > stable.
> > > >
> > > > The big question however would be what is the actual use-case.
> > > > Without that this begs the question of why do we bother at all.
> > >
> > > To me the main motivation is as I have described here:
> > > https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html
> > >
> > > 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
>
> This one was fixed, right?
yes
>
>
> > > 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.
> >
> > Essentially what I do not like is that we are imposing acpi hotplug on
> > q35 for the entire community without giving them a choice to revert
> > back to native hotplug though libvirt.
>
> The reason qemu did it is because it was expected it's more or less
> transparent. Barring bugs bug hey, there's always bugs with any change.
Right and it takes time to say confidently that we have ironed out
almost all the issues.
>
> > >
> > > > To allow hotplug of bridges? If it is really necessary for us then
> > > > we should think hard about questions that surround this:
> > > >
> > > > - how does one hotplug a pcie switch?
> > > > - any way to use e.g. dynamic ACPI to support hotplug of bridges?
> > > > - do we want to bite the bullet and create an option for management
> > > > to fully control guest memory layout including all pci devices?
> > > >
> > > >
> > > >
> > > > > Libvirt is also entering a new release cycle phaze. Hence, I am
> > > > > introducing this patchset early enough in the release cycles so that if
> > > > > we do see any issues on the qemu side during the rc0, rc1 cycles and if
> > > > > reversal of this patchset is again required, it can be done in time
> > > > > before the next libvirt release end of March.
> > > > >
> > > > > All the patches in this series had been previously reviewed. Some
> > > > > subsequent fixes were made after my initial patches were pushed. I have
> > > > > squashed all those fixes and consolidated them into four patches. I have
> > > > > also updated the documentation to reflect the new changes from the QEMU
> > > > > side and rebased my changes fixing the tests in the process.
> > > > >
> > > > > What changed in QEMU post version 6.1 ?
> > > > > =========================================
> > > > >
> > > > > We have made basically two major changes in QEMU. First is this change:
> > > > >
> > > > > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> > > > > Author: Julia Suvorova <jusual@redhat.com>
> > > > > Date: Fri Nov 12 06:08:56 2021 -0500
> > > > >
> > > > > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
> > > > >
> > > > > There are two ways to enable ACPI PCI Hot-plug:
> > > > >
> > > > > * Disable the Hot-plug Capable bit on PCIe slots.
> > > > >
> > > > > This was the first approach which led to regression [1-2], as
> > > > > I/O space for a port is allocated only when it is hot-pluggable,
> > > > > which is determined by HPC bit.
> > > > >
> > > > > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
> > > > > method.
> > > > >
> > > > > This removes the (future) ability of hot-plugging switches with PCIe
> > > > > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> > > > > bridges. If the user wants to explicitely use this feature, they can
> > > > > disable ACPI PCI Hot-plug with:
> > > > > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> > > > >
> > > > > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
> > > > > instead of PCIe Native.
> > > > >
> > > > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > > > >
> > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> > > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > >
> > > > >
> > > > > The patch description says it all. Instead of masking out the HPC bit in
> > > > > pcie slots, we keep them turned on. Instead, we do not advertize native
> > > > > hotplug capability for PCIE using _OSC control method. See section
> > > > > 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> > > > > these slots so now the guest OS can select ACPI hotplug instead.
> > > > >
> > > > > The second change is introduction of a property with which we keep the
> > > > > existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> > > > > and ACPI hotplug is enabled by default for pcie root ports.
> > > > > The QEMU commit is:
> > > > >
> > > > > (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> > > > > Author: Julia Suvorova <jusual@redhat.com>
> > > > > Date: Fri Nov 12 06:08:54 2021 -0500
> > > > >
> > > > > hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type
> > > > >
> > > > > To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
> > > > > turned on, while the switch to ACPI Hot-plug will be done in the
> > > > > DSDT table.
> > > > >
> > > > > Introducing 'x-keep-native-hpc' property disables the HPC bit only
> > > > > in 6.1 and as a result keeps the forced 'reserve-io' on
> > > > > pcie-root-ports in 6.1 too.
> > > > >
> > > > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > > > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> > > > >
> > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > >
> > > > > Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> > > > > mask out HPC bit in PCIE, the work done by this patch is no longer
> > > > > needed:
> > > > >
> > > > > (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> > > > > Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > > > Date: Mon Aug 2 12:00:57 2021 +0300
> > > > >
> > > > > hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
> > > > >
> > > > > Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> > > > > As opposed to native PCIe hotplug, guests like Fedora 34
> > > > > will not assign IO range to pcie-root-ports not supporting
> > > > > native hotplug, resulting into a regression.
> > > > >
> > > > > Reproduce by:
> > > > > qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> > > > > device_add e1000,bus=p1
> > > > > In the Guest OS the respective pcie-root-port will have the IO range
> > > > > disabled.
> > > > >
> > > > > Fix it by setting the "reserve-io" hint capability of the
> > > > > pcie-root-ports so the firmware will allocate the IO range instead.
> > > > >
> > > > > Acked-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > > > Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > >
> > > > >
> > > > > This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> > > > > patch (3) since we mask out HPC bit from pcie ports.
> > > > >
> > > > >
> > > > > I know this is convoluted mess. In fairness I am trying all I can in my
> > > > > spare time to help from the QEMU side. I am determined to see this
> > > > > patchset through into libvirt.
> > > > >
> > > > > Thanks
> > > > >
> > > > > Laine's comments ...
> > > > >
> > > > > My memory isn't completely clear, but I think there was also the issue
> > > > > that the option claims to enable ACPI hotplug when set to on, but
> > > > > instead what it actually does (in the Q35 case at least) is to enable
> > > > > native PCI hotplug when set to off (without actually disabling ACPI
> > > > > hotplug) and disable native PCI hotplug when set to on, or something
> > > > > like that. This ends up leaving it up to the guest OS to decide which
> > > > > type of hotplug to use, meaning its decision could override what's in
> > > > > the libvirt config, thus confusing everyone. Again, I probably have the
> > > > > details mixed up, but it was something like this.
> > > > >
> > > > > I asked mst about this this morning, and he suggested something that
> > > > > you've already done - Cc'ing the series to qemu-devel and the relevant
> > > > > maintainers so we can have a discussion with all involved parties about
> > > > > their opinions on whether we really should expose this existing option
> > > > > in libvirt, or if we should instead have two new options that are more
> > > > > orthogonal about enabling/disabling the two types of hotplug, so that
> > > > > libvirt config can more accurately represent what is being presented to
> > > > > the guest rather than a "best guess" of what we think the guest is going
> > > > > to do with what is presented.
> > > > >
> > > > > (Michael did also say that, with the current flurry of bug reports for
> > > > > the QEMU rc's, this discusion may not happen until closer to release
> > > > > when the bug reports die down. I know this doesn't mesh with your desire
> > > > > to "push now to allow for testing" (which in general would be a good
> > > > > thing if we were certain that we wanted the option like this and were
> > > > > just expecting some minor bugs that could be fixed), but my opinion is
> > > > > that 1) it's possible for anyone interested to test the functionality
> > > > > using <qemu:commandline>, and 2) we should avoid turning libvirt git
> > > > > into a revolving door of experiments. The only practical difference
> > > > > between using <qemu:commandline> and having a dedicated option is that
> > > > > the use of <qemu:commandline> causes the domain to be tainted, and the
> > > > > XML is a bit more complicated. But since the people we're talking about
> > > > > here will already have built their own libvirt binaries, the tainted
> > > > > status of any guests is irrelevant and the extra complexity of using
> > > > > <qemu:commandline> is probably trivial to them :-).
> > > > >
> > > > >
> > > > > Ani Sinha (4):
> > > > > qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> > > > > conf: introduce support for acpi-bridge-hotplug feature
> > > > > qemu: command: add support for acpi-bridge-hotplug feature
> > > > > NEWS: document new acpi pci hotplug config option
> > > > >
> > > > > NEWS.rst | 8 ++
> > > > > docs/formatdomain.rst | 32 +++++++
> > > > > docs/schemas/domaincommon.rng | 15 ++++
> > > > > src/conf/domain_conf.c | 89 ++++++++++++++++++-
> > > > > src/conf/domain_conf.h | 9 ++
> > > > > src/qemu/qemu_capabilities.c | 4 +
> > > > > src/qemu/qemu_capabilities.h | 3 +
> > > > > src/qemu/qemu_command.c | 19 ++++
> > > > > src/qemu/qemu_validate.c | 42 +++++++++
> > > > > .../caps_6.1.0.x86_64.xml | 1 +
> > > > > .../caps_6.2.0.x86_64.xml | 1 +
> > > > > .../caps_7.0.0.x86_64.xml | 1 +
> > > > > ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> > > > > .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> > > > > ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> > > > > .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> > > > > .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> > > > > ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> > > > > ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> > > > > .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> > > > > .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> > > > > tests/qemuxml2argvtest.c | 7 ++
> > > > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > > > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > > > > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > > > > tests/qemuxml2xmltest.c | 4 +
> > > > > 27 files changed, 504 insertions(+), 1 deletion(-)
> > > > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
> > > > > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> > > > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> > > > > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> > > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> > > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> > > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> > > > > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> > > > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > > > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > > > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > > > > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > > > >
> > > > > --
> > > > > 2.25.1
> > > >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge>
2022-03-08 16:57 ` Michael S. Tsirkin
2022-04-12 4:20 ` Ani Sinha
@ 2022-04-22 5:25 ` Ani Sinha
1 sibling, 0 replies; 18+ messages in thread
From: Ani Sinha @ 2022-04-22 5:25 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: libvir-list, imammedo, jusual, qemu-devel, laine
On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote:
> >
> > Change log:
> > v2: rebased the patchset. Laine's response is appended at the end.
> >
> > I am re-introducing the patchset for <acpi-hotplug-bridge> which got
> > reverted here few months back:
> >
> > https://www.spinics.net/linux/fedora/libvir/msg224089.html
> >
> > The reason for the reversal was that there seemed to be some
> > instability/issues around the use of the qemu commandline which this
> > patchset tries to support. In particular, some guest operating systems
> > did not like the way QEMU was trying to disable native hotplug on pcie
> > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism
> > using which we disable native hotplug. As I understand, we do not have
> > any reported issues so far in 6.2 around this area. QEMU will enter a
> > soft feature freeze in the first week of march in prep for 7.0 release.
>
> Right. But unfortunately we did not yet really work on
> a sane interface for this.
Ok so are we going to do something about this? I am still very unclear
as to what would be a sane interface both for i440fx and q35 (pci and
pcie).
>
> The way I see it, at high level we thinkably need two flags
> - disable ACPI hotplug
> - enable native hotplug (maybe separately for pci and pcie?)
>
> and with both enabled guests actually can switch between
> the two.
>
> This will at least reflect the hardware, so has a chance to be
> stable.
>
> The big question however would be what is the actual use-case.
> Without that this begs the question of why do we bother at all.
> To allow hotplug of bridges? If it is really necessary for us then
> we should think hard about questions that surround this:
>
> - how does one hotplug a pcie switch?
> - any way to use e.g. dynamic ACPI to support hotplug of bridges?
> - do we want to bite the bullet and create an option for management
> to fully control guest memory layout including all pci devices?
>
>
>
> > Libvirt is also entering a new release cycle phaze. Hence, I am
> > introducing this patchset early enough in the release cycles so that if
> > we do see any issues on the qemu side during the rc0, rc1 cycles and if
> > reversal of this patchset is again required, it can be done in time
> > before the next libvirt release end of March.
> >
> > All the patches in this series had been previously reviewed. Some
> > subsequent fixes were made after my initial patches were pushed. I have
> > squashed all those fixes and consolidated them into four patches. I have
> > also updated the documentation to reflect the new changes from the QEMU
> > side and rebased my changes fixing the tests in the process.
> >
> > What changed in QEMU post version 6.1 ?
> > =========================================
> >
> > We have made basically two major changes in QEMU. First is this change:
> >
> > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8
> > Author: Julia Suvorova <jusual@redhat.com>
> > Date: Fri Nov 12 06:08:56 2021 -0500
> >
> > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
> >
> > There are two ways to enable ACPI PCI Hot-plug:
> >
> > * Disable the Hot-plug Capable bit on PCIe slots.
> >
> > This was the first approach which led to regression [1-2], as
> > I/O space for a port is allocated only when it is hot-pluggable,
> > which is determined by HPC bit.
> >
> > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
> > method.
> >
> > This removes the (future) ability of hot-plugging switches with PCIe
> > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> > bridges. If the user wants to explicitely use this feature, they can
> > disable ACPI PCI Hot-plug with:
> > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> >
> > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
> > instead of PCIe Native.
> >
> > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Message-Id: <20211112110857.3116853-5-imammedo@redhat.com>
> > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > The patch description says it all. Instead of masking out the HPC bit in
> > pcie slots, we keep them turned on. Instead, we do not advertize native
> > hotplug capability for PCIE using _OSC control method. See section
> > 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for
> > these slots so now the guest OS can select ACPI hotplug instead.
> >
> > The second change is introduction of a property with which we keep the
> > existing behavior for pc-q35-6.1 machines. This means HPC bit is masked
> > and ACPI hotplug is enabled by default for pcie root ports.
> > The QEMU commit is:
> >
> > (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb
> > Author: Julia Suvorova <jusual@redhat.com>
> > Date: Fri Nov 12 06:08:54 2021 -0500
> >
> > hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type
> >
> > To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be
> > turned on, while the switch to ACPI Hot-plug will be done in the
> > DSDT table.
> >
> > Introducing 'x-keep-native-hpc' property disables the HPC bit only
> > in 6.1 and as a result keeps the forced 'reserve-io' on
> > pcie-root-ports in 6.1 too.
> >
> > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Message-Id: <20211112110857.3116853-3-imammedo@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Lastly, as a related side note, because from QEMU 6.2 onwards, we do not
> > mask out HPC bit in PCIE, the work done by this patch is no longer
> > needed:
> >
> > (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820
> > Author: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > Date: Mon Aug 2 12:00:57 2021 +0300
> >
> > hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
> >
> > Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
> > As opposed to native PCIe hotplug, guests like Fedora 34
> > will not assign IO range to pcie-root-ports not supporting
> > native hotplug, resulting into a regression.
> >
> > Reproduce by:
> > qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> > device_add e1000,bus=p1
> > In the Guest OS the respective pcie-root-port will have the IO range
> > disabled.
> >
> > Fix it by setting the "reserve-io" hint capability of the
> > pcie-root-ports so the firmware will allocate the IO range instead.
> >
> > Acked-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > Message-Id: <20210802090057.1709775-1-marcel@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > This is what commit (2) alludes to. In pc-q35-6.1 machines we do need
> > patch (3) since we mask out HPC bit from pcie ports.
> >
> >
> > I know this is convoluted mess. In fairness I am trying all I can in my
> > spare time to help from the QEMU side. I am determined to see this
> > patchset through into libvirt.
> >
> > Thanks
> >
> > Laine's comments ...
> >
> > My memory isn't completely clear, but I think there was also the issue
> > that the option claims to enable ACPI hotplug when set to on, but
> > instead what it actually does (in the Q35 case at least) is to enable
> > native PCI hotplug when set to off (without actually disabling ACPI
> > hotplug) and disable native PCI hotplug when set to on, or something
> > like that. This ends up leaving it up to the guest OS to decide which
> > type of hotplug to use, meaning its decision could override what's in
> > the libvirt config, thus confusing everyone. Again, I probably have the
> > details mixed up, but it was something like this.
> >
> > I asked mst about this this morning, and he suggested something that
> > you've already done - Cc'ing the series to qemu-devel and the relevant
> > maintainers so we can have a discussion with all involved parties about
> > their opinions on whether we really should expose this existing option
> > in libvirt, or if we should instead have two new options that are more
> > orthogonal about enabling/disabling the two types of hotplug, so that
> > libvirt config can more accurately represent what is being presented to
> > the guest rather than a "best guess" of what we think the guest is going
> > to do with what is presented.
> >
> > (Michael did also say that, with the current flurry of bug reports for
> > the QEMU rc's, this discusion may not happen until closer to release
> > when the bug reports die down. I know this doesn't mesh with your desire
> > to "push now to allow for testing" (which in general would be a good
> > thing if we were certain that we wanted the option like this and were
> > just expecting some minor bugs that could be fixed), but my opinion is
> > that 1) it's possible for anyone interested to test the functionality
> > using <qemu:commandline>, and 2) we should avoid turning libvirt git
> > into a revolving door of experiments. The only practical difference
> > between using <qemu:commandline> and having a dedicated option is that
> > the use of <qemu:commandline> causes the domain to be tainted, and the
> > XML is a bit more complicated. But since the people we're talking about
> > here will already have built their own libvirt binaries, the tainted
> > status of any guests is irrelevant and the extra complexity of using
> > <qemu:commandline> is probably trivial to them :-).
> >
> >
> > Ani Sinha (4):
> > qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
> > conf: introduce support for acpi-bridge-hotplug feature
> > qemu: command: add support for acpi-bridge-hotplug feature
> > NEWS: document new acpi pci hotplug config option
> >
> > NEWS.rst | 8 ++
> > docs/formatdomain.rst | 32 +++++++
> > docs/schemas/domaincommon.rng | 15 ++++
> > src/conf/domain_conf.c | 89 ++++++++++++++++++-
> > src/conf/domain_conf.h | 9 ++
> > src/qemu/qemu_capabilities.c | 4 +
> > src/qemu/qemu_capabilities.h | 3 +
> > src/qemu/qemu_command.c | 19 ++++
> > src/qemu/qemu_validate.c | 42 +++++++++
> > .../caps_6.1.0.x86_64.xml | 1 +
> > .../caps_6.2.0.x86_64.xml | 1 +
> > .../caps_7.0.0.x86_64.xml | 1 +
> > ...-hotplug-bridge-disable.aarch64-latest.err | 1 +
> > .../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
> > ...-hotplug-bridge-disable.x86_64-latest.args | 35 ++++++++
> > .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36 ++++++++
> > .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36 ++++++++
> > ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
> > ...-hotplug-bridge-disable.x86_64-latest.args | 38 ++++++++
> > .../q35-acpi-hotplug-bridge-disable.xml | 53 +++++++++++
> > .../q35-acpi-hotplug-bridge-enable.xml | 53 +++++++++++
> > tests/qemuxml2argvtest.c | 7 ++
> > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > ...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
> > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
> > tests/qemuxml2xmltest.c | 4 +
> > 27 files changed, 504 insertions(+), 1 deletion(-)
> > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
> > create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
> > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
> > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
> > create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
> > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
> > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
> > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
> > create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
> > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
> > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
> > create mode 120000 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
> >
> > --
> > 2.25.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-04-22 5:28 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220308063955.2285902-1-ani@anisinha.ca>
[not found] ` <399ca3a9-8b95-39af-8376-85f2edf00c7e@redhat.com>
2022-03-08 15:51 ` [libvirt] [PATCH RESEND v2 0/4] re-introduce <acpi-hotplug-bridge> Laine Stump
2022-03-08 16:27 ` Ani Sinha
2022-03-08 16:30 ` Michael S. Tsirkin
2022-03-08 16:45 ` Ani Sinha
2022-03-08 16:47 ` Michael S. Tsirkin
2022-03-08 16:53 ` Ani Sinha
2022-03-08 17:18 ` Michael S. Tsirkin
2022-03-09 7:17 ` Ani Sinha
2022-03-11 7:12 ` Erik Skultety
2022-03-08 16:45 Ani Sinha
2022-03-08 16:57 ` Michael S. Tsirkin
2022-04-12 4:20 ` Ani Sinha
2022-04-12 4:22 ` Ani Sinha
2022-04-12 7:11 ` Michael S. Tsirkin
2022-04-13 11:18 ` Ani Sinha
2022-04-12 7:04 ` Michael S. Tsirkin
2022-04-13 11:16 ` Ani Sinha
2022-04-22 5:25 ` Ani Sinha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).