Re: [Qemu-ppc] [PATCHv2 07/11] libqos: Implement mmio accessors in terms of mem{read, write}

[Laurent Vivier]

On 19/10/2016 14:25, David Gibson wrote:
> In the libqos PCI code we now have accessors both for registers (byte
> significance preserving) and for streaming data (byte address order
> preserving).  These exist in both the interface for qtest drivers and in
> the machine specific backends.
> 
> However, the register-style accessors aren't actually necessary in the
> backend.  They can be implemented in terms of the byte address order
> preserving accessors by the libqos wrappers.  This works because PCI is
> always little endian.
> 
> This does assume that the back end byte address order preserving accessors
> will perform the equivalent of a single bus transaction for short lengths.
> This is the case, and in fact they currently end up using the same
> cpu_physical_memory_rw() implementation within the qtest accelerator.
> 
> Signed-off-by: David Gibson <address@hidden>

Identical to v1:

Reviewed-by: Laurent Vivier <address@hidden>

> ---
>  tests/libqos/pci-pc.c    | 38 --------------------------------------
>  tests/libqos/pci-spapr.c | 44 --------------------------------------------
>  tests/libqos/pci.c       | 20 ++++++++++++++------
>  tests/libqos/pci.h       |  8 --------
>  4 files changed, 14 insertions(+), 96 deletions(-)
> 
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 84aee25..849ea56 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -32,61 +32,31 @@ static uint8_t qpci_pc_pio_readb(QPCIBus *bus, uint32_t 
> addr)
>      return inb(addr);
>  }
>  
> -static uint8_t qpci_pc_mmio_readb(QPCIBus *bus, uint32_t addr)
> -{
> -    return readb(addr);
> -}
> -
>  static void qpci_pc_pio_writeb(QPCIBus *bus, uint32_t addr, uint8_t val)
>  {
>      outb(addr, val);
>  }
>  
> -static void qpci_pc_mmio_writeb(QPCIBus *bus, uint32_t addr, uint8_t val)
> -{
> -    writeb(addr, val);
> -}
> -
>  static uint16_t qpci_pc_pio_readw(QPCIBus *bus, uint32_t addr)
>  {
>      return inw(addr);
>  }
>  
> -static uint16_t qpci_pc_mmio_readw(QPCIBus *bus, uint32_t addr)
> -{
> -    return readw(addr);
> -}
> -
>  static void qpci_pc_pio_writew(QPCIBus *bus, uint32_t addr, uint16_t val)
>  {
>      outw(addr, val);
>  }
>  
> -static void qpci_pc_mmio_writew(QPCIBus *bus, uint32_t addr, uint16_t val)
> -{
> -    writew(addr, val);
> -}
> -
>  static uint32_t qpci_pc_pio_readl(QPCIBus *bus, uint32_t addr)
>  {
>      return inl(addr);
>  }
>  
> -static uint32_t qpci_pc_mmio_readl(QPCIBus *bus, uint32_t addr)
> -{
> -    return readl(addr);
> -}
> -
>  static void qpci_pc_pio_writel(QPCIBus *bus, uint32_t addr, uint32_t val)
>  {
>      outl(addr, val);
>  }
>  
> -static void qpci_pc_mmio_writel(QPCIBus *bus, uint32_t addr, uint32_t val)
> -{
> -    writel(addr, val);
> -}
> -
>  static void qpci_pc_memread(QPCIBus *bus, uint32_t addr, void *buf, size_t 
> len)
>  {
>      memread(addr, buf, len);
> @@ -148,14 +118,6 @@ QPCIBus *qpci_init_pc(QGuestAllocator *alloc)
>      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.mmio_writeb = qpci_pc_mmio_writeb;
> -    ret->bus.mmio_writew = qpci_pc_mmio_writew;
> -    ret->bus.mmio_writel = qpci_pc_mmio_writel;
> -
>      ret->bus.memread = qpci_pc_memread;
>      ret->bus.memwrite = qpci_pc_memwrite;
>  
> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> index ad12c2e..f26488a 100644
> --- a/tests/libqos/pci-spapr.c
> +++ b/tests/libqos/pci-spapr.c
> @@ -48,72 +48,36 @@ static uint8_t qpci_spapr_pio_readb(QPCIBus *bus, 
> uint32_t addr)
>      return readb(s->pio_cpu_base + addr);
>  }
>  
> -static uint8_t qpci_spapr_mmio32_readb(QPCIBus *bus, uint32_t addr)
> -{
> -    QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> -    return readb(s->mmio32_cpu_base + addr);
> -}
> -
>  static void qpci_spapr_pio_writeb(QPCIBus *bus, uint32_t addr, uint8_t val)
>  {
>      QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
>      writeb(s->pio_cpu_base + addr, val);
>  }
>  
> -static void qpci_spapr_mmio32_writeb(QPCIBus *bus, uint32_t addr, uint8_t 
> val)
> -{
> -    QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> -    writeb(s->mmio32_cpu_base + addr, val);
> -}
> -
>  static uint16_t qpci_spapr_pio_readw(QPCIBus *bus, uint32_t addr)
>  {
>      QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
>      return bswap16(readw(s->pio_cpu_base + addr));
>  }
>  
> -static uint16_t qpci_spapr_mmio32_readw(QPCIBus *bus, uint32_t addr)
> -{
> -    QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> -    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 void qpci_spapr_memread(QPCIBus *bus, uint32_t addr,
>                                 void *buf, size_t len)
>  {
> @@ -194,14 +158,6 @@ QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
>      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.mmio_writeb = qpci_spapr_mmio32_writeb;
> -    ret->bus.mmio_writew = qpci_spapr_mmio32_writew;
> -    ret->bus.mmio_writel = qpci_spapr_mmio32_writel;
> -
>      ret->bus.memread = qpci_spapr_memread;
>      ret->bus.memwrite = qpci_spapr_memwrite;
>  
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 146b063..bdffeb3 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -230,7 +230,9 @@ uint8_t qpci_io_readb(QPCIDevice *dev, void *data)
>      if (addr < QPCI_PIO_LIMIT) {
>          return dev->bus->pio_readb(dev->bus, addr);
>      } else {
> -        return dev->bus->mmio_readb(dev->bus, addr);
> +        uint8_t val;
> +        dev->bus->memread(dev->bus, addr, &val, sizeof(val));
> +        return val;
>      }
>  }
>  
> @@ -241,7 +243,9 @@ uint16_t qpci_io_readw(QPCIDevice *dev, void *data)
>      if (addr < QPCI_PIO_LIMIT) {
>          return dev->bus->pio_readw(dev->bus, addr);
>      } else {
> -        return dev->bus->mmio_readw(dev->bus, addr);
> +        uint16_t val;
> +        dev->bus->memread(dev->bus, addr, &val, sizeof(val));
> +        return le16_to_cpu(val);
>      }
>  }
>  
> @@ -252,7 +256,9 @@ uint32_t qpci_io_readl(QPCIDevice *dev, void *data)
>      if (addr < QPCI_PIO_LIMIT) {
>          return dev->bus->pio_readl(dev->bus, addr);
>      } else {
> -        return dev->bus->mmio_readl(dev->bus, addr);
> +        uint32_t val;
> +        dev->bus->memread(dev->bus, addr, &val, sizeof(val));
> +        return le32_to_cpu(val);
>      }
>  }
>  
> @@ -263,7 +269,7 @@ void qpci_io_writeb(QPCIDevice *dev, void *data, uint8_t 
> value)
>      if (addr < QPCI_PIO_LIMIT) {
>          dev->bus->pio_writeb(dev->bus, addr, value);
>      } else {
> -        dev->bus->mmio_writeb(dev->bus, addr, value);
> +        dev->bus->memwrite(dev->bus, addr, &value, sizeof(value));
>      }
>  }
>  
> @@ -274,7 +280,8 @@ void qpci_io_writew(QPCIDevice *dev, void *data, uint16_t 
> value)
>      if (addr < QPCI_PIO_LIMIT) {
>          dev->bus->pio_writew(dev->bus, addr, value);
>      } else {
> -        dev->bus->mmio_writew(dev->bus, addr, value);
> +        value = cpu_to_le16(value);
> +        dev->bus->memwrite(dev->bus, addr, &value, sizeof(value));
>      }
>  }
>  
> @@ -285,7 +292,8 @@ void qpci_io_writel(QPCIDevice *dev, void *data, uint32_t 
> value)
>      if (addr < QPCI_PIO_LIMIT) {
>          dev->bus->pio_writel(dev->bus, addr, value);
>      } else {
> -        dev->bus->mmio_writel(dev->bus, addr, value);
> +        value = cpu_to_le32(value);
> +        dev->bus->memwrite(dev->bus, addr, &value, sizeof(value));
>      }
>  }
>  
> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
> index 2b08362..ce6ed08 100644
> --- a/tests/libqos/pci.h
> +++ b/tests/libqos/pci.h
> @@ -27,18 +27,10 @@ struct QPCIBus {
>      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 (*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);
> -
>      void (*memread)(QPCIBus *bus, uint32_t addr, void *buf, size_t len);
>      void (*memwrite)(QPCIBus *bus, uint32_t addr, const void *buf, size_t 
> len);
>  
>