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