On Sat, 27 Nov 2021 11:19:35 +0100 Paul Menzel <pmen...@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 <imamm...@redhat.com> > > CC: mapfe...@redhat.com > > CC: kra...@redhat.com > > CC: m...@redhat.com > > CC: lviv...@redhat.com > > CC: jus...@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-le...@seabios.org
