[net-next,v4,7/7] dpaa_eth: implement the A050385 erratum workaround for XDP

Commit Message

Camelia Alexandra Groza Nov. 23, 2020, 5:36 p.m. UTC

For XDP TX, even tough we start out with correctly aligned buffers, the
XDP program might change the data's alignment. For REDIRECT, we have no
control over the alignment either.

Create a new workaround for xdp_frame structures to verify the erratum
conditions and move the data to a fresh buffer if necessary. Create a new
xdp_frame for managing the new buffer and free the old one using the XDP
API.

Due to alignment constraints, all frames have a 256 byte headroom that
is offered fully to XDP under the erratum. If the XDP program uses all
of it, the data needs to be move to make room for the xdpf backpointer.

Disable the metadata support since the information can be lost.

Acked-by: Madalin Bucur <[e-mail]>
Signed-off-by: Camelia Groza <[e-mail]>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 82 +++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 3 deletions(-)

Comments

Fijalkowski, Maciej Nov. 24, 2020, 8:50 p.m. UTC | #1

On Mon, Nov 23, 2020 at 07:36:25PM +0200, Camelia Groza wrote:
> For XDP TX, even tough we start out with correctly aligned buffers, the
> XDP program might change the data's alignment. For REDIRECT, we have no
> control over the alignment either.
> 
> Create a new workaround for xdp_frame structures to verify the erratum
> conditions and move the data to a fresh buffer if necessary. Create a new
> xdp_frame for managing the new buffer and free the old one using the XDP
> API.
> 
> Due to alignment constraints, all frames have a 256 byte headroom that
> is offered fully to XDP under the erratum. If the XDP program uses all
> of it, the data needs to be move to make room for the xdpf backpointer.

Out of curiosity, wouldn't it be easier to decrease the headroom that is
given to xdp rather doing to full copy of a buffer in case you miss a few
bytes on headroom?

