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

[Alexey Kardashevskiy]

On 25/10/16 23:25, David Gibson wrote:
> 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.
>>>
>>> Signed-off-by: David Gibson <address@hidden>
> [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 = { .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]));
>>
>>
>> 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:
>>
>>  /* Construct SCSI CDB packet */
>>  pkt.opcode = 0x28;
>>  pkt.lba = cpu_to_be32(lba);
>>  pkt.nblocks = cpu_to_be16(nblocks);
>>
>> 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()

dev->bus->pio_writew() calls outw() which calls qtest_outw() and
qtest_sendf() where @value is a text - where does this implicit
cpu_to_le16() happen? Or I am reading the code wrong?

The other branch (for MMIO) in qpci_io_writew() calls cpu_to_le16() explicitly.

I'd expect a function with a generic name as qpci_io_writew() to always
take data in the some known and always the same endianness (LE in this case
as it is PCI).

In the chunk above we convert host-CPU-endian @lba to BE then treat it as
LE when converting to CPU-endian and then expect qpci_io_writew() to do
swapping again (or not - depends on BAR type - IO vs. MMIO - or conversion
always happens?), this confuses me a lot. However, everybody else is happy
so am I :)




-- 
Alexey

signature.asc
Description: OpenPGP digital signature