From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46769) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxW5F-0006dC-7r for qemu-devel@nongnu.org; Fri, 21 Oct 2016 05:25:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxW5E-0003ca-5t for qemu-devel@nongnu.org; Fri, 21 Oct 2016 05:25:49 -0400 Date: Fri, 21 Oct 2016 20:05:35 +1100 From: David Gibson Message-ID: <20161021090535.GA10218@umbus.fritz.box> References: <1477012792-3326-1-git-send-email-david@gibson.dropbear.id.au> <1477012792-3326-9-git-send-email-david@gibson.dropbear.id.au> <8f01103a-9c90-21d2-131c-1e58828615d4@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Nq2Wo0NMKNjxTN9z" Content-Disposition: inline In-Reply-To: <8f01103a-9c90-21d2-131c-1e58828615d4@redhat.com> Subject: Re: [Qemu-devel] [PATCHv4 08/11] tests: Clean up IO handling in ide-test List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de, stefanha@redhat.com, mst@redhat.com, aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, groug@kaod.org, thuth@redhat.com --Nq2Wo0NMKNjxTN9z Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 21, 2016 at 10:31:27AM +0200, Laurent Vivier wrote: >=20 >=20 > On 21/10/2016 03:19, David Gibson wrote: > > ide-test uses many explicit inb() / outb() operations for its IO, which > > means it's not portable to non-x86 platforms. This cleans it up to use > > the libqos PCI accessors instead. > >=20 > > Signed-off-by: David Gibson > > --- > > tests/ide-test.c | 179 ++++++++++++++++++++++++++++++++++++-----------= -------- > > 1 file changed, 118 insertions(+), 61 deletions(-) > >=20 > > diff --git a/tests/ide-test.c b/tests/ide-test.c > > index a8a4081..86c4373 100644 > > --- a/tests/ide-test.c > > +++ b/tests/ide-test.c > ... > > @@ -654,7 +700,8 @@ typedef struct Read10CDB { > > uint16_t padding; > > } __attribute__((__packed__)) Read10CDB; > > =20 > > -static void send_scsi_cdb_read10(uint64_t lba, int nblocks) > > +static void send_scsi_cdb_read10(QPCIDevice *dev, void *ide_base, > > + uint64_t lba, int nblocks) > > { > > Read10CDB pkt =3D { .padding =3D 0 }; > > int i; > > @@ -670,7 +717,8 @@ static void send_scsi_cdb_read10(uint64_t lba, int = nblocks) > > =20 > > /* Send Packet */ > > for (i =3D 0; i < sizeof(Read10CDB)/2; i++) { > > - outw(IDE_BASE + reg_data, cpu_to_le16(((uint16_t *)&pkt)[i])); > > + qpci_io_writew(dev, ide_base + reg_data, > > + le16_to_cpu(((uint16_t *)&pkt)[i])); > > } > > } > > =20 > ... > > @@ -780,7 +836,8 @@ static void cdrom_pio_impl(int nblocks) > > =20 > > /* HP4: Transfer_Data */ > > for (j =3D 0; j < MIN((limit / 2), rem); j++) { > > - rx[offset + j] =3D le16_to_cpu(inw(IDE_BASE + reg_data)); > > + rx[offset + j] =3D cpu_to_le16(qpci_io_readw(dev, > > + ide_base + reg_= data)); > > } > > } >=20 > Why do you swap le16_to_cpu() and cpu_to_le16()? So, obviously le16_to_cpu() and cpu_to_le16() are functionally identical. But I think my version is conceptually more correct. This is streaming data via PIO - we're essentially reading a byte stream in 16-bit chunks. So, overall we want to preserve byte address order. qpci_io_readw() (and inw()) is designed for reading registers so it preserves byte significance, rather than address order. So, since the IDE registers are LE, that means if implicitly contains an le16_to_cpu(). The cpu_to_le16() undoes that so that rx[] ends up with the bytestream in the correct order. > I think this is not semantically correct and the purpose of this patch > is only to replace inX()/outX() functions. No, I believe it is correct. And I think the original was technically wrong, because inw() reads the register in target endian rather than LE. Of course the only common platform with "real" in/out instructions is x86, which is LE anyway, so it hardly matters. --=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 --Nq2Wo0NMKNjxTN9z Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYCdpcAAoJEGw4ysog2bOSTdwP/RBabPOs9AjYsmFhcdjCz8k1 618ODF1bdOcxr3GDFk/cwK78uFXxnXXwIqSqB8lkadhp5LGgGZ43+UUozV22qa5b IMsVczFpj5CqiEBO1ADAxiESOyHsyw2F+m3+I5l2w4RCe16GBwKVxjbV8WujQq77 1w5KMFCqSziJN8hBenfyAASNtTn4h21FHjel6nxaRHUnDHZalnle16OFLTm50fsH HZ2xlSBrtpWiWhpiVft5hSxVMYmXV6RSOyAk1yhR65asBK71ueDIYl1ahNymnKhw Tk3EiGw4JibJMT2gPKMsFFUtF9V5dG2g5e6uHLvjxKghGSwqe/g1PxUsG0jnxbkS 78Hb2HBVjtOiTHQQmY6Zzkj9WLn/aA6bKIFHJKqu5M6gY6XlmTt3wTyPifXr8Yj5 JWRd5Ijo/7V5vo42z/hYXiiCv9pfM4lizE3oxKI7t9bis6jrNRN7Vaxfv3EZ0JOg PGhmYJWaX8rCZBYNvFD6PWNSHbPmlNm+Pk2oirJqFoPo24u3NZHtImNYwFD5HXNO 6z21qS80Yr1PPYH+nAkyAu9KYN6zY9H8e8ZuqYSkAwYgS9s/+ZWysHcCt1HUx6HO VpIJ8L32+SQIL8FUw/cUZ9BVL2hRUO76xxraz9QIFa/H1+waYUrq8+ke5hLRe+Mn zWtcKR0a7Dremm5HEEM3 =lo3e -----END PGP SIGNATURE----- --Nq2Wo0NMKNjxTN9z--