* [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-08-03 20:52 [PULL 0/5] pc,pci: bugfixes Michael S. Tsirkin
@ 2021-08-03 20:52 ` Michael S. Tsirkin
2021-09-27 9:33 ` Daniel P. Berrangé
2021-08-03 20:52 ` [PULL 2/5] acpi: x86: pcihp: add support hotplug on multifunction bridges Michael S. Tsirkin
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Michael S. Tsirkin @ 2021-08-03 20:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Marcel Apfelbaum, Peter Maydell, Igor Mammedov
From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
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>
---
hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
index ec9907917e..20099a8ae3 100644
--- a/hw/pci-bridge/gen_pcie_root_port.c
+++ b/hw/pci-bridge/gen_pcie_root_port.c
@@ -28,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(GenPCIERootPort, GEN_PCIE_ROOT_PORT)
(GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
#define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1
+#define GEN_PCIE_ROOT_DEFAULT_IO_RANGE 4096
struct GenPCIERootPort {
/*< private >*/
@@ -75,6 +76,7 @@ static bool gen_rp_test_migrate_msix(void *opaque, int version_id)
static void gen_rp_realize(DeviceState *dev, Error **errp)
{
PCIDevice *d = PCI_DEVICE(dev);
+ PCIESlot *s = PCIE_SLOT(d);
GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
Error *local_err = NULL;
@@ -85,6 +87,9 @@ static void gen_rp_realize(DeviceState *dev, Error **errp)
return;
}
+ if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug) {
+ grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE;
+ }
int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
grp->res_reserve, errp);
--
MST
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-08-03 20:52 ` [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO Michael S. Tsirkin
@ 2021-09-27 9:33 ` Daniel P. Berrangé
2021-09-27 9:49 ` Michael S. Tsirkin
0 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2021-09-27 9:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, Peter Maydell, qemu-devel, Igor Mammedov
On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>
> 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>
> ---
> hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> 1 file changed, 5 insertions(+)
This change, when combined with the switch to ACPI based hotplug by
default, is responsible for a significant regression in QEMU 6.1.0
It is no longer possible to have more than 15 pcie-root-port devices
added to a q35 VM in 6.1.0. Before this I've had as many as 80+ devices
present before I stopped trying to add more.
https://gitlab.com/qemu-project/qemu/-/issues/641
This regression is significant, because it has broken the out of the
box default configuration that OpenStack uses for booting all VMs.
They add 16 pcie-root-ports by defalt to allow empty slots for device
hotplug under q35 [1].
> diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> index ec9907917e..20099a8ae3 100644
> --- a/hw/pci-bridge/gen_pcie_root_port.c
> +++ b/hw/pci-bridge/gen_pcie_root_port.c
> @@ -28,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(GenPCIERootPort, GEN_PCIE_ROOT_PORT)
> (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
>
> #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1
> +#define GEN_PCIE_ROOT_DEFAULT_IO_RANGE 4096
>
> struct GenPCIERootPort {
> /*< private >*/
> @@ -75,6 +76,7 @@ static bool gen_rp_test_migrate_msix(void *opaque, int version_id)
> static void gen_rp_realize(DeviceState *dev, Error **errp)
> {
> PCIDevice *d = PCI_DEVICE(dev);
> + PCIESlot *s = PCIE_SLOT(d);
> GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
> PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
> Error *local_err = NULL;
> @@ -85,6 +87,9 @@ static void gen_rp_realize(DeviceState *dev, Error **errp)
> return;
> }
>
> + if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug) {
> + grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE;
> + }
> int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
> grp->res_reserve, errp);
>
> --
> MST
>
>
Regards,
Daniel
[1] https://github.com/openstack/tripleo-heat-templates/blob/7a6cd0640ec390a330f5699d8ed60f71b2a9f514/deployment/nova/nova-compute-container-puppet.yaml#L462-L472
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-09-27 9:33 ` Daniel P. Berrangé
@ 2021-09-27 9:49 ` Michael S. Tsirkin
2021-09-29 9:05 ` Daniel P. Berrangé
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2021-09-27 9:49 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Marcel Apfelbaum, Peter Maydell, qemu-devel, Igor Mammedov
On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> >
> > 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>
> > ---
> > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > 1 file changed, 5 insertions(+)
>
> This change, when combined with the switch to ACPI based hotplug by
> default, is responsible for a significant regression in QEMU 6.1.0
>
> It is no longer possible to have more than 15 pcie-root-port devices
> added to a q35 VM in 6.1.0. Before this I've had as many as 80+ devices
> present before I stopped trying to add more.
>
> https://gitlab.com/qemu-project/qemu/-/issues/641
>
> This regression is significant, because it has broken the out of the
> box default configuration that OpenStack uses for booting all VMs.
> They add 16 pcie-root-ports by defalt to allow empty slots for device
> hotplug under q35 [1].
Indeed, oops. Thanks for the report!
Going back and looking at seabios code, didn't we get confused?
Shouldn't we have reserved memory and not IO?
I see:
int resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
if (!sum && hotplug_support && !resource_optional)
sum = align; /* reserve min size for hot-plug */
generally maybe we should just add an ACPI-hotplug capability and
teach seabios about it?
Marcel?
> > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> > index ec9907917e..20099a8ae3 100644
> > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > @@ -28,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(GenPCIERootPort, GEN_PCIE_ROOT_PORT)
> > (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
> >
> > #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1
> > +#define GEN_PCIE_ROOT_DEFAULT_IO_RANGE 4096
> >
> > struct GenPCIERootPort {
> > /*< private >*/
> > @@ -75,6 +76,7 @@ static bool gen_rp_test_migrate_msix(void *opaque, int version_id)
> > static void gen_rp_realize(DeviceState *dev, Error **errp)
> > {
> > PCIDevice *d = PCI_DEVICE(dev);
> > + PCIESlot *s = PCIE_SLOT(d);
> > GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
> > PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
> > Error *local_err = NULL;
> > @@ -85,6 +87,9 @@ static void gen_rp_realize(DeviceState *dev, Error **errp)
> > return;
> > }
> >
> > + if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug) {
> > + grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE;
> > + }
> > int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
> > grp->res_reserve, errp);
> >
> > --
> > MST
> >
> >
>
> Regards,
> Daniel
>
> [1] https://github.com/openstack/tripleo-heat-templates/blob/7a6cd0640ec390a330f5699d8ed60f71b2a9f514/deployment/nova/nova-compute-container-puppet.yaml#L462-L472
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-09-27 9:49 ` Michael S. Tsirkin
@ 2021-09-29 9:05 ` Daniel P. Berrangé
2021-09-29 13:55 ` Marcel Apfelbaum
2021-09-29 13:41 ` Marcel Apfelbaum
2021-11-02 9:23 ` Daniel P. Berrangé
2 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2021-09-29 9:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, Peter Maydell, qemu-devel, Igor Mammedov
On Mon, Sep 27, 2021 at 05:49:15AM -0400, Michael S. Tsirkin wrote:
> On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> > On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > >
> > > 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>
> > > ---
> > > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> >
> > This change, when combined with the switch to ACPI based hotplug by
> > default, is responsible for a significant regression in QEMU 6.1.0
> >
> > It is no longer possible to have more than 15 pcie-root-port devices
> > added to a q35 VM in 6.1.0. Before this I've had as many as 80+ devices
> > present before I stopped trying to add more.
> >
> > https://gitlab.com/qemu-project/qemu/-/issues/641
> >
> > This regression is significant, because it has broken the out of the
> > box default configuration that OpenStack uses for booting all VMs.
> > They add 16 pcie-root-ports by defalt to allow empty slots for device
> > hotplug under q35 [1].
>
>
> Indeed, oops. Thanks for the report!
>
> Going back and looking at seabios code, didn't we get confused?
> Shouldn't we have reserved memory and not IO?
>
> I see:
> int resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
> if (!sum && hotplug_support && !resource_optional)
> sum = align; /* reserve min size for hot-plug */
>
>
> generally maybe we should just add an ACPI-hotplug capability and
> teach seabios about it?
Looking at the commit message example:
qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
device_add e1000,bus=p1
IIUC, that is plugging a PCI device into the PCIe root port.
docs/pcie.txt says that IO space is not allocated by SeaBIOS
or OVMF for pcie-root-port, if
(1) the port is empty, or
(2) the device behind the port has no IO BARs.
Point (2) is satisified if you only ever plug PCIe devces into
the pcie-root-port. The docs/pcie.txt recommends exactly that,
with any PCI device to be placed downstream of a pcie-pci-bridge
and pci-pci-bridge pair.
IOW that example from the commit message should have been
qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio \
-device pcie-pci-bridge,id=br1,bus=pcie.0] \
-device pci-bridge,id=br2,bus=br1,chassis_nr=1
device-add e1000,bus=br2
So why did we need IO space for the pcie-root-port at all ?
The example given as the reason just looks like user error
per the docs/pcie.txt
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-09-29 9:05 ` Daniel P. Berrangé
@ 2021-09-29 13:55 ` Marcel Apfelbaum
0 siblings, 0 replies; 19+ messages in thread
From: Marcel Apfelbaum @ 2021-09-29 13:55 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Marcel Apfelbaum, Peter Maydell, Igor Mammedov, qemu-devel,
Michael S. Tsirkin
[-- Attachment #1: Type: text/plain, Size: 4614 bytes --]
Hi Daniel,
On Wed, Sep 29, 2021 at 12:05 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:
> On Mon, Sep 27, 2021 at 05:49:15AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > > > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > >
> > > > 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>
> > > > ---
> > > > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > >
> > > This change, when combined with the switch to ACPI based hotplug by
> > > default, is responsible for a significant regression in QEMU 6.1.0
> > >
> > > It is no longer possible to have more than 15 pcie-root-port devices
> > > added to a q35 VM in 6.1.0. Before this I've had as many as 80+
> devices
> > > present before I stopped trying to add more.
> > >
> > > https://gitlab.com/qemu-project/qemu/-/issues/641
> > >
> > > This regression is significant, because it has broken the out of the
> > > box default configuration that OpenStack uses for booting all VMs.
> > > They add 16 pcie-root-ports by defalt to allow empty slots for device
> > > hotplug under q35 [1].
>
That's bad!
> >
> >
> > Indeed, oops. Thanks for the report!
> >
> > Going back and looking at seabios code, didn't we get confused?
> > Shouldn't we have reserved memory and not IO?
> >
> > I see:
> > int resource_optional = pcie_cap && (type ==
> PCI_REGION_TYPE_IO);
> > if (!sum && hotplug_support && !resource_optional)
> > sum = align; /* reserve min size for hot-plug */
> >
> >
> > generally maybe we should just add an ACPI-hotplug capability and
> > teach seabios about it?
>
> Looking at the commit message example:
>
> qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
> device_add e1000,bus=p1
>
> IIUC, that is plugging a PCI device into the PCIe root port.
>
> docs/pcie.txt says that IO space is not allocated by SeaBIOS
> or OVMF for pcie-root-port, if
>
> (1) the port is empty, or
> (2) the device behind the port has no IO BARs.
>
> Point (2) is satisified if you only ever plug PCIe devces into
> the pcie-root-port. The docs/pcie.txt recommends exactly that,
> with any PCI device to be placed downstream of a pcie-pci-bridge
> and pci-pci-bridge pair.
>
> IOW that example from the commit message should have been
>
> qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio \
> -device pcie-pci-bridge,id=br1,bus=pcie.0] \
> -device pci-bridge,id=br2,bus=br1,chassis_nr=1
> device-add e1000,bus=br2
>
> So why did we need IO space for the pcie-root-port at all ?
>
We don't... we need it for the pci-bridge.
The patch addressed a regression seen in Fedora 33/34 hosts.
PCIe hot-plug for pcie-root ports allowed legacy PCI devices to be
hot-plugged,
while the ACPI based hotplug didn't.
The patch tried to fix the problem by pre-allocating IO space at SeaBIOS
level,
but it seems it is not the optimal solution.
That means it was the Guest OS that allocated the IO range when necessary,
making the decision at firmware level is wrong.
I confirm we have to find a better solution.
Thanks,
marcel
> The example given as the reason just looks like user error
> per the docs/pcie.txt
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o-
> https://www.instagram.com/dberrange :|
>
>
[-- Attachment #2: Type: text/html, Size: 7031 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-09-27 9:49 ` Michael S. Tsirkin
2021-09-29 9:05 ` Daniel P. Berrangé
@ 2021-09-29 13:41 ` Marcel Apfelbaum
2021-09-29 14:12 ` Gerd Hoffmann
2021-09-29 21:47 ` Michael S. Tsirkin
2021-11-02 9:23 ` Daniel P. Berrangé
2 siblings, 2 replies; 19+ messages in thread
From: Marcel Apfelbaum @ 2021-09-29 13:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, Peter Maydell, Daniel P. Berrangé,
qemu-devel, Igor Mammedov
[-- Attachment #1: Type: text/plain, Size: 4636 bytes --]
Hi Michael,
On Mon, Sep 27, 2021 at 12:49 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> > On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > >
> > > 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>
> > > ---
> > > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> >
> > This change, when combined with the switch to ACPI based hotplug by
> > default, is responsible for a significant regression in QEMU 6.1.0
> >
> > It is no longer possible to have more than 15 pcie-root-port devices
> > added to a q35 VM in 6.1.0. Before this I've had as many as 80+ devices
> > present before I stopped trying to add more.
> >
> > https://gitlab.com/qemu-project/qemu/-/issues/641
> >
> > This regression is significant, because it has broken the out of the
> > box default configuration that OpenStack uses for booting all VMs.
> > They add 16 pcie-root-ports by defalt to allow empty slots for device
> > hotplug under q35 [1].
>
>
> Indeed, oops. Thanks for the report!
>
> Going back and looking at seabios code, didn't we get confused?
> Shouldn't we have reserved memory and not IO?
>
We need the IO space for the legacy PCI bridges, otherwise an empty PCI
bridge will become unusable.
>
> I see:
> int resource_optional = pcie_cap && (type ==
> PCI_REGION_TYPE_IO);
> if (!sum && hotplug_support && !resource_optional)
> sum = align; /* reserve min size for hot-plug */
>
>
> generally maybe we should just add an ACPI-hotplug capability and
> teach seabios about it?
>
I suppose it is possible.
Thanks,
Marcel
>
> Marcel?
>
> > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c
> b/hw/pci-bridge/gen_pcie_root_port.c
> > > index ec9907917e..20099a8ae3 100644
> > > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > > @@ -28,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(GenPCIERootPort,
> GEN_PCIE_ROOT_PORT)
> > > (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
> > >
> > > #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1
> > > +#define GEN_PCIE_ROOT_DEFAULT_IO_RANGE 4096
> > >
> > > struct GenPCIERootPort {
> > > /*< private >*/
> > > @@ -75,6 +76,7 @@ static bool gen_rp_test_migrate_msix(void *opaque,
> int version_id)
> > > static void gen_rp_realize(DeviceState *dev, Error **errp)
> > > {
> > > PCIDevice *d = PCI_DEVICE(dev);
> > > + PCIESlot *s = PCIE_SLOT(d);
> > > GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
> > > PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
> > > Error *local_err = NULL;
> > > @@ -85,6 +87,9 @@ static void gen_rp_realize(DeviceState *dev, Error
> **errp)
> > > return;
> > > }
> > >
> > > + if (grp->res_reserve.io == -1 && s->hotplug &&
> !s->native_hotplug) {
> > > + grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE;
> > > + }
> > > int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
> > > grp->res_reserve, errp);
> > >
> > > --
> > > MST
> > >
> > >
> >
> > Regards,
> > Daniel
> >
> > [1]
> https://github.com/openstack/tripleo-heat-templates/blob/7a6cd0640ec390a330f5699d8ed60f71b2a9f514/deployment/nova/nova-compute-container-puppet.yaml#L462-L472
> > --
> > |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> > |: https://entangle-photo.org -o-
> https://www.instagram.com/dberrange :|
>
>
[-- Attachment #2: Type: text/html, Size: 7430 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-09-29 13:41 ` Marcel Apfelbaum
@ 2021-09-29 14:12 ` Gerd Hoffmann
2021-09-29 21:47 ` Michael S. Tsirkin
1 sibling, 0 replies; 19+ messages in thread
From: Gerd Hoffmann @ 2021-09-29 14:12 UTC (permalink / raw)
To: marcel
Cc: Peter Maydell, Igor Mammedov, Daniel P. Berrangé,
qemu-devel, Michael S. Tsirkin
Hi,
> > generally maybe we should just add an ACPI-hotplug capability and
> > teach seabios about it?
>
> I suppose it is possible.
I doubt this solves the problem. As I understand it linux will happily
assign io address space for pcie hotplug root ports but not for acpi
hotplug root ports. So seabios simply not allocation io address space
isn't a problem for pcie hotplug root ports but is for acpi hotplug root
ports even though seabios doesn't make a difference.
So the patch asks seabios to allocate io address space, which in turn
leads to running out of io address space with many ports.
Changing the way to request io address space allocation doesn't fix the
underlying issue ...
take care,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-09-29 13:41 ` Marcel Apfelbaum
2021-09-29 14:12 ` Gerd Hoffmann
@ 2021-09-29 21:47 ` Michael S. Tsirkin
2021-09-30 8:34 ` Igor Mammedov
1 sibling, 1 reply; 19+ messages in thread
From: Michael S. Tsirkin @ 2021-09-29 21:47 UTC (permalink / raw)
To: marcel; +Cc: Peter Maydell, Daniel P. Berrangé, qemu-devel, Igor Mammedov
On Wed, Sep 29, 2021 at 04:41:49PM +0300, Marcel Apfelbaum wrote:
> Hi Michael,
>
> On Mon, Sep 27, 2021 at 12:49 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> > On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > >
> > > 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>
> > > ---
> > > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> >
> > This change, when combined with the switch to ACPI based hotplug by
> > default, is responsible for a significant regression in QEMU 6.1.0
> >
> > It is no longer possible to have more than 15 pcie-root-port devices
> > added to a q35 VM in 6.1.0. Before this I've had as many as 80+ devices
> > present before I stopped trying to add more.
> >
> > https://gitlab.com/qemu-project/qemu/-/issues/641
> >
> > This regression is significant, because it has broken the out of the
> > box default configuration that OpenStack uses for booting all VMs.
> > They add 16 pcie-root-ports by defalt to allow empty slots for device
> > hotplug under q35 [1].
>
>
> Indeed, oops. Thanks for the report!
>
> Going back and looking at seabios code, didn't we get confused?
> Shouldn't we have reserved memory and not IO?
>
>
> We need the IO space for the legacy PCI bridges, otherwise an empty PCI bridge
> will become unusable.
Maybe we should go back to using OSC then ... the issue
is we can't then mix acpi and native hotplug for bridges.
>
>
> I see:
> int resource_optional = pcie_cap && (type ==
> PCI_REGION_TYPE_IO);
> if (!sum && hotplug_support && !resource_optional)
> sum = align; /* reserve min size for hot-plug */
>
>
> generally maybe we should just add an ACPI-hotplug capability and
> teach seabios about it?
>
>
> I suppose it is possible.
>
> Thanks,
> Marcel
>
>
>
> Marcel?
>
> > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/
> gen_pcie_root_port.c
> > > index ec9907917e..20099a8ae3 100644
> > > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > > @@ -28,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(GenPCIERootPort,
> GEN_PCIE_ROOT_PORT)
> > > (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
> > >
> > > #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1
> > > +#define GEN_PCIE_ROOT_DEFAULT_IO_RANGE 4096
> > >
> > > struct GenPCIERootPort {
> > > /*< private >*/
> > > @@ -75,6 +76,7 @@ static bool gen_rp_test_migrate_msix(void *opaque,
> int version_id)
> > > static void gen_rp_realize(DeviceState *dev, Error **errp)
> > > {
> > > PCIDevice *d = PCI_DEVICE(dev);
> > > + PCIESlot *s = PCIE_SLOT(d);
> > > GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
> > > PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
> > > Error *local_err = NULL;
> > > @@ -85,6 +87,9 @@ static void gen_rp_realize(DeviceState *dev, Error
> **errp)
> > > return;
> > > }
> > >
> > > + if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug)
> {
> > > + grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE;
> > > + }
> > > int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
> > > grp->res_reserve, errp);
> > >
> > > --
> > > MST
> > >
> > >
> >
> > Regards,
> > Daniel
> >
> > [1] https://github.com/openstack/tripleo-heat-templates/blob/
> 7a6cd0640ec390a330f5699d8ed60f71b2a9f514/deployment/nova/
> nova-compute-container-puppet.yaml#L462-L472
> > --
> > |: https://berrange.com -o- https://www.flickr.com/photos/
> dberrange :|
> > |: https://libvirt.org -o- https://
> fstop138.berrange.com :|
> > |: https://entangle-photo.org -o- https://www.instagram.com/
> dberrange :|
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-09-29 21:47 ` Michael S. Tsirkin
@ 2021-09-30 8:34 ` Igor Mammedov
2021-09-30 10:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 19+ messages in thread
From: Igor Mammedov @ 2021-09-30 8:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: marcel, Peter Maydell, Daniel P. Berrangé, qemu-devel
On Wed, 29 Sep 2021 17:47:37 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Sep 29, 2021 at 04:41:49PM +0300, Marcel Apfelbaum wrote:
> > Hi Michael,
> >
> > On Mon, Sep 27, 2021 at 12:49 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > > > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > >
> > > > 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>
> > > > ---
> > > > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > >
> > > This change, when combined with the switch to ACPI based hotplug by
> > > default, is responsible for a significant regression in QEMU 6.1.0
> > >
> > > It is no longer possible to have more than 15 pcie-root-port devices
> > > added to a q35 VM in 6.1.0. Before this I've had as many as 80+ devices
> > > present before I stopped trying to add more.
> > >
> > > https://gitlab.com/qemu-project/qemu/-/issues/641
> > >
> > > This regression is significant, because it has broken the out of the
> > > box default configuration that OpenStack uses for booting all VMs.
> > > They add 16 pcie-root-ports by defalt to allow empty slots for device
> > > hotplug under q35 [1].
> >
> >
> > Indeed, oops. Thanks for the report!
> >
> > Going back and looking at seabios code, didn't we get confused?
> > Shouldn't we have reserved memory and not IO?
> >
> >
> > We need the IO space for the legacy PCI bridges, otherwise an empty PCI bridge
> > will become unusable.
>
> Maybe we should go back to using OSC then ... the issue
> is we can't then mix acpi and native hotplug for bridges.
How OSC could help with the issue?
> >
> >
> > I see:
> > int resource_optional = pcie_cap && (type ==
> > PCI_REGION_TYPE_IO);
> > if (!sum && hotplug_support && !resource_optional)
> > sum = align; /* reserve min size for hot-plug */
> >
> >
> > generally maybe we should just add an ACPI-hotplug capability and
> > teach seabios about it?
> >
> >
> > I suppose it is possible.
> >
> > Thanks,
> > Marcel
> >
> >
> >
> > Marcel?
> >
> > > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/
> > gen_pcie_root_port.c
> > > > index ec9907917e..20099a8ae3 100644
> > > > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > > > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > > > @@ -28,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(GenPCIERootPort,
> > GEN_PCIE_ROOT_PORT)
> > > > (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
> > > >
> > > > #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1
> > > > +#define GEN_PCIE_ROOT_DEFAULT_IO_RANGE 4096
> > > >
> > > > struct GenPCIERootPort {
> > > > /*< private >*/
> > > > @@ -75,6 +76,7 @@ static bool gen_rp_test_migrate_msix(void *opaque,
> > int version_id)
> > > > static void gen_rp_realize(DeviceState *dev, Error **errp)
> > > > {
> > > > PCIDevice *d = PCI_DEVICE(dev);
> > > > + PCIESlot *s = PCIE_SLOT(d);
> > > > GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
> > > > PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
> > > > Error *local_err = NULL;
> > > > @@ -85,6 +87,9 @@ static void gen_rp_realize(DeviceState *dev, Error
> > **errp)
> > > > return;
> > > > }
> > > >
> > > > + if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug)
> > {
> > > > + grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE;
> > > > + }
> > > > int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
> > > > grp->res_reserve, errp);
> > > >
> > > > --
> > > > MST
> > > >
> > > >
> > >
> > > Regards,
> > > Daniel
> > >
> > > [1] https://github.com/openstack/tripleo-heat-templates/blob/
> > 7a6cd0640ec390a330f5699d8ed60f71b2a9f514/deployment/nova/
> > nova-compute-container-puppet.yaml#L462-L472
> > > --
> > > |: https://berrange.com -o- https://www.flickr.com/photos/
> > dberrange :|
> > > |: https://libvirt.org -o- https://
> > fstop138.berrange.com :|
> > > |: https://entangle-photo.org -o- https://www.instagram.com/
> > dberrange :|
> >
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-09-30 8:34 ` Igor Mammedov
@ 2021-09-30 10:52 ` Michael S. Tsirkin
0 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2021-09-30 10:52 UTC (permalink / raw)
To: Igor Mammedov; +Cc: marcel, Peter Maydell, Daniel P. Berrangé, qemu-devel
On Thu, Sep 30, 2021 at 10:34:24AM +0200, Igor Mammedov wrote:
> On Wed, 29 Sep 2021 17:47:37 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Sep 29, 2021 at 04:41:49PM +0300, Marcel Apfelbaum wrote:
> > > Hi Michael,
> > >
> > > On Mon, Sep 27, 2021 at 12:49 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> > > > On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > > > > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > > >
> > > > > 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>
> > > > > ---
> > > > > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > > > > 1 file changed, 5 insertions(+)
> > > >
> > > > This change, when combined with the switch to ACPI based hotplug by
> > > > default, is responsible for a significant regression in QEMU 6.1.0
> > > >
> > > > It is no longer possible to have more than 15 pcie-root-port devices
> > > > added to a q35 VM in 6.1.0. Before this I've had as many as 80+ devices
> > > > present before I stopped trying to add more.
> > > >
> > > > https://gitlab.com/qemu-project/qemu/-/issues/641
> > > >
> > > > This regression is significant, because it has broken the out of the
> > > > box default configuration that OpenStack uses for booting all VMs.
> > > > They add 16 pcie-root-ports by defalt to allow empty slots for device
> > > > hotplug under q35 [1].
> > >
> > >
> > > Indeed, oops. Thanks for the report!
> > >
> > > Going back and looking at seabios code, didn't we get confused?
> > > Shouldn't we have reserved memory and not IO?
> > >
> > >
> > > We need the IO space for the legacy PCI bridges, otherwise an empty PCI bridge
> > > will become unusable.
> >
> > Maybe we should go back to using OSC then ... the issue
> > is we can't then mix acpi and native hotplug for bridges.
>
> How OSC could help with the issue?
guests see the pci capability and vonclude that bridge supports hotplug.
> > >
> > >
> > > I see:
> > > int resource_optional = pcie_cap && (type ==
> > > PCI_REGION_TYPE_IO);
> > > if (!sum && hotplug_support && !resource_optional)
> > > sum = align; /* reserve min size for hot-plug */
> > >
> > >
> > > generally maybe we should just add an ACPI-hotplug capability and
> > > teach seabios about it?
> > >
> > >
> > > I suppose it is possible.
> > >
> > > Thanks,
> > > Marcel
> > >
> > >
> > >
> > > Marcel?
> > >
> > > > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/
> > > gen_pcie_root_port.c
> > > > > index ec9907917e..20099a8ae3 100644
> > > > > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > > > > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > > > > @@ -28,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(GenPCIERootPort,
> > > GEN_PCIE_ROOT_PORT)
> > > > > (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
> > > > >
> > > > > #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1
> > > > > +#define GEN_PCIE_ROOT_DEFAULT_IO_RANGE 4096
> > > > >
> > > > > struct GenPCIERootPort {
> > > > > /*< private >*/
> > > > > @@ -75,6 +76,7 @@ static bool gen_rp_test_migrate_msix(void *opaque,
> > > int version_id)
> > > > > static void gen_rp_realize(DeviceState *dev, Error **errp)
> > > > > {
> > > > > PCIDevice *d = PCI_DEVICE(dev);
> > > > > + PCIESlot *s = PCIE_SLOT(d);
> > > > > GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
> > > > > PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
> > > > > Error *local_err = NULL;
> > > > > @@ -85,6 +87,9 @@ static void gen_rp_realize(DeviceState *dev, Error
> > > **errp)
> > > > > return;
> > > > > }
> > > > >
> > > > > + if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug)
> > > {
> > > > > + grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE;
> > > > > + }
> > > > > int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
> > > > > grp->res_reserve, errp);
> > > > >
> > > > > --
> > > > > MST
> > > > >
> > > > >
> > > >
> > > > Regards,
> > > > Daniel
> > > >
> > > > [1] https://github.com/openstack/tripleo-heat-templates/blob/
> > > 7a6cd0640ec390a330f5699d8ed60f71b2a9f514/deployment/nova/
> > > nova-compute-container-puppet.yaml#L462-L472
> > > > --
> > > > |: https://berrange.com -o- https://www.flickr.com/photos/
> > > dberrange :|
> > > > |: https://libvirt.org -o- https://
> > > fstop138.berrange.com :|
> > > > |: https://entangle-photo.org -o- https://www.instagram.com/
> > > dberrange :|
> > >
> > >
> >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-09-27 9:49 ` Michael S. Tsirkin
2021-09-29 9:05 ` Daniel P. Berrangé
2021-09-29 13:41 ` Marcel Apfelbaum
@ 2021-11-02 9:23 ` Daniel P. Berrangé
2021-11-02 9:33 ` Marcel Apfelbaum
2021-11-02 11:17 ` Julia Suvorova
2 siblings, 2 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2021-11-02 9:23 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, Peter Maydell, qemu-devel, Igor Mammedov
On Mon, Sep 27, 2021 at 05:49:15AM -0400, Michael S. Tsirkin wrote:
> On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> > On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > >
> > > 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>
> > > ---
> > > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> >
> > This change, when combined with the switch to ACPI based hotplug by
> > default, is responsible for a significant regression in QEMU 6.1.0
> >
> > It is no longer possible to have more than 15 pcie-root-port devices
> > added to a q35 VM in 6.1.0. Before this I've had as many as 80+ devices
> > present before I stopped trying to add more.
> >
> > https://gitlab.com/qemu-project/qemu/-/issues/641
> >
> > This regression is significant, because it has broken the out of the
> > box default configuration that OpenStack uses for booting all VMs.
> > They add 16 pcie-root-ports by defalt to allow empty slots for device
> > hotplug under q35 [1].
>
>
> Indeed, oops. Thanks for the report!
We're at soft freeze now and this is still broken in git master.
I don't recall seeing a fix for this problem on list and no one has
commented on the bug report.
Is anyone actively working on a fix for this release ?
If not, I'm going to post a patch to revert to PCI native
hotplug, because this was a significant regression in 6.1 that
breaks openstack out of the box and we can't leave it broken
again for 6.2.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-11-02 9:23 ` Daniel P. Berrangé
@ 2021-11-02 9:33 ` Marcel Apfelbaum
2021-11-02 11:17 ` Julia Suvorova
1 sibling, 0 replies; 19+ messages in thread
From: Marcel Apfelbaum @ 2021-11-02 9:33 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Peter Maydell, Michael S. Tsirkin, Julia Suvorova, qemu-devel,
Marcel Apfelbaum, Igor Mammedov
[-- Attachment #1: Type: text/plain, Size: 3013 bytes --]
Hi Daniel,
On Tue, Nov 2, 2021 at 11:24 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:
> On Mon, Sep 27, 2021 at 05:49:15AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > > > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > >
> > > > 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>
> > > > ---
> > > > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > >
> > > This change, when combined with the switch to ACPI based hotplug by
> > > default, is responsible for a significant regression in QEMU 6.1.0
> > >
> > > It is no longer possible to have more than 15 pcie-root-port devices
> > > added to a q35 VM in 6.1.0. Before this I've had as many as 80+
> devices
> > > present before I stopped trying to add more.
> > >
> > > https://gitlab.com/qemu-project/qemu/-/issues/641
> > >
> > > This regression is significant, because it has broken the out of the
> > > box default configuration that OpenStack uses for booting all VMs.
> > > They add 16 pcie-root-ports by defalt to allow empty slots for device
> > > hotplug under q35 [1].
> >
> >
> > Indeed, oops. Thanks for the report!
>
> We're at soft freeze now and this is still broken in git master.
> I don't recall seeing a fix for this problem on list and no one has
> commented on the bug report.
>
> Is anyone actively working on a fix for this release ?
>
I think Julia (cc-ed) is working on a fix.
Thanks,
Marcel
>
> If not, I'm going to post a patch to revert to PCI native
> hotplug, because this was a significant regression in 6.1 that
> breaks openstack out of the box and we can't leave it broken
> again for 6.2.
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o-
> https://www.instagram.com/dberrange :|
>
>
[-- Attachment #2: Type: text/html, Size: 4986 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO
2021-11-02 9:23 ` Daniel P. Berrangé
2021-11-02 9:33 ` Marcel Apfelbaum
@ 2021-11-02 11:17 ` Julia Suvorova
1 sibling, 0 replies; 19+ messages in thread
From: Julia Suvorova @ 2021-11-02 11:17 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Marcel Apfelbaum, Peter Maydell, Igor Mammedov, QEMU Developers,
Michael S. Tsirkin
On Tue, Nov 2, 2021 at 10:26 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Sep 27, 2021 at 05:49:15AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Sep 27, 2021 at 10:33:42AM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Aug 03, 2021 at 04:52:03PM -0400, Michael S. Tsirkin wrote:
> > > > From: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > >
> > > > 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>
> > > > ---
> > > > hw/pci-bridge/gen_pcie_root_port.c | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > >
> > > This change, when combined with the switch to ACPI based hotplug by
> > > default, is responsible for a significant regression in QEMU 6.1.0
> > >
> > > It is no longer possible to have more than 15 pcie-root-port devices
> > > added to a q35 VM in 6.1.0. Before this I've had as many as 80+ devices
> > > present before I stopped trying to add more.
> > >
> > > https://gitlab.com/qemu-project/qemu/-/issues/641
> > >
> > > This regression is significant, because it has broken the out of the
> > > box default configuration that OpenStack uses for booting all VMs.
> > > They add 16 pcie-root-ports by defalt to allow empty slots for device
> > > hotplug under q35 [1].
> >
> >
> > Indeed, oops. Thanks for the report!
>
> We're at soft freeze now and this is still broken in git master.
> I don't recall seeing a fix for this problem on list and no one has
> commented on the bug report.
>
> Is anyone actively working on a fix for this release ?
Yes, I'm working on a fix, going to send it soon.
Best regards, Julia Suvorova.
> If not, I'm going to post a patch to revert to PCI native
> hotplug, because this was a significant regression in 6.1 that
> breaks openstack out of the box and we can't leave it broken
> again for 6.2.
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PULL 2/5] acpi: x86: pcihp: add support hotplug on multifunction bridges
2021-08-03 20:52 [PULL 0/5] pc,pci: bugfixes Michael S. Tsirkin
2021-08-03 20:52 ` [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO Michael S. Tsirkin
@ 2021-08-03 20:52 ` Michael S. Tsirkin
2021-08-03 20:52 ` [PULL 3/5] arm/acpi: allow DSDT changes Michael S. Tsirkin
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2021-08-03 20:52 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Peter Maydell, Laurent, Richard Henderson,
<, Paolo Bonzini, Mammedov, Vivier, Igor Mammedov,
Eduardo Habkost
From: Igor Mammedov <imammedo@redhat.com>
Commit [1] switched PCI hotplug from native to ACPI one by default.
That however breaks hotplug on following CLI that used to work:
-nodefaults -machine q35 \
-device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \
-device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2
where PCI device is hotplugged to pcie-root-port-1 with error on guest side:
ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330)
ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531)
ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531)
ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515)
cause is that QEMU's ACPI hotplug never supported functions other then 0
and due to bug it was generating notification entries for not described
functions.
Technically there is no reason not to describe cold-plugged bridges
(root ports) on functions other then 0, as they similarly to bridge
on function 0 are unpluggable.
So since we need to describe multifunction devices iterate over
fuctions as well. But describe only cold-plugged bridges[root ports]
on functions other than 0 as well.
1)
Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35)
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reported-by: Laurent Vivier <lvivier@redhat.com>
Message-Id: <20210723090424.2092226-1-imammedo@redhat.com>
Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35)<br>
Signed-off-by: Igor Mammedov <<a href="mailto:imammedo@redhat.com" target="_blank">imammedo@redhat.com</a>><br>
Reported-by: Laurent Vivier <<a href="mailto:lvivier@redhat.com" target="_blank">lvivier@redhat.com</a>><br>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/i386/acpi-build.c | 44 ++++++++++++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 17836149fe..a33ac8b91e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
Aml *dev, *notify_method = NULL, *method;
QObject *bsel;
PCIBus *sec;
- int i;
+ int devfn;
bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
if (bsel) {
@@ -384,23 +384,31 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED);
}
- for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
+ for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
DeviceClass *dc;
PCIDeviceClass *pc;
- PCIDevice *pdev = bus->devices[i];
- int slot = PCI_SLOT(i);
+ PCIDevice *pdev = bus->devices[devfn];
+ int slot = PCI_SLOT(devfn);
+ int func = PCI_FUNC(devfn);
+ /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */
+ int adr = slot << 16 | func;
bool hotplug_enabled_dev;
bool bridge_in_acpi;
bool cold_plugged_bridge;
if (!pdev) {
- if (bsel) { /* add hotplug slots for non present devices */
+ /*
+ * add hotplug slots for non present devices.
+ * hotplug is supported only for non-multifunction device
+ * so generate device description only for function 0
+ */
+ if (bsel && !func) {
if (pci_bus_is_express(bus) && slot > 0) {
break;
}
- dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
+ dev = aml_device("S%.02X", devfn);
aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
- aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
+ aml_append(dev, aml_name_decl("_ADR", aml_int(adr)));
method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
aml_append(method,
aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN"))
@@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
continue;
}
- /* start to compose PCI slot descriptor */
- dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
- aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
+ /*
+ * allow describing coldplugged bridges in ACPI even if they are not
+ * on function 0, as they are not unpluggable, for all other devices
+ * generate description only for function 0 per slot
+ */
+ if (func && !bridge_in_acpi) {
+ continue;
+ }
+
+ /* start to compose PCI device descriptor */
+ dev = aml_device("S%.02X", devfn);
+ aml_append(dev, aml_name_decl("_ADR", aml_int(adr)));
if (bsel) {
/*
@@ -496,7 +513,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
}
- /* slot descriptor has been composed, add it into parent context */
+ /* device descriptor has been composed, add it into parent context */
aml_append(parent_scope, dev);
}
@@ -525,13 +542,12 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
/* Notify about child bus events in any case */
if (pcihp_bridge_en) {
QLIST_FOREACH(sec, &bus->child, sibling) {
- int32_t devfn = sec->parent_dev->devfn;
-
if (pci_bus_is_root(sec)) {
continue;
}
- aml_append(method, aml_name("^S%.02X.PCNT", devfn));
+ aml_append(method, aml_name("^S%.02X.PCNT",
+ sec->parent_dev->devfn));
}
}
--
MST
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PULL 3/5] arm/acpi: allow DSDT changes
2021-08-03 20:52 [PULL 0/5] pc,pci: bugfixes Michael S. Tsirkin
2021-08-03 20:52 ` [PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO Michael S. Tsirkin
2021-08-03 20:52 ` [PULL 2/5] acpi: x86: pcihp: add support hotplug on multifunction bridges Michael S. Tsirkin
@ 2021-08-03 20:52 ` Michael S. Tsirkin
2021-08-03 20:52 ` [PULL 4/5] Revert "acpi/gpex: Inform os to keep firmware resource map" Michael S. Tsirkin
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2021-08-03 20:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Igor Mammedov
We are going to commit ccee1a8140 ("acpi: Update _DSM method in expected files").
Allow changes to DSDT on ARM. Only configs with pci are
affected thus all virt variants but for microvm only the pcie variant.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..8a7fe463c5 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,6 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/DSDT",
+"tests/data/acpi/virt/DSDT.memhp",
+"tests/data/acpi/virt/DSDT.numamem",
+"tests/data/acpi/virt/DSDT.pxb",
+"tests/data/acpi/microvm/DSDT.pcie",
--
MST
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PULL 4/5] Revert "acpi/gpex: Inform os to keep firmware resource map"
2021-08-03 20:52 [PULL 0/5] pc,pci: bugfixes Michael S. Tsirkin
` (2 preceding siblings ...)
2021-08-03 20:52 ` [PULL 3/5] arm/acpi: allow DSDT changes Michael S. Tsirkin
@ 2021-08-03 20:52 ` Michael S. Tsirkin
2021-08-03 20:52 ` [PULL 5/5] Drop _DSM 5 from expected DSDTs on ARM Michael S. Tsirkin
2021-08-04 15:52 ` [PULL 0/5] pc,pci: bugfixes Peter Maydell
5 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2021-08-03 20:52 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Jiahui Cen, Yubo Miao, Gerd Hoffmann,
Igor Mammedov, Ard Biesheuvel, Guenter Roeck
This reverts commit 0cf8882fd06ba0aeb1e90fa6f23fce85504d7e14.
Which this commit, with aarch64 when using efi PCI devices with IO ports
do not work. The reason is that EFI creates I/O port mappings below
0x1000 (in fact, at 0). However Linux, for legacy reasons, does not
support I/O ports <= 0x1000 on PCI, so the I/O assignment created by EFI
is rejected.
EFI creates the mappings primarily for itself, and up until DSM #5
started to be enforced, all PCI resource allocations that existed at
boot were ignored by Linux and recreated from scratch.
Also, the commit in question looks dubious - it seems unlikely that
Linux would fail to create a resource tree. What does
happen is that BARs get moved around, which may cause trouble in some
cases: for instance, Linux had to add special code to the EFI framebuffer
driver to copy with framebuffer BARs being relocated.
DSM #5 has a long history of debate and misinterpretation.
Link: https://lore.kernel.org/r/20210724185234.GA2265457@roeck-us.net/
Fixes: 0cf8882fd06 ("acpi/gpex: Inform os to keep firmware resource map")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/pci-host/gpex-acpi.c | 20 ++------------------
1 file changed, 2 insertions(+), 18 deletions(-)
diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
index 0f01f13a6e..e7e162a00a 100644
--- a/hw/pci-host/gpex-acpi.c
+++ b/hw/pci-host/gpex-acpi.c
@@ -112,26 +112,10 @@ static void acpi_dsdt_add_pci_osc(Aml *dev)
UUID = aml_touuid("E5C937D0-3553-4D7A-9117-EA4D19C3434D");
ifctx = aml_if(aml_equal(aml_arg(0), UUID));
ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(0)));
- uint8_t byte_list[] = {
- 0x1 << 0 /* support for functions other than function 0 */ |
- 0x1 << 5 /* support for function 5 */
- };
- buf = aml_buffer(ARRAY_SIZE(byte_list), byte_list);
+ uint8_t byte_list[1] = {1};
+ buf = aml_buffer(1, byte_list);
aml_append(ifctx1, aml_return(buf));
aml_append(ifctx, ifctx1);
-
- /*
- * PCI Firmware Specification 3.1
- * 4.6.5. _DSM for Ignoring PCI Boot Configurations
- */
- /* Arg2: Function Index: 5 */
- ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(5)));
- /*
- * 0 - The operating system must not ignore the PCI configuration that
- * firmware has done at boot time.
- */
- aml_append(ifctx1, aml_return(aml_int(0)));
- aml_append(ifctx, ifctx1);
aml_append(method, ifctx);
byte_list[0] = 0;
--
MST
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PULL 5/5] Drop _DSM 5 from expected DSDTs on ARM
2021-08-03 20:52 [PULL 0/5] pc,pci: bugfixes Michael S. Tsirkin
` (3 preceding siblings ...)
2021-08-03 20:52 ` [PULL 4/5] Revert "acpi/gpex: Inform os to keep firmware resource map" Michael S. Tsirkin
@ 2021-08-03 20:52 ` Michael S. Tsirkin
2021-08-04 15:52 ` [PULL 0/5] pc,pci: bugfixes Peter Maydell
5 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2021-08-03 20:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Igor Mammedov
diff -rup /tmp/old/tests/data/acpi/microvm/DSDT.pcie.dsl /tmp/new/tests/data/acpi/microvm/DSDT.pcie.dsl
--- /tmp/old/tests/data/acpi/microvm/DSDT.pcie.dsl 2021-08-03 16:22:52.289295442 -0400
+++ /tmp/new/tests/data/acpi/microvm/DSDT.pcie.dsl 2021-08-03 16:22:40.102286317 -0400
@@ -1302,14 +1302,9 @@ DefinitionBlock ("", "DSDT", 2, "BOCHS "
{
Return (Buffer (One)
{
- 0x21 // !
+ 0x01 // .
})
}
-
- If ((Arg2 == 0x05))
- {
- Return (Zero)
- }
}
Return (Buffer (One)
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 5 -----
tests/data/acpi/microvm/DSDT.pcie | Bin 3031 -> 3023 bytes
tests/data/acpi/virt/DSDT | Bin 5204 -> 5196 bytes
tests/data/acpi/virt/DSDT.memhp | Bin 6565 -> 6557 bytes
tests/data/acpi/virt/DSDT.numamem | Bin 5204 -> 5196 bytes
tests/data/acpi/virt/DSDT.pxb | Bin 7695 -> 7679 bytes
6 files changed, 5 deletions(-)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 8a7fe463c5..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,6 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/virt/DSDT",
-"tests/data/acpi/virt/DSDT.memhp",
-"tests/data/acpi/virt/DSDT.numamem",
-"tests/data/acpi/virt/DSDT.pxb",
-"tests/data/acpi/microvm/DSDT.pcie",
diff --git a/tests/data/acpi/microvm/DSDT.pcie b/tests/data/acpi/microvm/DSDT.pcie
index 3fb373fd970f0a33f30f57b1917720754396f0e9..765f14ef3d1e54d3cadccbf0a880f8adb73b3f1f 100644
GIT binary patch
delta 51
zcmcaEeqNl*CD<k8JU0Ua(~XH-(oCK^H>#U6GCFLIXMD`bp&RcK?8~x1ak3Y;JR{@e
HBJNZGj^hr8
delta 59
zcmX>veqEf)CD<k8IyVCYlgC6ZX(q>A8`aGj89g?~Gd||zFpYN!_GMY1IoXR_o>OrF
P`{XPx)+G#+v$#_M_<|6J
diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
index 134d8ae5b602e0aaade6756b99c9abca45279284..c47503990715d389914fdf9c8bccb510761741ac 100644
GIT binary patch
delta 50
zcmcbjaYlp7CD<jzM}&caY5qhmX~y=AYLc8xe#;j-a&mF##=8XjvMf-X>?thI$T+!B
G_%Q%n84Z^J
delta 58
zcmX@3aYcj6CD<h-M1+BXiGL!OG-LlpHAzk;w-t*WIk`AY<6VM%Sr%wc_7s-qR9wJ5
OIg5*R3B%+};l}{F`45)>
diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
index 140976b23ebea792bec12435a2a547ac5f5cd8f9..bae36cdd397473afe3923c52f030641a5ab19d5d 100644
GIT binary patch
delta 52
zcmZ2#JlB}ZCD<iot|S8k(}9Ux(o7xGH>yi;GWjiE?8wQ*p&RcK?8~x1ak8hdJR{@g
ILSYj&0ETJ})&Kwi
delta 60
zcmbPhywsS>CD<iosU!mfll??4X{Lr58`UK^ncP+^cI4#ZFpYN!_GMY1IoVTKo>OrF
Q`{XPx)+G#^Glfmq0PR!{)&Kwi
diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
index 134d8ae5b602e0aaade6756b99c9abca45279284..c47503990715d389914fdf9c8bccb510761741ac 100644
GIT binary patch
delta 50
zcmcbjaYlp7CD<jzM}&caY5qhmX~y=AYLc8xe#;j-a&mF##=8XjvMf-X>?thI$T+!B
G_%Q%n84Z^J
delta 58
zcmX@3aYcj6CD<h-M1+BXiGL!OG-LlpHAzk;w-t*WIk`AY<6VM%Sr%wc_7s-qR9wJ5
OIg5*R3B%+};l}{F`45)>
diff --git a/tests/data/acpi/virt/DSDT.pxb b/tests/data/acpi/virt/DSDT.pxb
index 46b9c4cad5d65cb1b578410fb3168b70a05021be..fbd78f44c4785d19759daea909fe6d6f9a6e6b01 100644
GIT binary patch
delta 78
zcmeCT`ESkT66_N4UzUM^Y5znn8OFOC)g?F?9XC60Hgj_5#=8XjvMf-Xd|F7Ji*bn{
YGb2NEli%{ie}uSD<QN$z>&QL^0Gn_Z9smFU
delta 94
zcmexw-EYI?66_MfFUP>ZG;bo84CB3x>Jprco|_#wn>jg5<6VM%Sr%wc{v#tVq_}{6
hauyfs5{4y$%!~}tO>Qd|e-YwBQNsyWGg(IVF#z_a8;k$|
--
MST
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PULL 0/5] pc,pci: bugfixes
2021-08-03 20:52 [PULL 0/5] pc,pci: bugfixes Michael S. Tsirkin
` (4 preceding siblings ...)
2021-08-03 20:52 ` [PULL 5/5] Drop _DSM 5 from expected DSDTs on ARM Michael S. Tsirkin
@ 2021-08-04 15:52 ` Peter Maydell
5 siblings, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2021-08-04 15:52 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: QEMU Developers
On Tue, 3 Aug 2021 at 21:52, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit f2da205cb4142259d9bc6b9d4596ebbe2426fe49:
>
> Update version for v6.1.0-rc1 release (2021-07-27 18:07:52 +0100)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 62a4db5522cbbd8b5309a2949c22f00e5b0138e3:
>
> Drop _DSM 5 from expected DSDTs on ARM (2021-08-03 16:32:35 -0400)
>
> ----------------------------------------------------------------
> pc,pci: bugfixes
>
> Small bugfixes all over the place.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread