qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH 2/8] libqos: Handle PCI IO de-multiplexing in comm


From: David Gibson
Subject: Re: [Qemu-ppc] [PATCH 2/8] libqos: Handle PCI IO de-multiplexing in common code
Date: Wed, 19 Oct 2016 13:59:17 +1100
User-agent: Mutt/1.7.0 (2016-08-17)

On Tue, Oct 18, 2016 at 03:28:17PM +0200, Laurent Vivier wrote:
> 
> 
> On 18/10/2016 12:52, 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 <address@hidden>
> > ---
> >  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
> 
> Perhaps you can remove the definition of SPAPR_PCI_IO_WIN_SIZE and use
> QPCI_PIO_LIMIT instead in pci-spapr.c?
> or add a QEMU_BUILD_BUG_ON(SPAPR_PCI_IO_WIN_SIZE != QPCI_PIO_LIMIT)?

I don't think that's really right.  They'll match in practice because
PIO space is basically always 64kiB, but they don't actually have to,
technically.  QPCI_PIO_LIMIT describes how the multiplexed address
space for BAR "pointers" is divided.  SPAPR_PCI_IO_WIN_SIZE describes
how much IO space the hardware can really map.  If they don't match
you might not be able to access all the hardware's PIO space.  But
that's already true for MMIO space, so I don't think it's a
fundamental problem.

> > +
> >  #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);
> > 
> 
> Thanks,
> Laurent
> 

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

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]