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

[Marcel Apfelbaum]

On Thu, 2014-04-10 at 18:46 +0300, Michael S. Tsirkin wrote:
> 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 <address@hidden>
> 
> 
> Reviewed-by: Michael S. Tsirkin <address@hidden>
> 
> 
> 
> 
> > ---
> >  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() ?
No problem with that

> 
> There's no need to pass in test value either,
> it makes the interface fragile:
> passing e.g. 0x0 as value won't work.
Yes, the value would be the caller responsibility.
But I think that a value like 0xF0 (explained below)
will cover all we need.

> 
> > +
> > +    pci_config_writeb(pci->bdf, addr, val);
> 
> Practically that's overkill.
I have to restore prev settings. I do not see this as
an overkill: a query should preserve the prev value.
And is also not expensive, and it is made only by bridges,
not a real issue from my point of view.

> 
> Just set base > limit and there won't be
> need to save and restore: range is disabled.
That would be possible, yes. But again, I don't want
to change values in a query.

> 
> 
> 
> > +    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.
I can use the byte function, yes, thanks, I'll drop it.

> 
> > +
> > +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?
Better safe than sorry. If is not a bridge this will mess up
pci device's configuration and the bug will be hard to find.

> 
> > +
> > +    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.
I can test only one, yes. The spec said that both should be 0,
but going for one of them should be enough.

> Also why 0xF0 and not 0xFF? Seems like a random value.
0xF0 is not random, the last 4 bits are *always* read only, this
is why I don't test them. Any value starting from 0x10 will do.
0xFF it will still work, as the last 4 bits will be silently
dropped, but why not follow the spec. 
 
> 
> 
> > +    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)
> 
[...]

> > +    }
> > +
> > +    return 1;
> 
>       unreacheable code.
Right. Some compilation flags and automatic check tools might
have a problem with that, this why I left it there. I can remove
it, of course.
 
> 
> > +}
> 
> 
> 
> 
> 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.
Yes, but it can claim less. Why mess with the orig value for
a query? I think a query should not modify a value, even if does
not affect the system. It is hard to track this kind of changes,
and restoring the value is not a big price to pay IMHO.

Thanks for the review,
Marcel 
 
>      */
>     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