Re: [PATCH net 1/3] dpaa_eth: reserve space for the xdp_frame under the A050385 erratum

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Tue, Feb 02, 2021 at 07:34:42PM +0200, Camelia Groza wrote:
> When the erratum workaround is triggered, the newly created xdp_frame
> structure is stored at the start of the newly allocated buffer. Avoid
> the structure from being overwritten by explicitly reserving enough
> space in the buffer for storing it.
> 
> Account for the fact that the structure's size might increase in time by
> aligning the headroom to DPAA_FD_DATA_ALIGNMENT bytes, thus guaranteeing
> the data's alignment.
> 
> Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum workaround for XDP")
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 4360ce4d3fb6..e1d041c35ad9 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2182,6 +2182,7 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
>  	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
>  	void *new_buff;
>  	struct page *p;
> +	int headroom;
>  
>  	/* Check the data alignment and make sure the headroom is large
>  	 * enough to store the xdpf backpointer. Use an aligned headroom
> @@ -2197,19 +2198,31 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
>  		return 0;
>  	}
>  
> +	/* The new xdp_frame is stored in the new buffer. Reserve enough space
> +	 * in the headroom for storing it along with the driver's private
> +	 * info. The headroom needs to be aligned to DPAA_FD_DATA_ALIGNMENT to
> +	 * guarantee the data's alignment in the buffer.
> +	 */
> +	headroom = ALIGN(sizeof(*new_xdpf) + priv->tx_headroom,
> +			 DPAA_FD_DATA_ALIGNMENT);
> +
> +	/* Assure the extended headroom and data fit in a one-paged buffer */
> +	if (headroom + xdpf->len > DPAA_BP_RAW_SIZE)

This check might make more sense if you would be accounting for
skb_shared_info as well I suppose, so that you know you'll still provide
enough tailroom for future xdp multibuf support. Didn't all the previous
code path make sure that there's a room for that?

> +		return -ENOMEM;
> +
>  	p = dev_alloc_pages(0);
>  	if (unlikely(!p))
>  		return -ENOMEM;
>  
>  	/* Copy the data to the new buffer at a properly aligned offset */
>  	new_buff = page_address(p);
> -	memcpy(new_buff + priv->tx_headroom, xdpf->data, xdpf->len);
> +	memcpy(new_buff + headroom, xdpf->data, xdpf->len);
>  
>  	/* Create an XDP frame around the new buffer in a similar fashion
>  	 * to xdp_convert_buff_to_frame.
>  	 */
>  	new_xdpf = new_buff;
> -	new_xdpf->data = new_buff + priv->tx_headroom;
> +	new_xdpf->data = new_buff + headroom;
>  	new_xdpf->len = xdpf->len;
>  	new_xdpf->headroom = priv->tx_headroom;
>  	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
> -- 
> 2.17.1
>