diff mbox

[dpdk-dev] fm10k: fix the compilation on big endian platforms

Message ID 1438583517-19035-2-git-send-email-chaozhu@linux.vnet.ibm.com (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Chao Zhu Aug. 3, 2015, 6:31 a.m. UTC 
  The rte_cpu_to_le_32 function can't be used to define const variables
because it has different implementation on big endian platforms. If
doing so, it will cause 'initializer element is not constant' compiling
error. This patch fixes this problem.

Signed-off-by: Chao Zhu <chaozhu@linux.vnet.ibm.com>
---
 drivers/net/fm10k/base/fm10k_tlv.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

Comments

Thomas Monjalon Aug. 3, 2015, 9:06 a.m. UTC | #1 
2015-08-03 14:31, Chao Zhu:
> The rte_cpu_to_le_32 function can't be used to define const variables
> because it has different implementation on big endian platforms. If
> doing so, it will cause 'initializer element is not constant' compiling
> error. This patch fixes this problem.

> --- a/drivers/net/fm10k/base/fm10k_tlv.c
> +++ b/drivers/net/fm10k/base/fm10k_tlv.c

You cannot change a base driver file except the osdep header where
FM10K_CPU_TO_LE32 is defined.

I don't understand why it doesn't give you a constant, given this definition:

#define rte_bswap32(x) ((uint32_t)(__builtin_constant_p(x) ?        \                                            
                   rte_constant_bswap32(x) :        \
                   rte_arch_bswap32(x)))

Have you tried CONFIG_RTE_FORCE_INTRINSICS=y ?
It should trigger this definition:

#define rte_bswap32(x) __builtin_bswap32(x)

> -STATIC const __le32 test_le[2] = { FM10K_CPU_TO_LE32(0x12345678),
> -				   FM10K_CPU_TO_LE32(0x9abcdef0)};
> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +STATIC const __le32 test_le[2] = {0x78563412,0xf0debc9a};
> +#else
> +STATIC const __le32 test_le[2] = {0x12345678,0x9abcdef0};
> +#endif
Chao Zhu Aug. 3, 2015, 11:32 a.m. UTC | #2 
On 2015/8/3 17:06, Thomas Monjalon wrote:
> 2015-08-03 14:31, Chao Zhu:
>> The rte_cpu_to_le_32 function can't be used to define const variables
>> because it has different implementation on big endian platforms. If
>> doing so, it will cause 'initializer element is not constant' compiling
>> error. This patch fixes this problem.
>> --- a/drivers/net/fm10k/base/fm10k_tlv.c
>> +++ b/drivers/net/fm10k/base/fm10k_tlv.c
> You cannot change a base driver file except the osdep header where
> FM10K_CPU_TO_LE32 is defined.
>
> I don't understand why it doesn't give you a constant, given this definition:
>
> #define rte_bswap32(x) ((uint32_t)(__builtin_constant_p(x) ?        \
>                     rte_constant_bswap32(x) :        \
>                     rte_arch_bswap32(x)))
>
> Have you tried CONFIG_RTE_FORCE_INTRINSICS=y ?
> It should trigger this definition:
>
> #define rte_bswap32(x) __builtin_bswap32(x)
Yes.  CONFIG_RTE_FORCE_INTRINSICS=y works on Power Big endian.  But if I 
turn off this, the error happens.  Seems rte_constant_bswap32 doesn't 
work on Power8? I'll try to check it.
>
>> -STATIC const __le32 test_le[2] = { FM10K_CPU_TO_LE32(0x12345678),
>> -				   FM10K_CPU_TO_LE32(0x9abcdef0)};
>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
>> +STATIC const __le32 test_le[2] = {0x78563412,0xf0debc9a};
>> +#else
>> +STATIC const __le32 test_le[2] = {0x12345678,0x9abcdef0};
>> +#endif
Thomas Monjalon Aug. 3, 2015, 11:38 a.m. UTC | #3 
2015-08-03 19:32, Chao Zhu:
> On 2015/8/3 17:06, Thomas Monjalon wrote:
> > 2015-08-03 14:31, Chao Zhu:
> >> The rte_cpu_to_le_32 function can't be used to define const variables
> >> because it has different implementation on big endian platforms. If
> >> doing so, it will cause 'initializer element is not constant' compiling
> >> error. This patch fixes this problem.
> >> --- a/drivers/net/fm10k/base/fm10k_tlv.c
> >> +++ b/drivers/net/fm10k/base/fm10k_tlv.c
> > 
> > You cannot change a base driver file except the osdep header where
> > FM10K_CPU_TO_LE32 is defined.
> >
> > I don't understand why it doesn't give you a constant, given this definition:
> >
> > #define rte_bswap32(x) ((uint32_t)(__builtin_constant_p(x) ?        \
> >                     rte_constant_bswap32(x) :        \
> >                     rte_arch_bswap32(x)))
> >
> > Have you tried CONFIG_RTE_FORCE_INTRINSICS=y ?
> > It should trigger this definition:
> >
> > #define rte_bswap32(x) __builtin_bswap32(x)
> 
> Yes.  CONFIG_RTE_FORCE_INTRINSICS=y works on Power Big endian.  But if I 
> turn off this, the error happens.  Seems rte_constant_bswap32 doesn't 
> work on Power8? I'll try to check it.

