From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brEG0-0005ne-C2 for qemu-devel@nongnu.org; Mon, 03 Oct 2016 21:10:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brEFw-0001HN-2l for qemu-devel@nongnu.org; Mon, 03 Oct 2016 21:10:55 -0400 Date: Tue, 4 Oct 2016 11:22:23 +1100 From: David Gibson Message-ID: <20161004002223.GB18648@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TakKZr9L6Hm6aLOc" Content-Disposition: inline In-Reply-To: <08ecf131-8749-c468-58ea-3f9369fece07@kaod.org> 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: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: Laurent Vivier , thuth@redhat.com, qemu-devel@nongnu.org, Greg Kurz , qemu-ppc@nongnu.org, Gerd Hoffmann , dgibson@redhat.com --TakKZr9L6Hm6aLOc Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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/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 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_SPAC= ING) > >> +#define IOBASE(index) (PCIBASE(index) + SPAPR_PCI_IO_W= IN_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 > > 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. > >=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 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 > >=20 > > 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. > >=20 > > That should remove the need for swaps further up the test stack. > >=20 > > 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 beginning= =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 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 >=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. I like the idea, but we probably need buy in from some more people. --=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 --TakKZr9L6Hm6aLOc Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX8vY+AAoJEGw4ysog2bOSZZQP/3+q9LdV66h0fK5CkEPaprAR gao5WWyWxfS5eBZ2aThGxKPio25nEmVGEn2F3WYTrKXPAPhDphtLi5j0elBWWp2N TOOmRTUMzjkuiX9v9O1Y6SQgjOzJTvJhxpZDeHfIyRl6rp3LfW6Xo+kApWoLfLJ0 yRbRWttCkV4CFCWNLUi6mKbqB9ER0FGbOGhltwP7Y1SVITysp3N2q+NS9G0tIFgO 8EJsNIjwbMSNWlSQ6DCxHAW48GH+eDqQ5sEaIiaBti49FmtoNIq0FIC7nSvs72um kjMAsQFSnf3wMuUhQlU04p/rNb1QBjrv2/KcmJyNwGyoiOChceeetO182uNBdHto pbbMMTXXju03jaTt4bfbhlTvaTDYslaV9NwzVVFaomKJTXNN7Ut8df7ufieFG0cu jBD20uQU6UjNnmjtbkwey5nHtKI/pkMD9U3PHu5Js4WzQxnaAHiurA3uPIR6//rO RRPfHJBFh7GqoZGa2DXX2XHUtGgiSrdlbOV14uKWQlHxTQ8RLHoi+xkZcgDtpp19 nFAGN7r+ohFb2/jbkikAvfTwva5xufwEwu+dGSGzlj6rXrzErqVPxOjM883OMQ/e 8WBp2eSZLG2IFXmBt81mxiUppHXTZyvTlgOb0Qvx2uQiX9QZoSWryv3NrvabZBZ8 M/gDd6/1r2p1AouqDssb =TUvf -----END PGP SIGNATURE----- --TakKZr9L6Hm6aLOc--