Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anthony Liguori <anthony@codemonkey.ws>]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Anthony Liguori <anthony@codemonkey.ws> writes:
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>
>> The distinction is important in QEMU.  ppc64 is still
>> TARGET_WORDS_BIGENDIAN.  We still want most stl_phys to treat integers
>> as big endian.  There's just this extra concept that CPU loads/stores
>> are sometimes byte swapped.  That affects virtio but not a lot else.
>
> You've redefined endian here; please don't do that.  Endian is the order
> in memory which a CPU does loads and stores.  From any reasonable
> definition, PPC is bi-endian.
>
> It's actually a weird thing for the qemu core to know at all: almost
> everything which cares is in target-specific code.  The exceptions are
> gdb stubs and virtio, both of which are "native endian" (and that weird
> code in exec.c: what is notdirty_mem_write?).
>
> Your argument that we shouldn't fix stl_* might be justifiable (ie. just
> hack virtio and gdb as one-offs), but it's neither clear nor "least
> surprise".

That's not what I'm suggesting.

I'm suggesting that we should introduce multiple variants of {ld,st}*
for different types of memory access.

These are bad names, but I'm thinking along the lines of:

/* Read a word as the load/store instructions would */
cpu_ldst_ldw()

/* Read a word as the instruction fetch unit would */
cpu_fetch_ldw()

/* Read a word as the hardware MMU would */
cpu_mmu_ldw()

Peter was suggesting that instead of having separate functions, we
should use a context:

ldw(cpu->ldst, ..)
ldw(cpu->fetch, ..)
...

I think I prefer functions though over a context.  But this is really
about TCG, not virtio.  As Ben pointed out, virtio endianness needs to
be independent of CPUs.  We process the ring outside of a specific CPU
context and it's possible that if we pick an arbitrary one, it will be
in the wrong context (if running BE userspace).

The only real problem I have with your original patch is putting virtio
knowledge in the target code.  I think your adjusted version is fine.

Regards,

Anthony Liguori

>
> Chers,
> Rusty.