Re: [Qemu-devel] [PATCH] virtio-pci: implement cfg capability

[Michael S. Tsirkin]

On Mon, Jul 06, 2015 at 12:50:43PM +0100, Peter Maydell wrote:
> On 6 July 2015 at 11:31, Michael S. Tsirkin <address@hidden> wrote:
> > On Mon, Jul 06, 2015 at 11:04:24AM +0100, Peter Maydell wrote:
> >> On 6 July 2015 at 11:03, Michael S. Tsirkin <address@hidden> wrote:
> >> > On Mon, Jul 06, 2015 at 10:11:18AM +0100, Peter Maydell wrote:
> >> >> But address_space_rw() is just the "memcpy bytes to the
> >> >> target's memory" operation -- if you have a pile of bytes
> >> >> then there are no endianness concerns. If you don't have
> >> >> a pile of bytes then you need to know the structure of
> >> >> the data you're DMAing around, and you should probably
> >> >> have a loop doing things with the specify-the-width functions.
> >>
> >> > Absolutely. But what if DMA happens to target another device
> >> > and not memory? Device needs some endian-ness so it needs
> >> > to be converted to that.
> >>
> >> Yes, and address_space_rw() already deals with conversion to
> >> that device's specified endianness.
> 
> > Yes, but incorrectly if target endian != host endian.
> > For example, LE target and LE device on BE host.
> 
> Having walked through the code, got confused, talked to
> bonzini on IRC about it and got unconfused again, I believe
> we do get this correct.
> 
>  * address_space_rw() takes a pointer to a pile of bytes
>  * if the destination is RAM, we just memcpy them (because
>    guest RAM is also a pile of bytes)
>  * if the destination is a device, then we read a value
>    out of the pile of bytes at whatever width the target
>    device can handle. The functions we use for this are
>    ldl_q/ldl_p/etc, which do "load target endianness"
>    (ie "interpret this set of 4 bytes as if it were an
>    integer in the target-endianness") because the API of
>    memory_region_dispatch_write() is that it takes a uint64_t
>    data whose contents are the value to write in target
>    endianness order. (This is regrettably undocumented.)
>  * memory_region_dispatch_write() then calls adjust_endianness(),
>    converting a target-endian value to the endianness the
>    device says it requires
>  * we then call the device's read/write functions, whose API
>    is that they get a value in the endianness they asked for.

IMO what's missing here is that device read/write callbacks actually want
host endian-ness. See below for examples.

> > IO callbacks always get a native endian format so they expect to get
> > byte 0 of the buffer in MSB on this host.
> 
> IO callbacks get the format they asked for (which might
> be BE, LE or target endianness). They will get byte 0 of
> the buffer in the MSB if they said they were BE devices
> (or if they said they were target-endian on a BE target).
> 
> thanks
> -- PMM

I believe this last point is wrong.

For example typical PCI devices use little endian. This does not mean
they want an LE value in their callback.

This means guest will give them LE values and they want data
converted *from that* to the natural host endian, for
convenience.

Check e.g. e1000_mmio_write as a typical example.

Imagine that we have another device read data (e.g. from RAM
using address_space_rw) and then write it out to another device
usng address_space_rw.

Run it with x86 target on BE host.
        What do you get is byte 0 goes to bits 0-7 in the value.

Run it with x86 target on LE host.
        What do you get is byte 0 goes to bits 0-7 in the value.

The effect on the device will be very different, and clearly
this is a bug since host endian mustn't affect how
devices work.


-- 
MST