From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59037) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz1qY-00042I-Cn for qemu-devel@nongnu.org; Tue, 25 Oct 2016 09:32:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bz1qV-0000iv-6A for qemu-devel@nongnu.org; Tue, 25 Oct 2016 09:32:54 -0400 Date: Tue, 25 Oct 2016 23:25:12 +1100 From: David Gibson Message-ID: <20161025122512.GE11052@umbus.fritz.box> References: <1477285201-10244-1-git-send-email-david@gibson.dropbear.id.au> <1477285201-10244-9-git-send-email-david@gibson.dropbear.id.au> <035d0921-7031-8d3f-5d2c-4930d4c2788c@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Eldrgvv4EWsIM1sO" Content-Disposition: inline In-Reply-To: <035d0921-7031-8d3f-5d2c-4930d4c2788c@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCHv5 08/12] tests: Clean up IO handling in ide-test 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 --Eldrgvv4EWsIM1sO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 25, 2016 at 06:01:41PM +1100, Alexey Kardashevskiy wrote: > On 24/10/16 15:59, 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 [snip] > > -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 >=20 > cpu_to_le16 -> le16_to_cpu conversion here and below (at the very end) is > not obvious. Right above this chunk the @pkt fields are initialized as BE: >=20 > /* Construct SCSI CDB packet */ > pkt.opcode =3D 0x28; > pkt.lba =3D cpu_to_be32(lba); > pkt.nblocks =3D cpu_to_be16(nblocks); >=20 > outw() seems to be CPU-endian, and qpci_io_writew() as well, or not? outw() is guest CPU endian (which is stupid, but that's another matter). qpci_io_writew() is different - it is always LE, because PCI devices are always LE (well, ok, nearly always). So, yes, this is a bit confusing. Here's what's going on: * the SCSI standard uses BE, so that's what we put into the packet structure * We need to transfer the packet to the device as a bytestream - so no endianness conversions * But.. we do so in 16-bit chunks * .. and qpci_io_writew() is designed to take CPU values and write them out as LE - ie, it contains an implicit cpu_to_le16() * So, we use le16_to_cpu() to cancel out that swap, so that we write out the bytes in the correct order If this were a Linux driver here, then we'd use a writew_rep() type primitive, instead of writew(). The rep / streaming variants don't byteswap exactly because they're designed for streaming out bytestream data, not word values. Really what we want here is a memwrite() type operation, but that doesn't quite work because this is PIO, not MMIO, so on x86 it turns into CPU ins and outs which don't have "streaming" variants. > > } > > } > > =20 > > @@ -683,13 +731,17 @@ static void nsleep(int64_t nsecs) > > =20 > > static uint8_t ide_wait_clear(uint8_t flag) > > { > > + QPCIDevice *dev; > > + void *bmdma_base, *ide_base; > > uint8_t data; > > time_t st; > > =20 > > + dev =3D get_pci_device(&bmdma_base, &ide_base); > > + > > /* Wait with a 5 second timeout */ > > time(&st); > > while (true) { > > - data =3D inb(IDE_BASE + reg_status); > > + data =3D qpci_io_readb(dev, ide_base + reg_status); > > if (!(data & flag)) { > > return data; > > } > > @@ -723,6 +775,8 @@ static void ide_wait_intr(int irq) > > =20 > > static void cdrom_pio_impl(int nblocks) > > { > > + QPCIDevice *dev; > > + void *bmdma_base, *ide_base; > > FILE *fh; > > int patt_blocks =3D MAX(16, nblocks); > > size_t patt_len =3D ATAPI_BLOCK_SIZE * patt_blocks; > > @@ -741,13 +795,15 @@ static void cdrom_pio_impl(int nblocks) > > =20 > > ide_test_start("-drive if=3Dnone,file=3D%s,media=3Dcdrom,format=3D= raw,id=3Dsr0,index=3D0 " > > "-device ide-cd,drive=3Dsr0,bus=3Dide.0", tmp_path); > > + dev =3D get_pci_device(&bmdma_base, &ide_base); > > qtest_irq_intercept_in(global_qtest, "ioapic"); > > =20 > > /* PACKET command on device 0 */ > > - outb(IDE_BASE + reg_device, 0); > > - outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF); > > - outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF)); > > - outb(IDE_BASE + reg_command, CMD_PACKET); > > + qpci_io_writeb(dev, ide_base + reg_device, 0); > > + qpci_io_writeb(dev, ide_base + reg_lba_middle, BYTE_COUNT_LIMIT & = 0xFF); > > + qpci_io_writeb(dev, ide_base + reg_lba_high, > > + (BYTE_COUNT_LIMIT >> 8 & 0xFF)); > > + qpci_io_writeb(dev, ide_base + reg_command, CMD_PACKET); > > /* HP0: Check_Status_A State */ > > nsleep(400); > > data =3D ide_wait_clear(BSY); > > @@ -756,7 +812,7 @@ static void cdrom_pio_impl(int nblocks) > > assert_bit_clear(data, ERR | DF | BSY); > > =20 > > /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */ > > - send_scsi_cdb_read10(0, nblocks); > > + send_scsi_cdb_read10(dev, ide_base, 0, nblocks); > > =20 > > /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes. > > * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 by= tes. > > @@ -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 > >=20 >=20 >=20 --=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 --Eldrgvv4EWsIM1sO Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYD08oAAoJEGw4ysog2bOSGs0QALmLTLtH0bfLuMpuYcmKhLHN MWKK++mHZ/xTMR/wqGp0Z1P6qT/GCvNWmNiMwAVcLnE9s9n6x2126up1SFrr2Iw0 RnowtCJIzq43Ew6pP2WcyH/FCLa2+kb01DpOBtxXc0Zt3U792NhABz3awUQ4Lmd9 077nqmt6l3FYksXbIEBbV0RB1urEt36FtZlria5yByH+ylEQluDGpQiEZioOc96k iz9LwZHI/C1f0MD3bz2xcDKwA3HNDJkZGWa4NDC2EbpcGzmvbvizD0EFvHndEQ/F 37Ixb39JXRBjgR2QX6et/a5KvSQqv9Rxmao7WorZcbIFHwHB2FIvh/7SJIkQSgD4 wUqGiUzhd4rsTfvyCuWqmCDSOkIyVDgLWj7bFbxn550dqM3MKjOihhBzvl/dt1F7 z3OMEmePw9ltgP52anWais1tmN6BxVTlOHN0Xw5JdGUfVKRBIXzWAMJWTkBk4Z6p 32rMpuzFpO+FK+zBm+NEqiNTIvCQX1k1JVGqTmrZNt1Nti/8cGGRe0cxDZ+Z39ZT /L+ncaQSJI47qQ/KyOgfB4WA7PUb32T7U8MLdnnQSvS2mcCn6NC5fhrVYHEvrSRu 4+HyfTH7X/hDM+b3x0PyYYz5yWYz0DuXh+NUv0kYK9IxI3GYarzG6N9aECS+XpFm 5EV0gptulkf6HsW+xkSo =taTn -----END PGP SIGNATURE----- --Eldrgvv4EWsIM1sO--