[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V14 5/5] VMXNET3 device implementation
From: |
Alexander Graf |
Subject: |
Re: [Qemu-devel] [PATCH V14 5/5] VMXNET3 device implementation |
Date: |
Wed, 27 Mar 2013 17:06:23 +0100 |
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
- [Qemu-devel] [PATCH V14 0/5] VMXNET3 paravirtual NIC device implementation, Dmitry Fleytman, 2013/03/09
- [Qemu-devel] [PATCH V14 1/5] Checksum-related utility functions, Dmitry Fleytman, 2013/03/09
- [Qemu-devel] [PATCH V14 2/5] net: iovec checksum calculator, Dmitry Fleytman, 2013/03/09
- [Qemu-devel] [PATCH V14 3/5] Common definitions for VMWARE devices, Dmitry Fleytman, 2013/03/09
- [Qemu-devel] [PATCH V14 4/5] Packet abstraction for VMWARE network devices, Dmitry Fleytman, 2013/03/09
- [Qemu-devel] [PATCH V14 5/5] VMXNET3 device implementation, Dmitry Fleytman, 2013/03/09
Re: [Qemu-devel] [PATCH V14 0/5] VMXNET3 paravirtual NIC device implementation, Stefan Hajnoczi, 2013/03/12