[SeaBIOS] [PATCH v5 3/3] pci: enable RedHat PCI bridges to reserve additional resource on PCI init
Marcel Apfelbaum
marcel at redhat.com
Sun Aug 13 13:13:51 CEST 2017
On 11/08/2017 2:21, Aleksandr Bezzubikov wrote:
> In case of Red Hat Generic PCIE Root Port reserve additional buses
> and/or IO/MEM/PREF space, which values are provided in a vendor-specific capability.
>
Hi Aleksandr,
> Signed-off-by: Aleksandr Bezzubikov <zuban32s at gmail.com>
> ---
> src/fw/dev-pci.h | 2 +-
> src/fw/pciinit.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++------
> src/hw/pci_ids.h | 3 ++
> 3 files changed, 116 insertions(+), 14 deletions(-)
>
> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
> index cf16b2e..99ccc12 100644
> --- a/src/fw/dev-pci.h
> +++ b/src/fw/dev-pci.h
> @@ -38,7 +38,7 @@
> #define PCI_CAP_REDHAT_TYPE_OFFSET 3
>
> /* List of valid Red Hat vendor-specific capability types */
> -#define REDHAT_CAP_RESOURCE_RESERVE 1
> +#define REDHAT_CAP_RESOURCE_RESERVE 1
Do you need the above chunk? If not, please
get rid if it.
>
>
> /* Offsets of RESOURCE_RESERVE capability fields */
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 864954f..d9aef56 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -15,6 +15,7 @@
> #include "hw/pcidevice.h" // pci_probe_devices
> #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL
> #include "hw/pci_regs.h" // PCI_COMMAND
> +#include "fw/dev-pci.h" // REDHAT_CAP_RESOURCE_RESERVE
> #include "list.h" // struct hlist_node
> #include "malloc.h" // free
> #include "output.h" // dprintf
> @@ -522,6 +523,32 @@ static void pci_bios_init_platform(void)
> }
> }
>
> +static u8 pci_find_resource_reserve_capability(u16 bdf)
> +{
> + if (pci_config_readw(bdf, PCI_VENDOR_ID) == PCI_VENDOR_ID_REDHAT &&
> + pci_config_readw(bdf, PCI_DEVICE_ID) ==
> + PCI_DEVICE_ID_REDHAT_ROOT_PORT) {
> + u8 cap = 0;
> + do {
> + cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, cap);
> + } while (cap &&
> + pci_config_readb(bdf, cap + PCI_CAP_REDHAT_TYPE_OFFSET) !=
> + REDHAT_CAP_RESOURCE_RESERVE);
> + if (cap) {
> + u8 cap_len = pci_config_readb(bdf, cap + PCI_CAP_FLAGS);
> + if (cap_len < RES_RESERVE_CAP_SIZE) {
> + dprintf(1, "PCI: QEMU resource reserve cap length %d is invalid\n",
> + cap_len);
> + }
> + } else {
> + dprintf(1, "PCI: invalid QEMU resource reserve cap offset\n");
> + }
> + return cap;
> + } else {
> + dprintf(1, "PCI: QEMU resource reserve cap not found\n");
> + return 0;
> + }
> +}
>
> /****************************************************************
> * Bus initialization
> @@ -578,9 +605,28 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus)
> pci_bios_init_bus_rec(secbus, pci_bus);
>
> if (subbus != *pci_bus) {
> + u8 res_bus = 0;
> + u8 cap = pci_find_resource_reserve_capability(bdf);
> +
> + if (cap) {
> + u32 tmp_res_bus = pci_config_readl(bdf,
> + cap + RES_RESERVE_BUS_RES);
> + if (tmp_res_bus != (u32)-1) {
> + res_bus = tmp_res_bus & 0xFF;
> + if ((u8)(res_bus + secbus) < secbus ||
> + (u8)(res_bus + secbus) < res_bus) {
> + dprintf(1, "PCI: bus_reserve value %d is invalid\n",
> + res_bus);
> + res_bus = 0;
> + }
> + }
> + res_bus = (*pci_bus > secbus + res_bus) ? *pci_bus
> + : secbus + res_bus;
> + }
> dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n",
> - subbus, *pci_bus);
> - subbus = *pci_bus;
> + subbus, res_bus);
> + subbus = res_bus;
> + *pci_bus = res_bus;
> } else {
> dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus);
> }
> @@ -844,22 +890,74 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> */
> parent = &busses[0];
> int type;
> - u8 pcie_cap = pci_find_capability(s->bus_dev->bdf, PCI_CAP_ID_EXP, 0);
> + u16 bdf = s->bus_dev->bdf;
> + 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);
> for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
> u64 align = (type == PCI_REGION_TYPE_IO) ?
> - PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
> + PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
The above chunk is also not needed.
> if (!pci_bridge_has_region(s->bus_dev, type))
> continue;
> - 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);
> - if (!sum && hotplug_support && !resource_optional)
> - sum = align; /* reserve min size for hot-plug */
> - u64 size = ALIGN(sum, align);
> - int is64 = pci_bios_bridge_region_is64(&s->r[type],
> - s->bus_dev, type);
> + u64 size;
> + int is64;
I think 'is64' flag should be computed as before even if
we have QEMU hints.
The same about alignment. Please see below (1).
> + int qemu_cap_used = 0;
> + if (qemu_cap) {
> + u32 tmp_size;
> + u64 tmp_size_64;
> + switch(type) {
> + case PCI_REGION_TYPE_IO:
> + tmp_size_64 = (pci_config_readl(bdf, qemu_cap + RES_RESERVE_IO) |
> + (u64)pci_config_readl(bdf, qemu_cap + RES_RESERVE_IO + 4) << 32);
> + if (tmp_size_64 != (u64)-1) {
> + size = tmp_size_64;
> + is64 = 0;
> + qemu_cap_used = 1;
> + }
> + break;
> + case PCI_REGION_TYPE_MEM:
> + tmp_size = pci_config_readl(bdf, qemu_cap + RES_RESERVE_MEM);
> + if (tmp_size != (u32)-1) {
> + size = tmp_size;
> + is64 = 0;
> + qemu_cap_used = 1;
> + }
> + break;
> + case PCI_REGION_TYPE_PREFMEM:
> + tmp_size = pci_config_readl(bdf, qemu_cap + RES_RESERVE_PREF_MEM_32);
> + tmp_size_64 = (pci_config_readl(bdf, qemu_cap + RES_RESERVE_PREF_MEM_64) |
> + (u64)pci_config_readl(bdf, qemu_cap + RES_RESERVE_PREF_MEM_64 + 4) << 32);
> + if (tmp_size != (u32)-1 && tmp_size_64 == (u64)-1) {
> + size = tmp_size;
> + is64 = 0;
> + qemu_cap_used = 1;
> + } else if (tmp_size == (u32)-1 && tmp_size_64 != (u64)-1) {
> + size = tmp_size_64;
> + is64 = 1;
> + qemu_cap_used = 1;
> + } else if (tmp_size != (u32)-1 && tmp_size_64 != (u64)-1) {
> + dprintf(1, "PCI: resource reserve cap PREF32 and PREF64"
> + " conflict\n");
> + }
> + break;
> + default:
> + break;
> + }
> + }
> + if (!qemu_cap_used) {
> + if (pci_region_align(&s->r[type]) > align)
> + align = pci_region_align(&s->r[type]);
> + u64 sum = pci_region_sum(&s->r[type]);
Even if qemu_cap_used you still need to compute the sum and take
max (sum, cap_res_size)
in case QEMU hint is smaller than what devices attached to bridge need.
so you use:
size = ALIGN(max(sum,cap_size), align)
> + int resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
> + if (!sum && hotplug_support && !resource_optional)
> + sum = align; /* reserve min size for hot-plug */
> + size = ALIGN(sum, align);
> + is64 = pci_bios_bridge_region_is64(&s->r[type],
> + s->bus_dev, type);
(1) I think you need the last 2 lines for both cases.
> + } else {
> + dprintf(1, "PCI: resource reserve cap used\n");
> + }
> // entry->bar is -1 if the entry represents a bridge region
> struct pci_region_entry *entry = pci_region_create_entry(
> parent, s->bus_dev, -1, size, align, type, is64);
> @@ -951,6 +1049,7 @@ pci_region_map_one_entry(struct pci_region_entry *entry, u64 addr)
>
> u16 bdf = entry->dev->bdf;
> u64 limit = addr + entry->size - 1;
> +
Get rid of this too, please.
> if (entry->type == PCI_REGION_TYPE_IO) {
> pci_config_writeb(bdf, PCI_IO_BASE, addr >> PCI_IO_SHIFT);
> pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
> diff --git a/src/hw/pci_ids.h b/src/hw/pci_ids.h
> index 4ac73b4..38fa2ca 100644
> --- a/src/hw/pci_ids.h
> +++ b/src/hw/pci_ids.h
> @@ -2263,6 +2263,9 @@
> #define PCI_DEVICE_ID_KORENIX_JETCARDF0 0x1600
> #define PCI_DEVICE_ID_KORENIX_JETCARDF1 0x16ff
>
> +#define PCI_VENDOR_ID_REDHAT 0x1b36
> +#define PCI_DEVICE_ID_REDHAT_ROOT_PORT 0x000C
> +
> #define PCI_VENDOR_ID_TEKRAM 0x1de1
> #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29
>
>
Other than taking into account both the capability and the devices
behind the bridge, the patch looks ready.
Thanks for all the work you put into it,
Marcel
More information about the SeaBIOS
mailing list