From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35640) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1br1ZL-0007Kc-1U for qemu-devel@nongnu.org; Mon, 03 Oct 2016 07:38:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1br1ZF-0008KK-2V for qemu-devel@nongnu.org; Mon, 03 Oct 2016 07:38:02 -0400 References: <1475088693-29091-1-git-send-email-lvivier@redhat.com> <1475088693-29091-2-git-send-email-lvivier@redhat.com> <20160929052745.GQ8390@umbus.fritz.box> <08ecf131-8749-c468-58ea-3f9369fece07@kaod.org> From: Laurent Vivier Message-ID: <00a2d9ed-4063-4310-e5d6-23d7fba318f9@redhat.com> Date: Mon, 3 Oct 2016 13:37:51 +0200 MIME-Version: 1.0 In-Reply-To: <08ecf131-8749-c468-58ea-3f9369fece07@kaod.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/6] libqos: add PPC64 PCI support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= , David Gibson Cc: thuth@redhat.com, qemu-devel@nongnu.org, Greg Kurz , qemu-ppc@nongnu.org, Gerd Hoffmann , dgibson@redhat.com On 03/10/2016 13:23, C=C3=A9dric 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 >>> --- >>> 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 +=3D tests/libqos/i2c.o tests/libqos= /libqos.o >>> libqos-spapr-obj-y =3D $(libqos-obj-y) tests/libqos/malloc-spapr.o >>> libqos-spapr-obj-y +=3D tests/libqos/libqos-spapr.o >>> libqos-spapr-obj-y +=3D tests/libqos/rtas.o >>> +libqos-spapr-obj-y +=3D tests/libqos/pci-spapr.o >>> libqos-pc-obj-y =3D $(libqos-obj-y) tests/libqos/pci-pc.o >>> libqos-pc-obj-y +=3D tests/libqos/malloc-pc.o tests/libqos/libqos-pc= .o >>> libqos-pc-obj-y +=3D 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); >>> } >>> =20 >>> -void qpci_plug_device_test(const char *driver, const char *id, >>> - uint8_t slot, const char *opts) >>> -{ >>> - QDict *response; >>> - char *cmd; >>> - >>> - cmd =3D g_strdup_printf("{'execute': 'device_add'," >>> - " 'arguments': {" >>> - " 'driver': '%s'," >>> - " 'addr': '%d'," >>> - " %s%s" >>> - " 'id': '%s'" >>> - "}}", driver, slot, >>> - opts ? opts : "", opts ? "," : "", >>> - id); >>> - response =3D 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 o= r 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_SPA= CING) >>> +#define IOBASE(index) (PCIBASE(index) + SPAPR_PCI_IO_= WIN_OFF) >>> +#define MMIOBASE(index) (PCIBASE(index) + SPAPR_PCI_MMI= O_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 =3D (uint64_t)addr; >>> + uint8_t v; >>> + if (port < SPAPR_PCI_IO_WIN_SIZE) { >>> + v =3D readb(IOBASE(0) + port); >>> + } else { >>> + v =3D readb(MMIOBASE(0) + port); >>> + } >>> + return v; >>> +} >>> + >>> +static uint16_t qpci_spapr_io_readw(QPCIBus *bus, void *addr) >>> +{ >>> + uint64_t port =3D (uint64_t)addr; >>> + uint16_t v; >>> + if (port < SPAPR_PCI_IO_WIN_SIZE) { >>> + v =3D readw(IOBASE(0) + port); >>> + } else { >>> + v =3D 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. >=20 >=20 > I have a similar need for a unit test of the AST2400 flash controller.=20 >=20 > The flash module is accessible through a memory window and, when in=20 > SPI mode, the commands are sent to the slave by writing at the beginnin= g=20 > of the region. Addresses are necessarily BE to be understood by the SPI > flash module. So, sequences like >=20 > writeb(AST2400_FLASH_BASE, EN_4BYTE_ADDR); > writeb(AST2400_FLASH_BASE, READ); > writel(AST2400_FLASH_BASE, cpu_to_be32(addr)); >=20 > 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.=20 > Maybe something like below would be helpful in libqtest.h >=20 >=20 > +/* > + * Generic read/write helpers for a BE region > + */ > +static inline void writew_be(uint64_t addr, uint16_t value) > +{ > + value =3D 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 =3D 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); > +} >=20 > If this is correct, I can add the LE equivalents and send a patch. >=20 Yes, I think it's a good idea, but add also the functions with the QTestState parameter as it is done with the others: libqtest.c: uint32_t qtest_readl_be(QTestState *s, uint64_t addr) { uint32_t value; qtest_memread(s, addr, &value, sizeof(value)); return be32_to_cpu(value); } libqtest.h: static inline uint32_t readl_be(uint64_t addr) { return qtest_readl_be(global_qtest, addr); } Thanks, Laurent