Re: [Qemu-devel] [PATCH V3 2/2] hw/pci: check if pci2pci bridges implement optional limit registers

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Apr 10, 2014 at 04:29:41PM +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 <marcel.a@redhat.com>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>




> ---
>  src/fw/pciinit.c |  9 ++-------
>  src/hw/pci.c     | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/hw/pci.h     |  9 +++++++++
>  3 files changed, 59 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 055353d..27e7b1c 100644
> --- a/src/hw/pci.c
> +++ b/src/hw/pci.c
> @@ -243,6 +243,54 @@ u8 pci_find_capability(struct pci_device *pci, u8 cap_id)
>      return 0;
>  }
>  
> +static int pci_config_writableb(struct pci_device *pci, u32 addr, u8 test_val)

Don't like this name.
pci_config_writable_byte or better see below.

> +{
> +    u8 val;
> +
> +    val = pci_config_readb(pci->bdf, addr);
> +    pci_config_writeb(pci->bdf, addr, test_val);
> +
> +    if (!(pci_config_readb(pci->bdf, addr)))
> +        return 0;

This only works for readonly fields which return 0
on read. Maybe name should reflect this:
how about pci_config_is_reserved() ?

There's no need to pass in test value either,
it makes the interface fragile:
passing e.g. 0x0 as value won't work.

> +
> +    pci_config_writeb(pci->bdf, addr, val);

Practically that's overkill.

Just set base > limit and there won't be
need to save and restore: range is disabled.



> +    return 1;
> +}
> +
> +static int pci_config_writablew(struct pci_device *pci, u32 addr, u16 test_val)
> +{
> +    u16 val;
> +
> +    val = pci_config_readw(pci->bdf, addr);
> +    pci_config_writew(pci->bdf, addr, test_val);
> +
> +    if (!(pci_config_readw(pci->bdf, addr)))
> +        return 0;
> +
> +    pci_config_writew(pci->bdf, addr, val);
> +    return 1;
> +}


This one isn't needed at all.

> +
> +int pci_bridge_has_region(struct pci_device *pci,
> +                          enum pci_region_type region_type)
> +{
> +    if (pci->class != PCI_CLASS_BRIDGE_PCI)
> +        return 0;

Do we really need this test here?

> +
> +    switch (region_type) {
> +    case PCI_REGION_TYPE_IO:
> +        return pci_config_writableb(pci, PCI_IO_BASE, 0xF0) &&
> +               pci_config_writableb(pci, PCI_IO_LIMIT, 0xF0);

I think we should only test base.
Testing limit like this might cause issues as bridge now
claims some resources temporarily.
Also why 0xF0 and not 0xFF? Seems like a random value.


> +    case PCI_REGION_TYPE_PREFMEM:
> +        return pci_config_writablew(pci, PCI_PREF_MEMORY_BASE, 0xFFF0) &&
> +               pci_config_writablew(pci, PCI_PREF_MEMORY_LIMIT, 0xFFF0);
> +    case PCI_REGION_TYPE_MEM:    /* fall through */
> +    default:
> +        return 1;
> +    }
> +
> +    return 1;
> +}
>  
>  void
>  pci_reboot(void)


Same comments apply to prefetch and writeableb,
but also - there is no need to write all bytes.
So really

Basically:

int pci_bridge_has_region(struct pci_device *pci,
> +                          enum pci_region_type region_type)
> +{
> +    if (pci->class != PCI_CLASS_BRIDGE_PCI)
> +        return 0;

Do we really need this test here?

> +
> +    switch (region_type) {
> +    case PCI_REGION_TYPE_IO:
> +        return pci_config_writableb(pci, PCI_IO_BASE, 0xF0) &&
> +               pci_config_writableb(pci, PCI_IO_LIMIT, 0xF0);

I think we should only test base.
Testing limit like this might cause issues as bridge now
claims some resources temporarily.
Also why 0xF0 and not 0xFF? Seems like a random value.


> +    case PCI_REGION_TYPE_PREFMEM:
> +        return pci_config_writablew(pci, PCI_PREF_MEMORY_BASE, 0xFFF0) &&
> +               pci_config_writablew(pci, PCI_PREF_MEMORY_LIMIT, 0xFFF0);
> +    case PCI_REGION_TYPE_MEM:    /* fall through */
> +    default:
> +        return 1;
> +    }
> +
> +    return 1;

	unreacheable code.

> +}




Here's a simpler implementation.

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;
    }

    /* Check that base is writeable. Safe as this increases base so it
     * won't cause bridge to claim new resources.
     */
    pci_config_writeb(pci->bdf, base, 0xFF);
    return !!pci_config_readb(pci->bdf, base);
}




> 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