On 19/10/2016 14:25, David Gibson wrote:
> The PCI IO space (aka PIO, aka legacy IO) and PCI memory space (aka MMIO)
> are distinct address spaces by the PCI spec (although parts of one might be
> aliased to parts of the other in some cases).
>
> However, qpci_io_read*() and qpci_io_write*() can perform accesses to
> either space depending on parameter. That's convenient for test case
> drivers, since there are a fair few devices which can be controlled via
> either a PIO or MMIO BAR but with an otherwise identical driver.
>
> This is implemented by having addresses below 64kiB treated as PIO, and
> those above treated as MMIO. This works because low addresses in memory
> space are generally reserved for DMA rather than MMIO.
>
> At the moment, this demultiplexing must be handled by each PCI backend
> (pc and spapr, so far). There's no real reason for this - the current
> encoding is likely to work for all platforms, and even if it doesn't we
> can still use a more complex common encoding since the value returned from
> iomap are semi-opaque.
>
> This patch moves the demultiplexing into the common part of the libqos PCI
> code, with the backends having simpler, separate accessors for PIO and
> MMIO space. This also means we have a way of explicitly accessing either
> space if it's necessary for some special case.
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
Identical to the version 1 and my comment was not relevant, so:
Reviewed-by: Laurent Vivier <lviv...@redhat.com>
> ---
> tests/libqos/pci-pc.c | 107 ++++++++++++++++++++----------------------
> tests/libqos/pci-spapr.c | 118
> +++++++++++++++++++++++++----------------------
> tests/libqos/pci.c | 49 +++++++++++++++++---
> tests/libqos/pci.h | 22 ++++++---
> 4 files changed, 170 insertions(+), 126 deletions(-)
>
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 9600ed6..51dff8a 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -36,79 +36,64 @@ typedef struct QPCIBusPC
> uint16_t pci_iohole_alloc;
> } QPCIBusPC;
>
> -static uint8_t qpci_pc_io_readb(QPCIBus *bus, void *addr)
> +static uint8_t qpci_pc_pio_readb(QPCIBus *bus, uint32_t addr)
> {
> - uintptr_t port = (uintptr_t)addr;
> - uint8_t value;
> -
> - if (port < 0x10000) {
> - value = inb(port);
> - } else {
> - value = readb(port);
> - }
> -
> - return value;
> + return inb(addr);
> }
>
> -static uint16_t qpci_pc_io_readw(QPCIBus *bus, void *addr)
> +static uint8_t qpci_pc_mmio_readb(QPCIBus *bus, uint32_t addr)
> {
> - uintptr_t port = (uintptr_t)addr;
> - uint16_t value;
> -
> - if (port < 0x10000) {
> - value = inw(port);
> - } else {
> - value = readw(port);
> - }
> + return readb(addr);
> +}
>
> - return value;
> +static void qpci_pc_pio_writeb(QPCIBus *bus, uint32_t addr, uint8_t val)
> +{
> + outb(addr, val);
> }
>
> -static uint32_t qpci_pc_io_readl(QPCIBus *bus, void *addr)
> +static void qpci_pc_mmio_writeb(QPCIBus *bus, uint32_t addr, uint8_t val)
> {
> - uintptr_t port = (uintptr_t)addr;
> - uint32_t value;
> + writeb(addr, val);
> +}
>
> - if (port < 0x10000) {
> - value = inl(port);
> - } else {
> - value = readl(port);
> - }
> +static uint16_t qpci_pc_pio_readw(QPCIBus *bus, uint32_t addr)
> +{
> + return inw(addr);
> +}
>
> - return value;
> +static uint16_t qpci_pc_mmio_readw(QPCIBus *bus, uint32_t addr)
> +{
> + return readw(addr);
> }
>
> -static void qpci_pc_io_writeb(QPCIBus *bus, void *addr, uint8_t value)
> +static void qpci_pc_pio_writew(QPCIBus *bus, uint32_t addr, uint16_t val)
> {
> - uintptr_t port = (uintptr_t)addr;
> + outw(addr, val);
> +}
>
> - if (port < 0x10000) {
> - outb(port, value);
> - } else {
> - writeb(port, value);
> - }
> +static void qpci_pc_mmio_writew(QPCIBus *bus, uint32_t addr, uint16_t val)
> +{
> + writew(addr, val);
> }
>
> -static void qpci_pc_io_writew(QPCIBus *bus, void *addr, uint16_t value)
> +static uint32_t qpci_pc_pio_readl(QPCIBus *bus, uint32_t addr)
> {
> - uintptr_t port = (uintptr_t)addr;
> + return inl(addr);
> +}
>
> - if (port < 0x10000) {
> - outw(port, value);
> - } else {
> - writew(port, value);
> - }
> +static uint32_t qpci_pc_mmio_readl(QPCIBus *bus, uint32_t addr)
> +{
> + return readl(addr);
> }
>
> -static void qpci_pc_io_writel(QPCIBus *bus, void *addr, uint32_t value)
> +static void qpci_pc_pio_writel(QPCIBus *bus, uint32_t addr, uint32_t val)
> {
> - uintptr_t port = (uintptr_t)addr;
> + outl(addr, val);
> +}
>
> - if (port < 0x10000) {
> - outl(port, value);
> - } else {
> - writel(port, value);
> - }
> +static void qpci_pc_mmio_writel(QPCIBus *bus, uint32_t addr, uint32_t val)
> +{
> + writel(addr, val);
> }
>
> static uint8_t qpci_pc_config_readb(QPCIBus *bus, int devfn, uint8_t offset)
> @@ -218,13 +203,21 @@ QPCIBus *qpci_init_pc(QGuestAllocator *alloc)
>
> ret = g_malloc(sizeof(*ret));
>
> - ret->bus.io_readb = qpci_pc_io_readb;
> - ret->bus.io_readw = qpci_pc_io_readw;
> - ret->bus.io_readl = qpci_pc_io_readl;
> + ret->bus.pio_readb = qpci_pc_pio_readb;
> + ret->bus.pio_readw = qpci_pc_pio_readw;
> + ret->bus.pio_readl = qpci_pc_pio_readl;
> +
> + ret->bus.pio_writeb = qpci_pc_pio_writeb;
> + ret->bus.pio_writew = qpci_pc_pio_writew;
> + ret->bus.pio_writel = qpci_pc_pio_writel;
> +
> + ret->bus.mmio_readb = qpci_pc_mmio_readb;
> + ret->bus.mmio_readw = qpci_pc_mmio_readw;
> + ret->bus.mmio_readl = qpci_pc_mmio_readl;
>
> - ret->bus.io_writeb = qpci_pc_io_writeb;
> - ret->bus.io_writew = qpci_pc_io_writew;
> - ret->bus.io_writel = qpci_pc_io_writel;
> + ret->bus.mmio_writeb = qpci_pc_mmio_writeb;
> + ret->bus.mmio_writew = qpci_pc_mmio_writew;
> + ret->bus.mmio_writel = qpci_pc_mmio_writel;
>
> ret->bus.config_readb = qpci_pc_config_readb;
> ret->bus.config_readw = qpci_pc_config_readw;
> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> index 2eaaf91..2d26a94 100644
> --- a/tests/libqos/pci-spapr.c
> +++ b/tests/libqos/pci-spapr.c
> @@ -50,78 +50,76 @@ typedef struct QPCIBusSPAPR {
> * so PCI accessors need to swap data endianness
> */
>
> -static uint8_t qpci_spapr_io_readb(QPCIBus *bus, void *addr)
> +static uint8_t qpci_spapr_pio_readb(QPCIBus *bus, uint32_t addr)
> {
> QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> - uint64_t port = (uintptr_t)addr;
> - uint8_t v;
> - if (port < s->pio.size) {
> - v = readb(s->pio_cpu_base + port);
> - } else {
> - v = readb(s->mmio32_cpu_base + port);
> - }
> - return v;
> + return readb(s->pio_cpu_base + addr);
> }
>
> -static uint16_t qpci_spapr_io_readw(QPCIBus *bus, void *addr)
> +static uint8_t qpci_spapr_mmio32_readb(QPCIBus *bus, uint32_t addr)
> {
> QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> - uint64_t port = (uintptr_t)addr;
> - uint16_t v;
> - if (port < s->pio.size) {
> - v = readw(s->pio_cpu_base + port);
> - } else {
> - v = readw(s->mmio32_cpu_base + port);
> - }
> - return bswap16(v);
> + return readb(s->mmio32_cpu_base + addr);
> }
>
> -static uint32_t qpci_spapr_io_readl(QPCIBus *bus, void *addr)
> +static void qpci_spapr_pio_writeb(QPCIBus *bus, uint32_t addr, uint8_t val)
> {
> QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> - uint64_t port = (uintptr_t)addr;
> - uint32_t v;
> - if (port < s->pio.size) {
> - v = readl(s->pio_cpu_base + port);
> - } else {
> - v = readl(s->mmio32_cpu_base + port);
> - }
> - return bswap32(v);
> + writeb(s->pio_cpu_base + addr, val);
> }
>
> -static void qpci_spapr_io_writeb(QPCIBus *bus, void *addr, uint8_t value)
> +static void qpci_spapr_mmio32_writeb(QPCIBus *bus, uint32_t addr, uint8_t
> val)
> {
> QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> - uint64_t port = (uintptr_t)addr;
> - if (port < s->pio.size) {
> - writeb(s->pio_cpu_base + port, value);
> - } else {
> - writeb(s->mmio32_cpu_base + port, value);
> - }
> + writeb(s->mmio32_cpu_base + addr, val);
> }
>
> -static void qpci_spapr_io_writew(QPCIBus *bus, void *addr, uint16_t value)
> +static uint16_t qpci_spapr_pio_readw(QPCIBus *bus, uint32_t addr)
> {
> QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> - uint64_t port = (uintptr_t)addr;
> - value = bswap16(value);
> - if (port < s->pio.size) {
> - writew(s->pio_cpu_base + port, value);
> - } else {
> - writew(s->mmio32_cpu_base + port, value);
> - }
> + return bswap16(readw(s->pio_cpu_base + addr));
> }
>
> -static void qpci_spapr_io_writel(QPCIBus *bus, void *addr, uint32_t value)
> +static uint16_t qpci_spapr_mmio32_readw(QPCIBus *bus, uint32_t addr)
> {
> QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> - uint64_t port = (uintptr_t)addr;
> - value = bswap32(value);
> - if (port < s->pio.size) {
> - writel(s->pio_cpu_base + port, value);
> - } else {
> - writel(s->mmio32_cpu_base + port, value);
> - }
> + return bswap16(readw(s->mmio32_cpu_base + addr));
> +}
> +
> +static void qpci_spapr_pio_writew(QPCIBus *bus, uint32_t addr, uint16_t val)
> +{
> + QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> + writew(s->pio_cpu_base + addr, bswap16(val));
> +}
> +
> +static void qpci_spapr_mmio32_writew(QPCIBus *bus, uint32_t addr, uint16_t
> val)
> +{
> + QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> + writew(s->mmio32_cpu_base + addr, bswap16(val));
> +}
> +
> +static uint32_t qpci_spapr_pio_readl(QPCIBus *bus, uint32_t addr)
> +{
> + QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> + return bswap32(readl(s->pio_cpu_base + addr));
> +}
> +
> +static uint32_t qpci_spapr_mmio32_readl(QPCIBus *bus, uint32_t addr)
> +{
> + QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> + return bswap32(readl(s->mmio32_cpu_base + addr));
> +}
> +
> +static void qpci_spapr_pio_writel(QPCIBus *bus, uint32_t addr, uint32_t val)
> +{
> + QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> + writel(s->pio_cpu_base + addr, bswap32(val));
> +}
> +
> +static void qpci_spapr_mmio32_writel(QPCIBus *bus, uint32_t addr, uint32_t
> val)
> +{
> + QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> + writel(s->mmio32_cpu_base + addr, bswap32(val));
> }
>
> static uint8_t qpci_spapr_config_readb(QPCIBus *bus, int devfn, uint8_t
> offset)
> @@ -248,13 +246,21 @@ QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
>
> ret->alloc = alloc;
>
> - ret->bus.io_readb = qpci_spapr_io_readb;
> - ret->bus.io_readw = qpci_spapr_io_readw;
> - ret->bus.io_readl = qpci_spapr_io_readl;
> + ret->bus.pio_readb = qpci_spapr_pio_readb;
> + ret->bus.pio_readw = qpci_spapr_pio_readw;
> + ret->bus.pio_readl = qpci_spapr_pio_readl;
> +
> + ret->bus.pio_writeb = qpci_spapr_pio_writeb;
> + ret->bus.pio_writew = qpci_spapr_pio_writew;
> + ret->bus.pio_writel = qpci_spapr_pio_writel;
> +
> + ret->bus.mmio_readb = qpci_spapr_mmio32_readb;
> + ret->bus.mmio_readw = qpci_spapr_mmio32_readw;
> + ret->bus.mmio_readl = qpci_spapr_mmio32_readl;
>
> - ret->bus.io_writeb = qpci_spapr_io_writeb;
> - ret->bus.io_writew = qpci_spapr_io_writew;
> - ret->bus.io_writel = qpci_spapr_io_writel;
> + ret->bus.mmio_writeb = qpci_spapr_mmio32_writeb;
> + ret->bus.mmio_writew = qpci_spapr_mmio32_writew;
> + ret->bus.mmio_writel = qpci_spapr_mmio32_writel;
>
> ret->bus.config_readb = qpci_spapr_config_readb;
> ret->bus.config_readw = qpci_spapr_config_readw;
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index c3f3382..55b01df 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -224,33 +224,68 @@ void qpci_config_writel(QPCIDevice *dev, uint8_t
> offset, uint32_t value)
>
> uint8_t qpci_io_readb(QPCIDevice *dev, void *data)
> {
> - return dev->bus->io_readb(dev->bus, data);
> + uintptr_t addr = (uintptr_t)data;
> +
> + if (addr < QPCI_PIO_LIMIT) {
> + return dev->bus->pio_readb(dev->bus, addr);
> + } else {
> + return dev->bus->mmio_readb(dev->bus, addr);
> + }
> }
>
> uint16_t qpci_io_readw(QPCIDevice *dev, void *data)
> {
> - return dev->bus->io_readw(dev->bus, data);
> + uintptr_t addr = (uintptr_t)data;
> +
> + if (addr < QPCI_PIO_LIMIT) {
> + return dev->bus->pio_readw(dev->bus, addr);
> + } else {
> + return dev->bus->mmio_readw(dev->bus, addr);
> + }
> }
>
> uint32_t qpci_io_readl(QPCIDevice *dev, void *data)
> {
> - return dev->bus->io_readl(dev->bus, data);
> -}
> + uintptr_t addr = (uintptr_t)data;
>
> + if (addr < QPCI_PIO_LIMIT) {
> + return dev->bus->pio_readl(dev->bus, addr);
> + } else {
> + return dev->bus->mmio_readl(dev->bus, addr);
> + }
> +}
>
> void qpci_io_writeb(QPCIDevice *dev, void *data, uint8_t value)
> {
> - dev->bus->io_writeb(dev->bus, data, value);
> + uintptr_t addr = (uintptr_t)data;
> +
> + if (addr < QPCI_PIO_LIMIT) {
> + dev->bus->pio_writeb(dev->bus, addr, value);
> + } else {
> + dev->bus->mmio_writeb(dev->bus, addr, value);
> + }
> }
>
> void qpci_io_writew(QPCIDevice *dev, void *data, uint16_t value)
> {
> - dev->bus->io_writew(dev->bus, data, value);
> + uintptr_t addr = (uintptr_t)data;
> +
> + if (addr < QPCI_PIO_LIMIT) {
> + dev->bus->pio_writew(dev->bus, addr, value);
> + } else {
> + dev->bus->mmio_writew(dev->bus, addr, value);
> + }
> }
>
> void qpci_io_writel(QPCIDevice *dev, void *data, uint32_t value)
> {
> - dev->bus->io_writel(dev->bus, data, value);
> + uintptr_t addr = (uintptr_t)data;
> +
> + if (addr < QPCI_PIO_LIMIT) {
> + dev->bus->pio_writel(dev->bus, addr, value);
> + } else {
> + dev->bus->mmio_writel(dev->bus, addr, value);
> + }
> }
>
> void *qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
> index c06add8..72a2245 100644
> --- a/tests/libqos/pci.h
> +++ b/tests/libqos/pci.h
> @@ -15,6 +15,8 @@
>
> #include "libqtest.h"
>
> +#define QPCI_PIO_LIMIT 0x10000
> +
> #define QPCI_DEVFN(dev, fn) (((dev) << 3) | (fn))
>
> typedef struct QPCIDevice QPCIDevice;
> @@ -22,13 +24,21 @@ typedef struct QPCIBus QPCIBus;
>
> struct QPCIBus
> {
> - uint8_t (*io_readb)(QPCIBus *bus, void *addr);
> - uint16_t (*io_readw)(QPCIBus *bus, void *addr);
> - uint32_t (*io_readl)(QPCIBus *bus, void *addr);
> + uint8_t (*pio_readb)(QPCIBus *bus, uint32_t addr);
> + uint16_t (*pio_readw)(QPCIBus *bus, uint32_t addr);
> + uint32_t (*pio_readl)(QPCIBus *bus, uint32_t addr);
> +
> + uint8_t (*mmio_readb)(QPCIBus *bus, uint32_t addr);
> + uint16_t (*mmio_readw)(QPCIBus *bus, uint32_t addr);
> + uint32_t (*mmio_readl)(QPCIBus *bus, uint32_t addr);
> +
> + void (*pio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value);
> + void (*pio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value);
> + void (*pio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value);
>
> - void (*io_writeb)(QPCIBus *bus, void *addr, uint8_t value);
> - void (*io_writew)(QPCIBus *bus, void *addr, uint16_t value);
> - void (*io_writel)(QPCIBus *bus, void *addr, uint32_t value);
> + void (*mmio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value);
> + void (*mmio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value);
> + void (*mmio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value);
>
> uint8_t (*config_readb)(QPCIBus *bus, int devfn, uint8_t offset);
> uint16_t (*config_readw)(QPCIBus *bus, int devfn, uint8_t offset);
>
