[dpdk-stable] [PATCH] net/af_xdp: fix 32-bit build for older kernels

[dpdk-stable] [PATCH] net/af_xdp: fix 32-bit build for older kernels

[Ciara Loftus]

'uint64_t' is used to hold pointers in multiple locations in the
copy-mode code (used for kernels before 5.4). For a 32-bit build
this assumption is wrong and results in build errors. This commit
replaces such instances of 'uint64_t' with 'uintptr_t'.

While the copy-mode code will now compile for 32-bit, the PMD is
not expected to work and will fail at initialisation due to some
limitations in the kernel that were subsequently removed in v5.4.
Add a note to the docs to flag this limitation.

Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks")
Cc: stable@dpdk.org

Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
 doc/guides/nics/af_xdp.rst          | 1 +
 drivers/net/af_xdp/rte_eth_af_xdp.c | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/doc/guides/nics/af_xdp.rst b/doc/guides/nics/af_xdp.rst
index 052e59a3ae..5ed24374f8 100644
--- a/doc/guides/nics/af_xdp.rst
+++ b/doc/guides/nics/af_xdp.rst
@@ -50,6 +50,7 @@ This is a Linux-specific PMD, thus the following prerequisites apply:
 *  For PMD zero copy, it requires kernel version later than v5.4-rc1;
 *  For shared_umem, it requires kernel version v5.10 or later and libbpf version
    v0.2.0 or later.
