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

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

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

On Wed, Oct 26, 2016 at 12:57:26PM +1100, Alexey Kardashevskiy wrote:
> 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 <david@gibson.dropbear.id.au>
> > [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?

You're looking at the PC specific backend, which knows that the target
endianness is LE, and so target_to_le16() is a NOP.  The translation
from hsot to guest endianness happens down inside the outw logic.
qtest.c calls outw, which calls stw_p, which is defined to do the swap
for the target endianness in include/exec/cpu-all.h

If you look at the spapr backend, you'll see that the PIO callbacks
have an unconditional byteswap in them.  The spapr backend is ppc
specific which is notionally BE, so it always needs a swap in order to
get LE writes.

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

It does.  It's just the means to accomplishing that is a bit
convoluted for the PIO case.  That's exactly why I think the base
in/out operations should also be fixed endianness, rather than guest
endian, but that's an argument I'm having elsewhere.

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

You need to think of this in two different parts.  Building the buffer
as a bytestream, which includes BE components.  Then sending the
buffer to the hardware as a bytestream.  This has balanced le<->cpu
conversions in order to preserve bytestream order.

Remember than endian is a property of a value - something that has a
specific length and location, not of a bytestream or bus of itself.
The fields in the request are BE, hence the BE conversions.  The data
*register* which we write stuff out to is treated as LE, hence the LE
conversions.

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