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

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

On Wed, Mar 27, 2013 at 6:06 PM, Alexander Graf <agraf@suse.de> 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>>

On Wed, Mar 27, 2013 at 06:36:59PM +0200, Dmitry Fleytman wrote:
> On Wed, Mar 27, 2013 at 6:06 PM, Alexander Graf <agraf@suse.de> 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.
It may not be obvious since it's part of the source tree but slirp is
not a good example of QEMU code. It actually comes from
http://slirp.sourceforge.net/.
Stefan