Re: [SeaBIOS] [PATCH v4 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init

[Michael S. Tsirkin]

On Sat, Aug 05, 2017 at 11:29:54PM +0300, Aleksandr Bezzubikov wrote:
> In case of Red Hat Generic PCIE Root Port reserve additional buses,
> which number is provided in a vendor-specific capability.
> Signed-off-by: Aleksandr Bezzubikov
>
> src/fw/pciinit.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  src/hw/pci_ids.h |  3 +++
>  2 files changed, 68 insertions(+), 4 deletions(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 864954f..d241d66 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" // qemu_pci_cap
>  #include "list.h" // struct hlist_node
>  #include "malloc.h" // free
>  #include "output.h" // dprintf
> @@ -578,9 +579,42 @@ 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;
>        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;
>            do {
>                cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0);
>            } while (cap &&
>                     pci_config_readb(bdf, cap + PCI_CAP_REDHAT_TYPE) !=
>                            REDHAT_CAP_TYPE_QEMU);
>            if (cap) {
>                u8 cap_len = pci_config_readb(bdf, cap + PCI_CAP_FLAGS);
>                if (cap_len != QEMU_PCI_CAP_SIZE) {
>                    dprintf(1, "PCI: QEMU cap length %d is invalid\n",
>                            cap_len);

I would do cap_len < QEMU_PCI_CAP_SIZE here - this way you can extend
the capability without breaking things.
>                } else {
>                    u32 tmp_res_bus = pci_config_readl(bdf,
>                                               cap + QEMU_PCI_CAP_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);
>    }
>
> @@ -951,11 +985,38 @@ pci_region_map_one_entry(struct pci_region_entry *entry, u64 addr)
>  u16 bdf = entry->dev->bdf;
>  u64 limit = addr + entry->size - 1;
> 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;
>    do {
>        cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0);
>    } while (cap &&
>             pci_config_readb(bdf, cap + PCI_CAP_REDHAT_TYPE) !=
>                    REDHAT_CAP_TYPE_QEMU);
>    if (cap) {
>        u8 cap_len = pci_config_readb(bdf, cap + PCI_CAP_FLAGS);
>        if (cap_len != QEMU_PCI_CAP_SIZE) {
>            dprintf(1, "PCI: QEMU cap length %d is invalid\n",
>                    cap_len);

Same here.
>        } else {
>            u32 offset = cap + QEMU_PCI_CAP_LIMITS_OFFSET + entry->type * 8;

what is this doing exactly? type is an enum so it's a trick. I'd rather
have an inline with a switch. Use it for bus as well.
>            u64 tmp_limit = (pci_config_readl(bdf, offset) |
>                        (u64)pci_config_readl(bdf, offset + 4) << 32);
>            if (tmp_limit != (u64)-1) {
>                tmp_limit += addr - 1;
>                limit = (limit > tmp_limit) ? limit : tmp_limit;
>            }
>        }
>    }
> }
> 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);
>    pci_config_writew(bdf, PCI_IO_BASE_UPPER16, limit >> 16);
>    pci_config_writeb(bdf, PCI_IO_LIMIT, limit >> PCI_IO_SHIFT);
>    pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, 0);
>    pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, limit >> 16);
> }
>    if (entry->type == PCI_REGION_TYPE_MEM) {
>        pci_config_writew(bdf, PCI_MEMORY_BASE, addr >> PCI_MEMORY_SHIFT);
>
> 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