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