Re: [SeaBIOS] [PATCH V4 2/2] hw/pci: check if pci2pci bridges implement optional limit registers

[Michael S. Tsirkin]

On Thu, Apr 10, 2014 at 08:44:18PM +0300, Marcel Apfelbaum wrote:
> <I/O Base Register, I/O Limit Register> pair and
> <Prefetchable Memory Base Register, Prefetchable Memory Limit Register> pair
> are both optional.
> Do not reserve ranges if the above registers are not implemented.
> Signed-off-by: Marcel Apfelbaum
>
> src/fw/pciinit.c |  9 ++-------
>  src/hw/pci.c     | 34 ++++++++++++++++++++++++++++++++++
>  src/hw/pci.h     |  9 +++++++++
>  3 files changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 9b5d7ad..bbaecd6 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -26,13 +26,6 @@
>  #define PCI_BRIDGE_MEM_MIN    (1<<21)  // 2M == hugepage size
>  #define PCI_BRIDGE_IO_MIN      0x1000  // mandated by pci bridge spec
>
> -enum pci_region_type {
> PCI_REGION_TYPE_IO,
> PCI_REGION_TYPE_MEM,
> PCI_REGION_TYPE_PREFMEM,
> PCI_REGION_TYPE_COUNT,
>
> -};
>
> static const char *region_type_name[] = {
>      [ PCI_REGION_TYPE_IO ]      = "io",
>      [ PCI_REGION_TYPE_MEM ]     = "mem",
> @@ -681,6 +674,8 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>          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_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]);
>
> diff --git a/src/hw/pci.c b/src/hw/pci.c
> index 77cdba2..d5979af 100644
> --- a/src/hw/pci.c
> +++ b/src/hw/pci.c
> @@ -244,6 +244,40 @@ u8 pci_find_capability(struct pci_device *pci, u8 cap_id)
>      return 0;
>  }
>
> +static int pci_config_is_reserved(struct pci_device *pci, u32 addr)

Why u32? It's u8 isn't it?
> +{
> u8 val;
> val = pci_config_readb(pci->bdf, addr);
> pci_config_writeb(pci->bdf, addr, 0xFF);
> if (!(pci_config_readb(pci->bdf, addr)))

() not needed after !
>    return 1;
> pci_config_writeb(pci->bdf, addr, val);
> return 0;
>
> +}

Above read simply saves the register value, write now restores it.
It really should have a comment explaning what each stage does.

But I don't see why we still waste cycles on this save/restore
dance. This is simply the result of making the API too generic.
Open-code it below, and add a comment:

/* Test whether bridge support forwarding of transactions
 * of a specific type.
 * Note: disables bridge's window registers as a side effect.
 */
int pci_bridge_has_region(struct pci_device *pci,
                          enum pci_region_type region_type)
{
    u8 base;

switch (region_type) {
    case PCI_REGION_TYPE_IO:
        base = PCI_IO_BASE;
        break;
    case PCI_REGION_TYPE_PREFMEM:
        base = PCI_PREF_MEMORY_BASE;
        break;
    default:
        /* Regular memory support is mandatory */
        return 1;
    }

pci_config_writeb(pci->bdf, addr, 0xFF);

return pci_config_readb(pci->bdf, addr) != 0;
}
> void
>  pci_reboot(void)
>  {
> diff --git a/src/hw/pci.h b/src/hw/pci.h
> index e828225..0aaa84c 100644
> --- a/src/hw/pci.h
> +++ b/src/hw/pci.h
> @@ -12,6 +12,13 @@
>  #define PCI_NUM_REGIONS 7
>  #define PCI_BRIDGE_NUM_REGIONS 2
>
> +enum pci_region_type {
> PCI_REGION_TYPE_IO,
> PCI_REGION_TYPE_MEM,
> PCI_REGION_TYPE_PREFMEM,
> PCI_REGION_TYPE_COUNT,
>
> +};
>
> static inline u8 pci_bdf_to_bus(u16 bdf) {
>      return bdf >> 8;
>  }
> @@ -117,6 +124,8 @@ int pci_init_device(const struct pci_device_id *ids
>  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);
> +int pci_bridge_has_region(struct pci_device *pci,
>                      enum pci_region_type region_type);
>
> void pci_reboot(void);
> #endif
>
> 1.8.3.1