Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/6] libqos: add PPC64 PCI support

[David Gibson]

On Tue, Oct 04, 2016 at 08:58:35AM +0200, Greg Kurz wrote:
> On Tue, 4 Oct 2016 11:24:54 +1100
> David Gibson <address@hidden> wrote:
> 
> > On Mon, Oct 03, 2016 at 04:03:14PM +0200, Greg Kurz wrote:
> > > On Mon, 3 Oct 2016 13:23:27 +0200
> > > Cédric Le Goater <address@hidden> wrote:
> > >   
> > > > On 09/29/2016 07:27 AM, David Gibson wrote:  
> > > > > On Wed, Sep 28, 2016 at 08:51:28PM +0200, Laurent Vivier wrote:    
> > > > >> Signed-off-by: Laurent Vivier <address@hidden>
> > > > >> ---
> > > > >>  tests/Makefile.include   |   1 +
> > > > >>  tests/libqos/pci-pc.c    |  22 ----
> > > > >>  tests/libqos/pci-spapr.c | 280 
> > > > >> +++++++++++++++++++++++++++++++++++++++++++++++
> > > > >>  tests/libqos/pci-spapr.h |  17 +++
> > > > >>  tests/libqos/pci.c       |  22 +++-
> > > > >>  tests/libqos/rtas.c      |  45 ++++++++
> > > > >>  tests/libqos/rtas.h      |   4 +
> > > > >>  7 files changed, 368 insertions(+), 23 deletions(-)
> > > > >>  create mode 100644 tests/libqos/pci-spapr.c
> > > > >>  create mode 100644 tests/libqos/pci-spapr.h
> > > > >>
> > > > >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> > > > >> index 8162f6f..92c82d8 100644
> > > > >> --- a/tests/Makefile.include
> > > > >> +++ b/tests/Makefile.include
> > > > >> @@ -590,6 +590,7 @@ libqos-obj-y += tests/libqos/i2c.o 
> > > > >> tests/libqos/libqos.o
> > > > >>  libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
> > > > >>  libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
> > > > >>  libqos-spapr-obj-y += tests/libqos/rtas.o
> > > > >> +libqos-spapr-obj-y += tests/libqos/pci-spapr.o
> > > > >>  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
> > > > >>  libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
> > > > >>  libqos-pc-obj-y += tests/libqos/ahci.o
> > > > >> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> > > > >> index 1ae2d37..82066b8 100644
> > > > >> --- a/tests/libqos/pci-pc.c
> > > > >> +++ b/tests/libqos/pci-pc.c
> > > > >> @@ -255,28 +255,6 @@ void qpci_free_pc(QPCIBus *bus)
> > > > >>      g_free(s);
> > > > >>  }
> > > > >>  
> > > > >> -void qpci_plug_device_test(const char *driver, const char *id,
> > > > >> -                           uint8_t slot, const char *opts)
> > > > >> -{
> > > > >> -    QDict *response;
> > > > >> -    char *cmd;
> > > > >> -
> > > > >> -    cmd = g_strdup_printf("{'execute': 'device_add',"
> > > > >> -                          " 'arguments': {"
> > > > >> -                          "   'driver': '%s',"
> > > > >> -                          "   'addr': '%d',"
> > > > >> -                          "   %s%s"
> > > > >> -                          "   'id': '%s'"
> > > > >> -                          "}}", driver, slot,
> > > > >> -                          opts ? opts : "", opts ? "," : "",
> > > > >> -                          id);
> > > > >> -    response = qmp(cmd);
> > > > >> -    g_free(cmd);
> > > > >> -    g_assert(response);
> > > > >> -    g_assert(!qdict_haskey(response, "error"));
> > > > >> -    QDECREF(response);
> > > > >> -}
> > > > >> -
> > > > >>  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
> > > > >>  {
> > > > >>      QDict *response;
> > > > >> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> > > > >> new file mode 100644
> > > > >> index 0000000..78df823
> > > > >> --- /dev/null
> > > > >> +++ b/tests/libqos/pci-spapr.c
> > > > >> @@ -0,0 +1,280 @@
> > > > >> +/*
> > > > >> + * libqos PCI bindings for SPAPR
> > > > >> + *
> > > > >> + * This work is licensed under the terms of the GNU GPL, version 2 
> > > > >> or later.
> > > > >> + * See the COPYING file in the top-level directory.
> > > > >> + */
> > > > >> +
> > > > >> +#include "qemu/osdep.h"
> > > > >> +#include "libqtest.h"
> > > > >> +#include "libqos/pci-spapr.h"
> > > > >> +#include "libqos/rtas.h"
> > > > >> +
> > > > >> +#include "hw/pci/pci_regs.h"
> > > > >> +
> > > > >> +#include "qemu-common.h"
> > > > >> +#include "qemu/host-utils.h"
> > > > >> +
> > > > >> +
> > > > >> +/* From include/hw/pci-host/spapr.h */
> > > > >> +
> > > > >> +#define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
> > > > >> +
> > > > >> +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> > > > >> +
> > > > >> +#define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
> > > > >> +#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
> > > > >> +#define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
> > > > >> +#define SPAPR_PCI_MMIO_WIN_SIZE      (SPAPR_PCI_WINDOW_SPACING - \
> > > > >> +                                     SPAPR_PCI_MEM_WIN_BUS_OFFSET)
> > > > >> +#define SPAPR_PCI_IO_WIN_OFF         0x80000000
> > > > >> +#define SPAPR_PCI_IO_WIN_SIZE        0x10000
> > > > >> +
> > > > >> +/* index is the phb index */
> > > > >> +
> > > > >> +#define BUIDBASE(index)              (SPAPR_PCI_BASE_BUID + (index))
> > > > >> +#define PCIBASE(index)               (SPAPR_PCI_WINDOW_BASE + \
> > > > >> +                                      (index) * 
> > > > >> SPAPR_PCI_WINDOW_SPACING)
> > > > >> +#define IOBASE(index)                (PCIBASE(index) + 
> > > > >> SPAPR_PCI_IO_WIN_OFF)
> > > > >> +#define MMIOBASE(index)              (PCIBASE(index) + 
> > > > >> SPAPR_PCI_MMIO_WIN_OFF)
> > > > >> +
> > > > >> +typedef struct QPCIBusSPAPR {
> > > > >> +    QPCIBus bus;
> > > > >> +    QGuestAllocator *alloc;
> > > > >> +
> > > > >> +    uint64_t pci_hole_start;
> > > > >> +    uint64_t pci_hole_size;
> > > > >> +    uint64_t pci_hole_alloc;
> > > > >> +
> > > > >> +    uint32_t pci_iohole_start;
> > > > >> +    uint32_t pci_iohole_size;
> > > > >> +    uint32_t pci_iohole_alloc;
> > > > >> +} QPCIBusSPAPR;
> > > > >> +
> > > > >> +static uint8_t qpci_spapr_io_readb(QPCIBus *bus, void *addr)
> > > > >> +{
> > > > >> +    uint64_t port = (uint64_t)addr;
> > > > >> +    uint8_t v;
> > > > >> +    if (port < SPAPR_PCI_IO_WIN_SIZE) {
> > > > >> +        v = readb(IOBASE(0) + port);
> > > > >> +    } else {
> > > > >> +        v = readb(MMIOBASE(0) + port);
> > > > >> +    }
> > > > >> +    return v;
> > > > >> +}
> > > > >> +
> > > > >> +static uint16_t qpci_spapr_io_readw(QPCIBus *bus, void *addr)
> > > > >> +{
> > > > >> +    uint64_t port = (uint64_t)addr;
> > > > >> +    uint16_t v;
> > > > >> +    if (port < SPAPR_PCI_IO_WIN_SIZE) {
> > > > >> +        v = readw(IOBASE(0) + port);
> > > > >> +    } else {
> > > > >> +        v = readw(MMIOBASE(0) + port);
> > > > >> +    }    
> > > > > 
> > > > > Ok, so, I've looked through the qtest stuff in more detail now, and
> > > > > I've got a better idea of how the endianness works.  Some of my
> > > > > earlier comments were confused about which pieces were in the test
> > > > > case side and which were on the qtest accel stub side.
> > > > > 
> > > > > So, what I think you need is an unconditional byteswap in each of the
> > > > > spapr pci IO functions.  Why?
> > > > > 
> > > > >    * The plain readw() etc. functions return values read from memory
> > > > >      assuming guest endianness.  For guest native values in memory, so
> > > > >      far so good.
> > > > >    * Generic users of the pci read/write functions in qtest expect to
> > > > >      get native values.
> > > > >    * But PCI devices are always LE, not guest endian
> > > > > 
> > > > > The guest endianness of spapr (as far as tswap() etc. are concerned)
> > > > > is BE, even though that's not really true in practice these days, so
> > > > > to correct for the PCI registers being read as BE when they should be
> > > > > LE we always need a swap.
> > > > > 
> > > > > That should remove the need for swaps further up the test stack.
> > > > > 
> > > > > Of course, "guest endianness" is a poorly defined concept, so longer
> > > > > term it might be better to completely replace the "readw" etc. qtest
> > > > > operations with explicit "readw_le" and "readw_be" ops.    
> > > > 
> > > > 
> > > > I have a similar need for a unit test of the AST2400 flash controller. 
> > > > 
> > > > The flash module is accessible through a memory window and, when in 
> > > > SPI mode, the commands are sent to the slave by writing at the 
> > > > beginning 
> > > > of the region. Addresses are necessarily BE to be understood by the SPI
> > > > flash module. So, sequences like
> > > > 
> > > >     writeb(AST2400_FLASH_BASE, EN_4BYTE_ADDR);
> > > >     writeb(AST2400_FLASH_BASE, READ);
> > > >     writel(AST2400_FLASH_BASE, cpu_to_be32(addr));
> > > > 
> > > > will just fail under a BE host as an extra swap is done by the qtest  
> > > 
> > > cpu_to_be32() is indeed what a real guest does... but the real guest runs
> > > with the target endianness, whereas the qtest program runs with the host
> > > endianness... so cpu_to_be32() looks wrong here.
> > >   
> > > > layer. I have used memwrite to have endian-agnostic mem accessors. 
> > > > Maybe something like below would be helpful in libqtest.h
> > > > 
> > > > 
> > > > +/*
> > > > + * Generic read/write helpers for a BE region
> > > > + */
> > > > +static inline void writew_be(uint64_t addr, uint16_t value)
> > > > +{
> > > > +    value = cpu_to_be16(value);
> > > > +    memwrite(addr, &value, sizeof(value));
> > > > +}
> > > > +
> > > > +static inline uint16_t readw_be(uint64_t addr)
> > > > +{
> > > > +    uint16_t value;
> > > > +
> > > > +    memread(addr, &value, sizeof(value));
> > > > +    return be16_to_cpu(value);
> > > > +}
> > > > +
> > > > +static inline void writel_be(uint64_t addr, uint32_t value)
> > > > +{
> > > > +    value = cpu_to_be32(value);
> > > > +    memwrite(addr, &value, sizeof(value));
> > > > +}
> > > > +
> > > > +static inline uint32_t readl_be(uint64_t addr)
> > > > +{
> > > > +    uint32_t value;
> > > > +
> > > > +    memread(addr, &value, sizeof(value));
> > > > +    return be32_to_cpu(value);
> > > > +}
> > > > 
> > > > If this is correct, I can add the LE equivalents and send a patch.
> > > >   
> > > 
> > > The API is correct, but the implementation has the same issue: it uses
> > > cpu_to_be32() and bypasses the tswap32() in qtest... the result is that
> > > the target endianness is totally ignored.  
> > 
> > Yes, this is precisely the intended result.
> > 
> > The target endianness is not well defined, and simply an extra point
> > of confusion for qtest code.
> > 
> 
> I agree that target endianness is confusing in the test program code but
> tswap32() I'm referring to is the one in qtest.c.

Yes, that's the one I'm referring to as well.  Bypassing it is
*precisely* the intention of this patch.

> > > Let's suppose that a similar AST2400 platform exists with a BE processor:
> > > would the same test program work for both LE and BE cases ?  
> > 
> > Yes, that's the idea - assuming the AST2400 has the same IO endianness
> > in either case, which would be usual for most IO devices.
> > 
> > > Both the test program and QEMU run with the host endianness acutally,
> > > but only QEMU knows about the target endianness. So I guess, the qtest
> > > protocol should provide explicit BE/LE operations: the test program
> > > would ask for an access with a specific endianness and the qtest
> > > accelerator in QEMU would do the swap based on the target endianness.  
> > 
> > Half right.  The target endiannness need not enter into this.
> > 
> 
> I'm lost here... are you saying that the qtest accelerator does not need
> to care for the target endianness when accessing the guest memory ? What
> about the tswap*() call sites in qtest.c then ?

Bad idea from the start as far as I can tell.

Well.. maybe.  For accessing IO devices we certainly want to bypass it
- they have their own endianness independent from the target cpu.  For
accessing guest RAM, possibly we want target endianness accessors.


Even then, though, I don't see much call for it.  If it's some
hardware protocol (e.g. DMA buffers), the peripheral hardware still
defines the endianness independent of the target cpu.  If it's just
for communication between pieces of our test, then we control both
sides so it doesn't matter what we choose, as long as we're
consistent.  Might as well make it universally consistent instead of
arbitrarily varying based on target cpu.

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