Re: [Qemu-devel] KVM and variable-endianness guest CPUs

[Victor Kamensky]

Hi Peter,

On 22 January 2014 02:22, Peter Maydell <address@hidden> wrote:
> On 22 January 2014 05:39, Victor Kamensky <address@hidden> wrote:
>> Hi Guys,
>>
>> Christoffer and I had a bit heated chat :) on this
>> subject last night. Christoffer, really appreciate
>> your time! We did not really reach agreement
>> during the chat and Christoffer asked me to follow
>> up on this thread.
>> Here it goes. Sorry, it is very long email.
>>
>> I don't believe we can assign any endianity to
>> mmio.data[] byte array. I believe mmio.data[] and
>> mmio.len acts just memcpy and that is all. As
>> memcpy does not imply any endianity of underlying
>> data mmio.data[] should not either.
>
> This email is about five times too long to be actually
> useful,

Sorry, about that you may be right.
My below responses much shorter :)

> but the major issue here is that the data being
> transferred is not just a bag of bytes. The data[]
> array plus the size field are being (mis)used to indicate
> that the memory transaction is one of:
>  * an 8 bit access
>  * a 16 bit access of some uint16_t value
>  * a 32 bit access of some uint32_t value
>  * a 64 bit access of some uint64_t value
>
> exactly as a CPU hardware bus would do. It's
> because the API is defined in this awkward way with
> a uint8_t[] array that we need to specify how both
> sides should go from the actual properties of the
> memory transaction (value and size) to filling in the
> array.

While responding to Alex last night, I found, I think,
easiest and shortest way to think about mmio.data[]

Just for discussion reference here it is again
                struct {
                        __u64 phys_addr;
                        __u8  data[8];
                        __u32 len;
                        __u8  is_write;
                } mmio;
I believe that in all cases it should be interpreted
in the following sense
   byte data[0] goes into byte at phys_addr + 0
   byte data[1] goes into byte at phys_addr + 1
   byte data[2] goes into byte at phys_addr + 2
   and so on up to len size

Basically if it would be on real bus, get byte value
that corresponds to phys_addr + 0 address place
it into data[0], get byte value that corresponds to
phys_addr + 1 address place it into data[1], etc.

I believe it is true for current ARM LE case and
PPC BE case. I am asking you to keep it this way
for all other cases. My ARM BE V7 KVM patches
still use it in the same sense.

What is wrong with it?

Note nowhere in my above description I've talked
about endianity of anything: device, access (E bit),
KVM host, guest, hypervisor. All these endianities
are irrelevant to mmio interface.

> Furthermore, device endianness is entirely irrelevant
> for deciding the properties of mmio.data[], because the
> thing we're modelling here is essentially the CPU->bus
> interface. In real hardware, the properties of individual
> devices on the bus are irrelevant to how the CPU's
> interface to the bus behaves, and similarly here the
> properties of emulated devices don't affect how KVM's
> interface to QEMU userspace needs to work.

As far as mmio interface concerned I claim that any
endianity is irrelevant here. I am utterly lost about
endianity of what you care about? Consider
the following ARM code snippets:

setend le
mov r1, #0x04030201
str r1, [r0]

and

setend be
mov r1, #0x01020304
str r1, [r0]

when above snippets are executed memory bus
sees absolutely the same thing, can you tell by
looking at this memory transaction what endianity
is it? And endianity of what? I can't.
The only thing you can tell by looking at this bus
memory transaction is that 0x01 byte value goes at
r0 address, 0x02 byte value goes at r0 + 1 address,
etc.

Thanks,
Victor

> MemoryRegion's 'endianness' field, incidentally, is
> a dreadful mess that we should get rid of. It is attempting
> to model the property that some buses/bridges have of
> doing byte-lane-swaps on data that passes through as
> a property of the device itself. It would be better if we
> modelled it properly, with container regions having possible
> byte-swapping and devices just being devices.
>
> thanks
> -- PMM