Re: [SeaBIOS] [PATCH] hw/pci: reserve IO and mem for pci-2-pci bridges with no devices attached

[Michael S. Tsirkin]

On Mon, Apr 07, 2014 at 01:59:02PM +0300, Marcel Apfelbaum wrote:
> If a pci-2-pci bridge supports hot-plug functionality but there are no devices
> connected to it, reserve IO/mem in order to be able to attach devices
> later. Do not waste space, use minimum allowed.
> Signed-off-by: Marcel Apfelbaum
>
> src/fw/pciinit.c |  3 +++
>  src/hw/pci.c     | 17 +++++++++++++++++
>  src/hw/pci.h     |  1 +
>  3 files changed, 21 insertions(+)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 64f1d41..9b5d7ad 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -677,12 +677,15 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>              continue;
>          struct pci_bus *parent = &busses[pci_bdf_to_bus(s->bus_dev->bdf)];
>          int type;
>    u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC);
>    for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
>        u64 align = (type == PCI_REGION_TYPE_IO) ?
>            PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
>        if (pci_region_align(&s->r[type]) > align)
>             align = pci_region_align(&s->r[type]);
>        u64 sum = pci_region_sum(&s->r[type]);
>        if (!sum && shpc_cap)
>            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);

One thing I'd do is maybe check that the relevant memory type is
enabled in the bridge (probably just by writing fff to base and reading
it back).

This will give hypervisors an option to avoid wasting resources:
e.g. it's uncommon for express devices to claim IO.
> diff --git a/src/hw/pci.c b/src/hw/pci.c
> index caf9265..ee8a7f1 100644
> --- a/src/hw/pci.c
> +++ b/src/hw/pci.c
> @@ -225,6 +225,23 @@ pci_find_init_device(const struct pci_device_id *ids, void *arg)
>      return NULL;
>  }
>
> +u8 pci_find_capability(struct pci_device *pci, u8 cap_id)
> +{
> u8 cap;
> u16 status = pci_config_readw(pci->bdf, PCI_STATUS);
> if (!(status & PCI_STATUS_CAP_LIST))
>    return 0;
> for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap;
>            cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT))
>    if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id)
>        return cap;

I would also limit this to 256 iterations, to make sure
we dont' get into an infinite loop with a broken device.
> return 0;
>
> +}
>
> void
>  pci_reboot(void)
>  {
> diff --git a/src/hw/pci.h b/src/hw/pci.h
> index 167a027..e828225 100644
> --- a/src/hw/pci.h
> +++ b/src/hw/pci.h
> @@ -116,6 +116,7 @@ int pci_init_device(const struct pci_device_id *ids
>                      , struct pci_device *pci, void *arg);
>  struct pci_device *pci_find_init_device(const struct pci_device_id *ids
>                                          , void *arg);
> +u8 pci_find_capability(struct pci_device *pci, u8 cap_id);
>  void pci_reboot(void);
> #endif
>
> 1.8.3.1