On Mon, Oct 03, 2016 at 01:23:27PM +0200, Cédric Le Goater 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 <lviv...@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
> layer. I have used memwrite to have endian-agnostic mem accessors.
Right. You could fix this by writing tswap32(cpu_to_be32(addr)), of
course but it's starting to get ugly.
> 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.
I like the idea, but we probably need buy in from some more people.
--
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
