|
From: | Dmitry Fleytman |
Subject: | Re: [Qemu-devel] [PATCH V14 5/5] VMXNET3 device implementation |
Date: | Wed, 27 Mar 2013 18:36:59 +0200 |
No, but it indicates that the code isn't written with portability in mind.
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.
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.
In file included from hw/vmxnet3.c:30:
> 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.
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
[Prev in Thread] | Current Thread | [Next in Thread] |