> 
> Disable the metadata support since the information can be lost.
> 
> Acked-by: Madalin Bucur <[e-mail]>
> Signed-off-by: Camelia Groza <[e-mail]>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 82 +++++++++++++++++++++++++-
>  1 file changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 149b549..d8fc19d 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2170,6 +2170,52 @@ static int dpaa_a050385_wa_skb(struct net_device *net_dev, struct sk_buff **s)
>  
>  	return 0;
>  }
> +
> +static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
> +				struct xdp_frame **init_xdpf)
> +{
> +	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
> +	void *new_buff;
> +	struct page *p;
> +
> +	/* Check the data alignment and make sure the headroom is large
> +	 * enough to store the xdpf backpointer. Use an aligned headroom
> +	 * value.
> +	 *
> +	 * Due to alignment constraints, we give XDP access to the full 256
> +	 * byte frame headroom. If the XDP program uses all of it, copy the
> +	 * data to a new buffer and make room for storing the backpointer.
> +	 */
> +	if (PTR_IS_ALIGNED(xdpf->data, DPAA_A050385_ALIGN) &&
> +	    xdpf->headroom >= priv->tx_headroom) {
> +		xdpf->headroom = priv->tx_headroom;
> +		return 0;
> +	}
> +
> +	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);
> +
> +	/* 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->len = xdpf->len;
> +	new_xdpf->headroom = priv->tx_headroom;

What if ptr was not aligned so you got here but tx_headroom was less than
xdpf->headroom? Shouldn't you choose the bigger one? Aren't you shrinking
the headroom for this case.

> +	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
> +	new_xdpf->mem.type = MEM_TYPE_PAGE_ORDER0;
> +
> +	/* Release the initial buffer */
> +	xdp_return_frame_rx_napi(xdpf);
> +
> +	*init_xdpf = new_xdpf;
> +	return 0;
> +}
>  #endif
>  
>  static netdev_tx_t
> @@ -2406,6 +2452,15 @@ static int dpaa_xdp_xmit_frame(struct net_device *net_dev,
>  	percpu_priv = this_cpu_ptr(priv->percpu_priv);
>  	percpu_stats = &percpu_priv->stats;
>  
> +#ifdef CONFIG_DPAA_ERRATUM_A050385
> +	if (unlikely(fman_has_errata_a050385())) {
> +		if (dpaa_a050385_wa_xdpf(priv, &xdpf)) {
> +			err = -ENOMEM;
> +			goto out_error;
> +		}
> +	}
> +#endif
> +
>  	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
>  		err = -EINVAL;
>  		goto out_error;
> @@ -2479,6 +2534,20 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
>  	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
>  	xdp.rxq = &dpaa_fq->xdp_rxq;
>  
> +	/* We reserve a fixed headroom of 256 bytes under the erratum and we
> +	 * offer it all to XDP programs to use. If no room is left for the
> +	 * xdpf backpointer on TX, we will need to copy the data.
> +	 * Disable metadata support since data realignments might be required
> +	 * and the information can be lost.
> +	 */
> +#ifdef CONFIG_DPAA_ERRATUM_A050385
> +	if (unlikely(fman_has_errata_a050385())) {
> +		xdp_set_data_meta_invalid(&xdp);
> +		xdp.data_hard_start = vaddr;
> +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> +	}
> +#endif
> +
>  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
>  
>  	/* Update the length and the offset of the FD */
> @@ -2486,7 +2555,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
>  
>  	switch (xdp_act) {
>  	case XDP_PASS:
> -		*xdp_meta_len = xdp.data - xdp.data_meta;
> +		*xdp_meta_len = xdp_data_meta_unsupported(&xdp) ? 0 :
> +				xdp.data - xdp.data_meta;

You could consider surrounding this with ifdef and keep the old version in
the else branch so that old case is not hurt with that additional branch
that you're introducing with that ternary operator.

>  		break;
>  	case XDP_TX:
>  		/* We can access the full headroom when sending the frame
> @@ -3188,10 +3258,16 @@ static u16 dpaa_get_headroom(struct dpaa_buffer_layout *bl,
>  	 */
>  	headroom = (u16)(bl[port].priv_data_size + DPAA_HWA_SIZE);
>  
> -	if (port == RX)
> +	if (port == RX) {
> +#ifdef CONFIG_DPAA_ERRATUM_A050385
> +		if (unlikely(fman_has_errata_a050385()))
> +			headroom = XDP_PACKET_HEADROOM;
> +#endif
> +
>  		return ALIGN(headroom, DPAA_FD_RX_DATA_ALIGNMENT);
> -	else
> +	} else {
>  		return ALIGN(headroom, DPAA_FD_DATA_ALIGNMENT);
> +	}
>  }
>  
>  static int dpaa_eth_probe(struct platform_device *pdev)
> -- 
> 1.9.1
>

Camelia Alexandra Groza Nov. 25, 2020, 3:52 p.m. UTC | #2

> -----Original Message-----
> From: Maciej Fijalkowski <[e-mail]>
> Sent: Tuesday, November 24, 2020 22:51
> To: Camelia Alexandra Groza <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Madalin Bucur (OSS)
> <[e-mail]>; Ioana Ciornei <[e-mail]>;
> [e-mail]
> Subject: Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385
> erratum workaround for XDP
> 
> On Mon, Nov 23, 2020 at 07:36:25PM +0200, Camelia Groza wrote:
> > For XDP TX, even tough we start out with correctly aligned buffers, the
> > XDP program might change the data's alignment. For REDIRECT, we have no
> > control over the alignment either.
> >
> > Create a new workaround for xdp_frame structures to verify the erratum
> > conditions and move the data to a fresh buffer if necessary. Create a new
> > xdp_frame for managing the new buffer and free the old one using the
> XDP
> > API.
> >
> > Due to alignment constraints, all frames have a 256 byte headroom that
> > is offered fully to XDP under the erratum. If the XDP program uses all
> > of it, the data needs to be move to make room for the xdpf backpointer.
> 
> Out of curiosity, wouldn't it be easier to decrease the headroom that is
> given to xdp rather doing to full copy of a buffer in case you miss a few
> bytes on headroom?

First of all, I'm not sure if offering less than XDP_PACKET_HEADROOM to XDP programs is allowed. Second, we require the data to be strictly aligned under this erratum. This first condition would be broken even if we reduce the size of the offered headroom.

> >
> > Disable the metadata support since the information can be lost.
> >
> > Acked-by: Madalin Bucur <[e-mail]>
> > Signed-off-by: Camelia Groza <[e-mail]>
> > ---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 82
> +++++++++++++++++++++++++-
> >  1 file changed, 79 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index 149b549..d8fc19d 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -2170,6 +2170,52 @@ static int dpaa_a050385_wa_skb(struct
> net_device *net_dev, struct sk_buff **s)
> >
> >  	return 0;
> >  }
> > +
> > +static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
> > +				struct xdp_frame **init_xdpf)
> > +{
> > +	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
> > +	void *new_buff;
> > +	struct page *p;
> > +
> > +	/* Check the data alignment and make sure the headroom is large
> > +	 * enough to store the xdpf backpointer. Use an aligned headroom
> > +	 * value.
> > +	 *
> > +	 * Due to alignment constraints, we give XDP access to the full 256
> > +	 * byte frame headroom. If the XDP program uses all of it, copy the
> > +	 * data to a new buffer and make room for storing the backpointer.
> > +	 */
> > +	if (PTR_IS_ALIGNED(xdpf->data, DPAA_A050385_ALIGN) &&
> > +	    xdpf->headroom >= priv->tx_headroom) {
> > +		xdpf->headroom = priv->tx_headroom;
> > +		return 0;
> > +	}
> > +
> > +	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);
> > +
> > +	/* 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->len = xdpf->len;
> > +	new_xdpf->headroom = priv->tx_headroom;
> 
> What if ptr was not aligned so you got here but tx_headroom was less than
> xdpf->headroom? Shouldn't you choose the bigger one? Aren't you shrinking
> the headroom for this case.

Yes, I am shrinking the headroom. At this point, the headroom's content isn't relevant anymore (this path is executed when transmitting the frame after TX or REDIRECT). What is important is the data's alignment, and it is dictated by the headroom's (fd's offset) size.

> > +	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
> > +	new_xdpf->mem.type = MEM_TYPE_PAGE_ORDER0;
> > +
> > +	/* Release the initial buffer */
> > +	xdp_return_frame_rx_napi(xdpf);
> > +
> > +	*init_xdpf = new_xdpf;
> > +	return 0;
> > +}
> >  #endif
> >
> >  static netdev_tx_t
> > @@ -2406,6 +2452,15 @@ static int dpaa_xdp_xmit_frame(struct
> net_device *net_dev,
> >  	percpu_priv = this_cpu_ptr(priv->percpu_priv);
> >  	percpu_stats = &percpu_priv->stats;
> >
> > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > +	if (unlikely(fman_has_errata_a050385())) {
> > +		if (dpaa_a050385_wa_xdpf(priv, &xdpf)) {
> > +			err = -ENOMEM;
> > +			goto out_error;
> > +		}
> > +	}
> > +#endif
> > +
> >  	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
> >  		err = -EINVAL;
> >  		goto out_error;
> > @@ -2479,6 +2534,20 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv,
> struct qm_fd *fd, void *vaddr,
> >  	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
> >  	xdp.rxq = &dpaa_fq->xdp_rxq;
> >
> > +	/* We reserve a fixed headroom of 256 bytes under the erratum and
> we
> > +	 * offer it all to XDP programs to use. If no room is left for the
> > +	 * xdpf backpointer on TX, we will need to copy the data.
> > +	 * Disable metadata support since data realignments might be
> required
> > +	 * and the information can be lost.
> > +	 */
> > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > +	if (unlikely(fman_has_errata_a050385())) {
> > +		xdp_set_data_meta_invalid(&xdp);
> > +		xdp.data_hard_start = vaddr;
> > +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > +	}
> > +#endif
> > +
> >  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> >
> >  	/* Update the length and the offset of the FD */
> > @@ -2486,7 +2555,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv,
> struct qm_fd *fd, void *vaddr,
> >
> >  	switch (xdp_act) {
> >  	case XDP_PASS:
> > -		*xdp_meta_len = xdp.data - xdp.data_meta;
> > +		*xdp_meta_len = xdp_data_meta_unsupported(&xdp) ? 0 :
> > +				xdp.data - xdp.data_meta;
> 
> You could consider surrounding this with ifdef and keep the old version in
> the else branch so that old case is not hurt with that additional branch
> that you're introducing with that ternary operator.

Sure, I'll do that. Thanks.

> >  		break;
> >  	case XDP_TX:
> >  		/* We can access the full headroom when sending the frame
> > @@ -3188,10 +3258,16 @@ static u16 dpaa_get_headroom(struct
> dpaa_buffer_layout *bl,
> >  	 */
> >  	headroom = (u16)(bl[port].priv_data_size + DPAA_HWA_SIZE);
> >
> > -	if (port == RX)
> > +	if (port == RX) {
> > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > +		if (unlikely(fman_has_errata_a050385()))
> > +			headroom = XDP_PACKET_HEADROOM;
> > +#endif
> > +
> >  		return ALIGN(headroom,
> DPAA_FD_RX_DATA_ALIGNMENT);
> > -	else
> > +	} else {
> >  		return ALIGN(headroom, DPAA_FD_DATA_ALIGNMENT);
> > +	}
> >  }
> >
> >  static int dpaa_eth_probe(struct platform_device *pdev)
> > --
> > 1.9.1
> >

Fijalkowski, Maciej Nov. 27, 2020, 2:44 p.m. UTC | #3

On Wed, Nov 25, 2020 at 03:52:33PM +0000, Camelia Alexandra Groza wrote:
> > -----Original Message-----
> > From: Maciej Fijalkowski <[e-mail]>
> > Sent: Tuesday, November 24, 2020 22:51
> > To: Camelia Alexandra Groza <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Madalin Bucur (OSS)
> > <[e-mail]>; Ioana Ciornei <[e-mail]>;
> > [e-mail]
> > Subject: Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385
> > erratum workaround for XDP
> > 
> > On Mon, Nov 23, 2020 at 07:36:25PM +0200, Camelia Groza wrote:
> > > For XDP TX, even tough we start out with correctly aligned buffers, the
> > > XDP program might change the data's alignment. For REDIRECT, we have no
> > > control over the alignment either.
> > >
> > > Create a new workaround for xdp_frame structures to verify the erratum
> > > conditions and move the data to a fresh buffer if necessary. Create a new
> > > xdp_frame for managing the new buffer and free the old one using the
> > XDP
> > > API.
> > >
> > > Due to alignment constraints, all frames have a 256 byte headroom that
> > > is offered fully to XDP under the erratum. If the XDP program uses all
> > > of it, the data needs to be move to make room for the xdpf backpointer.
> > 
> > Out of curiosity, wouldn't it be easier to decrease the headroom that is
> > given to xdp rather doing to full copy of a buffer in case you miss a few
> > bytes on headroom?
> 
> First of all, I'm not sure if offering less than XDP_PACKET_HEADROOM to XDP programs is allowed. Second, we require the data to be strictly aligned under this erratum. This first condition would be broken even if we reduce the size of the offered headroom.
> 
> > >
> > > Disable the metadata support since the information can be lost.
> > >
> > > Acked-by: Madalin Bucur <[e-mail]>
> > > Signed-off-by: Camelia Groza <[e-mail]>
> > > ---
> > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 82
> > +++++++++++++++++++++++++-
> > >  1 file changed, 79 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > index 149b549..d8fc19d 100644
> > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > @@ -2170,6 +2170,52 @@ static int dpaa_a050385_wa_skb(struct
> > net_device *net_dev, struct sk_buff **s)
> > >
> > >  	return 0;
> > >  }
> > > +
> > > +static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
> > > +				struct xdp_frame **init_xdpf)
> > > +{
> > > +	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
> > > +	void *new_buff;
> > > +	struct page *p;
> > > +
> > > +	/* Check the data alignment and make sure the headroom is large
> > > +	 * enough to store the xdpf backpointer. Use an aligned headroom
> > > +	 * value.
> > > +	 *
> > > +	 * Due to alignment constraints, we give XDP access to the full 256
> > > +	 * byte frame headroom. If the XDP program uses all of it, copy the
> > > +	 * data to a new buffer and make room for storing the backpointer.
> > > +	 */
> > > +	if (PTR_IS_ALIGNED(xdpf->data, DPAA_A050385_ALIGN) &&
> > > +	    xdpf->headroom >= priv->tx_headroom) {
> > > +		xdpf->headroom = priv->tx_headroom;
> > > +		return 0;
> > > +	}
> > > +
> > > +	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);
> > > +
> > > +	/* 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->len = xdpf->len;
> > > +	new_xdpf->headroom = priv->tx_headroom;
> > 
> > What if ptr was not aligned so you got here but tx_headroom was less than
> > xdpf->headroom? Shouldn't you choose the bigger one? Aren't you shrinking
> > the headroom for this case.
> 
> Yes, I am shrinking the headroom. At this point, the headroom's content isn't relevant anymore (this path is executed when transmitting the frame after TX or REDIRECT). What is important is the data's alignment, and it is dictated by the headroom's (fd's offset) size.

So would it be possible to do a memmove within the existing buffer under
some circumstances and then have this current logic as a worst case
scenario? Majority of XDP progs won't consume all of the XDP headroom so I
think it's something worth pursuing.

Please also tell us the performance impact of that workaround. Grabbing
new page followed by memcpy is expensive.

> 
> > > +	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
> > > +	new_xdpf->mem.type = MEM_TYPE_PAGE_ORDER0;
> > > +
> > > +	/* Release the initial buffer */
> > > +	xdp_return_frame_rx_napi(xdpf);
> > > +
> > > +	*init_xdpf = new_xdpf;
> > > +	return 0;
> > > +}
> > >  #endif
> > >
> > >  static netdev_tx_t
> > > @@ -2406,6 +2452,15 @@ static int dpaa_xdp_xmit_frame(struct
> > net_device *net_dev,
> > >  	percpu_priv = this_cpu_ptr(priv->percpu_priv);
> > >  	percpu_stats = &percpu_priv->stats;
> > >
> > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > +	if (unlikely(fman_has_errata_a050385())) {
> > > +		if (dpaa_a050385_wa_xdpf(priv, &xdpf)) {
> > > +			err = -ENOMEM;
> > > +			goto out_error;
> > > +		}
> > > +	}
> > > +#endif
> > > +
> > >  	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
> > >  		err = -EINVAL;
> > >  		goto out_error;
> > > @@ -2479,6 +2534,20 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv,
> > struct qm_fd *fd, void *vaddr,
> > >  	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
> > >  	xdp.rxq = &dpaa_fq->xdp_rxq;
> > >
> > > +	/* We reserve a fixed headroom of 256 bytes under the erratum and
> > we
> > > +	 * offer it all to XDP programs to use. If no room is left for the
> > > +	 * xdpf backpointer on TX, we will need to copy the data.
> > > +	 * Disable metadata support since data realignments might be
> > required
> > > +	 * and the information can be lost.
> > > +	 */
> > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > +	if (unlikely(fman_has_errata_a050385())) {
> > > +		xdp_set_data_meta_invalid(&xdp);
> > > +		xdp.data_hard_start = vaddr;
> > > +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > > +	}
> > > +#endif
> > > +
> > >  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > >
> > >  	/* Update the length and the offset of the FD */
> > > @@ -2486,7 +2555,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv,
> > struct qm_fd *fd, void *vaddr,
> > >
> > >  	switch (xdp_act) {
> > >  	case XDP_PASS:
> > > -		*xdp_meta_len = xdp.data - xdp.data_meta;
> > > +		*xdp_meta_len = xdp_data_meta_unsupported(&xdp) ? 0 :
> > > +				xdp.data - xdp.data_meta;
> > 
> > You could consider surrounding this with ifdef and keep the old version in
> > the else branch so that old case is not hurt with that additional branch
> > that you're introducing with that ternary operator.
> 
> Sure, I'll do that. Thanks.
> 
> > >  		break;
> > >  	case XDP_TX:
> > >  		/* We can access the full headroom when sending the frame
> > > @@ -3188,10 +3258,16 @@ static u16 dpaa_get_headroom(struct
> > dpaa_buffer_layout *bl,
> > >  	 */
> > >  	headroom = (u16)(bl[port].priv_data_size + DPAA_HWA_SIZE);
> > >
> > > -	if (port == RX)
> > > +	if (port == RX) {
> > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > +		if (unlikely(fman_has_errata_a050385()))
> > > +			headroom = XDP_PACKET_HEADROOM;
> > > +#endif
> > > +
> > >  		return ALIGN(headroom,
> > DPAA_FD_RX_DATA_ALIGNMENT);
> > > -	else
> > > +	} else {
> > >  		return ALIGN(headroom, DPAA_FD_DATA_ALIGNMENT);
> > > +	}
> > >  }
> > >
> > >  static int dpaa_eth_probe(struct platform_device *pdev)
> > > --
> > > 1.9.1
> > >

Camelia Alexandra Groza Nov. 27, 2020, 5:35 p.m. UTC | #4

> -----Original Message-----
> From: Maciej Fijalkowski <[e-mail]>
> Sent: Friday, November 27, 2020 16:44
> To: Camelia Alexandra Groza <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Madalin Bucur (OSS)
> <[e-mail]>; Ioana Ciornei <[e-mail]>;
> [e-mail]
> Subject: Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385
> erratum workaround for XDP
> 
> On Wed, Nov 25, 2020 at 03:52:33PM +0000, Camelia Alexandra Groza wrote:
> > > -----Original Message-----
> > > From: Maciej Fijalkowski <[e-mail]>
> > > Sent: Tuesday, November 24, 2020 22:51
> > > To: Camelia Alexandra Groza <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; Madalin Bucur (OSS)
> > > <[e-mail]>; Ioana Ciornei <[e-mail]>;
> > > [e-mail]
> > > Subject: Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385
> > > erratum workaround for XDP
> > >
> > > On Mon, Nov 23, 2020 at 07:36:25PM +0200, Camelia Groza wrote:
> > > > For XDP TX, even tough we start out with correctly aligned buffers, the
> > > > XDP program might change the data's alignment. For REDIRECT, we
> have no
> > > > control over the alignment either.
> > > >
> > > > Create a new workaround for xdp_frame structures to verify the
> erratum
> > > > conditions and move the data to a fresh buffer if necessary. Create a
> new
> > > > xdp_frame for managing the new buffer and free the old one using the
> > > XDP
> > > > API.
> > > >
> > > > Due to alignment constraints, all frames have a 256 byte headroom that
> > > > is offered fully to XDP under the erratum. If the XDP program uses all
> > > > of it, the data needs to be move to make room for the xdpf
> backpointer.
> > >
> > > Out of curiosity, wouldn't it be easier to decrease the headroom that is
> > > given to xdp rather doing to full copy of a buffer in case you miss a few
> > > bytes on headroom?
> >
> > First of all, I'm not sure if offering less than XDP_PACKET_HEADROOM to
> XDP programs is allowed. Second, we require the data to be strictly aligned
> under this erratum. This first condition would be broken even if we reduce
> the size of the offered headroom.
> >
> > > >
> > > > Disable the metadata support since the information can be lost.
> > > >
> > > > Acked-by: Madalin Bucur <[e-mail]>
> > > > Signed-off-by: Camelia Groza <[e-mail]>
> > > > ---
> > > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 82
> > > +++++++++++++++++++++++++-
> > > >  1 file changed, 79 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > index 149b549..d8fc19d 100644
> > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > @@ -2170,6 +2170,52 @@ static int dpaa_a050385_wa_skb(struct
> > > net_device *net_dev, struct sk_buff **s)
> > > >
> > > >  	return 0;
> > > >  }
> > > > +
> > > > +static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
> > > > +				struct xdp_frame **init_xdpf)
> > > > +{
> > > > +	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
> > > > +	void *new_buff;
> > > > +	struct page *p;
> > > > +
> > > > +	/* Check the data alignment and make sure the headroom is large
> > > > +	 * enough to store the xdpf backpointer. Use an aligned headroom
> > > > +	 * value.
> > > > +	 *
> > > > +	 * Due to alignment constraints, we give XDP access to the full 256
> > > > +	 * byte frame headroom. If the XDP program uses all of it, copy the
> > > > +	 * data to a new buffer and make room for storing the backpointer.
> > > > +	 */
> > > > +	if (PTR_IS_ALIGNED(xdpf->data, DPAA_A050385_ALIGN) &&
> > > > +	    xdpf->headroom >= priv->tx_headroom) {
> > > > +		xdpf->headroom = priv->tx_headroom;
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	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);
> > > > +
> > > > +	/* 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->len = xdpf->len;
> > > > +	new_xdpf->headroom = priv->tx_headroom;
> > >
> > > What if ptr was not aligned so you got here but tx_headroom was less
> than
> > > xdpf->headroom? Shouldn't you choose the bigger one? Aren't you
> shrinking
> > > the headroom for this case.
> >
> > Yes, I am shrinking the headroom. At this point, the headroom's content
> isn't relevant anymore (this path is executed when transmitting the frame
> after TX or REDIRECT). What is important is the data's alignment, and it is
> dictated by the headroom's (fd's offset) size.
> 
> So would it be possible to do a memmove within the existing buffer under
> some circumstances and then have this current logic as a worst case
> scenario? Majority of XDP progs won't consume all of the XDP headroom so I
> think it's something worth pursuing.
> 
> Please also tell us the performance impact of that workaround. Grabbing
> new page followed by memcpy is expensive.

Yes, using memmove() might be an optimization if enough headroom is available to shift the data. Thanks for the suggestion. I can send this in separately as an optimization if you don't think is a blocker and if there aren't any other comments on v5.

I don't have numbers to share at the moment for the performance impact.

> >
> > > > +	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
> > > > +	new_xdpf->mem.type = MEM_TYPE_PAGE_ORDER0;
> > > > +
> > > > +	/* Release the initial buffer */
> > > > +	xdp_return_frame_rx_napi(xdpf);
> > > > +
> > > > +	*init_xdpf = new_xdpf;
> > > > +	return 0;
> > > > +}
> > > >  #endif
> > > >
> > > >  static netdev_tx_t
> > > > @@ -2406,6 +2452,15 @@ static int dpaa_xdp_xmit_frame(struct
> > > net_device *net_dev,
> > > >  	percpu_priv = this_cpu_ptr(priv->percpu_priv);
> > > >  	percpu_stats = &percpu_priv->stats;
> > > >
> > > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > > +	if (unlikely(fman_has_errata_a050385())) {
> > > > +		if (dpaa_a050385_wa_xdpf(priv, &xdpf)) {
> > > > +			err = -ENOMEM;
> > > > +			goto out_error;
> > > > +		}
> > > > +	}
> > > > +#endif
> > > > +
> > > >  	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
> > > >  		err = -EINVAL;
> > > >  		goto out_error;
> > > > @@ -2479,6 +2534,20 @@ static u32 dpaa_run_xdp(struct dpaa_priv
> *priv,
> > > struct qm_fd *fd, void *vaddr,
> > > >  	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
> > > >  	xdp.rxq = &dpaa_fq->xdp_rxq;
> > > >
> > > > +	/* We reserve a fixed headroom of 256 bytes under the erratum and
> > > we
> > > > +	 * offer it all to XDP programs to use. If no room is left for the
> > > > +	 * xdpf backpointer on TX, we will need to copy the data.
> > > > +	 * Disable metadata support since data realignments might be
> > > required
> > > > +	 * and the information can be lost.
> > > > +	 */
> > > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > > +	if (unlikely(fman_has_errata_a050385())) {
> > > > +		xdp_set_data_meta_invalid(&xdp);
> > > > +		xdp.data_hard_start = vaddr;
> > > > +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > > > +	}
> > > > +#endif
> > > > +
> > > >  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > > >
> > > >  	/* Update the length and the offset of the FD */
> > > > @@ -2486,7 +2555,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv
> *priv,
> > > struct qm_fd *fd, void *vaddr,
> > > >
> > > >  	switch (xdp_act) {
> > > >  	case XDP_PASS:
> > > > -		*xdp_meta_len = xdp.data - xdp.data_meta;
> > > > +		*xdp_meta_len = xdp_data_meta_unsupported(&xdp) ? 0 :
> > > > +				xdp.data - xdp.data_meta;
> > >
> > > You could consider surrounding this with ifdef and keep the old version in
> > > the else branch so that old case is not hurt with that additional branch
> > > that you're introducing with that ternary operator.
> >
> > Sure, I'll do that. Thanks.
> >
> > > >  		break;
> > > >  	case XDP_TX:
> > > >  		/* We can access the full headroom when sending the frame
> > > > @@ -3188,10 +3258,16 @@ static u16 dpaa_get_headroom(struct
> > > dpaa_buffer_layout *bl,
> > > >  	 */
> > > >  	headroom = (u16)(bl[port].priv_data_size + DPAA_HWA_SIZE);
> > > >
> > > > -	if (port == RX)
> > > > +	if (port == RX) {
> > > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > > +		if (unlikely(fman_has_errata_a050385()))
> > > > +			headroom = XDP_PACKET_HEADROOM;
> > > > +#endif
> > > > +
> > > >  		return ALIGN(headroom,
> > > DPAA_FD_RX_DATA_ALIGNMENT);
> > > > -	else
> > > > +	} else {
> > > >  		return ALIGN(headroom, DPAA_FD_DATA_ALIGNMENT);
> > > > +	}
> > > >  }
> > > >
> > > >  static int dpaa_eth_probe(struct platform_device *pdev)
> > > > --
> > > > 1.9.1
> > > >

Fijalkowski, Maciej Nov. 28, 2020, 11:02 p.m. UTC | #5

On Fri, Nov 27, 2020 at 05:35:00PM +0000, Camelia Alexandra Groza wrote:
> > -----Original Message-----
> > From: Maciej Fijalkowski <[e-mail]>
> > Sent: Friday, November 27, 2020 16:44
> > To: Camelia Alexandra Groza <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Madalin Bucur (OSS)
> > <[e-mail]>; Ioana Ciornei <[e-mail]>;
> > [e-mail]
> > Subject: Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385
> > erratum workaround for XDP
> > 
> > On Wed, Nov 25, 2020 at 03:52:33PM +0000, Camelia Alexandra Groza wrote:
> > > > -----Original Message-----
> > > > From: Maciej Fijalkowski <[e-mail]>
> > > > Sent: Tuesday, November 24, 2020 22:51
> > > > To: Camelia Alexandra Groza <[e-mail]>
> > > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; Madalin Bucur (OSS)
> > > > <[e-mail]>; Ioana Ciornei <[e-mail]>;
> > > > [e-mail]
> > > > Subject: Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385
> > > > erratum workaround for XDP
> > > >
> > > > On Mon, Nov 23, 2020 at 07:36:25PM +0200, Camelia Groza wrote:
> > > > > For XDP TX, even tough we start out with correctly aligned buffers, the
> > > > > XDP program might change the data's alignment. For REDIRECT, we
> > have no
> > > > > control over the alignment either.
> > > > >
> > > > > Create a new workaround for xdp_frame structures to verify the
> > erratum
> > > > > conditions and move the data to a fresh buffer if necessary. Create a
> > new
> > > > > xdp_frame for managing the new buffer and free the old one using the
> > > > XDP
> > > > > API.
> > > > >
> > > > > Due to alignment constraints, all frames have a 256 byte headroom that
> > > > > is offered fully to XDP under the erratum. If the XDP program uses all
> > > > > of it, the data needs to be move to make room for the xdpf
> > backpointer.
> > > >
> > > > Out of curiosity, wouldn't it be easier to decrease the headroom that is
> > > > given to xdp rather doing to full copy of a buffer in case you miss a few
> > > > bytes on headroom?
> > >
> > > First of all, I'm not sure if offering less than XDP_PACKET_HEADROOM to
> > XDP programs is allowed. Second, we require the data to be strictly aligned
> > under this erratum. This first condition would be broken even if we reduce
> > the size of the offered headroom.
> > >
> > > > >
> > > > > Disable the metadata support since the information can be lost.
> > > > >
> > > > > Acked-by: Madalin Bucur <[e-mail]>
> > > > > Signed-off-by: Camelia Groza <[e-mail]>
> > > > > ---
> > > > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 82
> > > > +++++++++++++++++++++++++-
> > > > >  1 file changed, 79 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > index 149b549..d8fc19d 100644
> > > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > @@ -2170,6 +2170,52 @@ static int dpaa_a050385_wa_skb(struct
> > > > net_device *net_dev, struct sk_buff **s)
> > > > >
> > > > >  	return 0;
> > > > >  }
> > > > > +
> > > > > +static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
> > > > > +				struct xdp_frame **init_xdpf)
> > > > > +{
> > > > > +	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
> > > > > +	void *new_buff;
> > > > > +	struct page *p;
> > > > > +
> > > > > +	/* Check the data alignment and make sure the headroom is large
> > > > > +	 * enough to store the xdpf backpointer. Use an aligned headroom
> > > > > +	 * value.
> > > > > +	 *
> > > > > +	 * Due to alignment constraints, we give XDP access to the full 256
> > > > > +	 * byte frame headroom. If the XDP program uses all of it, copy the
> > > > > +	 * data to a new buffer and make room for storing the backpointer.
> > > > > +	 */
> > > > > +	if (PTR_IS_ALIGNED(xdpf->data, DPAA_A050385_ALIGN) &&
> > > > > +	    xdpf->headroom >= priv->tx_headroom) {
> > > > > +		xdpf->headroom = priv->tx_headroom;
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	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);
> > > > > +
> > > > > +	/* 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->len = xdpf->len;
> > > > > +	new_xdpf->headroom = priv->tx_headroom;
> > > >
> > > > What if ptr was not aligned so you got here but tx_headroom was less
> > than
> > > > xdpf->headroom? Shouldn't you choose the bigger one? Aren't you
> > shrinking
> > > > the headroom for this case.
> > >
> > > Yes, I am shrinking the headroom. At this point, the headroom's content
> > isn't relevant anymore (this path is executed when transmitting the frame
> > after TX or REDIRECT). What is important is the data's alignment, and it is
> > dictated by the headroom's (fd's offset) size.
> > 
> > So would it be possible to do a memmove within the existing buffer under
> > some circumstances and then have this current logic as a worst case
> > scenario? Majority of XDP progs won't consume all of the XDP headroom so I
> > think it's something worth pursuing.
> > 
> > Please also tell us the performance impact of that workaround. Grabbing
> > new page followed by memcpy is expensive.
> 
> Yes, using memmove() might be an optimization if enough headroom is available to shift the data. Thanks for the suggestion. I can send this in separately as an optimization if you don't think is a blocker and if there aren't any other comments on v5.

Fine, but please remember to provide the numbers that I asked for.

> 
> I don't have numbers to share at the moment for the performance impact.
> 
> > >
> > > > > +	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
> > > > > +	new_xdpf->mem.type = MEM_TYPE_PAGE_ORDER0;
> > > > > +
> > > > > +	/* Release the initial buffer */
> > > > > +	xdp_return_frame_rx_napi(xdpf);
> > > > > +
> > > > > +	*init_xdpf = new_xdpf;
> > > > > +	return 0;
> > > > > +}
> > > > >  #endif
> > > > >
> > > > >  static netdev_tx_t
> > > > > @@ -2406,6 +2452,15 @@ static int dpaa_xdp_xmit_frame(struct
> > > > net_device *net_dev,
> > > > >  	percpu_priv = this_cpu_ptr(priv->percpu_priv);
> > > > >  	percpu_stats = &percpu_priv->stats;
> > > > >
> > > > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > > > +	if (unlikely(fman_has_errata_a050385())) {
> > > > > +		if (dpaa_a050385_wa_xdpf(priv, &xdpf)) {
> > > > > +			err = -ENOMEM;
> > > > > +			goto out_error;
> > > > > +		}
> > > > > +	}
> > > > > +#endif
> > > > > +
> > > > >  	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
> > > > >  		err = -EINVAL;
> > > > >  		goto out_error;
> > > > > @@ -2479,6 +2534,20 @@ static u32 dpaa_run_xdp(struct dpaa_priv
> > *priv,
> > > > struct qm_fd *fd, void *vaddr,
> > > > >  	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
> > > > >  	xdp.rxq = &dpaa_fq->xdp_rxq;
> > > > >
> > > > > +	/* We reserve a fixed headroom of 256 bytes under the erratum and
> > > > we
> > > > > +	 * offer it all to XDP programs to use. If no room is left for the
> > > > > +	 * xdpf backpointer on TX, we will need to copy the data.
> > > > > +	 * Disable metadata support since data realignments might be
> > > > required
> > > > > +	 * and the information can be lost.
> > > > > +	 */
> > > > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > > > +	if (unlikely(fman_has_errata_a050385())) {
> > > > > +		xdp_set_data_meta_invalid(&xdp);
> > > > > +		xdp.data_hard_start = vaddr;
> > > > > +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > > > > +	}
> > > > > +#endif
> > > > > +
> > > > >  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > > > >
> > > > >  	/* Update the length and the offset of the FD */
> > > > > @@ -2486,7 +2555,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv
> > *priv,
> > > > struct qm_fd *fd, void *vaddr,
> > > > >
> > > > >  	switch (xdp_act) {
> > > > >  	case XDP_PASS:
> > > > > -		*xdp_meta_len = xdp.data - xdp.data_meta;
> > > > > +		*xdp_meta_len = xdp_data_meta_unsupported(&xdp) ? 0 :
> > > > > +				xdp.data - xdp.data_meta;
> > > >
> > > > You could consider surrounding this with ifdef and keep the old version in
> > > > the else branch so that old case is not hurt with that additional branch
> > > > that you're introducing with that ternary operator.
> > >
> > > Sure, I'll do that. Thanks.
> > >
> > > > >  		break;
> > > > >  	case XDP_TX:
> > > > >  		/* We can access the full headroom when sending the frame
> > > > > @@ -3188,10 +3258,16 @@ static u16 dpaa_get_headroom(struct
> > > > dpaa_buffer_layout *bl,
> > > > >  	 */
> > > > >  	headroom = (u16)(bl[port].priv_data_size + DPAA_HWA_SIZE);
> > > > >
> > > > > -	if (port == RX)
> > > > > +	if (port == RX) {
> > > > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > > > +		if (unlikely(fman_has_errata_a050385()))
> > > > > +			headroom = XDP_PACKET_HEADROOM;
> > > > > +#endif
> > > > > +
> > > > >  		return ALIGN(headroom,
> > > > DPAA_FD_RX_DATA_ALIGNMENT);
> > > > > -	else
> > > > > +	} else {
> > > > >  		return ALIGN(headroom, DPAA_FD_DATA_ALIGNMENT);
> > > > > +	}
> > > > >  }
> > > > >
> > > > >  static int dpaa_eth_probe(struct platform_device *pdev)
> > > > > --
> > > > > 1.9.1
> > > > >

Patch

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 149b549..d8fc19d 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2170,6 +2170,52 @@  static int dpaa_a050385_wa_skb(struct net_device *net_dev, struct sk_buff **s)
 
 	return 0;
 }
+
+static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
+				struct xdp_frame **init_xdpf)
+{
+	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
+	void *new_buff;
+	struct page *p;
+
+	/* Check the data alignment and make sure the headroom is large
+	 * enough to store the xdpf backpointer. Use an aligned headroom
+	 * value.
+	 *
+	 * Due to alignment constraints, we give XDP access to the full 256
+	 * byte frame headroom. If the XDP program uses all of it, copy the
+	 * data to a new buffer and make room for storing the backpointer.
+	 */
+	if (PTR_IS_ALIGNED(xdpf->data, DPAA_A050385_ALIGN) &&
+	    xdpf->headroom >= priv->tx_headroom) {
+		xdpf->headroom = priv->tx_headroom;
+		return 0;
+	}
+
+	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);
+
+	/* 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->len = xdpf->len;
+	new_xdpf->headroom = priv->tx_headroom;
+	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
+	new_xdpf->mem.type = MEM_TYPE_PAGE_ORDER0;
+
+	/* Release the initial buffer */
+	xdp_return_frame_rx_napi(xdpf);
+
+	*init_xdpf = new_xdpf;
+	return 0;
+}
 #endif
 
 static netdev_tx_t