+*  For 32-bit OS, a kernel with version 5.4 or later is required.
 
 Set up an af_xdp interface
 -----------------------------
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 4076ff797c..75ff1c00b2 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -349,7 +349,7 @@ af_xdp_rx_cp(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 	for (i = 0; i < rcvd; i++) {
 		const struct xdp_desc *desc;
-		uint64_t addr;
+		uintptr_t addr;
 		uint32_t len;
 		void *pkt;
 
@@ -402,7 +402,7 @@ pull_umem_cq(struct xsk_umem_info *umem, int size, struct xsk_ring_cons *cq)
 	n = xsk_ring_cons__peek(cq, size, &idx_cq);
 
 	for (i = 0; i < n; i++) {
-		uint64_t addr;
+		uintptr_t addr;
 		addr = *xsk_ring_cons__comp_addr(cq, idx_cq++);
 #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)
 		addr = xsk_umem__extract_addr(addr);
@@ -1005,7 +1005,7 @@ xsk_umem_info *xdp_umem_configure(struct pmd_internals *internals,
 	char ring_name[RTE_RING_NAMESIZE];
 	char mz_name[RTE_MEMZONE_NAMESIZE];
 	int ret;
-	uint64_t i;
+	uintptr_t i;
 
 	umem = rte_zmalloc_socket("umem", sizeof(*umem), 0, rte_socket_id());
 	if (umem == NULL) {
-- 
2.17.1

Re: [dpdk-stable] [PATCH] net/af_xdp: fix 32-bit build for older kernels

[Ferruh Yigit]

On 11/12/2020 4:35 PM, Ciara Loftus wrote:
> 'uint64_t' is used to hold pointers in multiple locations in the
> copy-mode code (used for kernels before 5.4). For a 32-bit build
> this assumption is wrong and results in build errors. This commit
> replaces such instances of 'uint64_t' with 'uintptr_t'.
> 
> While the copy-mode code will now compile for 32-bit, the PMD is
> not expected to work and will fail at initialisation due to some
> limitations in the kernel that were subsequently removed in v5.4.
> Add a note to the docs to flag this limitation.
> 
> Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
> Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
>   doc/guides/nics/af_xdp.rst          | 1 +
>   drivers/net/af_xdp/rte_eth_af_xdp.c | 6 +++---
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/nics/af_xdp.rst b/doc/guides/nics/af_xdp.rst
> index 052e59a3ae..5ed24374f8 100644
> --- a/doc/guides/nics/af_xdp.rst
> +++ b/doc/guides/nics/af_xdp.rst
> @@ -50,6 +50,7 @@ This is a Linux-specific PMD, thus the following prerequisites apply:
>   *  For PMD zero copy, it requires kernel version later than v5.4-rc1;
>   *  For shared_umem, it requires kernel version v5.10 or later and libbpf version
>      v0.2.0 or later.
> +*  For 32-bit OS, a kernel with version 5.4 or later is required.
>   

+1 to doc update

>   Set up an af_xdp interface
>   -----------------------------
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 4076ff797c..75ff1c00b2 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -349,7 +349,7 @@ af_xdp_rx_cp(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   
>   	for (i = 0; i < rcvd; i++) {
>   		const struct xdp_desc *desc;
> -		uint64_t addr;
> +		uintptr_t addr;
>   		uint32_t len;
>   		void *pkt;
>   
> @@ -402,7 +402,7 @@ pull_umem_cq(struct xsk_umem_info *umem, int size, struct xsk_ring_cons *cq)
>   	n = xsk_ring_cons__peek(cq, size, &idx_cq);
>   
>   	for (i = 0; i < n; i++) {
> -		uint64_t addr;
> +		uintptr_t addr;
>   		addr = *xsk_ring_cons__comp_addr(cq, idx_cq++);

Hi Ciara,

As far as I can see the API 'xsk_ring_cons__comp_addr()' returns fixed size 
variable ('__u64'),
and when the PMD is compiled for 32bit, won't it be assigning a 64bit variable 
to the 32bit storage.

I guess libbpf also needs to be adjusted for the 32bit support, what about 
making PMD changes after libbpf changed?

>   #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)
>   		addr = xsk_umem__extract_addr(addr);
> @@ -1005,7 +1005,7 @@ xsk_umem_info *xdp_umem_configure(struct pmd_internals *internals,
>   	char ring_name[RTE_RING_NAMESIZE];
>   	char mz_name[RTE_MEMZONE_NAMESIZE];
>   	int ret;
> -	uint64_t i;
> +	uintptr_t i;
>   

Not sure on this one, 'i' seems not to hold a pointer but index, and result of 
calculation cast to "void *", I assume intention is to prevent calculation 
result to be 64 bit to cover the case "void *" is 4 bytes, for that what do you 
think making variable uint32_t?

Re: [dpdk-stable] [PATCH] net/af_xdp: fix 32-bit build for older kernels

[Loftus, Ciara]

> 
> On 11/12/2020 4:35 PM, Ciara Loftus wrote:
> > 'uint64_t' is used to hold pointers in multiple locations in the
> > copy-mode code (used for kernels before 5.4). For a 32-bit build
> > this assumption is wrong and results in build errors. This commit
> > replaces such instances of 'uint64_t' with 'uintptr_t'.
> >
> > While the copy-mode code will now compile for 32-bit, the PMD is
> > not expected to work and will fail at initialisation due to some
> > limitations in the kernel that were subsequently removed in v5.4.
> > Add a note to the docs to flag this limitation.
> >
> > Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
> > Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > ---
> >   doc/guides/nics/af_xdp.rst          | 1 +
> >   drivers/net/af_xdp/rte_eth_af_xdp.c | 6 +++---
> >   2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/guides/nics/af_xdp.rst b/doc/guides/nics/af_xdp.rst
> > index 052e59a3ae..5ed24374f8 100644
> > --- a/doc/guides/nics/af_xdp.rst
> > +++ b/doc/guides/nics/af_xdp.rst
> > @@ -50,6 +50,7 @@ This is a Linux-specific PMD, thus the following
> prerequisites apply:
> >   *  For PMD zero copy, it requires kernel version later than v5.4-rc1;
> >   *  For shared_umem, it requires kernel version v5.10 or later and libbpf
> version
> >      v0.2.0 or later.
> > +*  For 32-bit OS, a kernel with version 5.4 or later is required.
> >
> 
> +1 to doc update
> 
> >   Set up an af_xdp interface
> >   -----------------------------
> > diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> > index 4076ff797c..75ff1c00b2 100644
> > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> > @@ -349,7 +349,7 @@ af_xdp_rx_cp(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> >
> >   	for (i = 0; i < rcvd; i++) {
> >   		const struct xdp_desc *desc;
> > -		uint64_t addr;
> > +		uintptr_t addr;
> >   		uint32_t len;
> >   		void *pkt;
> >
> > @@ -402,7 +402,7 @@ pull_umem_cq(struct xsk_umem_info *umem, int
> size, struct xsk_ring_cons *cq)
> >   	n = xsk_ring_cons__peek(cq, size, &idx_cq);
> >
> >   	for (i = 0; i < n; i++) {
> > -		uint64_t addr;
> > +		uintptr_t addr;
> >   		addr = *xsk_ring_cons__comp_addr(cq, idx_cq++);
> 
> Hi Ciara,
> 
> As far as I can see the API 'xsk_ring_cons__comp_addr()' returns fixed size
> variable ('__u64'),
> and when the PMD is compiled for 32bit, won't it be assigning a 64bit variable
> to the 32bit storage.

Correct. However we can assume the higher 32bits are zero in this case.
The 'addr' we are consuming via this API will be one which we previously enqueued to the buf_ring and we always cast to (void *) on enqueue.

> 
> I guess libbpf also needs to be adjusted for the 32bit support, what about
> making PMD changes after libbpf changed?

I'm not sure whether this is planned but maybe it makes sense to wait and see rather than relying on assumptions above.

> 
> >   #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)
> >   		addr = xsk_umem__extract_addr(addr);
> > @@ -1005,7 +1005,7 @@ xsk_umem_info *xdp_umem_configure(struct
> pmd_internals *internals,
> >   	char ring_name[RTE_RING_NAMESIZE];
> >   	char mz_name[RTE_MEMZONE_NAMESIZE];
> >   	int ret;
> > -	uint64_t i;
> > +	uintptr_t i;
> >
> 
> Not sure on this one, 'i' seems not to hold a pointer but index, and result of
> calculation cast to "void *", I assume intention is to prevent calculation
> result to be 64 bit to cover the case "void *" is 4 bytes, for that what do you
> think making variable uint32_t?

Do you suggest something like:
#ifdef RTE_ARCH_64
       uint64_t i;
#else
       uint32_t i;
#endif

I can submit a v2 with just the doc update and hold off on the other changes until the necessary changes to libbpf are in place. Let me know what you think.

Thanks,
Ciara

Re: [dpdk-stable] [PATCH] net/af_xdp: fix 32-bit build for older kernels

[Ferruh Yigit]

On 11/16/2020 2:24 PM, Loftus, Ciara wrote:
>>
>> On 11/12/2020 4:35 PM, Ciara Loftus wrote:
>>> 'uint64_t' is used to hold pointers in multiple locations in the
>>> copy-mode code (used for kernels before 5.4). For a 32-bit build
>>> this assumption is wrong and results in build errors. This commit
>>> replaces such instances of 'uint64_t' with 'uintptr_t'.
>>>
>>> While the copy-mode code will now compile for 32-bit, the PMD is
>>> not expected to work and will fail at initialisation due to some
>>> limitations in the kernel that were subsequently removed in v5.4.
>>> Add a note to the docs to flag this limitation.
>>>
>>> Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
>>> Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
>>> ---
>>>    doc/guides/nics/af_xdp.rst          | 1 +
>>>    drivers/net/af_xdp/rte_eth_af_xdp.c | 6 +++---
>>>    2 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/doc/guides/nics/af_xdp.rst b/doc/guides/nics/af_xdp.rst
>>> index 052e59a3ae..5ed24374f8 100644
>>> --- a/doc/guides/nics/af_xdp.rst
>>> +++ b/doc/guides/nics/af_xdp.rst
>>> @@ -50,6 +50,7 @@ This is a Linux-specific PMD, thus the following
>> prerequisites apply:
>>>    *  For PMD zero copy, it requires kernel version later than v5.4-rc1;
>>>    *  For shared_umem, it requires kernel version v5.10 or later and libbpf
>> version
>>>       v0.2.0 or later.
>>> +*  For 32-bit OS, a kernel with version 5.4 or later is required.
>>>
>>
>> +1 to doc update
>>
>>>    Set up an af_xdp interface
>>>    -----------------------------
>>> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> b/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> index 4076ff797c..75ff1c00b2 100644
>>> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> @@ -349,7 +349,7 @@ af_xdp_rx_cp(void *queue, struct rte_mbuf
>> **bufs, uint16_t nb_pkts)
>>>
>>>    	for (i = 0; i < rcvd; i++) {
>>>    		const struct xdp_desc *desc;
>>> -		uint64_t addr;
>>> +		uintptr_t addr;
>>>    		uint32_t len;
>>>    		void *pkt;
>>>
>>> @@ -402,7 +402,7 @@ pull_umem_cq(struct xsk_umem_info *umem, int
>> size, struct xsk_ring_cons *cq)
>>>    	n = xsk_ring_cons__peek(cq, size, &idx_cq);
>>>
>>>    	for (i = 0; i < n; i++) {
>>> -		uint64_t addr;
>>> +		uintptr_t addr;
>>>    		addr = *xsk_ring_cons__comp_addr(cq, idx_cq++);
>>
>> Hi Ciara,
>>
>> As far as I can see the API 'xsk_ring_cons__comp_addr()' returns fixed size
>> variable ('__u64'),
>> and when the PMD is compiled for 32bit, won't it be assigning a 64bit variable
>> to the 32bit storage.
> 
> Correct. However we can assume the higher 32bits are zero in this case.
> The 'addr' we are consuming via this API will be one which we previously enqueued to the buf_ring and we always cast to (void *) on enqueue.
> 

My concern was if compiler complains about this, since from compiler perspective 
there is a 64 -> 32 bit variable assignment which can lead data loss.

>>
>> I guess libbpf also needs to be adjusted for the 32bit support, what about
>> making PMD changes after libbpf changed?
> 
> I'm not sure whether this is planned but maybe it makes sense to wait and see rather than relying on assumptions above.
> 
>>
>>>    #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)
>>>    		addr = xsk_umem__extract_addr(addr);
>>> @@ -1005,7 +1005,7 @@ xsk_umem_info *xdp_umem_configure(struct
>> pmd_internals *internals,
>>>    	char ring_name[RTE_RING_NAMESIZE];
>>>    	char mz_name[RTE_MEMZONE_NAMESIZE];
>>>    	int ret;
>>> -	uint64_t i;
>>> +	uintptr_t i;
>>>
>>
>> Not sure on this one, 'i' seems not to hold a pointer but index, and result of
>> calculation cast to "void *", I assume intention is to prevent calculation
>> result to be 64 bit to cover the case "void *" is 4 bytes, for that what do you
>> think making variable uint32_t?
> 
> Do you suggest something like:
> #ifdef RTE_ARCH_64
>         uint64_t i;
> #else
>         uint32_t i;
> #endif

I was thinking only use 32 bit, since 32 -> 64 bit assignment will not cause any 
problem.

> 
> I can submit a v2 with just the doc update and hold off on the other changes until the necessary changes to libbpf are in place. Let me know what you think.
> 

+1 to this, after 32 bit libbpf is ready, it can be easier to do changes to the PMD.