[SeaBIOS] Re: [PATCH v2] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX
Hi Gerd, Yes, you are right. I think reading these two config register is fine during init and align with the virtio blk spec. In the near future, we will working on some related work on these two value. To be specific, we will do some modification in virtio blk driver. If driver reads data larger than VIRTIO_BLK_F_SIZE_MAX, it will cause some issue to the DMA engine. To be frankly, I think our future work will have more discussion, so I think it is better to merge these two register reading first. -Original Message- From: Gerd Hoffmann Sent: Monday, November 29, 2021 6:53 PM To: Pei, Andy Cc: seabios@seabios.org; ke...@koconnor.net; Ding Limin Subject: Re: [PATCH v2] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX On Fri, Nov 26, 2021 at 04:06:23PM +0800, Andy Pei wrote: > according to virtio spec, add feature VIRTIO_BLK_F_SIZE_MAX and > VIRTIO_BLK_F_SEG_MAX parse to virtio blk driver. Why is this needed? The code never actually uses max_segment_size and max_segments (other than logging the values) ... take care, Gerd ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH v3 2/2] pci: let firmware reserve IO for pcie-pci-bridge
On Mon, Nov 29, 2021 at 06:48:12AM -0500, Igor Mammedov wrote:
> With [1] patch hotplug of rtl8139 succeeds, with caveat that it
> fails to initialize IO bar, which is caused by [2] that makes
> firmware skip IO reservation for any PCIe device, which isn't
> correct in case of pcie-pci-bridge.
> Fix it by exposing hotplug type and making IO resource optional
> only if PCIe hotplug is in use.
>
> [1]
> "pci: reserve resources for pcie-pci-bridge to fix regressed hotplug on q35"
> [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
Acked-by: Michael S. Tsirkin
> ---
> src/fw/pciinit.c | 19 ++-
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 7342d8d8..badf13d3 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;
>
> @@ -819,12 +825,13 @@ static int pci_bus_hotplug_support(struct pci_bus *bus,
> u8 pcie_cap)
> */
> u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
>
> -return downstream_port && slot_implemented;
> +return downstream_port && slot_implemented ?
> +HOTPLUG_PCIE : HOTPLUG_NO_SUPPORTED;
> }
>
> check_shpc:
> 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
> @@ -909,7 +916,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;
> @@ -953,7 +960,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-le...@seabios.org
[SeaBIOS] Re: [PATCH v3 1/2] pci: reserve resources for pcie-pci-bridge to fix regressed hotplug on q35
On Mon, Nov 29, 2021 at 06:48:11AM -0500, Igor Mammedov wrote:
> If QEMU is started with unpopulated pcie-pci-bridge with ACPI PCI
> hotplug enabled (default since QEMU-6.1), hotplugging a PCI device
> into one of the bridge slots fails due to lack of resources.
>
> 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 :01:02.0: [10ec:8139] type 00 class 0x02
> pci :01:02.0: reg 0x10: [io 0x-0x00ff]
> pci :01:02.0: reg 0x14: [mem 0x-0x00ff]
> pci :01:02.0: reg 0x30: [mem 0x-0x0003 pref]
> pci :01:02.0: BAR 6: no space for [mem size 0x0004 pref]
> pci :01:02.0: BAR 6: failed to assign [mem size 0x0004 pref]
> pci :01:02.0: BAR 0: no space for [io size 0x0100]
> pci :01:02.0: BAR 0: failed to assign [io size 0x0100]
> pci :01:02.0: BAR 1: no space for [mem size 0x0100]
> pci :01:02.0: BAR 1: failed to assign [mem size 0x0100]
> 8139cp: 8139cp: 10/100 PCI Ethernet driver v1.3 (Mar 22, 2004)
> PCI Interrupt Link [GSIG] enabled at IRQ 22
> 8139cp :01:02.0: no MMIO resource
> 8139cp: probe of :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.
>
> Fixes regression in QEMU-6.1 and later, since it was 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")
> 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
Acked-by: Michael S. Tsirkin
> ---
> src/fw/pciinit.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index d25931bb..7342d8d8 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -802,6 +802,10 @@ static int pci_bus_hotplug_support(struct pci_bus *bus,
> u8 pcie_cap)
>pcie_cap + PCI_EXP_FLAGS);
> u8 port_type = ((pcie_flags & PCI_EXP_FLAGS_TYPE) >>
> (__builtin_ffs(PCI_EXP_FLAGS_TYPE) - 1));
> +
> +if (port_type == PCI_EXP_TYPE_PCI_BRIDGE)
> +goto check_shpc;
> +
> u8 downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
> (port_type == PCI_EXP_TYPE_ROOT_PORT);
> /*
> @@ -818,6 +822,7 @@ static int pci_bus_hotplug_support(struct pci_bus *bus,
> u8 pcie_cap)
> return downstream_port && slot_implemented;
> }
>
> +check_shpc:
> shpc_cap = pci_find_capability(bus->bus_dev->bdf, PCI_CAP_ID_SHPC, 0);
> return !!shpc_cap;
> }
> --
> 2.27.0
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] [PATCH v3 2/2] pci: let firmware reserve IO for pcie-pci-bridge
With [1] patch hotplug of rtl8139 succeeds, with caveat that it
fails to initialize IO bar, which is caused by [2] that makes
firmware skip IO reservation for any PCIe device, which isn't
correct in case of pcie-pci-bridge.
Fix it by exposing hotplug type and making IO resource optional
only if PCIe hotplug is in use.
[1]
"pci: reserve resources for pcie-pci-bridge to fix regressed hotplug on q35"
[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 | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index 7342d8d8..badf13d3 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;
@@ -819,12 +825,13 @@ static int pci_bus_hotplug_support(struct pci_bus *bus,
u8 pcie_cap)
*/
u16 slot_implemented = pcie_flags & PCI_EXP_FLAGS_SLOT;
-return downstream_port && slot_implemented;
+return downstream_port && slot_implemented ?
+HOTPLUG_PCIE : HOTPLUG_NO_SUPPORTED;
}
check_shpc:
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
@@ -909,7 +916,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;
@@ -953,7 +960,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-le...@seabios.org
[SeaBIOS] [PATCH v3 1/2] pci: reserve resources for pcie-pci-bridge to fix regressed hotplug on q35
If QEMU is started with unpopulated pcie-pci-bridge with ACPI PCI
hotplug enabled (default since QEMU-6.1), hotplugging a PCI device
into one of the bridge slots fails due to lack of resources.
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 :01:02.0: [10ec:8139] type 00 class 0x02
pci :01:02.0: reg 0x10: [io 0x-0x00ff]
pci :01:02.0: reg 0x14: [mem 0x-0x00ff]
pci :01:02.0: reg 0x30: [mem 0x-0x0003 pref]
pci :01:02.0: BAR 6: no space for [mem size 0x0004 pref]
pci :01:02.0: BAR 6: failed to assign [mem size 0x0004 pref]
pci :01:02.0: BAR 0: no space for [io size 0x0100]
pci :01:02.0: BAR 0: failed to assign [io size 0x0100]
pci :01:02.0: BAR 1: no space for [mem size 0x0100]
pci :01:02.0: BAR 1: failed to assign [mem size 0x0100]
8139cp: 8139cp: 10/100 PCI Ethernet driver v1.3 (Mar 22, 2004)
PCI Interrupt Link [GSIG] enabled at IRQ 22
8139cp :01:02.0: no MMIO resource
8139cp: probe of :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.
Fixes regression in QEMU-6.1 and later, since it was 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")
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 | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index d25931bb..7342d8d8 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -802,6 +802,10 @@ static int pci_bus_hotplug_support(struct pci_bus *bus, u8
pcie_cap)
pcie_cap + PCI_EXP_FLAGS);
u8 port_type = ((pcie_flags & PCI_EXP_FLAGS_TYPE) >>
(__builtin_ffs(PCI_EXP_FLAGS_TYPE) - 1));
+
+if (port_type == PCI_EXP_TYPE_PCI_BRIDGE)
+goto check_shpc;
+
u8 downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
(port_type == PCI_EXP_TYPE_ROOT_PORT);
/*
@@ -818,6 +822,7 @@ static int pci_bus_hotplug_support(struct pci_bus *bus, u8
pcie_cap)
return downstream_port && slot_implemented;
}
+check_shpc:
shpc_cap = pci_find_capability(bus->bus_dev->bdf, PCI_CAP_ID_SHPC, 0);
return !!shpc_cap;
}
--
2.27.0
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] [PATCH v3 0/2] pci: reserve resources for pcie-pci-bridge to fix regressed hotplug on q35 with ACPI hotplug enabled
Since QEMU-6.1, Q35 machine is using ACPI PCI hotplug by default. However SeaBIOS did not take in account pcie-pci-bridge and treated it as any other PCIe device which is not correct in the case of the bridge. Patch [1/2] fixes main issue by treating the bridge as a device with possible SHPC capability. Patch [2/2] fixes missed IO reservation, by making sure that in the case of the bridge firmware reserves IO resource. Igor Mammedov (2): pci: reserve resources for pcie-pci-bridge to fix regressed hotplug on q35 pci: let firmware reserve IO for pcie-pci-bridge src/fw/pciinit.c | 24 +++- 1 file changed, 19 insertions(+), 5 deletions(-) -- 2.27.0 ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH v2] pci: reserve resources for pcie-pci-bridge to fix regressed hotplug on q35 with ACPI hotplug enabled
Hi,
> 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.
Fix #1
> With this hotplug of rtl8139 succeeds, with caveat that it fails
> 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.
Fix #2
Can we make them two separate patches please?
> +switch (port_type) {
> +case PCI_EXP_TYPE_PCI_BRIDGE:
> +/* do nothing and check later if SHPC is enabled */
> +break;
if (port_type == PCI_EXP_TYPE_PCI_BRIDGE)
goto check_shcp;
check_shpc:
> shpc_cap = pci_find_capability(bus->bus_dev->bdf, PCI_CAP_ID_SHPC, 0);
I think this would be more readable.
thanks,
Gerd
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH v2] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX
On Fri, Nov 26, 2021 at 04:06:23PM +0800, Andy Pei wrote: > according to virtio spec, add feature VIRTIO_BLK_F_SIZE_MAX > and VIRTIO_BLK_F_SEG_MAX parse to virtio blk driver. Why is this needed? The code never actually uses max_segment_size and max_segments (other than logging the values) ... take care, Gerd ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
