From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46766) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brapA-0003rd-5k for qemu-devel@nongnu.org; Tue, 04 Oct 2016 21:16:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brap6-0007Z5-PC for qemu-devel@nongnu.org; Tue, 04 Oct 2016 21:16:43 -0400 Date: Wed, 5 Oct 2016 12:15:18 +1100 From: David Gibson Message-ID: <20161005011518.GH18648@umbus.fritz.box> 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> <20161003160314.54771281@bahia> <20161004002454.GC18648@umbus.fritz.box> <20161004085835.29402918@bahia> <20161004093630.30c0be63@bahia> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="fmEUq8M7S0s+Fl0V" Content-Disposition: inline In-Reply-To: <20161004093630.30c0be63@bahia> 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: Greg Kurz Cc: Laurent Vivier , thuth@redhat.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Gerd Hoffmann , dgibson@redhat.com, =?iso-8859-1?Q?C=E9dric?= Le Goater --fmEUq8M7S0s+Fl0V Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 04, 2016 at 09:36:30AM +0200, Greg Kurz wrote: > On Tue, 4 Oct 2016 08:58:35 +0200 > Greg Kurz wrote: >=20 > > On Tue, 4 Oct 2016 11:24:54 +1100 > > David Gibson wrote: > >=20 > > > On Mon, Oct 03, 2016 at 04:03:14PM +0200, Greg Kurz wrote: =20 > > > > On Mon, 3 Oct 2016 13:23:27 +0200 > > > > C=E9dric Le Goater wrote: > > > > =20 > > > > > On 09/29/2016 07:27 AM, David Gibson wrote: =20 > > > > > > On Wed, Sep 28, 2016 at 08:51:28PM +0200, Laurent Vivier wrote:= =20 > > > > > >> 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-sp= apr.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/li= bqos-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 slo= t) > > > > > >> { > > > > > >> QDict *response; > > > > > >> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr= =2Ec > > > > > >> 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, vers= ion 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_SPACIN= G - \ > > > > > >> + SPAPR_PCI_MEM_WIN_BUS_OF= FSET) > > > > > >> +#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_WIN= DOW_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 =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); > > > > > >> + } =20 > > > > > >=20 > > > > > > 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 t= est > > > > > > case side and which were on the qtest accel stub side. > > > > > >=20 > > > > > > So, what I think you need is an unconditional byteswap in each = of the > > > > > > spapr pci IO functions. Why? > > > > > >=20 > > > > > > * The plain readw() etc. functions return values read from m= emory > > > > > > assuming guest endianness. For guest native values in mem= ory, so > > > > > > far so good. > > > > > > * Generic users of the pci read/write functions in qtest exp= ect to > > > > > > get native values. > > > > > > * But PCI devices are always LE, not guest endian > > > > > >=20 > > > > > > The guest endianness of spapr (as far as tswap() etc. are conce= rned) > > > > > > is BE, even though that's not really true in practice these day= s, so > > > > > > to correct for the PCI registers being read as BE when they sho= uld be > > > > > > LE we always need a swap. > > > > > >=20 > > > > > > That should remove the need for swaps further up the test stack. > > > > > >=20 > > > > > > Of course, "guest endianness" is a poorly defined concept, so l= onger > > > > > > term it might be better to completely replace the "readw" etc. = qtest > > > > > > operations with explicit "readw_le" and "readw_be" ops. =20 > > > > >=20 > > > > >=20 > > > > > I have a similar need for a unit test of the AST2400 flash contro= ller.=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 be= ginning=20 > > > > > of the region. Addresses are necessarily BE to be understood by t= he 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 qt= est =20 > > > >=20 > > > > cpu_to_be32() is indeed what a real guest does... but the real gues= t runs > > > > with the target endianness, whereas the qtest program runs with the= host > > > > endianness... so cpu_to_be32() looks wrong here. > > > > =20 > > > > > layer. I have used memwrite to have endian-agnostic mem accessors= =2E=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 > > > >=20 > > > > The API is correct, but the implementation has the same issue: it u= ses > > > > cpu_to_be32() and bypasses the tswap32() in qtest... the result is = that > > > > the target endianness is totally ignored. =20 > > >=20 > > > Yes, this is precisely the intended result. > > >=20 > > > The target endianness is not well defined, and simply an extra point > > > of confusion for qtest code. > > > =20 > >=20 > > I agree that target endianness is confusing in the test program code but > > tswap32() I'm referring to is the one in qtest.c. > >=20 > > > > Let's suppose that a similar AST2400 platform exists with a BE proc= essor: > > > > would the same test program work for both LE and BE cases ? =20 > > >=20 > > > Yes, that's the idea - assuming the AST2400 has the same IO endianness > > > in either case, which would be usual for most IO devices. > > > =20 > > > > Both the test program and QEMU run with the host endianness acutall= y, > > > > but only QEMU knows about the target endianness. So I guess, the qt= est > > > > 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 endiannes= s. =20 > > >=20 > > > Half right. The target endiannness need not enter into this. > > > =20 > >=20 > > 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 ? >=20 > Ohh... maybe I see now: if the test program says I'm writing BE or LE, th= en > it needs to call cpu_to_*() and then we just want qtest.c to copy the val= ue > to guest memory without modifying it again. That's basically what Cedric's > accessors ^^ do. >=20 > 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. --=20 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 --fmEUq8M7S0s+Fl0V Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX9FQmAAoJEGw4ysog2bOSICUQANt6xZwEY1PhW1ZjfNlZm4iw bxTtZL9z0KWskdXZFPOdh+zxXIU18c7LFEM3vgeAPegyOWovpDU6pNWoKPWVD9mZ E5AW+V4mvSiwN00rLAaR9lWxofblBnIai5uxyfucGW77LGCxgorgbyoyIU16E7d1 upiAPRmsxSbmKN+pOAnsYC3sjYLBZDS7ZQo+Fn2tJFaTIBKUC5Dg8xpR6EGVKhRV V29VeyblzzbHdD6EX+/y+8gNQ5xAH26UeuVaysPeQGtLIoigzP7w0oBOawhvD26W pvsGP+ax+rb0lXUjfQcFaf+8CY7/z3t1NYsMQWs2986A9/48EU5SS1R8R13m7uAB /eaipq/78UUWgVQtwK61Q+FmIgFLIPKdYIjSsBcOP40XdFB+vfx1tCA9xwDwE51I n1kCosMFq5uYJLSkIbUpv1uwRN7gWttqqnEoYdlVjQyzmJG5h0sHtLMEvVqbhUPd XGmBNUtHFKNjpHwMGV72h8E8unGwHfZV5v+QI8MjGcYk29qbqpSItk1RQH4mTuH3 8R+K2ynYH3PnbH08TAOSEceAVzyz15MzKu4GMz/ioOBlVpU+WbAgpLpstgb8axIS FwDbueAWFAX5XMEYsc4B6bWkIaX5MSbZJ+8pqh+Twlt6YJgjjSs7yQBdr6T+xhM0 40j+m01ora2u6C84+n92 =0tjP -----END PGP SIGNATURE----- --fmEUq8M7S0s+Fl0V--