@@ -2406,6 +2452,15 @@  static int dpaa_xdp_xmit_frame(struct net_device *net_dev,
 	percpu_priv = this_cpu_ptr(priv->percpu_priv);
 	percpu_stats = &percpu_priv->stats;
 
+#ifdef CONFIG_DPAA_ERRATUM_A050385
+	if (unlikely(fman_has_errata_a050385())) {
+		if (dpaa_a050385_wa_xdpf(priv, &xdpf)) {
+			err = -ENOMEM;
+			goto out_error;
+		}
+	}
+#endif
+
 	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
 		err = -EINVAL;
 		goto out_error;
@@ -2479,6 +2534,20 @@  static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
 	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
 	xdp.rxq = &dpaa_fq->xdp_rxq;
 
+	/* We reserve a fixed headroom of 256 bytes under the erratum and we
+	 * offer it all to XDP programs to use. If no room is left for the
+	 * xdpf backpointer on TX, we will need to copy the data.
+	 * Disable metadata support since data realignments might be required
+	 * and the information can be lost.
+	 */
+#ifdef CONFIG_DPAA_ERRATUM_A050385
+	if (unlikely(fman_has_errata_a050385())) {
+		xdp_set_data_meta_invalid(&xdp);
+		xdp.data_hard_start = vaddr;
+		xdp.frame_sz = DPAA_BP_RAW_SIZE;
+	}
+#endif
+
 	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
 
 	/* Update the length and the offset of the FD */
@@ -2486,7 +2555,8 @@  static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
 
 	switch (xdp_act) {
 	case XDP_PASS:
-		*xdp_meta_len = xdp.data - xdp.data_meta;
+		*xdp_meta_len = xdp_data_meta_unsupported(&xdp) ? 0 :
+				xdp.data - xdp.data_meta;
 		break;
 	case XDP_TX:
 		/* We can access the full headroom when sending the frame
@@ -3188,10 +3258,16 @@  static u16 dpaa_get_headroom(struct dpaa_buffer_layout *bl,
 	 */
 	headroom = (u16)(bl[port].priv_data_size + DPAA_HWA_SIZE);
 
-	if (port == RX)
+	if (port == RX) {
+#ifdef CONFIG_DPAA_ERRATUM_A050385
+		if (unlikely(fman_has_errata_a050385()))
+			headroom = XDP_PACKET_HEADROOM;
+#endif
+
 		return ALIGN(headroom, DPAA_FD_RX_DATA_ALIGNMENT);
-	else
+	} else {
 		return ALIGN(headroom, DPAA_FD_DATA_ALIGNMENT);
+	}
 }
 
 static int dpaa_eth_probe(struct platform_device *pdev)