Re: [Qemu-devel] [PATCHv2 04/11] libqos: Better handling of PCI legacy IO

[David Gibson]

On Wed, Oct 19, 2016 at 03:33:33PM +0200, Laurent Vivier wrote:
> 
> 
> On 19/10/2016 14:25, David Gibson wrote:
> > The usual model for PCI IO with libqos is to use qpci_iomap() to map a
> > specific BAR for a PCI device, then perform IOs within that BAR using
> > qpci_io_{read,write}*().
> > 
> > However, certain devices also have legacy PCI IO.  In this case, instead of
> > (or as well as) being accessed via PCI BARs, the device can be accessed
> > via certain well-known, fixed addresses in PCI IO space.
> > 
> > Two existing tests use legacy PCI IO, and take different flawed approaches
> > to it:
> >     * tco-test manually constructs a tco_io_base value instead of calling
> >       qpci_iomap(), which assumes internal knowledge of the structure of
> >       the value it shouldn't have
> >     * ide-test uses direct in*() and out*() calls instead of using
> >       qpci_io_*() accessors, meaning it's not portable to non-x86 machine
> >       types.
> > 
> > tco_test uses the libqos PCI code to access the device.  This makes perfect
> > sense for the PCI config space accesses.  However for IO, rather than the
> > usual PCI approach of mapping a PCI BAR, then accessing that, it instead
> > uses the legacy approach of fixed, known addresses in PCI IO space.
> > 
> > That doesn't work very well with the qpci_io_{read,write} functions because
> > we never use qpci_iomap() and so have to make assumptions about the
> > internal encoding of the address tokens iomap() returns.
> > 
> > This patch avoids that, by directly using the bus's pio_{read,write}
> > callbacks, which are defined to take addresses within the PCI IO space.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> 
> This patch makes sense but it is not obvious (at least for me) why you
> are doing this if I don't read patch 11/11 before...

Probably doesn't help that I didn't finish rewriting the commit
message properly.  I've removed no longer relevant stuff about
tco_test and putting some clarifying information about why this is
useful instead.

> 
> Reviewed-by: Laurent Vivier <address@hidden>
> 
> > ---
> >  tests/libqos/pci.c | 5 +++++
> >  tests/libqos/pci.h | 1 +
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> > index bf1c532..98a2e56 100644
> > --- a/tests/libqos/pci.c
> > +++ b/tests/libqos/pci.c
> > @@ -350,6 +350,11 @@ void qpci_iounmap(QPCIDevice *dev, void *data)
> >      /* FIXME */
> >  }
> >  
> > +void *qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
> > +{
> > +    return (void *)(uintptr_t)addr;
> > +}
> > +
> >  void qpci_plug_device_test(const char *driver, const char *id,
> >                             uint8_t slot, const char *opts)
> >  {
> > diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
> > index f6f916d..b6f855e 100644
> > --- a/tests/libqos/pci.h
> > +++ b/tests/libqos/pci.h
> > @@ -94,6 +94,7 @@ void qpci_io_writel(QPCIDevice *dev, void *data, uint32_t 
> > value);
> >  
> >  void *qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr);
> >  void qpci_iounmap(QPCIDevice *dev, void *data);
> > +void *qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
> >  
> >  void qpci_plug_device_test(const char *driver, const char *id,
> >                             uint8_t slot, const char *opts);
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature