From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39815) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzFoV-0003b9-6P for qemu-devel@nongnu.org; Wed, 26 Oct 2016 00:27:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzFoU-0006aJ-1M for qemu-devel@nongnu.org; Wed, 26 Oct 2016 00:27:43 -0400 Date: Wed, 26 Oct 2016 15:22:14 +1100 From: David Gibson Message-ID: <20161026042214.GX11052@umbus.fritz.box> References: <1477285201-10244-1-git-send-email-david@gibson.dropbear.id.au> <1477285201-10244-8-git-send-email-david@gibson.dropbear.id.au> <20161025121654.GD11052@umbus.fritz.box> <325755c7-d493-145a-eeb4-c898e484dd53@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="uR2Be0xXxbzP+34t" Content-Disposition: inline In-Reply-To: <325755c7-d493-145a-eeb4-c898e484dd53@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCHv5 07/12] libqos: Implement mmio accessors in terms of mem{read, write} List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, lvivier@redhat.com, agraf@suse.de, stefanha@redhat.com, mst@redhat.com, mdroth@linux.vnet.ibm.com, groug@kaod.org, thuth@redhat.com --uR2Be0xXxbzP+34t Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Oct 26, 2016 at 12:18:03PM +1100, Alexey Kardashevskiy wrote: > On 25/10/16 23:16, David Gibson wrote: > > On Tue, Oct 25, 2016 at 05:47:43PM +1100, Alexey Kardashevskiy wrote: > >> On 24/10/16 15:59, David Gibson wrote: > >>> In the libqos PCI code we now have accessors both for registers (byte > >>> significance preserving) and for streaming data (byte address order > >>> preserving). These exist in both the interface for qtest drivers and= in > >>> the machine specific backends. > >>> > >>> However, the register-style accessors aren't actually necessary in the > >>> backend. They can be implemented in terms of the byte address order > >>> preserving accessors by the libqos wrappers. This works because PCI = is > >>> always little endian. > >>> > >>> This does assume that the back end byte address order preserving acce= ssors > >>> will perform the equivalent of a single bus transaction for short len= gths. > >>> This is the case, and in fact they currently end up using the same > >>> cpu_physical_memory_rw() implementation within the qtest accelerator. > >>> > >>> Signed-off-by: David Gibson > >>> Reviewed-by: Laurent Vivier > >>> Reviewed-by: Greg Kurz > >>> --- > >>> tests/libqos/pci-pc.c | 38 -------------------------------------- > >>> tests/libqos/pci-spapr.c | 44 --------------------------------------= ------ > >>> tests/libqos/pci.c | 20 ++++++++++++++------ > >>> tests/libqos/pci.h | 8 -------- > >>> 4 files changed, 14 insertions(+), 96 deletions(-) > >>> > >> > >> [...] > >> > >>> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h > >>> index 2b08362..ce6ed08 100644 > >>> --- a/tests/libqos/pci.h > >>> +++ b/tests/libqos/pci.h > >>> @@ -27,18 +27,10 @@ struct QPCIBus { > >>> uint16_t (*pio_readw)(QPCIBus *bus, uint32_t addr); > >>> uint32_t (*pio_readl)(QPCIBus *bus, uint32_t addr); > >>> =20 > >>> - uint8_t (*mmio_readb)(QPCIBus *bus, uint32_t addr); > >>> - uint16_t (*mmio_readw)(QPCIBus *bus, uint32_t addr); > >>> - uint32_t (*mmio_readl)(QPCIBus *bus, uint32_t addr); > >>> - > >>> void (*pio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); > >>> void (*pio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); > >>> void (*pio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); > >>> =20 > >>> - void (*mmio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); > >>> - void (*mmio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); > >>> - void (*mmio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); > >>> - > >>> void (*memread)(QPCIBus *bus, uint32_t addr, void *buf, size_t l= en); > >>> void (*memwrite)(QPCIBus *bus, uint32_t addr, const void *buf, s= ize_t len); > >>> =20 > >>> > >> > >> You added them in "libqos: Handle PCI IO de-multiplexing in common cod= e" > >> (few patched before) and removing them now - if you moved this patch > >> earlier, it would reduce the series, or what do I miss? > >=20 > > Well, it can't go before the PIO / MMIO split, because on x86 the PIO > > part is implemented with inw/outw instead of readw/writew, and those > > don't have a memread/memwrite equivalent. > >=20 > > The change could go at the same time, but my feeling was that logical > > separation of the steps was worth a bit of temporary extra code. >=20 > It is a bit hard to follow the logic of the patchset when you do not know > if the new code is going to stay or not - I automatically assumed it is > staying and when I saw it is being removed - I wondered if you are removi= ng > what you just added, and this - in my opinion - kills the idea of making > smaller patches to make review easier, better just squash them all... But > since Greg is happy and things seems not working worse (make check fails = on > my setup but whatever), you can ignore me :) Well, I guess it's a trade-off between conceptual simplicity and minimal code changes. Putting those callbacks in temporarily means more code change, but I think it's worth it to make each patch conceptually simpler. --=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 --uR2Be0xXxbzP+34t Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYEC9zAAoJEGw4ysog2bOS6t8P/3c4mB/zGUlMM+AmEA48SCTb woshkBl+9vBS8X0RYwzmmRR2N09twDdEKlLZCHG+z+N/ccqAOnT+pS2hKPif5rxy cQPdWc4VkW8qAEjyl2wnPzeOGAB83PVZ6cO7iisQ+Ijg5uKaG+1hyGhL3EIsDHsa OvsHR0sZrMJ2eyYTO/WQDGJ6fol2GYdx9GxwJgfP4iXnxCTAMqJJuf/aRua2fpUl frbktJLgbmm/nSScH6ajp3ozHiFWNtgkL89ypb2GEvPSkvqYIuP6dsuvx43ZhNaF 9/+8rkWD6kxjbHHR6Ooe25wd3okka1rYczeUxO1WOabRSwORfDgehqk4ziF5GwYW NslndnHjtp7se+WiLKiJuA9zgTdYPIna6pK3v+FM5pMeRB5HHVzFCVZ21P7SsUl4 RmAu/e3r3cqXCobjqGFWnx0hCyAr/y6ripEBJY/2f9ztAPjyKLSV8t3LrFe0eooc lM8VnwfYieAVJpIFyNbmVSe+u17OANVT2Y4TENwndSc1Ths9I3bCF3/fj7pds1cF Vy0QxcwLN+6E2N+3ZgAyvNxo/BGKTlbPqyW7PhAhA8V7S23Sm8dN/pNqiyiLEyBb VhUKSbIdf9Y170G52qMc2J0GmtxzJN4eXbBmzUVhoprCBQFfJVE1/wh3giM7/SrO xOnBaDjrbB4aida67/vs =+HOE -----END PGP SIGNATURE----- --uR2Be0xXxbzP+34t--