Re: [Qemu-devel] [PATCH V14 5/5] VMXNET3 device implementation

[Alexander Graf]

On 27.03.2013, at 16:46, Paolo Bonzini wrote:

> Il 27/03/2013 15:49, Alexander Graf ha scritto:
>>>> +#if defined(HOST_WORDS_BIGENDIAN)
>>>> +#define const_cpu_to_le64(x) bswap_64(x)
>>>> +#define __BIG_ENDIAN_BITFIELD
>> Ah, sorry, I replied to the wrong version.
>> 
>> ARE YOU KIDDING ME? BIG ENDIAN BITFIELD? BITFIELDS ARE _IMPLEMENTATION 
>> SPECIFIC_!
>> 
>> Can we please revert this whole patch set and send the authors back to 
>> school?
> 
> Can we please maintain a decent tone?
> 
> First, this file comes from Linux.  __BIG_ENDIAN_BITFIELD is a Linux
> #define.  No doubt it is wrong to define it based on
> HOST_WORDS_BIGENDIAN, it is better to use a configure check.  But it's
> not the reason why PPC compilation fails.

No, but it indicates that the code isn't written with portability in mind.

It's simply wrong to define it in the first place. You shouldn't do any 
assumptions how bitfields are laid out in memory / registers. Linux gets away 
with it mostly because it's heavily tied to gcc, but we shouldn't take the same 
assumptions in QEMU code.

> Second, you haven't said _how_ it breaks PPC compilation.  Just
> cut-and-paste from the compiler is enough.  Ok, I can guess it but not
> always.

In file included from hw/vmxnet3.c:30:
hw/vmxnet3.h:140: error: braced-group within expression allowed only inside a 
function
hw/vmxnet3.h:141: error: braced-group within expression allowed only inside a 
function
hw/vmxnet3.h:142: error: braced-group within expression allowed only inside a 
function
hw/vmxnet3.h:143: error: braced-group within expression allowed only inside a 
function

But it doesn't really matter what the reason for the breakage is, the code 
won't work on big endian hosts even with that bswap removed. In fact, the bswap 
was even a 64bit bswap on a 32bit guest field, so that code wouldn't even have 
worked if gcc hadn't complained (which probably means the Linux code here is 
broken).

> Third, there is no need to revert the patch set.  The const_cpu_to_le64
> should simply be removed, since little-endian conversion is already done
> in vmw_shmem_ld32.

Yes, and bitfields should be converted to bitmasks. Then you don't have to 
worry at all about anything big or little endian except for the few interfaces 
to guest memory.

Whenever you see an explicit endianness swap, be sure to get very suspicious.


Alex