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

[Camelia Alexandra Groza <camelia.groza@nxp.com>]

> -----Original Message-----
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Sent: Thursday, February 4, 2021 10:52
> To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> Cc: kuba@kernel.org; davem@davemloft.net; Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH net 1/3] dpaa_eth: reserve space for the xdp_frame
> under the A050385 erratum
> 
> 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?

Hi Maciej

This function will have to go through large changes anyway once we enable
multibuf support, so making it future-proof isn't a priority.

Instead, I do want to return a valid xdp_frame so I need to guarantee the
tailroom is there, even if we don't access it . I'll send a v2. Thanks.

Camelia