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 <da...@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
signature.asc
Description: PGP signature