Re: [Qemu-devel] [PATCHv4 08/11] tests: Clean up IO handling in ide-test

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

On Fri, Oct 21, 2016 at 10:31:27AM +0200, Laurent Vivier wrote:
> 
> 
> 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.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tests/ide-test.c | 179 ++++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 118 insertions(+), 61 deletions(-)
> > 
> > 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;
> >  
> > -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 = { .padding = 0 };
> >      int i;
> > @@ -670,7 +717,8 @@ static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
> >  
> >      /* Send Packet */
> >      for (i = 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]));
> >      }
> >  }
> >  
> ...
> > @@ -780,7 +836,8 @@ static void cdrom_pio_impl(int nblocks)
> >  
> >          /* HP4: Transfer_Data */
> >          for (j = 0; j < MIN((limit / 2), rem); j++) {
> > -            rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
> > +            rx[offset + j] = cpu_to_le16(qpci_io_readw(dev,
> > +                                                       ide_base + reg_data));
> >          }
> >      }
> 
> 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.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]