src/fw/pciinit.c | 49 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 16 deletions(-)
hotplug of a PCI device to empty at startup pcie-pci-bridge fails when
ACPI PCI hotplug (default since 6.1) is enabled due to lack of resources.
Reproduced with:
#QEMU-6.2-rc1 -machine q35 -device pcie-pci-bridge,id=pcie-pci-bridge-0,addr=0x4
once linux guest is booted (test used Fedora 34), hotplug NIC from
QEMU monitor:
(qemu) device_add rtl8139,bus=pcie-pci-bridge-0,addr=0x2
guest fails hotplug with:
pci 0000:01:02.0: [10ec:8139] type 00 class 0x020000
pci 0000:01:02.0: reg 0x10: [io 0x0000-0x00ff]
pci 0000:01:02.0: reg 0x14: [mem 0x00000000-0x000000ff]
pci 0000:01:02.0: reg 0x30: [mem 0x00000000-0x0003ffff pref]
pci 0000:01:02.0: BAR 6: no space for [mem size 0x00040000 pref]
pci 0000:01:02.0: BAR 6: failed to assign [mem size 0x00040000 pref]
pci 0000:01:02.0: BAR 0: no space for [io size 0x0100]
pci 0000:01:02.0: BAR 0: failed to assign [io size 0x0100]
pci 0000:01:02.0: BAR 1: no space for [mem size 0x00000100]
pci 0000:01:02.0: BAR 1: failed to assign [mem size 0x00000100]
8139cp: 8139cp: 10/100 PCI Ethernet driver v1.3 (Mar 22, 2004)
PCI Interrupt Link [GSIG] enabled at IRQ 22
8139cp 0000:01:02.0: no MMIO resource
8139cp: probe of 0000:01:02.0 failed with error -5
Reason for this is that commit [1] didn't take into account
pcie-pci-bridge, marking bridge as non hotpluggable instead of
handling it as possibly SHPC capable bridge.
Fix issue by checking if pcie-pci-bridge is SHPC capable and
if it is mark it as hotpluggable.
With this hotplug of rtl8139 succeeds, with caveat that it fail
initialize IO bar, which is caused by [2] which makes firmware
skip IO reservation for any PCI device which isn't correct in case
of pcie-pci-bridge.
Fix it by exposing hotplug type and making resource optional
only if PCIe hotplug is in use.
Fixes regression in QEMU-6.1 and later, since it's switched to ACPI
based PCI hotplug on Q35 by default at that time.
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2001732
[1]
Fixes: 3aa31d7d637 ("hw/pci: reserve IO and mem for pci express downstream ports with no devices attached")
[2]
Fixes: 76327b9f32a ("fw/pci: do not automatically allocate IO region for PCIe bridges")
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
CC: mapfelba@redhat.com
CC: kraxel@redhat.com
CC: mst@redhat.com
CC: lvivier@redhat.com
CC: jusual@redhat.com
---
src/fw/pciinit.c | 49 ++++++++++++++++++++++++++++++++----------------
1 file changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index d25931bb..3107a0e6 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -793,7 +793,13 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
return entry;
}
-static int pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
+typedef enum hotplug_type_t {
+ HOTPLUG_NO_SUPPORTED = 0,
+ HOTPLUG_PCIE,
+ HOTPLUG_SHPC
+} hotplug_type_t;
+
+static hotplug_type_t pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
{
u8 shpc_cap;
@@ -804,22 +810,31 @@ static int pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
(__builtin_ffs(PCI_EXP_FLAGS_TYPE) - 1));
u8 downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
(port_type == PCI_EXP_TYPE_ROOT_PORT);
- /*
- * PCI Express SPEC, 7.8.2:
- * Slot Implemented – When Set, this bit indicates that the Link
- * HwInit associated with this Port is connected to a slot (as
- * compared to being connected to a system-integrated device or
- * being disabled).
- * This bit is valid for Downstream Ports. This bit is undefined
- * for Upstream Ports.
- */
- u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
-
- return downstream_port && slot_implemented;
+
+ switch (port_type) {
+ case PCI_EXP_TYPE_PCI_BRIDGE:
+ /* fall-through to check if SHPC is enabled on bridge */
+ break;
+ default: {
+ /*
+ * PCI Express SPEC, 7.8.2:
+ * Slot Implemented – When Set, this bit indicates that the Link
+ * HwInit associated with this Port is connected to a slot (as
+ * compared to being connected to a system-integrated device or
+ * being disabled).
+ * This bit is valid for Downstream Ports. This bit is undefined
+ * for Upstream Ports.
+ */
+ u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
+ return downstream_port && slot_implemented ?
+ HOTPLUG_PCIE : HOTPLUG_NO_SUPPORTED;
+ break;
+ }
+ }
}
shpc_cap = pci_find_capability(bus->bus_dev->bdf, PCI_CAP_ID_SHPC, 0);
- return !!shpc_cap;
+ return !!shpc_cap ? HOTPLUG_SHPC : HOTPLUG_NO_SUPPORTED;
}
/* Test whether bridge support forwarding of transactions
@@ -904,7 +919,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
u8 pcie_cap = pci_find_capability(bdf, PCI_CAP_ID_EXP, 0);
u8 qemu_cap = pci_find_resource_reserve_capability(bdf);
- int hotplug_support = pci_bus_hotplug_support(s, pcie_cap);
+ hotplug_type_t hotplug_support = pci_bus_hotplug_support(s, pcie_cap);
for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
u64 align = (type == PCI_REGION_TYPE_IO) ?
PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
@@ -948,7 +963,9 @@ static int pci_bios_check_devices(struct pci_bus *busses)
if (pci_region_align(&s->r[type]) > align)
align = pci_region_align(&s->r[type]);
u64 sum = pci_region_sum(&s->r[type]);
- int resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
+ int resource_optional = 0;
+ if (hotplug_support == HOTPLUG_PCIE)
+ resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
if (!sum && hotplug_support && !resource_optional)
sum = align; /* reserve min size for hot-plug */
if (size > sum) {
--
2.27.0
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
Dear Igor,
Am 26.11.21 um 19:46 schrieb Igor Mammedov:
> hotplug of a PCI device to empty at startup pcie-pci-bridge fails when
“to empty”, what does that mean?
> ACPI PCI hotplug (default since 6.1) is enabled due to lack of resources.
QEMU 6.1?
> Reproduced with:
>
> #QEMU-6.2-rc1 -machine q35 -device pcie-pci-bridge,id=pcie-pci-bridge-0,addr=0x4
>
> once linux guest is booted (test used Fedora 34), hotplug NIC from
> QEMU monitor:
> (qemu) device_add rtl8139,bus=pcie-pci-bridge-0,addr=0x2
>
> guest fails hotplug with:
> pci 0000:01:02.0: [10ec:8139] type 00 class 0x020000
> pci 0000:01:02.0: reg 0x10: [io 0x0000-0x00ff]
> pci 0000:01:02.0: reg 0x14: [mem 0x00000000-0x000000ff]
> pci 0000:01:02.0: reg 0x30: [mem 0x00000000-0x0003ffff pref]
> pci 0000:01:02.0: BAR 6: no space for [mem size 0x00040000 pref]
> pci 0000:01:02.0: BAR 6: failed to assign [mem size 0x00040000 pref]
> pci 0000:01:02.0: BAR 0: no space for [io size 0x0100]
> pci 0000:01:02.0: BAR 0: failed to assign [io size 0x0100]
> pci 0000:01:02.0: BAR 1: no space for [mem size 0x00000100]
> pci 0000:01:02.0: BAR 1: failed to assign [mem size 0x00000100]
> 8139cp: 8139cp: 10/100 PCI Ethernet driver v1.3 (Mar 22, 2004)
> PCI Interrupt Link [GSIG] enabled at IRQ 22
> 8139cp 0000:01:02.0: no MMIO resource
> 8139cp: probe of 0000:01:02.0 failed with error -5
>
> Reason for this is that commit [1] didn't take into account
> pcie-pci-bridge, marking bridge as non hotpluggable instead of
> handling it as possibly SHPC capable bridge.
> Fix issue by checking if pcie-pci-bridge is SHPC capable and
> if it is mark it as hotpluggable.
>
> With this hotplug of rtl8139 succeeds, with caveat that it fail
Nit: fail*s* *to*
> initialize IO bar, which is caused by [2] which makes firmware
> skip IO reservation for any PCI device which isn't correct in case
> of pcie-pci-bridge.
> Fix it by exposing hotplug type and making resource optional
> only if PCIe hotplug is in use.
>
> Fixes regression in QEMU-6.1 and later, since it's switched to ACPI
Nit: was switched
> based PCI hotplug on Q35 by default at that time.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2001732
> [1]
> Fixes: 3aa31d7d637 ("hw/pci: reserve IO and mem for pci express downstream ports with no devices attached")
> [2]
> Fixes: 76327b9f32a ("fw/pci: do not automatically allocate IO region for PCIe bridges")
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> CC: mapfelba@redhat.com
> CC: kraxel@redhat.com
> CC: mst@redhat.com
> CC: lvivier@redhat.com
> CC: jusual@redhat.com
> ---
> src/fw/pciinit.c | 49 ++++++++++++++++++++++++++++++++----------------
> 1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index d25931bb..3107a0e6 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -793,7 +793,13 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
> return entry;
> }
>
> -static int pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
> +typedef enum hotplug_type_t {
> + HOTPLUG_NO_SUPPORTED = 0,
> + HOTPLUG_PCIE,
> + HOTPLUG_SHPC
> +} hotplug_type_t;
> +
> +static hotplug_type_t pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
> {
> u8 shpc_cap;
>
> @@ -804,22 +810,31 @@ static int pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
> (__builtin_ffs(PCI_EXP_FLAGS_TYPE) - 1));
> u8 downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
> (port_type == PCI_EXP_TYPE_ROOT_PORT);
> - /*
> - * PCI Express SPEC, 7.8.2:
> - * Slot Implemented – When Set, this bit indicates that the Link
> - * HwInit associated with this Port is connected to a slot (as
> - * compared to being connected to a system-integrated device or
> - * being disabled).
> - * This bit is valid for Downstream Ports. This bit is undefined
> - * for Upstream Ports.
> - */
> - u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
> -
> - return downstream_port && slot_implemented;
> +
> + switch (port_type) {
> + case PCI_EXP_TYPE_PCI_BRIDGE:
> + /* fall-through to check if SHPC is enabled on bridge */
Doesn’t fall-through for switch statements mean, there is no break
statement?
> + break;
> + default: {
> + /*
> + * PCI Express SPEC, 7.8.2:
> + * Slot Implemented – When Set, this bit indicates that the Link
> + * HwInit associated with this Port is connected to a slot (as
> + * compared to being connected to a system-integrated device or
> + * being disabled).
> + * This bit is valid for Downstream Ports. This bit is undefined
> + * for Upstream Ports.
> + */
> + u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
> + return downstream_port && slot_implemented ?
> + HOTPLUG_PCIE : HOTPLUG_NO_SUPPORTED;
> + break;
> + }
> + }
> }
>
> shpc_cap = pci_find_capability(bus->bus_dev->bdf, PCI_CAP_ID_SHPC, 0);
> - return !!shpc_cap;
> + return !!shpc_cap ? HOTPLUG_SHPC : HOTPLUG_NO_SUPPORTED;
> }
>
> /* Test whether bridge support forwarding of transactions
> @@ -904,7 +919,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> u8 pcie_cap = pci_find_capability(bdf, PCI_CAP_ID_EXP, 0);
> u8 qemu_cap = pci_find_resource_reserve_capability(bdf);
>
> - int hotplug_support = pci_bus_hotplug_support(s, pcie_cap);
> + hotplug_type_t hotplug_support = pci_bus_hotplug_support(s, pcie_cap);
> for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
> u64 align = (type == PCI_REGION_TYPE_IO) ?
> PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
> @@ -948,7 +963,9 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> if (pci_region_align(&s->r[type]) > align)
> align = pci_region_align(&s->r[type]);
> u64 sum = pci_region_sum(&s->r[type]);
> - int resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
> + int resource_optional = 0;
> + if (hotplug_support == HOTPLUG_PCIE)
> + resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
> if (!sum && hotplug_support && !resource_optional)
> sum = align; /* reserve min size for hot-plug */
> if (size > sum) {
>
Rest looks good.
Kind regards,
Paul
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
On Sat, 27 Nov 2021 11:19:35 +0100
Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> Dear Igor,
>
>
> Am 26.11.21 um 19:46 schrieb Igor Mammedov:
> > hotplug of a PCI device to empty at startup pcie-pci-bridge fails when
>
> “to empty”, what does that mean?
meaning is pcie-pci-bridge that does not have any devices plugged into it.
maybe 'not populated' would be more clear?
>
> > ACPI PCI hotplug (default since 6.1) is enabled due to lack of resources.
>
> QEMU 6.1?
>
> > Reproduced with:
> >
> > #QEMU-6.2-rc1 -machine q35 -device pcie-pci-bridge,id=pcie-pci-bridge-0,addr=0x4
> >
> > once linux guest is booted (test used Fedora 34), hotplug NIC from
> > QEMU monitor:
> > (qemu) device_add rtl8139,bus=pcie-pci-bridge-0,addr=0x2
> >
> > guest fails hotplug with:
> > pci 0000:01:02.0: [10ec:8139] type 00 class 0x020000
> > pci 0000:01:02.0: reg 0x10: [io 0x0000-0x00ff]
> > pci 0000:01:02.0: reg 0x14: [mem 0x00000000-0x000000ff]
> > pci 0000:01:02.0: reg 0x30: [mem 0x00000000-0x0003ffff pref]
> > pci 0000:01:02.0: BAR 6: no space for [mem size 0x00040000 pref]
> > pci 0000:01:02.0: BAR 6: failed to assign [mem size 0x00040000 pref]
> > pci 0000:01:02.0: BAR 0: no space for [io size 0x0100]
> > pci 0000:01:02.0: BAR 0: failed to assign [io size 0x0100]
> > pci 0000:01:02.0: BAR 1: no space for [mem size 0x00000100]
> > pci 0000:01:02.0: BAR 1: failed to assign [mem size 0x00000100]
> > 8139cp: 8139cp: 10/100 PCI Ethernet driver v1.3 (Mar 22, 2004)
> > PCI Interrupt Link [GSIG] enabled at IRQ 22
> > 8139cp 0000:01:02.0: no MMIO resource
> > 8139cp: probe of 0000:01:02.0 failed with error -5
> >
> > Reason for this is that commit [1] didn't take into account
> > pcie-pci-bridge, marking bridge as non hotpluggable instead of
> > handling it as possibly SHPC capable bridge.
> > Fix issue by checking if pcie-pci-bridge is SHPC capable and
> > if it is mark it as hotpluggable.
> >
> > With this hotplug of rtl8139 succeeds, with caveat that it fail
>
> Nit: fail*s* *to*
>
> > initialize IO bar, which is caused by [2] which makes firmware
> > skip IO reservation for any PCI device which isn't correct in case
> > of pcie-pci-bridge.
> > Fix it by exposing hotplug type and making resource optional
> > only if PCIe hotplug is in use.
> >
> > Fixes regression in QEMU-6.1 and later, since it's switched to ACPI
>
> Nit: was switched
>
> > based PCI hotplug on Q35 by default at that time.
> >
> > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2001732
> > [1]
> > Fixes: 3aa31d7d637 ("hw/pci: reserve IO and mem for pci express downstream ports with no devices attached")
> > [2]
> > Fixes: 76327b9f32a ("fw/pci: do not automatically allocate IO region for PCIe bridges")
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > CC: mapfelba@redhat.com
> > CC: kraxel@redhat.com
> > CC: mst@redhat.com
> > CC: lvivier@redhat.com
> > CC: jusual@redhat.com
> > ---
> > src/fw/pciinit.c | 49 ++++++++++++++++++++++++++++++++----------------
> > 1 file changed, 33 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> > index d25931bb..3107a0e6 100644
> > --- a/src/fw/pciinit.c
> > +++ b/src/fw/pciinit.c
> > @@ -793,7 +793,13 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
> > return entry;
> > }
> >
> > -static int pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
> > +typedef enum hotplug_type_t {
> > + HOTPLUG_NO_SUPPORTED = 0,
> > + HOTPLUG_PCIE,
> > + HOTPLUG_SHPC
> > +} hotplug_type_t;
> > +
> > +static hotplug_type_t pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
> > {
> > u8 shpc_cap;
> >
> > @@ -804,22 +810,31 @@ static int pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
> > (__builtin_ffs(PCI_EXP_FLAGS_TYPE) - 1));
> > u8 downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
> > (port_type == PCI_EXP_TYPE_ROOT_PORT);
> > - /*
> > - * PCI Express SPEC, 7.8.2:
> > - * Slot Implemented – When Set, this bit indicates that the Link
> > - * HwInit associated with this Port is connected to a slot (as
> > - * compared to being connected to a system-integrated device or
> > - * being disabled).
> > - * This bit is valid for Downstream Ports. This bit is undefined
> > - * for Upstream Ports.
> > - */
> > - u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
> > -
> > - return downstream_port && slot_implemented;
> > +
> > + switch (port_type) {
> > + case PCI_EXP_TYPE_PCI_BRIDGE:
> > + /* fall-through to check if SHPC is enabled on bridge */
>
> Doesn’t fall-through for switch statements mean, there is no break
> statement?
how about
/* do nothing and check later if SHPC is enabled */
>
> > + break;
> > + default: {
> > + /*
> > + * PCI Express SPEC, 7.8.2:
> > + * Slot Implemented – When Set, this bit indicates that the Link
> > + * HwInit associated with this Port is connected to a slot (as
> > + * compared to being connected to a system-integrated device or
> > + * being disabled).
> > + * This bit is valid for Downstream Ports. This bit is undefined
> > + * for Upstream Ports.
> > + */
> > + u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
> > + return downstream_port && slot_implemented ?
> > + HOTPLUG_PCIE : HOTPLUG_NO_SUPPORTED;
> > + break;
> > + }
> > + }
> > }
> >
> > shpc_cap = pci_find_capability(bus->bus_dev->bdf, PCI_CAP_ID_SHPC, 0);
> > - return !!shpc_cap;
> > + return !!shpc_cap ? HOTPLUG_SHPC : HOTPLUG_NO_SUPPORTED;
> > }
> >
> > /* Test whether bridge support forwarding of transactions
> > @@ -904,7 +919,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> > u8 pcie_cap = pci_find_capability(bdf, PCI_CAP_ID_EXP, 0);
> > u8 qemu_cap = pci_find_resource_reserve_capability(bdf);
> >
> > - int hotplug_support = pci_bus_hotplug_support(s, pcie_cap);
> > + hotplug_type_t hotplug_support = pci_bus_hotplug_support(s, pcie_cap);
> > for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
> > u64 align = (type == PCI_REGION_TYPE_IO) ?
> > PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
> > @@ -948,7 +963,9 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> > if (pci_region_align(&s->r[type]) > align)
> > align = pci_region_align(&s->r[type]);
> > u64 sum = pci_region_sum(&s->r[type]);
> > - int resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
> > + int resource_optional = 0;
> > + if (hotplug_support == HOTPLUG_PCIE)
> > + resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
> > if (!sum && hotplug_support && !resource_optional)
> > sum = align; /* reserve min size for hot-plug */
> > if (size > sum) {
> >
>
> Rest looks good.
>
>
> Kind regards,
>
> Paul
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
Dear Igor,
Am 27.11.21 um 12:16 schrieb Igor Mammedov:
> On Sat, 27 Nov 2021 11:19:35 +0100 wrote:
>> Am 26.11.21 um 19:46 schrieb Igor Mammedov:
>>> hotplug of a PCI device to empty at startup pcie-pci-bridge fails when
>>
>> “to empty”, what does that mean?
>
> meaning is pcie-pci-bridge that does not have any devices plugged into it.
> maybe 'not populated' would be more clear?
Ah, the “at startup“ confused me then. Maybe (but I am no native speaker):
Hotplugging a PCI device to a since startup unpopulated pcee-pci-bridge
fails …
>>> ACPI PCI hotplug (default since 6.1) is enabled due to lack of resources.
>>
>> QEMU 6.1?
>>
>>> Reproduced with:
>>>
>>> #QEMU-6.2-rc1 -machine q35 -device pcie-pci-bridge,id=pcie-pci-bridge-0,addr=0x4
>>>
>>> once linux guest is booted (test used Fedora 34), hotplug NIC from
>>> QEMU monitor:
>>> (qemu) device_add rtl8139,bus=pcie-pci-bridge-0,addr=0x2
>>>
>>> guest fails hotplug with:
>>> pci 0000:01:02.0: [10ec:8139] type 00 class 0x020000
>>> pci 0000:01:02.0: reg 0x10: [io 0x0000-0x00ff]
>>> pci 0000:01:02.0: reg 0x14: [mem 0x00000000-0x000000ff]
>>> pci 0000:01:02.0: reg 0x30: [mem 0x00000000-0x0003ffff pref]
>>> pci 0000:01:02.0: BAR 6: no space for [mem size 0x00040000 pref]
>>> pci 0000:01:02.0: BAR 6: failed to assign [mem size 0x00040000 pref]
>>> pci 0000:01:02.0: BAR 0: no space for [io size 0x0100]
>>> pci 0000:01:02.0: BAR 0: failed to assign [io size 0x0100]
>>> pci 0000:01:02.0: BAR 1: no space for [mem size 0x00000100]
>>> pci 0000:01:02.0: BAR 1: failed to assign [mem size 0x00000100]
>>> 8139cp: 8139cp: 10/100 PCI Ethernet driver v1.3 (Mar 22, 2004)
>>> PCI Interrupt Link [GSIG] enabled at IRQ 22
>>> 8139cp 0000:01:02.0: no MMIO resource
>>> 8139cp: probe of 0000:01:02.0 failed with error -5
>>>
>>> Reason for this is that commit [1] didn't take into account
>>> pcie-pci-bridge, marking bridge as non hotpluggable instead of
>>> handling it as possibly SHPC capable bridge.
>>> Fix issue by checking if pcie-pci-bridge is SHPC capable and
>>> if it is mark it as hotpluggable.
>>>
>>> With this hotplug of rtl8139 succeeds, with caveat that it fail
>>
>> Nit: fail*s* *to*
>>
>>> initialize IO bar, which is caused by [2] which makes firmware
>>> skip IO reservation for any PCI device which isn't correct in case
>>> of pcie-pci-bridge.
>>> Fix it by exposing hotplug type and making resource optional
>>> only if PCIe hotplug is in use.
>>>
>>> Fixes regression in QEMU-6.1 and later, since it's switched to ACPI
>>
>> Nit: was switched
>>
>>> based PCI hotplug on Q35 by default at that time.
>>>
>>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2001732
>>> [1]
>>> Fixes: 3aa31d7d637 ("hw/pci: reserve IO and mem for pci express downstream ports with no devices attached")
>>> [2]
>>> Fixes: 76327b9f32a ("fw/pci: do not automatically allocate IO region for PCIe bridges")
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> CC: mapfelba@redhat.com
>>> CC: kraxel@redhat.com
>>> CC: mst@redhat.com
>>> CC: lvivier@redhat.com
>>> CC: jusual@redhat.com
>>> ---
>>> src/fw/pciinit.c | 49 ++++++++++++++++++++++++++++++++----------------
>>> 1 file changed, 33 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
>>> index d25931bb..3107a0e6 100644
>>> --- a/src/fw/pciinit.c
>>> +++ b/src/fw/pciinit.c
>>> @@ -793,7 +793,13 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
>>> return entry;
>>> }
>>>
>>> -static int pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
>>> +typedef enum hotplug_type_t {
>>> + HOTPLUG_NO_SUPPORTED = 0,
>>> + HOTPLUG_PCIE,
>>> + HOTPLUG_SHPC
>>> +} hotplug_type_t;
>>> +
>>> +static hotplug_type_t pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
>>> {
>>> u8 shpc_cap;
>>>
>>> @@ -804,22 +810,31 @@ static int pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap)
>>> (__builtin_ffs(PCI_EXP_FLAGS_TYPE) - 1));
>>> u8 downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
>>> (port_type == PCI_EXP_TYPE_ROOT_PORT);
>>> - /*
>>> - * PCI Express SPEC, 7.8.2:
>>> - * Slot Implemented – When Set, this bit indicates that the Link
>>> - * HwInit associated with this Port is connected to a slot (as
>>> - * compared to being connected to a system-integrated device or
>>> - * being disabled).
>>> - * This bit is valid for Downstream Ports. This bit is undefined
>>> - * for Upstream Ports.
>>> - */
>>> - u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
>>> -
>>> - return downstream_port && slot_implemented;
>>> +
>>> + switch (port_type) {
>>> + case PCI_EXP_TYPE_PCI_BRIDGE:
>>> + /* fall-through to check if SHPC is enabled on bridge */
>>
>> Doesn’t fall-through for switch statements mean, there is no break
>> statement?
>
> how about
> /* do nothing and check later if SHPC is enabled */
Sounds good.
Kind regards,
Paul
>>> + break;
>>> + default: {
>>> + /*
>>> + * PCI Express SPEC, 7.8.2:
>>> + * Slot Implemented – When Set, this bit indicates that the Link
>>> + * HwInit associated with this Port is connected to a slot (as
>>> + * compared to being connected to a system-integrated device or
>>> + * being disabled).
>>> + * This bit is valid for Downstream Ports. This bit is undefined
>>> + * for Upstream Ports.
>>> + */
>>> + u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
>>> + return downstream_port && slot_implemented ?
>>> + HOTPLUG_PCIE : HOTPLUG_NO_SUPPORTED;
>>> + break;
>>> + }
>>> + }
>>> }
>>>
>>> shpc_cap = pci_find_capability(bus->bus_dev->bdf, PCI_CAP_ID_SHPC, 0);
>>> - return !!shpc_cap;
>>> + return !!shpc_cap ? HOTPLUG_SHPC : HOTPLUG_NO_SUPPORTED;
>>> }
>>>
>>> /* Test whether bridge support forwarding of transactions
>>> @@ -904,7 +919,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>>> u8 pcie_cap = pci_find_capability(bdf, PCI_CAP_ID_EXP, 0);
>>> u8 qemu_cap = pci_find_resource_reserve_capability(bdf);
>>>
>>> - int hotplug_support = pci_bus_hotplug_support(s, pcie_cap);
>>> + hotplug_type_t hotplug_support = pci_bus_hotplug_support(s, pcie_cap);
>>> for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
>>> u64 align = (type == PCI_REGION_TYPE_IO) ?
>>> PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
>>> @@ -948,7 +963,9 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>>> if (pci_region_align(&s->r[type]) > align)
>>> align = pci_region_align(&s->r[type]);
>>> u64 sum = pci_region_sum(&s->r[type]);
>>> - int resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
>>> + int resource_optional = 0;
>>> + if (hotplug_support == HOTPLUG_PCIE)
>>> + resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
>>> if (!sum && hotplug_support && !resource_optional)
>>> sum = align; /* reserve min size for hot-plug */
>>> if (size > sum) {
>>>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
© 2016 - 2023 Red Hat, Inc.