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

[Paolo Bonzini]

On 06/07/2015 14:12, Michael S. Tsirkin wrote:
> 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.

Absolutely not.  The endianness of the device read/write callbacks is
included in the MemoryRegionOps.

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

An X-bit unsigned integer, such as the one passed to e1000_mmio_write,
doesn't have an endianness--neither big nor little.  It's just a number
between 0 and 2^X-1.  Endianness becomes a property of uintX_t only when
you read it as bytes through memcpy, which e1000_mmio_write doesn't do
(mac_reg is an array of uint32_t).

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

You mean byte 7.

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

I fail to see how host endianness enters the picture.  As Peter said,
there isn't a single access based on host endianness in the MMIO
dispatch path.

If you think there is a bug, please write a testcase.  Then we can
discuss based on something concrete, or just fix it.  See the recent
problems with access clamping, where based on concrete testcases we were
able to fix all the bugs and actually show that the patch was desirable
despite the number of bugs it unveiled.

Paolo