Patchwork vmxnet: Don't use bswap_64 for constants

login
register
mail settings
Submitter Richard Henderson
Date March 27, 2013, 6:47 p.m.
Message ID <1364410030-24008-1-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/231809/
State New
Headers show

Comments

Richard Henderson - March 27, 2013, 6:47 p.m.
This macro is used in the context of defining enum values.
We can't use a function call in that case.

Cc: qemu-trivial@nongnu.org
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/vmxnet3.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
Andreas Färber - March 27, 2013, 11:42 p.m.
Am 27.03.2013 19:47, schrieb Richard Henderson:
> This macro is used in the context of defining enum values.
> We can't use a function call in that case.
> 
> Cc: qemu-trivial@nongnu.org
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  hw/vmxnet3.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vmxnet3.h b/hw/vmxnet3.h
> index 7db0c8f..cd9ac85 100644
> --- a/hw/vmxnet3.h
> +++ b/hw/vmxnet3.h
> @@ -37,7 +37,15 @@
>  #define __packed QEMU_PACKED
>  
>  #if defined(HOST_WORDS_BIGENDIAN)
> -#define const_cpu_to_le64(x) bswap_64(x)
> +#define const_cpu_to_le64(x) \
> +        (((x & 0x00000000000000ffULL) << 56) | \
> +         ((x & 0x000000000000ff00ULL) << 40) | \
> +         ((x & 0x0000000000ff0000ULL) << 24) | \
> +         ((x & 0x00000000ff000000ULL) <<  8) | \
> +         ((x & 0x000000ff00000000ULL) >>  8) | \
> +         ((x & 0x0000ff0000000000ULL) >> 24) | \
> +         ((x & 0x00ff000000000000ULL) >> 40) | \
> +         ((x & 0xff00000000000000ULL) >> 56))

Being a macro, shouldn't this better use (x) for operator precedence?

Andreas

>  #define __BIG_ENDIAN_BITFIELD
>  #else
>  #define const_cpu_to_le64(x) (x)
>
Richard Henderson - March 28, 2013, 12:52 a.m.
On 2013-03-27 16:42, Andreas Färber wrote:
> Am 27.03.2013 19:47, schrieb Richard Henderson:
>> This macro is used in the context of defining enum values.
>> We can't use a function call in that case.
>>
>> Cc: qemu-trivial@nongnu.org
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> ---
>>   hw/vmxnet3.h | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vmxnet3.h b/hw/vmxnet3.h
>> index 7db0c8f..cd9ac85 100644
>> --- a/hw/vmxnet3.h
>> +++ b/hw/vmxnet3.h
>> @@ -37,7 +37,15 @@
>>   #define __packed QEMU_PACKED
>>
>>   #if defined(HOST_WORDS_BIGENDIAN)
>> -#define const_cpu_to_le64(x) bswap_64(x)
>> +#define const_cpu_to_le64(x) \
>> +        (((x & 0x00000000000000ffULL) << 56) | \
>> +         ((x & 0x000000000000ff00ULL) << 40) | \
>> +         ((x & 0x0000000000ff0000ULL) << 24) | \
>> +         ((x & 0x00000000ff000000ULL) <<  8) | \
>> +         ((x & 0x000000ff00000000ULL) >>  8) | \
>> +         ((x & 0x0000ff0000000000ULL) >> 24) | \
>> +         ((x & 0x00ff000000000000ULL) >> 40) | \
>> +         ((x & 0xff00000000000000ULL) >> 56))
>
> Being a macro, shouldn't this better use (x) for operator precedence?

It doesn't matter for this usage.

Nor, according to other threads that appeared on the list today, is this
the right fix, since the bswap itself turns out to be bogus.

Myself, I never tested the driver code, just fixed the compile error.


r~
Stefan Hajnoczi - March 28, 2013, 9:45 a.m.
On Wed, Mar 27, 2013 at 05:52:56PM -0700, Richard Henderson wrote:
> On 2013-03-27 16:42, Andreas Färber wrote:
> >Am 27.03.2013 19:47, schrieb Richard Henderson:
> >>This macro is used in the context of defining enum values.
> >>We can't use a function call in that case.
> >>
> >>Cc: qemu-trivial@nongnu.org
> >>Signed-off-by: Richard Henderson <rth@twiddle.net>
> >>---
> >>  hw/vmxnet3.h | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/hw/vmxnet3.h b/hw/vmxnet3.h
> >>index 7db0c8f..cd9ac85 100644
> >>--- a/hw/vmxnet3.h
> >>+++ b/hw/vmxnet3.h
> >>@@ -37,7 +37,15 @@
> >>  #define __packed QEMU_PACKED
> >>
> >>  #if defined(HOST_WORDS_BIGENDIAN)
> >>-#define const_cpu_to_le64(x) bswap_64(x)
> >>+#define const_cpu_to_le64(x) \
> >>+        (((x & 0x00000000000000ffULL) << 56) | \
> >>+         ((x & 0x000000000000ff00ULL) << 40) | \
> >>+         ((x & 0x0000000000ff0000ULL) << 24) | \
> >>+         ((x & 0x00000000ff000000ULL) <<  8) | \
> >>+         ((x & 0x000000ff00000000ULL) >>  8) | \
> >>+         ((x & 0x0000ff0000000000ULL) >> 24) | \
> >>+         ((x & 0x00ff000000000000ULL) >> 40) | \
> >>+         ((x & 0xff00000000000000ULL) >> 56))
> >
> >Being a macro, shouldn't this better use (x) for operator precedence?
> 
> It doesn't matter for this usage.
> 
> Nor, according to other threads that appeared on the list today, is this
> the right fix, since the bswap itself turns out to be bogus.

Dmitry said he's sending a fix, will wait for that.

Stefan

Patch

diff --git a/hw/vmxnet3.h b/hw/vmxnet3.h
index 7db0c8f..cd9ac85 100644
--- a/hw/vmxnet3.h
+++ b/hw/vmxnet3.h
@@ -37,7 +37,15 @@ 
 #define __packed QEMU_PACKED
 
 #if defined(HOST_WORDS_BIGENDIAN)
-#define const_cpu_to_le64(x) bswap_64(x)
+#define const_cpu_to_le64(x) \
+        (((x & 0x00000000000000ffULL) << 56) | \
+         ((x & 0x000000000000ff00ULL) << 40) | \
+         ((x & 0x0000000000ff0000ULL) << 24) | \
+         ((x & 0x00000000ff000000ULL) <<  8) | \
+         ((x & 0x000000ff00000000ULL) >>  8) | \
+         ((x & 0x0000ff0000000000ULL) >> 24) | \
+         ((x & 0x00ff000000000000ULL) >> 40) | \
+         ((x & 0xff00000000000000ULL) >> 56))
 #define __BIG_ENDIAN_BITFIELD
 #else
 #define const_cpu_to_le64(x) (x)