| Message ID | 1476879941-14360-5-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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 <david@gibson.dropbear.id.au> 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... Reviewed-by: Laurent Vivier <lvivier@redhat.com> > --- > 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); >
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 <david@gibson.dropbear.id.au> > > 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 <lvivier@redhat.com> > > > --- > > 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); > > >
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);
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 <david@gibson.dropbear.id.au> --- tests/libqos/pci.c | 5 +++++ tests/libqos/pci.h | 1 + 2 files changed, 6 insertions(+)