The tile arch force intrinsics:
	http://dpdk.org/browse/dpdk/tree/config/defconfig_tile-tilegx-linuxapp-gcc#n39
	http://dpdk.org/browse/dpdk/tree/lib/librte_eal/common/include/arch/tile/rte_byteorder.h#n36
I don't know wether it was for the same reason.
Zhigang Lu Aug. 4, 2015, 12:50 a.m. UTC | #4 
>-----Original Message-----
>From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>Sent: Monday, August 03, 2015 7:39 PM
>To: Chao Zhu
>Cc: dev@dpdk.org; Zhigang Lu
>Subject: Re: [dpdk-dev] [PATCH] fm10k: fix the compilation on big endian
>platforms
>
>2015-08-03 19:32, Chao Zhu:
>> On 2015/8/3 17:06, Thomas Monjalon wrote:
>> > 2015-08-03 14:31, Chao Zhu:
>> >> The rte_cpu_to_le_32 function can't be used to define const
>> >> variables because it has different implementation on big endian
>> >> platforms. If doing so, it will cause 'initializer element is not
>> >> constant' compiling error. This patch fixes this problem.
>> >> --- a/drivers/net/fm10k/base/fm10k_tlv.c
>> >> +++ b/drivers/net/fm10k/base/fm10k_tlv.c
>> >
>> > You cannot change a base driver file except the osdep header where
>> > FM10K_CPU_TO_LE32 is defined.
>> >
>> > I don't understand why it doesn't give you a constant, given this
definition:
>> >
>> > #define rte_bswap32(x) ((uint32_t)(__builtin_constant_p(x) ?        \
>> >                     rte_constant_bswap32(x) :        \
>> >                     rte_arch_bswap32(x)))
>> >
>> > Have you tried CONFIG_RTE_FORCE_INTRINSICS=y ?
>> > It should trigger this definition:
>> >
>> > #define rte_bswap32(x) __builtin_bswap32(x)
>>
>> Yes.  CONFIG_RTE_FORCE_INTRINSICS=y works on Power Big endian.  But if
>> I turn off this, the error happens.  Seems rte_constant_bswap32
>> doesn't work on Power8? I'll try to check it.
>
>The tile arch force intrinsics:
>
http://dpdk.org/browse/dpdk/tree/config/defconfig_tile-tilegx-linuxapp-gcc
>#n39
>
http://dpdk.org/browse/dpdk/tree/lib/librte_eal/common/include/arch/tile
>/rte_byteorder.h#n36
>I don't know wether it was for the same reason.

Not really, the reason Tile arch force intrinsics is to reuse some generic
functions in
lib/librte_eal/common/include/generic/, such as
rte_atomic16_cmpset()
rte_atomic16_inc()
rte_spinlock_lock()
....

Thanks
-Zhigang
diff mbox

Patch

diff --git a/drivers/net/fm10k/base/fm10k_tlv.c b/drivers/net/fm10k/base/fm10k_tlv.c
index 1d9d7d8..5b2937d 100644
--- a/drivers/net/fm10k/base/fm10k_tlv.c
+++ b/drivers/net/fm10k/base/fm10k_tlv.c
@@ -664,8 +664,11 @@  STATIC const s64 test_s64 = -0x123456789abcdef0ll;
 STATIC const s32 test_s32 = -0x1235678;
 STATIC const s16 test_s16 = -0x1234;
 STATIC const s8  test_s8  = -0x12;
-STATIC const __le32 test_le[2] = { FM10K_CPU_TO_LE32(0x12345678),
-				   FM10K_CPU_TO_LE32(0x9abcdef0)};
+#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+STATIC const __le32 test_le[2] = {0x78563412,0xf0debc9a};
+#else
+STATIC const __le32 test_le[2] = {0x12345678,0x9abcdef0};
+#endif
 
 /* The message below is meant to be used as a test message to demonstrate
  * how to use the TLV interface and to test the types.  Normally this code