From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55557) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brJUB-0002rg-Vp for qemu-devel@nongnu.org; Tue, 04 Oct 2016 02:45:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brJU8-0006zO-Pe for qemu-devel@nongnu.org; Tue, 04 Oct 2016 02:45:56 -0400 Received: from 7.mo68.mail-out.ovh.net ([46.105.63.230]:40652) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brJU8-0006ym-HY for qemu-devel@nongnu.org; Tue, 04 Oct 2016 02:45:52 -0400 Received: from player711.ha.ovh.net (b9.ovh.net [213.186.33.59]) by mo68.mail-out.ovh.net (Postfix) with ESMTP id 9CBAEFFBB46 for ; Tue, 4 Oct 2016 08:45:51 +0200 (CEST) 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> <20161004002223.GB18648@umbus.fritz.box> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: Date: Tue, 4 Oct 2016 08:45:43 +0200 MIME-Version: 1.0 In-Reply-To: <20161004002223.GB18648@umbus.fritz.box> Content-Type: text/plain; charset=windows-1252 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: David Gibson Cc: Laurent Vivier , thuth@redhat.com, qemu-devel@nongnu.org, Greg Kurz , qemu-ppc@nongnu.org, Gerd Hoffmann , dgibson@redhat.com On 10/04/2016 02:22 AM, David Gibson wrote: > On Mon, Oct 03, 2016 at 01:23:27PM +0200, C=E9dric 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/libqo= s/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-p= c.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 = 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_SP= ACING) >>>> +#define IOBASE(index) (PCIBASE(index) + SPAPR_PCI_IO= _WIN_OFF) >>>> +#define MMIOBASE(index) (PCIBASE(index) + SPAPR_PCI_MM= IO_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, s= o >>> 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.= =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 beginni= ng=20 >> of the region. Addresses are necessarily BE to be understood by the SP= I >> 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.=20 >=20 > Right. You could fix this by writing tswap32(cpu_to_be32(addr)), of > course but it's starting to get ugly. >=20 >> Maybe something like below would be helpful in libqtest.h >=20 >=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); >> +} >> >> If this is correct, I can add the LE equivalents and send a patch. >=20 > I like the idea, but we probably need buy in from some more people. Yes. I should be sending the latest patch as an RFC but I need to test=20 the LE read/write first.=20 Thanks, C.=20 =20