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

[Laurent Vivier]

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.

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()".

But I also think it should be the purpose of another patch...

Thanks,
Laurent