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

[David Gibson]

On Fri, Oct 21, 2016 at 12:07:28PM +0200, Laurent Vivier wrote:
> 
> 
> On 21/10/2016 11:05, David Gibson wrote:
> > 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 <address@hidden>
> >>> ---
> >>>  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.
> 
> I agree
> 
> > 
> > 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.
> 
> In fact, it has an implicit "le16 to target CPU", so you should not have
> a "cpu_to_le16()" but a "qtest_big_endian() != host_big_endian
> bswap16()" to swap it.

No.. it really is le16_to_cpu().  For memory space accesses, there's
an explicit le16_to_cpu in qpci_io_readw() on top of the
order-preserving memread backend.  For IO space accesses, we call the
backend directly.  In all likely cases that will be turned into a (non
PCI) either inw() or readw().

Both inw() and readw() are defined to access in target endianness
(which is stupid, but that argument is in several other threads).  So
they have an implicit "target_to_host16()".  But the PCI versions are
defined to be always LE - on BE targets the backend will add an extra
swap, changing that target_to_host16() into an le16_to_cpu().

> As this test works only with a little endian guest, the first "LE16 to
> target CPU" is a no-op, and the second one does something only on big
> endian host so when "qtest_big_endian() != host_big_endian".
> 
> >> 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.
> 
> I agree, this is why I think a "qtest_big_endian() != host_big_endian
> then bswap16()" is better than a "cpu_to_le16()".

No, cpu_to_le16() is correct.  Well.. actually using an
order-preserving read would be correct, but that's a bit awkward for
tne io port case.

> 
> But I also think it should be the purpose of another patch...
> 
> Thanks,
> Laurent
> 

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

signature.asc
Description: PGP signature