qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Dmitry Fleytman
Subject: Re: [Qemu-devel] [PATCH V14 5/5] VMXNET3 device implementation
Date: Wed, 27 Mar 2013 18:36:59 +0200

On Wed, Mar 27, 2013 at 6:06 PM, Alexander Graf <address@hidden> wrote:

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.

First of all I'd like to emphasize that vmxnet3 and pvscsi patches are our first experience in writing QEMU devices so most probably we miss some common practices of the project.
The codebase is huge and it takes time to learn things like this.
At the same time we are open for constructive criticism and ready to improve the code we submit.

Regarding this specific case, we used an approach from current QEMU code, namely bitfield definitions technique from slirp/ip.h and slirp/tcp.h files.
We do realize this is not the best way, if there are examples of better techniques in QEMU codebase please point it out.
 

> 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.


Right, we got these definitions as is from VMXNET3 Linux driver header as is including cpu_to_le64() wrapping.
Indeed it defined improperly and should be removed/redefined empty as shared memory accessors doing byte swap automatically.
This is a bug, we'll fix this.

 
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.


The issue is we got device interface definition from Linux and we'd like to leave it unchanged where possible for easier merges of future driver changes.
Indeed bitmasks are much more clear.

Dmitry.
 

Alex



reply via email to

[Prev in Thread] Current Thread [Next in Thread]