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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 12991 bytes --]

On Tue, Oct 04, 2016 at 09:36:30AM +0200, Greg Kurz wrote:
> On Tue, 4 Oct 2016 08:58:35 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Tue, 4 Oct 2016 11:24:54 +1100
> > David Gibson <david@gibson.dropbear.id.au> 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 <clg@kaod.org> 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 <lvivier@redhat.com>
> > > > > >> ---
> > > > > >>  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.
> > 
> > > > 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 ?
> 
> Ohh... maybe I see now: if the test program says I'm writing BE or LE, then
> it needs to call cpu_to_*() and then we just want qtest.c to copy the value
> to guest memory without modifying it again. That's basically what Cedric's
> accessors ^^ do.
> 
> Th tswap*() in qtest.c only makes sense when accessing memory in native
> endianness.

s/native/guest native/, yes.  Except that guest native endianness is
vague to begin with CPUs that support both modes, and completely
meaningless when we've essentially replaced the guest cpu with the
qtest stub.

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]