[PATCH net-next v4 0/7] dpaa_eth: add XDP support

[PATCH net-next v4 0/7] dpaa_eth: add XDP support

[Camelia Groza]

Enable XDP support for the QorIQ DPAA1 platforms.

Implement all the current actions (DROP, ABORTED, PASS, TX, REDIRECT). No
Tx batching is added at this time.

Additional XDP_PACKET_HEADROOM bytes are reserved in each frame's headroom.

After transmit, a reference to the xdp_frame is saved in the buffer for
clean-up on confirmation in a newly created structure for software
annotations. DPAA_TX_PRIV_DATA_SIZE bytes are reserved in the buffer for
storing this structure and the XDP program is restricted from accessing
them.

The driver shares the egress frame queues used for XDP with the network
stack. The DPAA driver is a LLTX driver so no explicit locking is required
on transmission.

Changes in v2:
- warn only once if extracting the timestamp from a received frame fails
  in 2/7

Changes in v3:
- drop received S/G frames when XDP is enabled in 2/7

Changes in v4:
- report a warning if the MTU is too hight for running XDP in 2/7
- report an error if opening the device fails in the XDP setup in 2/7
- call xdp_rxq_info_is_reg() before unregistering in 4/7
- minor cleanups (remove unneeded variable, print error code) in 4/7
- add more details in the commit message in 4/7
- did not call qman_destroy_fq() in case of xdp_rxq_info_reg() failure
since it would lead to a double free of the fq resources in 4/7

Camelia Groza (7):
  dpaa_eth: add struct for software backpointers
  dpaa_eth: add basic XDP support
  dpaa_eth: limit the possible MTU range when XDP is enabled
  dpaa_eth: add XDP_TX support
  dpaa_eth: add XDP_REDIRECT support
  dpaa_eth: rename current skb A050385 erratum workaround
  dpaa_eth: implement the A050385 erratum workaround for XDP

 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 458 +++++++++++++++++++++++--
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |  13 +
 2 files changed, 441 insertions(+), 30 deletions(-)

--
1.9.1

[PATCH net-next v4 1/7] dpaa_eth: add struct for software backpointers

[Camelia Groza]

We maintain an skb backpointer in the software annotations area of Tx
frames. Introduce a structure for explicit handling.

Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 16 +++++++++-------
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |  8 ++++++++
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 8867693..88533a2 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -1633,6 +1633,7 @@ static struct sk_buff *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
 	dma_addr_t addr = qm_fd_addr(fd);
 	void *vaddr = phys_to_virt(addr);
 	const struct qm_sg_entry *sgt;
+	struct dpaa_eth_swbp *swbp;
 	struct sk_buff *skb;
 	u64 ns;
 	int i;
@@ -1665,7 +1666,8 @@ static struct sk_buff *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
 				 dma_dir);
 	}
 
-	skb = *(struct sk_buff **)vaddr;
+	swbp = (struct dpaa_eth_swbp *)vaddr;
+	skb = swbp->skb;
 
 	/* DMA unmapping is required before accessing the HW provided info */
 	if (ts && priv->tx_tstamp &&
@@ -1879,8 +1881,8 @@ static int skb_to_contig_fd(struct dpaa_priv *priv,
 {
 	struct net_device *net_dev = priv->net_dev;
 	enum dma_data_direction dma_dir;
+	struct dpaa_eth_swbp *swbp;
 	unsigned char *buff_start;
-	struct sk_buff **skbh;
 	dma_addr_t addr;
 	int err;
 
@@ -1891,8 +1893,8 @@ static int skb_to_contig_fd(struct dpaa_priv *priv,
 	buff_start = skb->data - priv->tx_headroom;
 	dma_dir = DMA_TO_DEVICE;
 
-	skbh = (struct sk_buff **)buff_start;
-	*skbh = skb;
+	swbp = (struct dpaa_eth_swbp *)buff_start;
+	swbp->skb = skb;
 
 	/* Enable L3/L4 hardware checksum computation.
 	 *
@@ -1931,8 +1933,8 @@ static int skb_to_sg_fd(struct dpaa_priv *priv,
 	const enum dma_data_direction dma_dir = DMA_TO_DEVICE;
 	const int nr_frags = skb_shinfo(skb)->nr_frags;
 	struct net_device *net_dev = priv->net_dev;
+	struct dpaa_eth_swbp *swbp;
 	struct qm_sg_entry *sgt;
-	struct sk_buff **skbh;
 	void *buff_start;
 	skb_frag_t *frag;
 	dma_addr_t addr;
@@ -2005,8 +2007,8 @@ static int skb_to_sg_fd(struct dpaa_priv *priv,
 	qm_fd_set_sg(fd, priv->tx_headroom, skb->len);
 
 	/* DMA map the SGT page */
-	skbh = (struct sk_buff **)buff_start;
-	*skbh = skb;
+	swbp = (struct dpaa_eth_swbp *)buff_start;
+	swbp->skb = skb;
 
 	addr = dma_map_page(priv->tx_dma_dev, p, 0,
 			    priv->tx_headroom + DPAA_SGT_SIZE, dma_dir);
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
index fc2cc4c..da30e5d 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
@@ -144,6 +144,14 @@ struct dpaa_buffer_layout {
 	u16 priv_data_size;
 };
 
+/* Information to be used on the Tx confirmation path. Stored just
+ * before the start of the transmit buffer. Maximum size allowed
+ * is DPAA_TX_PRIV_DATA_SIZE bytes.
+ */
+struct dpaa_eth_swbp {
+	struct sk_buff *skb;
+};
+
 struct dpaa_priv {
 	struct dpaa_percpu_priv __percpu *percpu_priv;
 	struct dpaa_bp *dpaa_bp;
-- 
1.9.1

[PATCH net-next v4 2/7] dpaa_eth: add basic XDP support

[Camelia Groza]

Implement the XDP_DROP and XDP_PASS actions.

Avoid draining and reconfiguring the buffer pool at each XDP
setup/teardown by increasing the frame headroom and reserving
XDP_PACKET_HEADROOM bytes from the start. Since we always reserve an
entire page per buffer, this change only impacts Jumbo frame scenarios
where the maximum linear frame size is reduced by 256 bytes. Multi
buffer Scatter/Gather frames are now used instead in these scenarios.

Allow XDP programs to access the entire buffer.

The data in the received frame's headroom can be overwritten by the XDP
program. Extract the relevant fields from the headroom while they are
still available, before running the XDP program.

Since the headroom might be resized before the frame is passed up to the
stack, remove the check for a fixed headroom value when building an skb.

Allow the meta data to be updated and pass the information up the stack.

Scatter/Gather frames are dropped when XDP is enabled.

Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
Changes in v2:
- warn only once if extracting the timestamp from a received frame fails

Changes in v3:
- drop received S/G frames when XDP is enabled

Changes in v4:
- report a warning if the MTU is too hight for running XDP
- report an error if opening the device fails in the XDP setup

 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 166 ++++++++++++++++++++++---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |   2 +
 2 files changed, 152 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 88533a2..8acce62 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -53,6 +53,8 @@
 #include <linux/dma-mapping.h>
 #include <linux/sort.h>
 #include <linux/phy_fixed.h>
+#include <linux/bpf.h>
+#include <linux/bpf_trace.h>
 #include <soc/fsl/bman.h>
 #include <soc/fsl/qman.h>
 #include "fman.h"
@@ -177,7 +179,7 @@
 #define DPAA_HWA_SIZE (DPAA_PARSE_RESULTS_SIZE + DPAA_TIME_STAMP_SIZE \
 		       + DPAA_HASH_RESULTS_SIZE)
 #define DPAA_RX_PRIV_DATA_DEFAULT_SIZE (DPAA_TX_PRIV_DATA_SIZE + \
-					dpaa_rx_extra_headroom)
+					XDP_PACKET_HEADROOM - DPAA_HWA_SIZE)
 #ifdef CONFIG_DPAA_ERRATUM_A050385
 #define DPAA_RX_PRIV_DATA_A050385_SIZE (DPAA_A050385_ALIGN - DPAA_HWA_SIZE)
 #define DPAA_RX_PRIV_DATA_SIZE (fman_has_errata_a050385() ? \
@@ -1733,7 +1735,6 @@ static struct sk_buff *contig_fd_to_skb(const struct dpaa_priv *priv,
 			SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
 	if (WARN_ONCE(!skb, "Build skb failure on Rx\n"))
 		goto free_buffer;
-	WARN_ON(fd_off != priv->rx_headroom);
 	skb_reserve(skb, fd_off);
 	skb_put(skb, qm_fd_get_length(fd));

@@ -2349,12 +2350,62 @@ static enum qman_cb_dqrr_result rx_error_dqrr(struct qman_portal *portal,
 	return qman_cb_dqrr_consume;
 }

+static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
+			unsigned int *xdp_meta_len)
+{
+	ssize_t fd_off = qm_fd_get_offset(fd);
+	struct bpf_prog *xdp_prog;
+	struct xdp_buff xdp;
+	u32 xdp_act;
+
+	rcu_read_lock();
+
+	xdp_prog = READ_ONCE(priv->xdp_prog);
+	if (!xdp_prog) {
+		rcu_read_unlock();
+		return XDP_PASS;
+	}
+
+	xdp.data = vaddr + fd_off;
+	xdp.data_meta = xdp.data;
+	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
+	xdp.data_end = xdp.data + qm_fd_get_length(fd);
+	xdp.frame_sz = DPAA_BP_RAW_SIZE;
+
+	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
+
+	/* Update the length and the offset of the FD */
+	qm_fd_set_contig(fd, xdp.data - vaddr, xdp.data_end - xdp.data);
+
+	switch (xdp_act) {
+	case XDP_PASS:
+		*xdp_meta_len = xdp.data - xdp.data_meta;
+		break;
+	default:
+		bpf_warn_invalid_xdp_action(xdp_act);
+		fallthrough;
+	case XDP_ABORTED:
+		trace_xdp_exception(priv->net_dev, xdp_prog, xdp_act);
+		fallthrough;
+	case XDP_DROP:
+		/* Free the buffer */
+		free_pages((unsigned long)vaddr, 0);
+		break;
+	}
+
+	rcu_read_unlock();
+
+	return xdp_act;
+}
+
 static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
 						struct qman_fq *fq,
 						const struct qm_dqrr_entry *dq,
 						bool sched_napi)
 {
+	bool ts_valid = false, hash_valid = false;
 	struct skb_shared_hwtstamps *shhwtstamps;
+	unsigned int skb_len, xdp_meta_len = 0;
 	struct rtnl_link_stats64 *percpu_stats;
 	struct dpaa_percpu_priv *percpu_priv;
 	const struct qm_fd *fd = &dq->fd;
@@ -2362,12 +2413,14 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
 	enum qm_fd_format fd_format;
 	struct net_device *net_dev;
 	u32 fd_status, hash_offset;
+	struct qm_sg_entry *sgt;
 	struct dpaa_bp *dpaa_bp;
 	struct dpaa_priv *priv;
-	unsigned int skb_len;
 	struct sk_buff *skb;
 	int *count_ptr;
+	u32 xdp_act;
 	void *vaddr;
+	u32 hash;
 	u64 ns;

 	fd_status = be32_to_cpu(fd->status);
@@ -2423,35 +2476,67 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
 	count_ptr = this_cpu_ptr(dpaa_bp->percpu_count);
 	(*count_ptr)--;

-	if (likely(fd_format == qm_fd_contig))
+	/* Extract the timestamp stored in the headroom before running XDP */
+	if (priv->rx_tstamp) {
+		if (!fman_port_get_tstamp(priv->mac_dev->port[RX], vaddr, &ns))
+			ts_valid = true;
+		else
+			WARN_ONCE(1, "fman_port_get_tstamp failed!\n");
+	}
+
+	/* Extract the hash stored in the headroom before running XDP */
+	if (net_dev->features & NETIF_F_RXHASH && priv->keygen_in_use &&
+	    !fman_port_get_hash_result_offset(priv->mac_dev->port[RX],
+					      &hash_offset)) {
+		hash = be32_to_cpu(*(u32 *)(vaddr + hash_offset));
+		hash_valid = true;
+	}
+
+	if (likely(fd_format == qm_fd_contig)) {
+		xdp_act = dpaa_run_xdp(priv, (struct qm_fd *)fd, vaddr,
+				       &xdp_meta_len);
+		if (xdp_act != XDP_PASS) {
+			percpu_stats->rx_packets++;
+			percpu_stats->rx_bytes += qm_fd_get_length(fd);
+			return qman_cb_dqrr_consume;
+		}
 		skb = contig_fd_to_skb(priv, fd);
-	else
+	} else {
+		/* XDP doesn't support S/G frames. Return the fragments to the
+		 * buffer pool and release the SGT.
+		 */
+		if (READ_ONCE(priv->xdp_prog)) {
+			WARN_ONCE(1, "S/G frames not supported under XDP\n");
+			sgt = vaddr + qm_fd_get_offset(fd);
+			dpaa_release_sgt_members(sgt);
+			free_pages((unsigned long)vaddr, 0);
+			return qman_cb_dqrr_consume;
+		}
 		skb = sg_fd_to_skb(priv, fd);
+	}
 	if (!skb)
 		return qman_cb_dqrr_consume;

-	if (priv->rx_tstamp) {
+	if (xdp_meta_len)
+		skb_metadata_set(skb, xdp_meta_len);
+
+	/* Set the previously extracted timestamp */
+	if (ts_valid) {
 		shhwtstamps = skb_hwtstamps(skb);
 		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
-
-		if (!fman_port_get_tstamp(priv->mac_dev->port[RX], vaddr, &ns))
-			shhwtstamps->hwtstamp = ns_to_ktime(ns);
-		else
-			dev_warn(net_dev->dev.parent, "fman_port_get_tstamp failed!\n");
+		shhwtstamps->hwtstamp = ns_to_ktime(ns);
 	}

 	skb->protocol = eth_type_trans(skb, net_dev);

-	if (net_dev->features & NETIF_F_RXHASH && priv->keygen_in_use &&
-	    !fman_port_get_hash_result_offset(priv->mac_dev->port[RX],
-					      &hash_offset)) {
+	/* Set the previously extracted hash */
+	if (hash_valid) {
 		enum pkt_hash_types type;

 		/* if L4 exists, it was used in the hash generation */
 		type = be32_to_cpu(fd->status) & FM_FD_STAT_L4CV ?
 			PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3;
-		skb_set_hash(skb, be32_to_cpu(*(u32 *)(vaddr + hash_offset)),
-			     type);
+		skb_set_hash(skb, hash, type);
 	}

 	skb_len = skb->len;
@@ -2671,6 +2756,54 @@ static int dpaa_eth_stop(struct net_device *net_dev)
 	return err;
 }

+static int dpaa_setup_xdp(struct net_device *net_dev, struct bpf_prog *prog)
+{
+	struct dpaa_priv *priv = netdev_priv(net_dev);
+	struct bpf_prog *old_prog;
+	int err, max_contig_data;
+	bool up;
+
+	max_contig_data = priv->dpaa_bp->size - priv->rx_headroom;
+
+	/* S/G fragments are not supported in XDP-mode */
+	if (prog &&
+	    (net_dev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN > max_contig_data)) {
+		dev_warn(net_dev->dev.parent,
+			 "The maximum MTU for XDP is %d\n",
+			 max_contig_data - VLAN_ETH_HLEN - ETH_FCS_LEN);
+		return -EINVAL;
+	}
+
+	up = netif_running(net_dev);
+
+	if (up)
+		dpaa_eth_stop(net_dev);
+
+	old_prog = xchg(&priv->xdp_prog, prog);
+	if (old_prog)
+		bpf_prog_put(old_prog);
+
+	if (up) {
+		err = dpaa_open(net_dev);
+		if (err) {
+			dev_err(net_dev->dev.parent, "dpaa_open() failed\n");
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int dpaa_xdp(struct net_device *net_dev, struct netdev_bpf *xdp)
+{
+	switch (xdp->command) {
+	case XDP_SETUP_PROG:
+		return dpaa_setup_xdp(net_dev, xdp->prog);
+	default:
+		return -EINVAL;
+	}
+}
+
 static int dpaa_ts_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
 	struct dpaa_priv *priv = netdev_priv(dev);
@@ -2737,6 +2870,7 @@ static int dpaa_ioctl(struct net_device *net_dev, struct ifreq *rq, int cmd)
 	.ndo_set_rx_mode = dpaa_set_rx_mode,
 	.ndo_do_ioctl = dpaa_ioctl,
 	.ndo_setup_tc = dpaa_setup_tc,
+	.ndo_bpf = dpaa_xdp,
 };

 static int dpaa_napi_add(struct net_device *net_dev)
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
index da30e5d..94e8613 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
@@ -196,6 +196,8 @@ struct dpaa_priv {

 	bool tx_tstamp; /* Tx timestamping enabled */
 	bool rx_tstamp; /* Rx timestamping enabled */
+
+	struct bpf_prog *xdp_prog;
 };

 /* from dpaa_ethtool.c */
--
1.9.1

Re: [PATCH net-next v4 2/7] dpaa_eth: add basic XDP support

[Maciej Fijalkowski]

On Mon, Nov 23, 2020 at 07:36:20PM +0200, Camelia Groza wrote:
> Implement the XDP_DROP and XDP_PASS actions.
> 
> Avoid draining and reconfiguring the buffer pool at each XDP
> setup/teardown by increasing the frame headroom and reserving
> XDP_PACKET_HEADROOM bytes from the start. Since we always reserve an
> entire page per buffer, this change only impacts Jumbo frame scenarios
> where the maximum linear frame size is reduced by 256 bytes. Multi
> buffer Scatter/Gather frames are now used instead in these scenarios.
> 
> Allow XDP programs to access the entire buffer.
> 
> The data in the received frame's headroom can be overwritten by the XDP
> program. Extract the relevant fields from the headroom while they are
> still available, before running the XDP program.
> 
> Since the headroom might be resized before the frame is passed up to the
> stack, remove the check for a fixed headroom value when building an skb.
> 
> Allow the meta data to be updated and pass the information up the stack.
> 
> Scatter/Gather frames are dropped when XDP is enabled.
> 
> Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> ---
> Changes in v2:
> - warn only once if extracting the timestamp from a received frame fails
> 
> Changes in v3:
> - drop received S/G frames when XDP is enabled
> 
> Changes in v4:
> - report a warning if the MTU is too hight for running XDP
> - report an error if opening the device fails in the XDP setup
> 
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 166 ++++++++++++++++++++++---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |   2 +
>  2 files changed, 152 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 88533a2..8acce62 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -53,6 +53,8 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/sort.h>
>  #include <linux/phy_fixed.h>
> +#include <linux/bpf.h>
> +#include <linux/bpf_trace.h>
>  #include <soc/fsl/bman.h>
>  #include <soc/fsl/qman.h>
>  #include "fman.h"
> @@ -177,7 +179,7 @@
>  #define DPAA_HWA_SIZE (DPAA_PARSE_RESULTS_SIZE + DPAA_TIME_STAMP_SIZE \
>  		       + DPAA_HASH_RESULTS_SIZE)
>  #define DPAA_RX_PRIV_DATA_DEFAULT_SIZE (DPAA_TX_PRIV_DATA_SIZE + \
> -					dpaa_rx_extra_headroom)
> +					XDP_PACKET_HEADROOM - DPAA_HWA_SIZE)
>  #ifdef CONFIG_DPAA_ERRATUM_A050385
>  #define DPAA_RX_PRIV_DATA_A050385_SIZE (DPAA_A050385_ALIGN - DPAA_HWA_SIZE)
>  #define DPAA_RX_PRIV_DATA_SIZE (fman_has_errata_a050385() ? \
> @@ -1733,7 +1735,6 @@ static struct sk_buff *contig_fd_to_skb(const struct dpaa_priv *priv,
>  			SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
>  	if (WARN_ONCE(!skb, "Build skb failure on Rx\n"))
>  		goto free_buffer;
> -	WARN_ON(fd_off != priv->rx_headroom);
>  	skb_reserve(skb, fd_off);
>  	skb_put(skb, qm_fd_get_length(fd));
> 
> @@ -2349,12 +2350,62 @@ static enum qman_cb_dqrr_result rx_error_dqrr(struct qman_portal *portal,
>  	return qman_cb_dqrr_consume;
>  }
> 
> +static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
> +			unsigned int *xdp_meta_len)
> +{
> +	ssize_t fd_off = qm_fd_get_offset(fd);
> +	struct bpf_prog *xdp_prog;
> +	struct xdp_buff xdp;
> +	u32 xdp_act;
> +
> +	rcu_read_lock();
> +
> +	xdp_prog = READ_ONCE(priv->xdp_prog);
> +	if (!xdp_prog) {
> +		rcu_read_unlock();
> +		return XDP_PASS;
> +	}
> +
> +	xdp.data = vaddr + fd_off;
> +	xdp.data_meta = xdp.data;
> +	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
> +	xdp.data_end = xdp.data + qm_fd_get_length(fd);
> +	xdp.frame_sz = DPAA_BP_RAW_SIZE;
> +
> +	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> +
> +	/* Update the length and the offset of the FD */
> +	qm_fd_set_contig(fd, xdp.data - vaddr, xdp.data_end - xdp.data);

Shouldn't you do this update based on xdp.data_meta, not xdp.data?

> +
> +	switch (xdp_act) {
> +	case XDP_PASS:
> +		*xdp_meta_len = xdp.data - xdp.data_meta;
> +		break;
> +	default:
> +		bpf_warn_invalid_xdp_action(xdp_act);
> +		fallthrough;
> +	case XDP_ABORTED:
> +		trace_xdp_exception(priv->net_dev, xdp_prog, xdp_act);
> +		fallthrough;
> +	case XDP_DROP:
> +		/* Free the buffer */
> +		free_pages((unsigned long)vaddr, 0);
> +		break;
> +	}
> +
> +	rcu_read_unlock();
> +
> +	return xdp_act;
> +}
> +
>  static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
>  						struct qman_fq *fq,
>  						const struct qm_dqrr_entry *dq,
>  						bool sched_napi)
>  {
> +	bool ts_valid = false, hash_valid = false;
>  	struct skb_shared_hwtstamps *shhwtstamps;
> +	unsigned int skb_len, xdp_meta_len = 0;
>  	struct rtnl_link_stats64 *percpu_stats;
>  	struct dpaa_percpu_priv *percpu_priv;
>  	const struct qm_fd *fd = &dq->fd;
> @@ -2362,12 +2413,14 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
>  	enum qm_fd_format fd_format;
>  	struct net_device *net_dev;
>  	u32 fd_status, hash_offset;
> +	struct qm_sg_entry *sgt;
>  	struct dpaa_bp *dpaa_bp;
>  	struct dpaa_priv *priv;
> -	unsigned int skb_len;
>  	struct sk_buff *skb;
>  	int *count_ptr;
> +	u32 xdp_act;
>  	void *vaddr;
> +	u32 hash;
>  	u64 ns;
> 
>  	fd_status = be32_to_cpu(fd->status);
> @@ -2423,35 +2476,67 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
>  	count_ptr = this_cpu_ptr(dpaa_bp->percpu_count);
>  	(*count_ptr)--;
> 
> -	if (likely(fd_format == qm_fd_contig))
> +	/* Extract the timestamp stored in the headroom before running XDP */
> +	if (priv->rx_tstamp) {
> +		if (!fman_port_get_tstamp(priv->mac_dev->port[RX], vaddr, &ns))
> +			ts_valid = true;
> +		else
> +			WARN_ONCE(1, "fman_port_get_tstamp failed!\n");
> +	}
> +
> +	/* Extract the hash stored in the headroom before running XDP */
> +	if (net_dev->features & NETIF_F_RXHASH && priv->keygen_in_use &&
> +	    !fman_port_get_hash_result_offset(priv->mac_dev->port[RX],
> +					      &hash_offset)) {
> +		hash = be32_to_cpu(*(u32 *)(vaddr + hash_offset));
> +		hash_valid = true;
> +	}
> +
> +	if (likely(fd_format == qm_fd_contig)) {
> +		xdp_act = dpaa_run_xdp(priv, (struct qm_fd *)fd, vaddr,
> +				       &xdp_meta_len);
> +		if (xdp_act != XDP_PASS) {
> +			percpu_stats->rx_packets++;
> +			percpu_stats->rx_bytes += qm_fd_get_length(fd);
> +			return qman_cb_dqrr_consume;
> +		}
>  		skb = contig_fd_to_skb(priv, fd);
> -	else
> +	} else {
> +		/* XDP doesn't support S/G frames. Return the fragments to the
> +		 * buffer pool and release the SGT.
> +		 */
> +		if (READ_ONCE(priv->xdp_prog)) {
> +			WARN_ONCE(1, "S/G frames not supported under XDP\n");
> +			sgt = vaddr + qm_fd_get_offset(fd);
> +			dpaa_release_sgt_members(sgt);
> +			free_pages((unsigned long)vaddr, 0);
> +			return qman_cb_dqrr_consume;
> +		}
>  		skb = sg_fd_to_skb(priv, fd);
> +	}
>  	if (!skb)
>  		return qman_cb_dqrr_consume;
> 
> -	if (priv->rx_tstamp) {
> +	if (xdp_meta_len)
> +		skb_metadata_set(skb, xdp_meta_len);
> +
> +	/* Set the previously extracted timestamp */
> +	if (ts_valid) {
>  		shhwtstamps = skb_hwtstamps(skb);
>  		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> -
> -		if (!fman_port_get_tstamp(priv->mac_dev->port[RX], vaddr, &ns))
> -			shhwtstamps->hwtstamp = ns_to_ktime(ns);
> -		else
> -			dev_warn(net_dev->dev.parent, "fman_port_get_tstamp failed!\n");
> +		shhwtstamps->hwtstamp = ns_to_ktime(ns);
>  	}
> 
>  	skb->protocol = eth_type_trans(skb, net_dev);
> 
> -	if (net_dev->features & NETIF_F_RXHASH && priv->keygen_in_use &&
> -	    !fman_port_get_hash_result_offset(priv->mac_dev->port[RX],
> -					      &hash_offset)) {
> +	/* Set the previously extracted hash */
> +	if (hash_valid) {
>  		enum pkt_hash_types type;
> 
>  		/* if L4 exists, it was used in the hash generation */
>  		type = be32_to_cpu(fd->status) & FM_FD_STAT_L4CV ?
>  			PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3;
> -		skb_set_hash(skb, be32_to_cpu(*(u32 *)(vaddr + hash_offset)),
> -			     type);
> +		skb_set_hash(skb, hash, type);
>  	}
> 
>  	skb_len = skb->len;
> @@ -2671,6 +2756,54 @@ static int dpaa_eth_stop(struct net_device *net_dev)
>  	return err;
>  }
> 
> +static int dpaa_setup_xdp(struct net_device *net_dev, struct bpf_prog *prog)
> +{
> +	struct dpaa_priv *priv = netdev_priv(net_dev);
> +	struct bpf_prog *old_prog;
> +	int err, max_contig_data;
> +	bool up;
> +
> +	max_contig_data = priv->dpaa_bp->size - priv->rx_headroom;
> +
> +	/* S/G fragments are not supported in XDP-mode */
> +	if (prog &&
> +	    (net_dev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN > max_contig_data)) {
> +		dev_warn(net_dev->dev.parent,
> +			 "The maximum MTU for XDP is %d\n",
> +			 max_contig_data - VLAN_ETH_HLEN - ETH_FCS_LEN);
> +		return -EINVAL;
> +	}
> +
> +	up = netif_running(net_dev);
> +
> +	if (up)
> +		dpaa_eth_stop(net_dev);
> +
> +	old_prog = xchg(&priv->xdp_prog, prog);
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +
> +	if (up) {
> +		err = dpaa_open(net_dev);
> +		if (err) {
> +			dev_err(net_dev->dev.parent, "dpaa_open() failed\n");
> +			return err;

So you decided not to take advantage of extack messages?

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int dpaa_xdp(struct net_device *net_dev, struct netdev_bpf *xdp)
> +{
> +	switch (xdp->command) {
> +	case XDP_SETUP_PROG:
> +		return dpaa_setup_xdp(net_dev, xdp->prog);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int dpaa_ts_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  {
>  	struct dpaa_priv *priv = netdev_priv(dev);
> @@ -2737,6 +2870,7 @@ static int dpaa_ioctl(struct net_device *net_dev, struct ifreq *rq, int cmd)
>  	.ndo_set_rx_mode = dpaa_set_rx_mode,
>  	.ndo_do_ioctl = dpaa_ioctl,
>  	.ndo_setup_tc = dpaa_setup_tc,
> +	.ndo_bpf = dpaa_xdp,
>  };
> 
>  static int dpaa_napi_add(struct net_device *net_dev)
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> index da30e5d..94e8613 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> @@ -196,6 +196,8 @@ struct dpaa_priv {
> 
>  	bool tx_tstamp; /* Tx timestamping enabled */
>  	bool rx_tstamp; /* Rx timestamping enabled */
> +
> +	struct bpf_prog *xdp_prog;
>  };
> 
>  /* from dpaa_ethtool.c */
> --
> 1.9.1
> 

RE: [PATCH net-next v4 2/7] dpaa_eth: add basic XDP support

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Sent: Tuesday, November 24, 2020 15:51
> To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> davem@davemloft.net; Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next v4 2/7] dpaa_eth: add basic XDP support
> 
> On Mon, Nov 23, 2020 at 07:36:20PM +0200, Camelia Groza wrote:
> > Implement the XDP_DROP and XDP_PASS actions.
> >
> > Avoid draining and reconfiguring the buffer pool at each XDP
> > setup/teardown by increasing the frame headroom and reserving
> > XDP_PACKET_HEADROOM bytes from the start. Since we always reserve
> an
> > entire page per buffer, this change only impacts Jumbo frame scenarios
> > where the maximum linear frame size is reduced by 256 bytes. Multi
> > buffer Scatter/Gather frames are now used instead in these scenarios.
> >
> > Allow XDP programs to access the entire buffer.
> >
> > The data in the received frame's headroom can be overwritten by the XDP
> > program. Extract the relevant fields from the headroom while they are
> > still available, before running the XDP program.
> >
> > Since the headroom might be resized before the frame is passed up to the
> > stack, remove the check for a fixed headroom value when building an skb.
> >
> > Allow the meta data to be updated and pass the information up the stack.
> >
> > Scatter/Gather frames are dropped when XDP is enabled.
> >
> > Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > ---
> > Changes in v2:
> > - warn only once if extracting the timestamp from a received frame fails
> >
> > Changes in v3:
> > - drop received S/G frames when XDP is enabled
> >
> > Changes in v4:
> > - report a warning if the MTU is too hight for running XDP
> > - report an error if opening the device fails in the XDP setup
> >
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 166
> ++++++++++++++++++++++---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |   2 +
> >  2 files changed, 152 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index 88533a2..8acce62 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -53,6 +53,8 @@
> >  #include <linux/dma-mapping.h>
> >  #include <linux/sort.h>
> >  #include <linux/phy_fixed.h>
> > +#include <linux/bpf.h>
> > +#include <linux/bpf_trace.h>
> >  #include <soc/fsl/bman.h>
> >  #include <soc/fsl/qman.h>
> >  #include "fman.h"
> > @@ -177,7 +179,7 @@
> >  #define DPAA_HWA_SIZE (DPAA_PARSE_RESULTS_SIZE +
> DPAA_TIME_STAMP_SIZE \
> >  		       + DPAA_HASH_RESULTS_SIZE)
> >  #define DPAA_RX_PRIV_DATA_DEFAULT_SIZE
> (DPAA_TX_PRIV_DATA_SIZE + \
> > -					dpaa_rx_extra_headroom)
> > +					XDP_PACKET_HEADROOM -
> DPAA_HWA_SIZE)
> >  #ifdef CONFIG_DPAA_ERRATUM_A050385
> >  #define DPAA_RX_PRIV_DATA_A050385_SIZE (DPAA_A050385_ALIGN -
> DPAA_HWA_SIZE)
> >  #define DPAA_RX_PRIV_DATA_SIZE (fman_has_errata_a050385() ? \
> > @@ -1733,7 +1735,6 @@ static struct sk_buff *contig_fd_to_skb(const
> struct dpaa_priv *priv,
> >  			SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> >  	if (WARN_ONCE(!skb, "Build skb failure on Rx\n"))
> >  		goto free_buffer;
> > -	WARN_ON(fd_off != priv->rx_headroom);
> >  	skb_reserve(skb, fd_off);
> >  	skb_put(skb, qm_fd_get_length(fd));
> >
> > @@ -2349,12 +2350,62 @@ static enum qman_cb_dqrr_result
> rx_error_dqrr(struct qman_portal *portal,
> >  	return qman_cb_dqrr_consume;
> >  }
> >
> > +static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void
> *vaddr,
> > +			unsigned int *xdp_meta_len)
> > +{
> > +	ssize_t fd_off = qm_fd_get_offset(fd);
> > +	struct bpf_prog *xdp_prog;
> > +	struct xdp_buff xdp;
> > +	u32 xdp_act;
> > +
> > +	rcu_read_lock();
> > +
> > +	xdp_prog = READ_ONCE(priv->xdp_prog);
> > +	if (!xdp_prog) {
> > +		rcu_read_unlock();
> > +		return XDP_PASS;
> > +	}
> > +
> > +	xdp.data = vaddr + fd_off;
> > +	xdp.data_meta = xdp.data;
> > +	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
> > +	xdp.data_end = xdp.data + qm_fd_get_length(fd);
> > +	xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > +
> > +	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > +
> > +	/* Update the length and the offset of the FD */
> > +	qm_fd_set_contig(fd, xdp.data - vaddr, xdp.data_end - xdp.data);
> 
> Shouldn't you do this update based on xdp.data_meta, not xdp.data?

Both the xdp.data_meta and xdp.data can be updated when the XDP program runs.

In case of XDP_PASS, when building an skb around an fd, we need to know where the data starts for calling skb_reserve(). The metadata set by the XDP program is part of the skb's headroom, not part of the packet data. So we adjust the fd's offset and length based on the data, not the metadata. We let the skb know it has metadata set in its headroom through skb_metadata_set().

For XDP_TX we don't pass metadata to the hardware, and for XDP_REDIRECT the xdp_frame points to this information, so there is no need for keeping track of it in the fd.

> > +
> > +	switch (xdp_act) {
> > +	case XDP_PASS:
> > +		*xdp_meta_len = xdp.data - xdp.data_meta;
> > +		break;
> > +	default:
> > +		bpf_warn_invalid_xdp_action(xdp_act);
> > +		fallthrough;
> > +	case XDP_ABORTED:
> > +		trace_xdp_exception(priv->net_dev, xdp_prog, xdp_act);
> > +		fallthrough;
> > +	case XDP_DROP:
> > +		/* Free the buffer */
> > +		free_pages((unsigned long)vaddr, 0);
> > +		break;
> > +	}
> > +
> > +	rcu_read_unlock();
> > +
> > +	return xdp_act;
> > +}
> > +
> >  static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal
> *portal,
> >  						struct qman_fq *fq,
> >  						const struct qm_dqrr_entry
> *dq,
> >  						bool sched_napi)
> >  {
> > +	bool ts_valid = false, hash_valid = false;
> >  	struct skb_shared_hwtstamps *shhwtstamps;
> > +	unsigned int skb_len, xdp_meta_len = 0;
> >  	struct rtnl_link_stats64 *percpu_stats;
> >  	struct dpaa_percpu_priv *percpu_priv;
> >  	const struct qm_fd *fd = &dq->fd;
> > @@ -2362,12 +2413,14 @@ static enum qman_cb_dqrr_result
> rx_default_dqrr(struct qman_portal *portal,
> >  	enum qm_fd_format fd_format;
> >  	struct net_device *net_dev;
> >  	u32 fd_status, hash_offset;
> > +	struct qm_sg_entry *sgt;
> >  	struct dpaa_bp *dpaa_bp;
> >  	struct dpaa_priv *priv;
> > -	unsigned int skb_len;
> >  	struct sk_buff *skb;
> >  	int *count_ptr;
> > +	u32 xdp_act;
> >  	void *vaddr;
> > +	u32 hash;
> >  	u64 ns;
> >
> >  	fd_status = be32_to_cpu(fd->status);
> > @@ -2423,35 +2476,67 @@ static enum qman_cb_dqrr_result
> rx_default_dqrr(struct qman_portal *portal,
> >  	count_ptr = this_cpu_ptr(dpaa_bp->percpu_count);
> >  	(*count_ptr)--;
> >
> > -	if (likely(fd_format == qm_fd_contig))
> > +	/* Extract the timestamp stored in the headroom before running
> XDP */
> > +	if (priv->rx_tstamp) {
> > +		if (!fman_port_get_tstamp(priv->mac_dev->port[RX], vaddr,
> &ns))
> > +			ts_valid = true;
> > +		else
> > +			WARN_ONCE(1, "fman_port_get_tstamp failed!\n");
> > +	}
> > +
> > +	/* Extract the hash stored in the headroom before running XDP */
> > +	if (net_dev->features & NETIF_F_RXHASH && priv->keygen_in_use
> &&
> > +	    !fman_port_get_hash_result_offset(priv->mac_dev->port[RX],
> > +					      &hash_offset)) {
> > +		hash = be32_to_cpu(*(u32 *)(vaddr + hash_offset));
> > +		hash_valid = true;
> > +	}
> > +
> > +	if (likely(fd_format == qm_fd_contig)) {
> > +		xdp_act = dpaa_run_xdp(priv, (struct qm_fd *)fd, vaddr,
> > +				       &xdp_meta_len);
> > +		if (xdp_act != XDP_PASS) {
> > +			percpu_stats->rx_packets++;
> > +			percpu_stats->rx_bytes += qm_fd_get_length(fd);
> > +			return qman_cb_dqrr_consume;
> > +		}
> >  		skb = contig_fd_to_skb(priv, fd);
> > -	else
> > +	} else {
> > +		/* XDP doesn't support S/G frames. Return the fragments to
> the
> > +		 * buffer pool and release the SGT.
> > +		 */
> > +		if (READ_ONCE(priv->xdp_prog)) {
> > +			WARN_ONCE(1, "S/G frames not supported under
> XDP\n");
> > +			sgt = vaddr + qm_fd_get_offset(fd);
> > +			dpaa_release_sgt_members(sgt);
> > +			free_pages((unsigned long)vaddr, 0);
> > +			return qman_cb_dqrr_consume;
> > +		}
> >  		skb = sg_fd_to_skb(priv, fd);
> > +	}
> >  	if (!skb)
> >  		return qman_cb_dqrr_consume;
> >
> > -	if (priv->rx_tstamp) {
> > +	if (xdp_meta_len)
> > +		skb_metadata_set(skb, xdp_meta_len);
> > +
> > +	/* Set the previously extracted timestamp */
> > +	if (ts_valid) {
> >  		shhwtstamps = skb_hwtstamps(skb);
> >  		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> > -
> > -		if (!fman_port_get_tstamp(priv->mac_dev->port[RX], vaddr,
> &ns))
> > -			shhwtstamps->hwtstamp = ns_to_ktime(ns);
> > -		else
> > -			dev_warn(net_dev->dev.parent,
> "fman_port_get_tstamp failed!\n");
> > +		shhwtstamps->hwtstamp = ns_to_ktime(ns);
> >  	}
> >
> >  	skb->protocol = eth_type_trans(skb, net_dev);
> >
> > -	if (net_dev->features & NETIF_F_RXHASH && priv->keygen_in_use
> &&
> > -	    !fman_port_get_hash_result_offset(priv->mac_dev->port[RX],
> > -					      &hash_offset)) {
> > +	/* Set the previously extracted hash */
> > +	if (hash_valid) {
> >  		enum pkt_hash_types type;
> >
> >  		/* if L4 exists, it was used in the hash generation */
> >  		type = be32_to_cpu(fd->status) & FM_FD_STAT_L4CV ?
> >  			PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3;
> > -		skb_set_hash(skb, be32_to_cpu(*(u32 *)(vaddr +
> hash_offset)),
> > -			     type);
> > +		skb_set_hash(skb, hash, type);
> >  	}
> >
> >  	skb_len = skb->len;
> > @@ -2671,6 +2756,54 @@ static int dpaa_eth_stop(struct net_device
> *net_dev)
> >  	return err;
> >  }
> >
> > +static int dpaa_setup_xdp(struct net_device *net_dev, struct bpf_prog
> *prog)
> > +{
> > +	struct dpaa_priv *priv = netdev_priv(net_dev);
> > +	struct bpf_prog *old_prog;
> > +	int err, max_contig_data;
> > +	bool up;
> > +
> > +	max_contig_data = priv->dpaa_bp->size - priv->rx_headroom;
> > +
> > +	/* S/G fragments are not supported in XDP-mode */
> > +	if (prog &&
> > +	    (net_dev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN >
> max_contig_data)) {
> > +		dev_warn(net_dev->dev.parent,
> > +			 "The maximum MTU for XDP is %d\n",
> > +			 max_contig_data - VLAN_ETH_HLEN -
> ETH_FCS_LEN);
> > +		return -EINVAL;
> > +	}
> > +
> > +	up = netif_running(net_dev);
> > +
> > +	if (up)
> > +		dpaa_eth_stop(net_dev);
> > +
> > +	old_prog = xchg(&priv->xdp_prog, prog);
> > +	if (old_prog)
> > +		bpf_prog_put(old_prog);
> > +
> > +	if (up) {
> > +		err = dpaa_open(net_dev);
> > +		if (err) {
> > +			dev_err(net_dev->dev.parent, "dpaa_open()
> failed\n");
> > +			return err;
> 
> So you decided not to take advantage of extack messages?

Sorry, I put more emphasis on reporting the messages than on the actual means to do it. I'll wait, if you have more comments, and respin.

> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dpaa_xdp(struct net_device *net_dev, struct netdev_bpf *xdp)
> > +{
> > +	switch (xdp->command) {
> > +	case XDP_SETUP_PROG:
> > +		return dpaa_setup_xdp(net_dev, xdp->prog);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static int dpaa_ts_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> >  {
> >  	struct dpaa_priv *priv = netdev_priv(dev);
> > @@ -2737,6 +2870,7 @@ static int dpaa_ioctl(struct net_device *net_dev,
> struct ifreq *rq, int cmd)
> >  	.ndo_set_rx_mode = dpaa_set_rx_mode,
> >  	.ndo_do_ioctl = dpaa_ioctl,
> >  	.ndo_setup_tc = dpaa_setup_tc,
> > +	.ndo_bpf = dpaa_xdp,
> >  };
> >
> >  static int dpaa_napi_add(struct net_device *net_dev)
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > index da30e5d..94e8613 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > @@ -196,6 +196,8 @@ struct dpaa_priv {
> >
> >  	bool tx_tstamp; /* Tx timestamping enabled */
> >  	bool rx_tstamp; /* Rx timestamping enabled */
> > +
> > +	struct bpf_prog *xdp_prog;
> >  };
> >
> >  /* from dpaa_ethtool.c */
> > --
> > 1.9.1
> >

[PATCH net-next v4 3/7] dpaa_eth: limit the possible MTU range when XDP is enabled

[Camelia Groza]

Implement the ndo_change_mtu callback to prevent users from setting an
MTU that would permit processing of S/G frames. The maximum MTU size
is dependent on the buffer size.

Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 40 ++++++++++++++++++++------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 8acce62..ee076f4 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2756,23 +2756,44 @@ static int dpaa_eth_stop(struct net_device *net_dev)
 	return err;
 }
 
+static bool xdp_validate_mtu(struct dpaa_priv *priv, int mtu)
+{
+	int max_contig_data = priv->dpaa_bp->size - priv->rx_headroom;
+
+	/* We do not support S/G fragments when XDP is enabled.
+	 * Limit the MTU in relation to the buffer size.
+	 */
+	if (mtu + VLAN_ETH_HLEN + ETH_FCS_LEN > max_contig_data) {
+		dev_warn(priv->net_dev->dev.parent,
+			 "The maximum MTU for XDP is %d\n",
+			 max_contig_data - VLAN_ETH_HLEN - ETH_FCS_LEN);
+		return false;
+	}
+
+	return true;
+}
+
+static int dpaa_change_mtu(struct net_device *net_dev, int new_mtu)
+{
+	struct dpaa_priv *priv = netdev_priv(net_dev);
+
+	if (priv->xdp_prog && !xdp_validate_mtu(priv, new_mtu))
+		return -EINVAL;
+
+	net_dev->mtu = new_mtu;
+	return 0;
+}
+
 static int dpaa_setup_xdp(struct net_device *net_dev, struct bpf_prog *prog)
 {
 	struct dpaa_priv *priv = netdev_priv(net_dev);
 	struct bpf_prog *old_prog;
-	int err, max_contig_data;
+	int err;
 	bool up;
 
-	max_contig_data = priv->dpaa_bp->size - priv->rx_headroom;
-
 	/* S/G fragments are not supported in XDP-mode */
-	if (prog &&
-	    (net_dev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN > max_contig_data)) {
-		dev_warn(net_dev->dev.parent,
-			 "The maximum MTU for XDP is %d\n",
-			 max_contig_data - VLAN_ETH_HLEN - ETH_FCS_LEN);
+	if (prog && !xdp_validate_mtu(priv, net_dev->mtu))
 		return -EINVAL;
-	}
 
 	up = netif_running(net_dev);
 
@@ -2870,6 +2891,7 @@ static int dpaa_ioctl(struct net_device *net_dev, struct ifreq *rq, int cmd)
 	.ndo_set_rx_mode = dpaa_set_rx_mode,
 	.ndo_do_ioctl = dpaa_ioctl,
 	.ndo_setup_tc = dpaa_setup_tc,
+	.ndo_change_mtu = dpaa_change_mtu,
 	.ndo_bpf = dpaa_xdp,
 };
 
-- 
1.9.1

Re: [PATCH net-next v4 3/7] dpaa_eth: limit the possible MTU range when XDP is enabled

[Maciej Fijalkowski]

On Mon, Nov 23, 2020 at 07:36:21PM +0200, Camelia Groza wrote:
> Implement the ndo_change_mtu callback to prevent users from setting an
> MTU that would permit processing of S/G frames. The maximum MTU size
> is dependent on the buffer size.
> 
> Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 40 ++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 8acce62..ee076f4 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2756,23 +2756,44 @@ static int dpaa_eth_stop(struct net_device *net_dev)
>  	return err;
>  }
>  
> +static bool xdp_validate_mtu(struct dpaa_priv *priv, int mtu)
> +{
> +	int max_contig_data = priv->dpaa_bp->size - priv->rx_headroom;
> +
> +	/* We do not support S/G fragments when XDP is enabled.
> +	 * Limit the MTU in relation to the buffer size.
> +	 */
> +	if (mtu + VLAN_ETH_HLEN + ETH_FCS_LEN > max_contig_data) {

Do you support VLAN double tagging? We normally take into acount to two vlan
headers in these checks.

Other than that:
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> +		dev_warn(priv->net_dev->dev.parent,
> +			 "The maximum MTU for XDP is %d\n",
> +			 max_contig_data - VLAN_ETH_HLEN - ETH_FCS_LEN);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int dpaa_change_mtu(struct net_device *net_dev, int new_mtu)
> +{
> +	struct dpaa_priv *priv = netdev_priv(net_dev);
> +
> +	if (priv->xdp_prog && !xdp_validate_mtu(priv, new_mtu))
> +		return -EINVAL;
> +
> +	net_dev->mtu = new_mtu;
> +	return 0;
> +}
> +
>  static int dpaa_setup_xdp(struct net_device *net_dev, struct bpf_prog *prog)
>  {
>  	struct dpaa_priv *priv = netdev_priv(net_dev);
>  	struct bpf_prog *old_prog;
> -	int err, max_contig_data;
> +	int err;
>  	bool up;
>  
> -	max_contig_data = priv->dpaa_bp->size - priv->rx_headroom;
> -
>  	/* S/G fragments are not supported in XDP-mode */
> -	if (prog &&
> -	    (net_dev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN > max_contig_data)) {
> -		dev_warn(net_dev->dev.parent,
> -			 "The maximum MTU for XDP is %d\n",
> -			 max_contig_data - VLAN_ETH_HLEN - ETH_FCS_LEN);
> +	if (prog && !xdp_validate_mtu(priv, net_dev->mtu))
>  		return -EINVAL;
> -	}
>  
>  	up = netif_running(net_dev);
>  
> @@ -2870,6 +2891,7 @@ static int dpaa_ioctl(struct net_device *net_dev, struct ifreq *rq, int cmd)
>  	.ndo_set_rx_mode = dpaa_set_rx_mode,
>  	.ndo_do_ioctl = dpaa_ioctl,
>  	.ndo_setup_tc = dpaa_setup_tc,
> +	.ndo_change_mtu = dpaa_change_mtu,
>  	.ndo_bpf = dpaa_xdp,
>  };
>  
> -- 
> 1.9.1
> 

RE: [PATCH net-next v4 3/7] dpaa_eth: limit the possible MTU range when XDP is enabled

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Sent: Tuesday, November 24, 2020 21:12
> To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> davem@davemloft.net; Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next v4 3/7] dpaa_eth: limit the possible MTU range
> when XDP is enabled
> 
> On Mon, Nov 23, 2020 at 07:36:21PM +0200, Camelia Groza wrote:
> > Implement the ndo_change_mtu callback to prevent users from setting an
> > MTU that would permit processing of S/G frames. The maximum MTU size
> > is dependent on the buffer size.
> >
> > Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 40
> ++++++++++++++++++++------
> >  1 file changed, 31 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index 8acce62..ee076f4 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -2756,23 +2756,44 @@ static int dpaa_eth_stop(struct net_device
> *net_dev)
> >  	return err;
> >  }
> >
> > +static bool xdp_validate_mtu(struct dpaa_priv *priv, int mtu)
> > +{
> > +	int max_contig_data = priv->dpaa_bp->size - priv->rx_headroom;
> > +
> > +	/* We do not support S/G fragments when XDP is enabled.
> > +	 * Limit the MTU in relation to the buffer size.
> > +	 */
> > +	if (mtu + VLAN_ETH_HLEN + ETH_FCS_LEN > max_contig_data) {
> 
> Do you support VLAN double tagging? We normally take into acount to two
> vlan
> headers in these checks.
> 
> Other than that:
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

No, we only account for one.

> > +		dev_warn(priv->net_dev->dev.parent,
> > +			 "The maximum MTU for XDP is %d\n",
> > +			 max_contig_data - VLAN_ETH_HLEN -
> ETH_FCS_LEN);
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static int dpaa_change_mtu(struct net_device *net_dev, int new_mtu)
> > +{
> > +	struct dpaa_priv *priv = netdev_priv(net_dev);
> > +
> > +	if (priv->xdp_prog && !xdp_validate_mtu(priv, new_mtu))
> > +		return -EINVAL;
> > +
> > +	net_dev->mtu = new_mtu;
> > +	return 0;
> > +}
> > +
> >  static int dpaa_setup_xdp(struct net_device *net_dev, struct bpf_prog
> *prog)
> >  {
> >  	struct dpaa_priv *priv = netdev_priv(net_dev);
> >  	struct bpf_prog *old_prog;
> > -	int err, max_contig_data;
> > +	int err;
> >  	bool up;
> >
> > -	max_contig_data = priv->dpaa_bp->size - priv->rx_headroom;
> > -
> >  	/* S/G fragments are not supported in XDP-mode */
> > -	if (prog &&
> > -	    (net_dev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN >
> max_contig_data)) {
> > -		dev_warn(net_dev->dev.parent,
> > -			 "The maximum MTU for XDP is %d\n",
> > -			 max_contig_data - VLAN_ETH_HLEN -
> ETH_FCS_LEN);
> > +	if (prog && !xdp_validate_mtu(priv, net_dev->mtu))
> >  		return -EINVAL;
> > -	}
> >
> >  	up = netif_running(net_dev);
> >
> > @@ -2870,6 +2891,7 @@ static int dpaa_ioctl(struct net_device *net_dev,
> struct ifreq *rq, int cmd)
> >  	.ndo_set_rx_mode = dpaa_set_rx_mode,
> >  	.ndo_do_ioctl = dpaa_ioctl,
> >  	.ndo_setup_tc = dpaa_setup_tc,
> > +	.ndo_change_mtu = dpaa_change_mtu,
> >  	.ndo_bpf = dpaa_xdp,
> >  };
> >
> > --
> > 1.9.1
> >

[PATCH net-next v4 4/7] dpaa_eth: add XDP_TX support

[Camelia Groza]

Use an xdp_frame structure for managing the frame. Store a backpointer to
the structure at the start of the buffer before enqueueing for cleanup
on TX confirmation. Reserve DPAA_TX_PRIV_DATA_SIZE bytes from the frame
size shared with the XDP program for this purpose. Use the XDP
API for freeing the buffer when it returns to the driver on the TX
confirmation path.

The frame queues are shared with the netstack.

This approach will be reused for XDP REDIRECT.

Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
Changes in v4:
- call xdp_rxq_info_is_reg() before unregistering
- minor cleanups (remove unneeded variable, print error code)
- add more details in the commit message
- did not call qman_destroy_fq() in case of xdp_rxq_info_reg() failure
since it would lead to a double free of the fq resources

 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 128 ++++++++++++++++++++++++-
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |   2 +
 2 files changed, 125 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index ee076f4..0deffcc 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -1130,6 +1130,24 @@ static int dpaa_fq_init(struct dpaa_fq *dpaa_fq, bool td_enable)

 	dpaa_fq->fqid = qman_fq_fqid(fq);

+	if (dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
+	    dpaa_fq->fq_type == FQ_TYPE_RX_PCD) {
+		err = xdp_rxq_info_reg(&dpaa_fq->xdp_rxq, dpaa_fq->net_dev,
+				       dpaa_fq->fqid);
+		if (err) {
+			dev_err(dev, "xdp_rxq_info_reg() = %d\n", err);
+			return err;
+		}
+
+		err = xdp_rxq_info_reg_mem_model(&dpaa_fq->xdp_rxq,
+						 MEM_TYPE_PAGE_ORDER0, NULL);
+		if (err) {
+			dev_err(dev, "xdp_rxq_info_reg_mem_model() = %d\n", err);
+			xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
+			return err;
+		}
+	}
+
 	return 0;
 }

@@ -1159,6 +1177,11 @@ static int dpaa_fq_free_entry(struct device *dev, struct qman_fq *fq)
 		}
 	}

+	if ((dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
+	     dpaa_fq->fq_type == FQ_TYPE_RX_PCD) &&
+	    xdp_rxq_info_is_reg(&dpaa_fq->xdp_rxq))
+		xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
+
 	qman_destroy_fq(fq);
 	list_del(&dpaa_fq->list);

@@ -1625,6 +1648,9 @@ static int dpaa_eth_refill_bpools(struct dpaa_priv *priv)
  *
  * Return the skb backpointer, since for S/G frames the buffer containing it
  * gets freed here.
+ *
+ * No skb backpointer is set when transmitting XDP frames. Cleanup the buffer
+ * and return NULL in this case.
  */
 static struct sk_buff *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
 					  const struct qm_fd *fd, bool ts)
@@ -1664,13 +1690,21 @@ static struct sk_buff *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
 		}
 	} else {
 		dma_unmap_single(priv->tx_dma_dev, addr,
-				 priv->tx_headroom + qm_fd_get_length(fd),
+				 qm_fd_get_offset(fd) + qm_fd_get_length(fd),
 				 dma_dir);
 	}

 	swbp = (struct dpaa_eth_swbp *)vaddr;
 	skb = swbp->skb;

+	/* No skb backpointer is set when running XDP. An xdp_frame
+	 * backpointer is saved instead.
+	 */
+	if (!skb) {
+		xdp_return_frame(swbp->xdpf);
+		return NULL;
+	}
+
 	/* DMA unmapping is required before accessing the HW provided info */
 	if (ts && priv->tx_tstamp &&
 	    skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
@@ -2350,11 +2384,76 @@ static enum qman_cb_dqrr_result rx_error_dqrr(struct qman_portal *portal,
 	return qman_cb_dqrr_consume;
 }

+static int dpaa_xdp_xmit_frame(struct net_device *net_dev,
+			       struct xdp_frame *xdpf)
+{
+	struct dpaa_priv *priv = netdev_priv(net_dev);
+	struct rtnl_link_stats64 *percpu_stats;
+	struct dpaa_percpu_priv *percpu_priv;
+	struct dpaa_eth_swbp *swbp;
+	struct netdev_queue *txq;
+	void *buff_start;
+	struct qm_fd fd;
+	dma_addr_t addr;
+	int err;
+
+	percpu_priv = this_cpu_ptr(priv->percpu_priv);
+	percpu_stats = &percpu_priv->stats;
+
+	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
+		err = -EINVAL;
+		goto out_error;
+	}
+
+	buff_start = xdpf->data - xdpf->headroom;
+
+	/* Leave empty the skb backpointer at the start of the buffer.
+	 * Save the XDP frame for easy cleanup on confirmation.
+	 */
+	swbp = (struct dpaa_eth_swbp *)buff_start;
+	swbp->skb = NULL;
+	swbp->xdpf = xdpf;
+
+	qm_fd_clear_fd(&fd);
+	fd.bpid = FSL_DPAA_BPID_INV;
+	fd.cmd |= cpu_to_be32(FM_FD_CMD_FCO);
+	qm_fd_set_contig(&fd, xdpf->headroom, xdpf->len);
+
+	addr = dma_map_single(priv->tx_dma_dev, buff_start,
+			      xdpf->headroom + xdpf->len,
+			      DMA_TO_DEVICE);
+	if (unlikely(dma_mapping_error(priv->tx_dma_dev, addr))) {
+		err = -EINVAL;
+		goto out_error;
+	}
+
+	qm_fd_addr_set64(&fd, addr);
+
+	/* Bump the trans_start */
+	txq = netdev_get_tx_queue(net_dev, smp_processor_id());
+	txq->trans_start = jiffies;
+
+	err = dpaa_xmit(priv, percpu_stats, smp_processor_id(), &fd);
+	if (err) {
+		dma_unmap_single(priv->tx_dma_dev, addr,
+				 qm_fd_get_offset(&fd) + qm_fd_get_length(&fd),
+				 DMA_TO_DEVICE);
+		goto out_error;
+	}
+
+	return 0;
+
+out_error:
+	percpu_stats->tx_errors++;
+	return err;
+}
+
 static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
-			unsigned int *xdp_meta_len)
+			struct dpaa_fq *dpaa_fq, unsigned int *xdp_meta_len)
 {
 	ssize_t fd_off = qm_fd_get_offset(fd);
 	struct bpf_prog *xdp_prog;
+	struct xdp_frame *xdpf;
 	struct xdp_buff xdp;
 	u32 xdp_act;

@@ -2370,7 +2469,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
 	xdp.data_meta = xdp.data;
 	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
 	xdp.data_end = xdp.data + qm_fd_get_length(fd);
-	xdp.frame_sz = DPAA_BP_RAW_SIZE;
+	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
+	xdp.rxq = &dpaa_fq->xdp_rxq;

 	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);

@@ -2381,6 +2481,22 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
 	case XDP_PASS:
 		*xdp_meta_len = xdp.data - xdp.data_meta;
 		break;
+	case XDP_TX:
+		/* We can access the full headroom when sending the frame
+		 * back out
+		 */
+		xdp.data_hard_start = vaddr;
+		xdp.frame_sz = DPAA_BP_RAW_SIZE;
+		xdpf = xdp_convert_buff_to_frame(&xdp);
+		if (unlikely(!xdpf)) {
+			free_pages((unsigned long)vaddr, 0);
+			break;
+		}
+
+		if (dpaa_xdp_xmit_frame(priv->net_dev, xdpf))
+			xdp_return_frame_rx_napi(xdpf);
+
+		break;
 	default:
 		bpf_warn_invalid_xdp_action(xdp_act);
 		fallthrough;
@@ -2415,6 +2531,7 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
 	u32 fd_status, hash_offset;
 	struct qm_sg_entry *sgt;
 	struct dpaa_bp *dpaa_bp;
+	struct dpaa_fq *dpaa_fq;
 	struct dpaa_priv *priv;
 	struct sk_buff *skb;
 	int *count_ptr;
@@ -2423,9 +2540,10 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
 	u32 hash;
 	u64 ns;

+	dpaa_fq = container_of(fq, struct dpaa_fq, fq_base);
 	fd_status = be32_to_cpu(fd->status);
 	fd_format = qm_fd_get_format(fd);
-	net_dev = ((struct dpaa_fq *)fq)->net_dev;
+	net_dev = dpaa_fq->net_dev;
 	priv = netdev_priv(net_dev);
 	dpaa_bp = dpaa_bpid2pool(dq->fd.bpid);
 	if (!dpaa_bp)
@@ -2494,7 +2612,7 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,

 	if (likely(fd_format == qm_fd_contig)) {
 		xdp_act = dpaa_run_xdp(priv, (struct qm_fd *)fd, vaddr,
-				       &xdp_meta_len);
+				       dpaa_fq, &xdp_meta_len);
 		if (xdp_act != XDP_PASS) {
 			percpu_stats->rx_packets++;
 			percpu_stats->rx_bytes += qm_fd_get_length(fd);
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
index 94e8613..5c8d52a 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
@@ -68,6 +68,7 @@ struct dpaa_fq {
 	u16 channel;
 	u8 wq;
 	enum dpaa_fq_type fq_type;
+	struct xdp_rxq_info xdp_rxq;
 };

 struct dpaa_fq_cbs {
@@ -150,6 +151,7 @@ struct dpaa_buffer_layout {
  */
 struct dpaa_eth_swbp {
 	struct sk_buff *skb;
+	struct xdp_frame *xdpf;
 };

 struct dpaa_priv {
--
1.9.1

Re: [PATCH net-next v4 4/7] dpaa_eth: add XDP_TX support

[Maciej Fijalkowski]

On Mon, Nov 23, 2020 at 07:36:22PM +0200, Camelia Groza wrote:
> Use an xdp_frame structure for managing the frame. Store a backpointer to
> the structure at the start of the buffer before enqueueing for cleanup
> on TX confirmation. Reserve DPAA_TX_PRIV_DATA_SIZE bytes from the frame
> size shared with the XDP program for this purpose. Use the XDP
> API for freeing the buffer when it returns to the driver on the TX
> confirmation path.
> 
> The frame queues are shared with the netstack.

Can you also provide the info from cover letter about locklessness (is
that even a word?) in here?

One question below and:

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> 
> This approach will be reused for XDP REDIRECT.
> 
> Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> ---
> Changes in v4:
> - call xdp_rxq_info_is_reg() before unregistering
> - minor cleanups (remove unneeded variable, print error code)
> - add more details in the commit message
> - did not call qman_destroy_fq() in case of xdp_rxq_info_reg() failure
> since it would lead to a double free of the fq resources
> 
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 128 ++++++++++++++++++++++++-
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |   2 +
>  2 files changed, 125 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index ee076f4..0deffcc 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -1130,6 +1130,24 @@ static int dpaa_fq_init(struct dpaa_fq *dpaa_fq, bool td_enable)
> 
>  	dpaa_fq->fqid = qman_fq_fqid(fq);
> 
> +	if (dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
> +	    dpaa_fq->fq_type == FQ_TYPE_RX_PCD) {
> +		err = xdp_rxq_info_reg(&dpaa_fq->xdp_rxq, dpaa_fq->net_dev,
> +				       dpaa_fq->fqid);
> +		if (err) {
> +			dev_err(dev, "xdp_rxq_info_reg() = %d\n", err);
> +			return err;
> +		}
> +
> +		err = xdp_rxq_info_reg_mem_model(&dpaa_fq->xdp_rxq,
> +						 MEM_TYPE_PAGE_ORDER0, NULL);
> +		if (err) {
> +			dev_err(dev, "xdp_rxq_info_reg_mem_model() = %d\n", err);
> +			xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
> +			return err;
> +		}
> +	}
> +
>  	return 0;
>  }
> 
> @@ -1159,6 +1177,11 @@ static int dpaa_fq_free_entry(struct device *dev, struct qman_fq *fq)
>  		}
>  	}
> 
> +	if ((dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
> +	     dpaa_fq->fq_type == FQ_TYPE_RX_PCD) &&
> +	    xdp_rxq_info_is_reg(&dpaa_fq->xdp_rxq))
> +		xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
> +
>  	qman_destroy_fq(fq);
>  	list_del(&dpaa_fq->list);
> 
> @@ -1625,6 +1648,9 @@ static int dpaa_eth_refill_bpools(struct dpaa_priv *priv)
>   *
>   * Return the skb backpointer, since for S/G frames the buffer containing it
>   * gets freed here.
> + *
> + * No skb backpointer is set when transmitting XDP frames. Cleanup the buffer
> + * and return NULL in this case.
>   */
>  static struct sk_buff *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
>  					  const struct qm_fd *fd, bool ts)
> @@ -1664,13 +1690,21 @@ static struct sk_buff *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
>  		}
>  	} else {
>  		dma_unmap_single(priv->tx_dma_dev, addr,
> -				 priv->tx_headroom + qm_fd_get_length(fd),
> +				 qm_fd_get_offset(fd) + qm_fd_get_length(fd),
>  				 dma_dir);
>  	}
> 
>  	swbp = (struct dpaa_eth_swbp *)vaddr;
>  	skb = swbp->skb;
> 
> +	/* No skb backpointer is set when running XDP. An xdp_frame
> +	 * backpointer is saved instead.
> +	 */
> +	if (!skb) {
> +		xdp_return_frame(swbp->xdpf);
> +		return NULL;
> +	}
> +
>  	/* DMA unmapping is required before accessing the HW provided info */
>  	if (ts && priv->tx_tstamp &&
>  	    skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> @@ -2350,11 +2384,76 @@ static enum qman_cb_dqrr_result rx_error_dqrr(struct qman_portal *portal,
>  	return qman_cb_dqrr_consume;
>  }
> 
> +static int dpaa_xdp_xmit_frame(struct net_device *net_dev,
> +			       struct xdp_frame *xdpf)
> +{
> +	struct dpaa_priv *priv = netdev_priv(net_dev);
> +	struct rtnl_link_stats64 *percpu_stats;
> +	struct dpaa_percpu_priv *percpu_priv;
> +	struct dpaa_eth_swbp *swbp;
> +	struct netdev_queue *txq;
> +	void *buff_start;
> +	struct qm_fd fd;
> +	dma_addr_t addr;
> +	int err;
> +
> +	percpu_priv = this_cpu_ptr(priv->percpu_priv);
> +	percpu_stats = &percpu_priv->stats;
> +
> +	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
> +		err = -EINVAL;
> +		goto out_error;
> +	}
> +
> +	buff_start = xdpf->data - xdpf->headroom;
> +
> +	/* Leave empty the skb backpointer at the start of the buffer.
> +	 * Save the XDP frame for easy cleanup on confirmation.
> +	 */
> +	swbp = (struct dpaa_eth_swbp *)buff_start;
> +	swbp->skb = NULL;
> +	swbp->xdpf = xdpf;
> +
> +	qm_fd_clear_fd(&fd);
> +	fd.bpid = FSL_DPAA_BPID_INV;
> +	fd.cmd |= cpu_to_be32(FM_FD_CMD_FCO);
> +	qm_fd_set_contig(&fd, xdpf->headroom, xdpf->len);
> +
> +	addr = dma_map_single(priv->tx_dma_dev, buff_start,
> +			      xdpf->headroom + xdpf->len,
> +			      DMA_TO_DEVICE);

Not sure if I asked that.  What is the purpose for including the headroom
in frame being set? I would expect to take into account only frame from
xdpf->data.

> +	if (unlikely(dma_mapping_error(priv->tx_dma_dev, addr))) {
> +		err = -EINVAL;
> +		goto out_error;
> +	}
> +
> +	qm_fd_addr_set64(&fd, addr);
> +
> +	/* Bump the trans_start */
> +	txq = netdev_get_tx_queue(net_dev, smp_processor_id());
> +	txq->trans_start = jiffies;
> +
> +	err = dpaa_xmit(priv, percpu_stats, smp_processor_id(), &fd);
> +	if (err) {
> +		dma_unmap_single(priv->tx_dma_dev, addr,
> +				 qm_fd_get_offset(&fd) + qm_fd_get_length(&fd),
> +				 DMA_TO_DEVICE);
> +		goto out_error;
> +	}
> +
> +	return 0;
> +
> +out_error:
> +	percpu_stats->tx_errors++;
> +	return err;
> +}
> +
>  static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
> -			unsigned int *xdp_meta_len)
> +			struct dpaa_fq *dpaa_fq, unsigned int *xdp_meta_len)
>  {
>  	ssize_t fd_off = qm_fd_get_offset(fd);
>  	struct bpf_prog *xdp_prog;
> +	struct xdp_frame *xdpf;
>  	struct xdp_buff xdp;
>  	u32 xdp_act;
> 
> @@ -2370,7 +2469,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
>  	xdp.data_meta = xdp.data;
>  	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
>  	xdp.data_end = xdp.data + qm_fd_get_length(fd);
> -	xdp.frame_sz = DPAA_BP_RAW_SIZE;
> +	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
> +	xdp.rxq = &dpaa_fq->xdp_rxq;
> 
>  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> 
> @@ -2381,6 +2481,22 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
>  	case XDP_PASS:
>  		*xdp_meta_len = xdp.data - xdp.data_meta;
>  		break;
> +	case XDP_TX:
> +		/* We can access the full headroom when sending the frame
> +		 * back out
> +		 */
> +		xdp.data_hard_start = vaddr;
> +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> +		xdpf = xdp_convert_buff_to_frame(&xdp);
> +		if (unlikely(!xdpf)) {
> +			free_pages((unsigned long)vaddr, 0);
> +			break;
> +		}
> +
> +		if (dpaa_xdp_xmit_frame(priv->net_dev, xdpf))
> +			xdp_return_frame_rx_napi(xdpf);
> +
> +		break;
>  	default:
>  		bpf_warn_invalid_xdp_action(xdp_act);
>  		fallthrough;
> @@ -2415,6 +2531,7 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
>  	u32 fd_status, hash_offset;
>  	struct qm_sg_entry *sgt;
>  	struct dpaa_bp *dpaa_bp;
> +	struct dpaa_fq *dpaa_fq;
>  	struct dpaa_priv *priv;
>  	struct sk_buff *skb;
>  	int *count_ptr;
> @@ -2423,9 +2540,10 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
>  	u32 hash;
>  	u64 ns;
> 
> +	dpaa_fq = container_of(fq, struct dpaa_fq, fq_base);
>  	fd_status = be32_to_cpu(fd->status);
>  	fd_format = qm_fd_get_format(fd);
> -	net_dev = ((struct dpaa_fq *)fq)->net_dev;
> +	net_dev = dpaa_fq->net_dev;
>  	priv = netdev_priv(net_dev);
>  	dpaa_bp = dpaa_bpid2pool(dq->fd.bpid);
>  	if (!dpaa_bp)
> @@ -2494,7 +2612,7 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
> 
>  	if (likely(fd_format == qm_fd_contig)) {
>  		xdp_act = dpaa_run_xdp(priv, (struct qm_fd *)fd, vaddr,
> -				       &xdp_meta_len);
> +				       dpaa_fq, &xdp_meta_len);
>  		if (xdp_act != XDP_PASS) {
>  			percpu_stats->rx_packets++;
>  			percpu_stats->rx_bytes += qm_fd_get_length(fd);
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> index 94e8613..5c8d52a 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> @@ -68,6 +68,7 @@ struct dpaa_fq {
>  	u16 channel;
>  	u8 wq;
>  	enum dpaa_fq_type fq_type;
> +	struct xdp_rxq_info xdp_rxq;
>  };
> 
>  struct dpaa_fq_cbs {
> @@ -150,6 +151,7 @@ struct dpaa_buffer_layout {
>   */
>  struct dpaa_eth_swbp {
>  	struct sk_buff *skb;
> +	struct xdp_frame *xdpf;
>  };
> 
>  struct dpaa_priv {
> --
> 1.9.1
> 

RE: [PATCH net-next v4 4/7] dpaa_eth: add XDP_TX support

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Sent: Tuesday, November 24, 2020 21:52
> To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> davem@davemloft.net; Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next v4 4/7] dpaa_eth: add XDP_TX support
> 
> On Mon, Nov 23, 2020 at 07:36:22PM +0200, Camelia Groza wrote:
> > Use an xdp_frame structure for managing the frame. Store a backpointer
> to
> > the structure at the start of the buffer before enqueueing for cleanup
> > on TX confirmation. Reserve DPAA_TX_PRIV_DATA_SIZE bytes from the
> frame
> > size shared with the XDP program for this purpose. Use the XDP
> > API for freeing the buffer when it returns to the driver on the TX
> > confirmation path.
> >
> > The frame queues are shared with the netstack.
> 
> Can you also provide the info from cover letter about locklessness (is
> that even a word?) in here?

Sure.

> One question below and:
> 
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> 
> >
> > This approach will be reused for XDP REDIRECT.
> >
> > Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > ---
> > Changes in v4:
> > - call xdp_rxq_info_is_reg() before unregistering
> > - minor cleanups (remove unneeded variable, print error code)
> > - add more details in the commit message
> > - did not call qman_destroy_fq() in case of xdp_rxq_info_reg() failure
> > since it would lead to a double free of the fq resources
> >
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 128
> ++++++++++++++++++++++++-
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |   2 +
> >  2 files changed, 125 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index ee076f4..0deffcc 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -1130,6 +1130,24 @@ static int dpaa_fq_init(struct dpaa_fq *dpaa_fq,
> bool td_enable)
> >
> >  	dpaa_fq->fqid = qman_fq_fqid(fq);
> >
> > +	if (dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
> > +	    dpaa_fq->fq_type == FQ_TYPE_RX_PCD) {
> > +		err = xdp_rxq_info_reg(&dpaa_fq->xdp_rxq, dpaa_fq-
> >net_dev,
> > +				       dpaa_fq->fqid);
> > +		if (err) {
> > +			dev_err(dev, "xdp_rxq_info_reg() = %d\n", err);
> > +			return err;
> > +		}
> > +
> > +		err = xdp_rxq_info_reg_mem_model(&dpaa_fq->xdp_rxq,
> > +						 MEM_TYPE_PAGE_ORDER0,
> NULL);
> > +		if (err) {
> > +			dev_err(dev, "xdp_rxq_info_reg_mem_model() =
> %d\n", err);
> > +			xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
> > +			return err;
> > +		}
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > @@ -1159,6 +1177,11 @@ static int dpaa_fq_free_entry(struct device
> *dev, struct qman_fq *fq)
> >  		}
> >  	}
> >
> > +	if ((dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
> > +	     dpaa_fq->fq_type == FQ_TYPE_RX_PCD) &&
> > +	    xdp_rxq_info_is_reg(&dpaa_fq->xdp_rxq))
> > +		xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
> > +
> >  	qman_destroy_fq(fq);
> >  	list_del(&dpaa_fq->list);
> >
> > @@ -1625,6 +1648,9 @@ static int dpaa_eth_refill_bpools(struct dpaa_priv
> *priv)
> >   *
> >   * Return the skb backpointer, since for S/G frames the buffer containing it
> >   * gets freed here.
> > + *
> > + * No skb backpointer is set when transmitting XDP frames. Cleanup the
> buffer
> > + * and return NULL in this case.
> >   */
> >  static struct sk_buff *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
> >  					  const struct qm_fd *fd, bool ts)
> > @@ -1664,13 +1690,21 @@ static struct sk_buff
> *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
> >  		}
> >  	} else {
> >  		dma_unmap_single(priv->tx_dma_dev, addr,
> > -				 priv->tx_headroom +
> qm_fd_get_length(fd),
> > +				 qm_fd_get_offset(fd) +
> qm_fd_get_length(fd),
> >  				 dma_dir);
> >  	}
> >
> >  	swbp = (struct dpaa_eth_swbp *)vaddr;
> >  	skb = swbp->skb;
> >
> > +	/* No skb backpointer is set when running XDP. An xdp_frame
> > +	 * backpointer is saved instead.
> > +	 */
> > +	if (!skb) {
> > +		xdp_return_frame(swbp->xdpf);
> > +		return NULL;
> > +	}
> > +
> >  	/* DMA unmapping is required before accessing the HW provided
> info */
> >  	if (ts && priv->tx_tstamp &&
> >  	    skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> > @@ -2350,11 +2384,76 @@ static enum qman_cb_dqrr_result
> rx_error_dqrr(struct qman_portal *portal,
> >  	return qman_cb_dqrr_consume;
> >  }
> >
> > +static int dpaa_xdp_xmit_frame(struct net_device *net_dev,
> > +			       struct xdp_frame *xdpf)
> > +{
> > +	struct dpaa_priv *priv = netdev_priv(net_dev);
> > +	struct rtnl_link_stats64 *percpu_stats;
> > +	struct dpaa_percpu_priv *percpu_priv;
> > +	struct dpaa_eth_swbp *swbp;
> > +	struct netdev_queue *txq;
> > +	void *buff_start;
> > +	struct qm_fd fd;
> > +	dma_addr_t addr;
> > +	int err;
> > +
> > +	percpu_priv = this_cpu_ptr(priv->percpu_priv);
> > +	percpu_stats = &percpu_priv->stats;
> > +
> > +	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
> > +		err = -EINVAL;
> > +		goto out_error;
> > +	}
> > +
> > +	buff_start = xdpf->data - xdpf->headroom;
> > +
> > +	/* Leave empty the skb backpointer at the start of the buffer.
> > +	 * Save the XDP frame for easy cleanup on confirmation.
> > +	 */
> > +	swbp = (struct dpaa_eth_swbp *)buff_start;
> > +	swbp->skb = NULL;
> > +	swbp->xdpf = xdpf;
> > +
> > +	qm_fd_clear_fd(&fd);
> > +	fd.bpid = FSL_DPAA_BPID_INV;
> > +	fd.cmd |= cpu_to_be32(FM_FD_CMD_FCO);
> > +	qm_fd_set_contig(&fd, xdpf->headroom, xdpf->len);
> > +
> > +	addr = dma_map_single(priv->tx_dma_dev, buff_start,
> > +			      xdpf->headroom + xdpf->len,
> > +			      DMA_TO_DEVICE);
> 
> Not sure if I asked that.  What is the purpose for including the headroom
> in frame being set? I would expect to take into account only frame from
> xdpf->data.

The xdpf headroom becomes the fd's offset, the area before the data where the backpointers for cleanup are stored. This area isn't sent out with the frame.

> > +	if (unlikely(dma_mapping_error(priv->tx_dma_dev, addr))) {
> > +		err = -EINVAL;
> > +		goto out_error;
> > +	}
> > +
> > +	qm_fd_addr_set64(&fd, addr);
> > +
> > +	/* Bump the trans_start */
> > +	txq = netdev_get_tx_queue(net_dev, smp_processor_id());
> > +	txq->trans_start = jiffies;
> > +
> > +	err = dpaa_xmit(priv, percpu_stats, smp_processor_id(), &fd);
> > +	if (err) {
> > +		dma_unmap_single(priv->tx_dma_dev, addr,
> > +				 qm_fd_get_offset(&fd) +
> qm_fd_get_length(&fd),
> > +				 DMA_TO_DEVICE);
> > +		goto out_error;
> > +	}
> > +
> > +	return 0;
> > +
> > +out_error:
> > +	percpu_stats->tx_errors++;
> > +	return err;
> > +}
> > +
> >  static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void
> *vaddr,
> > -			unsigned int *xdp_meta_len)
> > +			struct dpaa_fq *dpaa_fq, unsigned int
> *xdp_meta_len)
> >  {
> >  	ssize_t fd_off = qm_fd_get_offset(fd);
> >  	struct bpf_prog *xdp_prog;
> > +	struct xdp_frame *xdpf;
> >  	struct xdp_buff xdp;
> >  	u32 xdp_act;
> >
> > @@ -2370,7 +2469,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv,
> struct qm_fd *fd, void *vaddr,
> >  	xdp.data_meta = xdp.data;
> >  	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
> >  	xdp.data_end = xdp.data + qm_fd_get_length(fd);
> > -	xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > +	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
> > +	xdp.rxq = &dpaa_fq->xdp_rxq;
> >
> >  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> >
> > @@ -2381,6 +2481,22 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv,
> struct qm_fd *fd, void *vaddr,
> >  	case XDP_PASS:
> >  		*xdp_meta_len = xdp.data - xdp.data_meta;
> >  		break;
> > +	case XDP_TX:
> > +		/* We can access the full headroom when sending the frame
> > +		 * back out
> > +		 */
> > +		xdp.data_hard_start = vaddr;
> > +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > +		xdpf = xdp_convert_buff_to_frame(&xdp);
> > +		if (unlikely(!xdpf)) {
> > +			free_pages((unsigned long)vaddr, 0);
> > +			break;
> > +		}
> > +
> > +		if (dpaa_xdp_xmit_frame(priv->net_dev, xdpf))
> > +			xdp_return_frame_rx_napi(xdpf);
> > +
> > +		break;
> >  	default:
> >  		bpf_warn_invalid_xdp_action(xdp_act);
> >  		fallthrough;
> > @@ -2415,6 +2531,7 @@ static enum qman_cb_dqrr_result
> rx_default_dqrr(struct qman_portal *portal,
> >  	u32 fd_status, hash_offset;
> >  	struct qm_sg_entry *sgt;
> >  	struct dpaa_bp *dpaa_bp;
> > +	struct dpaa_fq *dpaa_fq;
> >  	struct dpaa_priv *priv;
> >  	struct sk_buff *skb;
> >  	int *count_ptr;
> > @@ -2423,9 +2540,10 @@ static enum qman_cb_dqrr_result
> rx_default_dqrr(struct qman_portal *portal,
> >  	u32 hash;
> >  	u64 ns;
> >
> > +	dpaa_fq = container_of(fq, struct dpaa_fq, fq_base);
> >  	fd_status = be32_to_cpu(fd->status);
> >  	fd_format = qm_fd_get_format(fd);
> > -	net_dev = ((struct dpaa_fq *)fq)->net_dev;
> > +	net_dev = dpaa_fq->net_dev;
> >  	priv = netdev_priv(net_dev);
> >  	dpaa_bp = dpaa_bpid2pool(dq->fd.bpid);
> >  	if (!dpaa_bp)
> > @@ -2494,7 +2612,7 @@ static enum qman_cb_dqrr_result
> rx_default_dqrr(struct qman_portal *portal,
> >
> >  	if (likely(fd_format == qm_fd_contig)) {
> >  		xdp_act = dpaa_run_xdp(priv, (struct qm_fd *)fd, vaddr,
> > -				       &xdp_meta_len);
> > +				       dpaa_fq, &xdp_meta_len);
> >  		if (xdp_act != XDP_PASS) {
> >  			percpu_stats->rx_packets++;
> >  			percpu_stats->rx_bytes += qm_fd_get_length(fd);
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > index 94e8613..5c8d52a 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > @@ -68,6 +68,7 @@ struct dpaa_fq {
> >  	u16 channel;
> >  	u8 wq;
> >  	enum dpaa_fq_type fq_type;
> > +	struct xdp_rxq_info xdp_rxq;
> >  };
> >
> >  struct dpaa_fq_cbs {
> > @@ -150,6 +151,7 @@ struct dpaa_buffer_layout {
> >   */
> >  struct dpaa_eth_swbp {
> >  	struct sk_buff *skb;
> > +	struct xdp_frame *xdpf;
> >  };
> >
> >  struct dpaa_priv {
> > --
> > 1.9.1
> >

Re: [PATCH net-next v4 4/7] dpaa_eth: add XDP_TX support

[Maciej Fijalkowski]

On Wed, Nov 25, 2020 at 03:49:45PM +0000, Camelia Alexandra Groza wrote:
> > -----Original Message-----
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Sent: Tuesday, November 24, 2020 21:52
> > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> > davem@davemloft.net; Madalin Bucur (OSS)
> > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> > netdev@vger.kernel.org
> > Subject: Re: [PATCH net-next v4 4/7] dpaa_eth: add XDP_TX support
> > 
> > On Mon, Nov 23, 2020 at 07:36:22PM +0200, Camelia Groza wrote:
> > > Use an xdp_frame structure for managing the frame. Store a backpointer
> > to
> > > the structure at the start of the buffer before enqueueing for cleanup
> > > on TX confirmation. Reserve DPAA_TX_PRIV_DATA_SIZE bytes from the
> > frame
> > > size shared with the XDP program for this purpose. Use the XDP
> > > API for freeing the buffer when it returns to the driver on the TX
> > > confirmation path.
> > >
> > > The frame queues are shared with the netstack.
> > 
> > Can you also provide the info from cover letter about locklessness (is
> > that even a word?) in here?
> 
> Sure.
> 
> > One question below and:
> > 
> > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > 
> > >
> > > This approach will be reused for XDP REDIRECT.
> > >
> > > Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > > ---
> > > Changes in v4:
> > > - call xdp_rxq_info_is_reg() before unregistering
> > > - minor cleanups (remove unneeded variable, print error code)
> > > - add more details in the commit message
> > > - did not call qman_destroy_fq() in case of xdp_rxq_info_reg() failure
> > > since it would lead to a double free of the fq resources
> > >
> > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 128
> > ++++++++++++++++++++++++-
> > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |   2 +
> > >  2 files changed, 125 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > index ee076f4..0deffcc 100644
> > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > @@ -1130,6 +1130,24 @@ static int dpaa_fq_init(struct dpaa_fq *dpaa_fq,
> > bool td_enable)
> > >
> > >  	dpaa_fq->fqid = qman_fq_fqid(fq);
> > >
> > > +	if (dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
> > > +	    dpaa_fq->fq_type == FQ_TYPE_RX_PCD) {
> > > +		err = xdp_rxq_info_reg(&dpaa_fq->xdp_rxq, dpaa_fq-
> > >net_dev,
> > > +				       dpaa_fq->fqid);
> > > +		if (err) {
> > > +			dev_err(dev, "xdp_rxq_info_reg() = %d\n", err);
> > > +			return err;
> > > +		}
> > > +
> > > +		err = xdp_rxq_info_reg_mem_model(&dpaa_fq->xdp_rxq,
> > > +						 MEM_TYPE_PAGE_ORDER0,
> > NULL);
> > > +		if (err) {
> > > +			dev_err(dev, "xdp_rxq_info_reg_mem_model() =
> > %d\n", err);
> > > +			xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
> > > +			return err;
> > > +		}
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -1159,6 +1177,11 @@ static int dpaa_fq_free_entry(struct device
> > *dev, struct qman_fq *fq)
> > >  		}
> > >  	}
> > >
> > > +	if ((dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
> > > +	     dpaa_fq->fq_type == FQ_TYPE_RX_PCD) &&
> > > +	    xdp_rxq_info_is_reg(&dpaa_fq->xdp_rxq))
> > > +		xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
> > > +
> > >  	qman_destroy_fq(fq);
> > >  	list_del(&dpaa_fq->list);
> > >
> > > @@ -1625,6 +1648,9 @@ static int dpaa_eth_refill_bpools(struct dpaa_priv
> > *priv)
> > >   *
> > >   * Return the skb backpointer, since for S/G frames the buffer containing it
> > >   * gets freed here.
> > > + *
> > > + * No skb backpointer is set when transmitting XDP frames. Cleanup the
> > buffer
> > > + * and return NULL in this case.
> > >   */
> > >  static struct sk_buff *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
> > >  					  const struct qm_fd *fd, bool ts)
> > > @@ -1664,13 +1690,21 @@ static struct sk_buff
> > *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
> > >  		}
> > >  	} else {
> > >  		dma_unmap_single(priv->tx_dma_dev, addr,
> > > -				 priv->tx_headroom +
> > qm_fd_get_length(fd),
> > > +				 qm_fd_get_offset(fd) +
> > qm_fd_get_length(fd),
> > >  				 dma_dir);
> > >  	}
> > >
> > >  	swbp = (struct dpaa_eth_swbp *)vaddr;
> > >  	skb = swbp->skb;
> > >
> > > +	/* No skb backpointer is set when running XDP. An xdp_frame
> > > +	 * backpointer is saved instead.
> > > +	 */
> > > +	if (!skb) {
> > > +		xdp_return_frame(swbp->xdpf);
> > > +		return NULL;
> > > +	}
> > > +
> > >  	/* DMA unmapping is required before accessing the HW provided
> > info */
> > >  	if (ts && priv->tx_tstamp &&
> > >  	    skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> > > @@ -2350,11 +2384,76 @@ static enum qman_cb_dqrr_result
> > rx_error_dqrr(struct qman_portal *portal,
> > >  	return qman_cb_dqrr_consume;
> > >  }
> > >
> > > +static int dpaa_xdp_xmit_frame(struct net_device *net_dev,
> > > +			       struct xdp_frame *xdpf)
> > > +{
> > > +	struct dpaa_priv *priv = netdev_priv(net_dev);
> > > +	struct rtnl_link_stats64 *percpu_stats;
> > > +	struct dpaa_percpu_priv *percpu_priv;
> > > +	struct dpaa_eth_swbp *swbp;
> > > +	struct netdev_queue *txq;
> > > +	void *buff_start;
> > > +	struct qm_fd fd;
> > > +	dma_addr_t addr;
> > > +	int err;
> > > +
> > > +	percpu_priv = this_cpu_ptr(priv->percpu_priv);
> > > +	percpu_stats = &percpu_priv->stats;
> > > +
> > > +	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
> > > +		err = -EINVAL;
> > > +		goto out_error;
> > > +	}
> > > +
> > > +	buff_start = xdpf->data - xdpf->headroom;
> > > +
> > > +	/* Leave empty the skb backpointer at the start of the buffer.
> > > +	 * Save the XDP frame for easy cleanup on confirmation.
> > > +	 */
> > > +	swbp = (struct dpaa_eth_swbp *)buff_start;
> > > +	swbp->skb = NULL;
> > > +	swbp->xdpf = xdpf;
> > > +
> > > +	qm_fd_clear_fd(&fd);
> > > +	fd.bpid = FSL_DPAA_BPID_INV;
> > > +	fd.cmd |= cpu_to_be32(FM_FD_CMD_FCO);
> > > +	qm_fd_set_contig(&fd, xdpf->headroom, xdpf->len);
> > > +
> > > +	addr = dma_map_single(priv->tx_dma_dev, buff_start,
> > > +			      xdpf->headroom + xdpf->len,
> > > +			      DMA_TO_DEVICE);
> > 
> > Not sure if I asked that.  What is the purpose for including the headroom
> > in frame being set? I would expect to take into account only frame from
> > xdpf->data.
> 
> The xdpf headroom becomes the fd's offset, the area before the data where the backpointers for cleanup are stored. This area isn't sent out with the frame.

But if I'm reading this right you clearly include the headroom space in
the DMA region that you are mapping. Why this couldn't be:

	addr = dma_map_single(priv->tx_dma_dev, xdpf->data,
			      xdpf->len, DMA_TO_DEVICE);

And then frame descriptor wouldn't need to have the offset at all?
Probably that's implementation details, but this way it seems
cleaner/easier to follow to me.

> 
> > > +	if (unlikely(dma_mapping_error(priv->tx_dma_dev, addr))) {
> > > +		err = -EINVAL;
> > > +		goto out_error;
> > > +	}
> > > +
> > > +	qm_fd_addr_set64(&fd, addr);
> > > +
> > > +	/* Bump the trans_start */
> > > +	txq = netdev_get_tx_queue(net_dev, smp_processor_id());
> > > +	txq->trans_start = jiffies;
> > > +
> > > +	err = dpaa_xmit(priv, percpu_stats, smp_processor_id(), &fd);
> > > +	if (err) {
> > > +		dma_unmap_single(priv->tx_dma_dev, addr,
> > > +				 qm_fd_get_offset(&fd) +
> > qm_fd_get_length(&fd),
> > > +				 DMA_TO_DEVICE);
> > > +		goto out_error;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +out_error:
> > > +	percpu_stats->tx_errors++;
> > > +	return err;
> > > +}
> > > +
> > >  static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void
> > *vaddr,
> > > -			unsigned int *xdp_meta_len)
> > > +			struct dpaa_fq *dpaa_fq, unsigned int
> > *xdp_meta_len)
> > >  {
> > >  	ssize_t fd_off = qm_fd_get_offset(fd);
> > >  	struct bpf_prog *xdp_prog;
> > > +	struct xdp_frame *xdpf;
> > >  	struct xdp_buff xdp;
> > >  	u32 xdp_act;
> > >
> > > @@ -2370,7 +2469,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv,
> > struct qm_fd *fd, void *vaddr,
> > >  	xdp.data_meta = xdp.data;
> > >  	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
> > >  	xdp.data_end = xdp.data + qm_fd_get_length(fd);
> > > -	xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > > +	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
> > > +	xdp.rxq = &dpaa_fq->xdp_rxq;
> > >
> > >  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > >
> > > @@ -2381,6 +2481,22 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv,
> > struct qm_fd *fd, void *vaddr,
> > >  	case XDP_PASS:
> > >  		*xdp_meta_len = xdp.data - xdp.data_meta;
> > >  		break;
> > > +	case XDP_TX:
> > > +		/* We can access the full headroom when sending the frame
> > > +		 * back out
> > > +		 */
> > > +		xdp.data_hard_start = vaddr;
> > > +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > > +		xdpf = xdp_convert_buff_to_frame(&xdp);
> > > +		if (unlikely(!xdpf)) {
> > > +			free_pages((unsigned long)vaddr, 0);
> > > +			break;
> > > +		}
> > > +
> > > +		if (dpaa_xdp_xmit_frame(priv->net_dev, xdpf))
> > > +			xdp_return_frame_rx_napi(xdpf);
> > > +
> > > +		break;
> > >  	default:
> > >  		bpf_warn_invalid_xdp_action(xdp_act);
> > >  		fallthrough;
> > > @@ -2415,6 +2531,7 @@ static enum qman_cb_dqrr_result
> > rx_default_dqrr(struct qman_portal *portal,
> > >  	u32 fd_status, hash_offset;
> > >  	struct qm_sg_entry *sgt;
> > >  	struct dpaa_bp *dpaa_bp;
> > > +	struct dpaa_fq *dpaa_fq;
> > >  	struct dpaa_priv *priv;
> > >  	struct sk_buff *skb;
> > >  	int *count_ptr;
> > > @@ -2423,9 +2540,10 @@ static enum qman_cb_dqrr_result
> > rx_default_dqrr(struct qman_portal *portal,
> > >  	u32 hash;
> > >  	u64 ns;
> > >
> > > +	dpaa_fq = container_of(fq, struct dpaa_fq, fq_base);
> > >  	fd_status = be32_to_cpu(fd->status);
> > >  	fd_format = qm_fd_get_format(fd);
> > > -	net_dev = ((struct dpaa_fq *)fq)->net_dev;
> > > +	net_dev = dpaa_fq->net_dev;
> > >  	priv = netdev_priv(net_dev);
> > >  	dpaa_bp = dpaa_bpid2pool(dq->fd.bpid);
> > >  	if (!dpaa_bp)
> > > @@ -2494,7 +2612,7 @@ static enum qman_cb_dqrr_result
> > rx_default_dqrr(struct qman_portal *portal,
> > >
> > >  	if (likely(fd_format == qm_fd_contig)) {
> > >  		xdp_act = dpaa_run_xdp(priv, (struct qm_fd *)fd, vaddr,
> > > -				       &xdp_meta_len);
> > > +				       dpaa_fq, &xdp_meta_len);
> > >  		if (xdp_act != XDP_PASS) {
> > >  			percpu_stats->rx_packets++;
> > >  			percpu_stats->rx_bytes += qm_fd_get_length(fd);
> > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > > index 94e8613..5c8d52a 100644
> > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > > @@ -68,6 +68,7 @@ struct dpaa_fq {
> > >  	u16 channel;
> > >  	u8 wq;
> > >  	enum dpaa_fq_type fq_type;
> > > +	struct xdp_rxq_info xdp_rxq;
> > >  };
> > >
> > >  struct dpaa_fq_cbs {
> > > @@ -150,6 +151,7 @@ struct dpaa_buffer_layout {
> > >   */
> > >  struct dpaa_eth_swbp {
> > >  	struct sk_buff *skb;
> > > +	struct xdp_frame *xdpf;
> > >  };
> > >
> > >  struct dpaa_priv {
> > > --
> > > 1.9.1
> > >

RE: [PATCH net-next v4 4/7] dpaa_eth: add XDP_TX support

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Sent: Friday, November 27, 2020 16:29
> To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> davem@davemloft.net; Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next v4 4/7] dpaa_eth: add XDP_TX support
> 
> On Wed, Nov 25, 2020 at 03:49:45PM +0000, Camelia Alexandra Groza wrote:
> > > -----Original Message-----
> > > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > Sent: Tuesday, November 24, 2020 21:52
> > > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> > > davem@davemloft.net; Madalin Bucur (OSS)
> > > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> > > netdev@vger.kernel.org
> > > Subject: Re: [PATCH net-next v4 4/7] dpaa_eth: add XDP_TX support
> > >
> > > On Mon, Nov 23, 2020 at 07:36:22PM +0200, Camelia Groza wrote:
> > > > Use an xdp_frame structure for managing the frame. Store a
> backpointer
> > > to
> > > > the structure at the start of the buffer before enqueueing for cleanup
> > > > on TX confirmation. Reserve DPAA_TX_PRIV_DATA_SIZE bytes from
> the
> > > frame
> > > > size shared with the XDP program for this purpose. Use the XDP
> > > > API for freeing the buffer when it returns to the driver on the TX
> > > > confirmation path.
> > > >
> > > > The frame queues are shared with the netstack.
> > >
> > > Can you also provide the info from cover letter about locklessness (is
> > > that even a word?) in here?
> >
> > Sure.
> >
> > > One question below and:
> > >
> > > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > >
> > > >
> > > > This approach will be reused for XDP REDIRECT.
> > > >
> > > > Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > > > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > > > ---
> > > > Changes in v4:
> > > > - call xdp_rxq_info_is_reg() before unregistering
> > > > - minor cleanups (remove unneeded variable, print error code)
> > > > - add more details in the commit message
> > > > - did not call qman_destroy_fq() in case of xdp_rxq_info_reg() failure
> > > > since it would lead to a double free of the fq resources
> > > >
> > > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 128
> > > ++++++++++++++++++++++++-
> > > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |   2 +
> > > >  2 files changed, 125 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > index ee076f4..0deffcc 100644
> > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > @@ -1130,6 +1130,24 @@ static int dpaa_fq_init(struct dpaa_fq
> *dpaa_fq,
> > > bool td_enable)
> > > >
> > > >  	dpaa_fq->fqid = qman_fq_fqid(fq);
> > > >
> > > > +	if (dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
> > > > +	    dpaa_fq->fq_type == FQ_TYPE_RX_PCD) {
> > > > +		err = xdp_rxq_info_reg(&dpaa_fq->xdp_rxq, dpaa_fq-
> > > >net_dev,
> > > > +				       dpaa_fq->fqid);
> > > > +		if (err) {
> > > > +			dev_err(dev, "xdp_rxq_info_reg() = %d\n", err);
> > > > +			return err;
> > > > +		}
> > > > +
> > > > +		err = xdp_rxq_info_reg_mem_model(&dpaa_fq->xdp_rxq,
> > > > +						 MEM_TYPE_PAGE_ORDER0,
> > > NULL);
> > > > +		if (err) {
> > > > +			dev_err(dev, "xdp_rxq_info_reg_mem_model() =
> > > %d\n", err);
> > > > +			xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
> > > > +			return err;
> > > > +		}
> > > > +	}
> > > > +
> > > >  	return 0;
> > > >  }
> > > >
> > > > @@ -1159,6 +1177,11 @@ static int dpaa_fq_free_entry(struct device
> > > *dev, struct qman_fq *fq)
> > > >  		}
> > > >  	}
> > > >
> > > > +	if ((dpaa_fq->fq_type == FQ_TYPE_RX_DEFAULT ||
> > > > +	     dpaa_fq->fq_type == FQ_TYPE_RX_PCD) &&
> > > > +	    xdp_rxq_info_is_reg(&dpaa_fq->xdp_rxq))
> > > > +		xdp_rxq_info_unreg(&dpaa_fq->xdp_rxq);
> > > > +
> > > >  	qman_destroy_fq(fq);
> > > >  	list_del(&dpaa_fq->list);
> > > >
> > > > @@ -1625,6 +1648,9 @@ static int dpaa_eth_refill_bpools(struct
> dpaa_priv
> > > *priv)
> > > >   *
> > > >   * Return the skb backpointer, since for S/G frames the buffer
> containing it
> > > >   * gets freed here.
> > > > + *
> > > > + * No skb backpointer is set when transmitting XDP frames. Cleanup
> the
> > > buffer
> > > > + * and return NULL in this case.
> > > >   */
> > > >  static struct sk_buff *dpaa_cleanup_tx_fd(const struct dpaa_priv
> *priv,
> > > >  					  const struct qm_fd *fd, bool ts)
> > > > @@ -1664,13 +1690,21 @@ static struct sk_buff
> > > *dpaa_cleanup_tx_fd(const struct dpaa_priv *priv,
> > > >  		}
> > > >  	} else {
> > > >  		dma_unmap_single(priv->tx_dma_dev, addr,
> > > > -				 priv->tx_headroom +
> > > qm_fd_get_length(fd),
> > > > +				 qm_fd_get_offset(fd) +
> > > qm_fd_get_length(fd),
> > > >  				 dma_dir);
> > > >  	}
> > > >
> > > >  	swbp = (struct dpaa_eth_swbp *)vaddr;
> > > >  	skb = swbp->skb;
> > > >
> > > > +	/* No skb backpointer is set when running XDP. An xdp_frame
> > > > +	 * backpointer is saved instead.
> > > > +	 */
> > > > +	if (!skb) {
> > > > +		xdp_return_frame(swbp->xdpf);
> > > > +		return NULL;
> > > > +	}
> > > > +
> > > >  	/* DMA unmapping is required before accessing the HW provided
> > > info */
> > > >  	if (ts && priv->tx_tstamp &&
> > > >  	    skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> > > > @@ -2350,11 +2384,76 @@ static enum qman_cb_dqrr_result
> > > rx_error_dqrr(struct qman_portal *portal,
> > > >  	return qman_cb_dqrr_consume;
> > > >  }
> > > >
> > > > +static int dpaa_xdp_xmit_frame(struct net_device *net_dev,
> > > > +			       struct xdp_frame *xdpf)
> > > > +{
> > > > +	struct dpaa_priv *priv = netdev_priv(net_dev);
> > > > +	struct rtnl_link_stats64 *percpu_stats;
> > > > +	struct dpaa_percpu_priv *percpu_priv;
> > > > +	struct dpaa_eth_swbp *swbp;
> > > > +	struct netdev_queue *txq;
> > > > +	void *buff_start;
> > > > +	struct qm_fd fd;
> > > > +	dma_addr_t addr;
> > > > +	int err;
> > > > +
> > > > +	percpu_priv = this_cpu_ptr(priv->percpu_priv);
> > > > +	percpu_stats = &percpu_priv->stats;
> > > > +
> > > > +	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
> > > > +		err = -EINVAL;
> > > > +		goto out_error;
> > > > +	}
> > > > +
> > > > +	buff_start = xdpf->data - xdpf->headroom;
> > > > +
> > > > +	/* Leave empty the skb backpointer at the start of the buffer.
> > > > +	 * Save the XDP frame for easy cleanup on confirmation.
> > > > +	 */
> > > > +	swbp = (struct dpaa_eth_swbp *)buff_start;
> > > > +	swbp->skb = NULL;
> > > > +	swbp->xdpf = xdpf;
> > > > +
> > > > +	qm_fd_clear_fd(&fd);
> > > > +	fd.bpid = FSL_DPAA_BPID_INV;
> > > > +	fd.cmd |= cpu_to_be32(FM_FD_CMD_FCO);
> > > > +	qm_fd_set_contig(&fd, xdpf->headroom, xdpf->len);
> > > > +
> > > > +	addr = dma_map_single(priv->tx_dma_dev, buff_start,
> > > > +			      xdpf->headroom + xdpf->len,
> > > > +			      DMA_TO_DEVICE);
> > >
> > > Not sure if I asked that.  What is the purpose for including the headroom
> > > in frame being set? I would expect to take into account only frame from
> > > xdpf->data.
> >
> > The xdpf headroom becomes the fd's offset, the area before the data
> where the backpointers for cleanup are stored. This area isn't sent out with
> the frame.
> 
> But if I'm reading this right you clearly include the headroom space in
> the DMA region that you are mapping. Why this couldn't be:
> 
> 	addr = dma_map_single(priv->tx_dma_dev, xdpf->data,
> 			      xdpf->len, DMA_TO_DEVICE);
> 
> And then frame descriptor wouldn't need to have the offset at all?
> Probably that's implementation details, but this way it seems
> cleaner/easier to follow to me.

The headroom is included in the mapped DMA region because we need access to the xdpf reference for cleanup on the confirmation path. The reference is stored in the headroom. We also use the headroom for other offloads such as TX checksum, but those aren't available on XDP yet. Overall, this is the driver's design and we need to map the headroom as well as the data.

> >
> > > > +	if (unlikely(dma_mapping_error(priv->tx_dma_dev, addr))) {
> > > > +		err = -EINVAL;
> > > > +		goto out_error;
> > > > +	}
> > > > +
> > > > +	qm_fd_addr_set64(&fd, addr);
> > > > +
> > > > +	/* Bump the trans_start */
> > > > +	txq = netdev_get_tx_queue(net_dev, smp_processor_id());
> > > > +	txq->trans_start = jiffies;
> > > > +
> > > > +	err = dpaa_xmit(priv, percpu_stats, smp_processor_id(), &fd);
> > > > +	if (err) {
> > > > +		dma_unmap_single(priv->tx_dma_dev, addr,
> > > > +				 qm_fd_get_offset(&fd) +
> > > qm_fd_get_length(&fd),
> > > > +				 DMA_TO_DEVICE);
> > > > +		goto out_error;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +
> > > > +out_error:
> > > > +	percpu_stats->tx_errors++;
> > > > +	return err;
> > > > +}
> > > > +
> > > >  static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void
> > > *vaddr,
> > > > -			unsigned int *xdp_meta_len)
> > > > +			struct dpaa_fq *dpaa_fq, unsigned int
> > > *xdp_meta_len)
> > > >  {
> > > >  	ssize_t fd_off = qm_fd_get_offset(fd);
> > > >  	struct bpf_prog *xdp_prog;
> > > > +	struct xdp_frame *xdpf;
> > > >  	struct xdp_buff xdp;
> > > >  	u32 xdp_act;
> > > >
> > > > @@ -2370,7 +2469,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv
> *priv,
> > > struct qm_fd *fd, void *vaddr,
> > > >  	xdp.data_meta = xdp.data;
> > > >  	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
> > > >  	xdp.data_end = xdp.data + qm_fd_get_length(fd);
> > > > -	xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > > > +	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
> > > > +	xdp.rxq = &dpaa_fq->xdp_rxq;
> > > >
> > > >  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > > >
> > > > @@ -2381,6 +2481,22 @@ static u32 dpaa_run_xdp(struct dpaa_priv
> *priv,
> > > struct qm_fd *fd, void *vaddr,
> > > >  	case XDP_PASS:
> > > >  		*xdp_meta_len = xdp.data - xdp.data_meta;
> > > >  		break;
> > > > +	case XDP_TX:
> > > > +		/* We can access the full headroom when sending the frame
> > > > +		 * back out
> > > > +		 */
> > > > +		xdp.data_hard_start = vaddr;
> > > > +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > > > +		xdpf = xdp_convert_buff_to_frame(&xdp);
> > > > +		if (unlikely(!xdpf)) {
> > > > +			free_pages((unsigned long)vaddr, 0);
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		if (dpaa_xdp_xmit_frame(priv->net_dev, xdpf))
> > > > +			xdp_return_frame_rx_napi(xdpf);
> > > > +
> > > > +		break;
> > > >  	default:
> > > >  		bpf_warn_invalid_xdp_action(xdp_act);
> > > >  		fallthrough;
> > > > @@ -2415,6 +2531,7 @@ static enum qman_cb_dqrr_result
> > > rx_default_dqrr(struct qman_portal *portal,
> > > >  	u32 fd_status, hash_offset;
> > > >  	struct qm_sg_entry *sgt;
> > > >  	struct dpaa_bp *dpaa_bp;
> > > > +	struct dpaa_fq *dpaa_fq;
> > > >  	struct dpaa_priv *priv;
> > > >  	struct sk_buff *skb;
> > > >  	int *count_ptr;
> > > > @@ -2423,9 +2540,10 @@ static enum qman_cb_dqrr_result
> > > rx_default_dqrr(struct qman_portal *portal,
> > > >  	u32 hash;
> > > >  	u64 ns;
> > > >
> > > > +	dpaa_fq = container_of(fq, struct dpaa_fq, fq_base);
> > > >  	fd_status = be32_to_cpu(fd->status);
> > > >  	fd_format = qm_fd_get_format(fd);
> > > > -	net_dev = ((struct dpaa_fq *)fq)->net_dev;
> > > > +	net_dev = dpaa_fq->net_dev;
> > > >  	priv = netdev_priv(net_dev);
> > > >  	dpaa_bp = dpaa_bpid2pool(dq->fd.bpid);
> > > >  	if (!dpaa_bp)
> > > > @@ -2494,7 +2612,7 @@ static enum qman_cb_dqrr_result
> > > rx_default_dqrr(struct qman_portal *portal,
> > > >
> > > >  	if (likely(fd_format == qm_fd_contig)) {
> > > >  		xdp_act = dpaa_run_xdp(priv, (struct qm_fd *)fd, vaddr,
> > > > -				       &xdp_meta_len);
> > > > +				       dpaa_fq, &xdp_meta_len);
> > > >  		if (xdp_act != XDP_PASS) {
> > > >  			percpu_stats->rx_packets++;
> > > >  			percpu_stats->rx_bytes += qm_fd_get_length(fd);
> > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > > > index 94e8613..5c8d52a 100644
> > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > > > @@ -68,6 +68,7 @@ struct dpaa_fq {
> > > >  	u16 channel;
> > > >  	u8 wq;
> > > >  	enum dpaa_fq_type fq_type;
> > > > +	struct xdp_rxq_info xdp_rxq;
> > > >  };
> > > >
> > > >  struct dpaa_fq_cbs {
> > > > @@ -150,6 +151,7 @@ struct dpaa_buffer_layout {
> > > >   */
> > > >  struct dpaa_eth_swbp {
> > > >  	struct sk_buff *skb;
> > > > +	struct xdp_frame *xdpf;
> > > >  };
> > > >
> > > >  struct dpaa_priv {
> > > > --
> > > > 1.9.1
> > > >

[PATCH net-next v4 5/7] dpaa_eth: add XDP_REDIRECT support

[Camelia Groza]

After transmission, the frame is returned on confirmation queues for
cleanup. For this, store a backpointer to the xdp_frame in the private
reserved area at the start of the TX buffer.

No TX batching support is implemented at this time.

Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 48 +++++++++++++++++++++++++-
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |  1 +
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 0deffcc..a96c994 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2304,8 +2304,11 @@ static int dpaa_eth_poll(struct napi_struct *napi, int budget)
 {
 	struct dpaa_napi_portal *np =
 			container_of(napi, struct dpaa_napi_portal, napi);
+	int cleaned;
 
-	int cleaned = qman_p_poll_dqrr(np->p, budget);
+	np->xdp_act = 0;
+
+	cleaned = qman_p_poll_dqrr(np->p, budget);
 
 	if (cleaned < budget) {
 		napi_complete_done(napi, cleaned);
@@ -2314,6 +2317,9 @@ static int dpaa_eth_poll(struct napi_struct *napi, int budget)
 		qman_p_irqsource_add(np->p, QM_PIRQ_DQRI);
 	}
 
+	if (np->xdp_act & XDP_REDIRECT)
+		xdp_do_flush();
+
 	return cleaned;
 }
 
@@ -2456,6 +2462,7 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
 	struct xdp_frame *xdpf;
 	struct xdp_buff xdp;
 	u32 xdp_act;
+	int err;
 
 	rcu_read_lock();
 
@@ -2497,6 +2504,17 @@ static u32 dpaa_run_xdp(struct dpaa_priv *priv, struct qm_fd *fd, void *vaddr,
 			xdp_return_frame_rx_napi(xdpf);
 
 		break;
+	case XDP_REDIRECT:
+		/* Allow redirect to use the full headroom */
+		xdp.data_hard_start = vaddr;
+		xdp.frame_sz = DPAA_BP_RAW_SIZE;
+
+		err = xdp_do_redirect(priv->net_dev, &xdp, xdp_prog);
+		if (err) {
+			trace_xdp_exception(priv->net_dev, xdp_prog, xdp_act);
+			free_pages((unsigned long)vaddr, 0);
+		}
+		break;
 	default:
 		bpf_warn_invalid_xdp_action(xdp_act);
 		fallthrough;
@@ -2526,6 +2544,7 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
 	struct dpaa_percpu_priv *percpu_priv;
 	const struct qm_fd *fd = &dq->fd;
 	dma_addr_t addr = qm_fd_addr(fd);
+	struct dpaa_napi_portal *np;
 	enum qm_fd_format fd_format;
 	struct net_device *net_dev;
 	u32 fd_status, hash_offset;
@@ -2540,6 +2559,7 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
 	u32 hash;
 	u64 ns;
 
+	np = container_of(&portal, struct dpaa_napi_portal, p);
 	dpaa_fq = container_of(fq, struct dpaa_fq, fq_base);
 	fd_status = be32_to_cpu(fd->status);
 	fd_format = qm_fd_get_format(fd);
@@ -2613,6 +2633,7 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct qman_portal *portal,
 	if (likely(fd_format == qm_fd_contig)) {
 		xdp_act = dpaa_run_xdp(priv, (struct qm_fd *)fd, vaddr,
 				       dpaa_fq, &xdp_meta_len);
+		np->xdp_act |= xdp_act;
 		if (xdp_act != XDP_PASS) {
 			percpu_stats->rx_packets++;
 			percpu_stats->rx_bytes += qm_fd_get_length(fd);
@@ -2943,6 +2964,30 @@ static int dpaa_xdp(struct net_device *net_dev, struct netdev_bpf *xdp)
 	}
 }
 
+static int dpaa_xdp_xmit(struct net_device *net_dev, int n,
+			 struct xdp_frame **frames, u32 flags)
+{
+	struct xdp_frame *xdpf;
+	int i, err, drops = 0;
+
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
+		return -EINVAL;
+
+	if (!netif_running(net_dev))
+		return -ENETDOWN;
+
+	for (i = 0; i < n; i++) {
+		xdpf = frames[i];
+		err = dpaa_xdp_xmit_frame(net_dev, xdpf);
+		if (err) {
+			xdp_return_frame_rx_napi(xdpf);
+			drops++;
+		}
+	}
+
+	return n - drops;
+}
+
 static int dpaa_ts_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
 	struct dpaa_priv *priv = netdev_priv(dev);
@@ -3011,6 +3056,7 @@ static int dpaa_ioctl(struct net_device *net_dev, struct ifreq *rq, int cmd)
 	.ndo_setup_tc = dpaa_setup_tc,
 	.ndo_change_mtu = dpaa_change_mtu,
 	.ndo_bpf = dpaa_xdp,
+	.ndo_xdp_xmit = dpaa_xdp_xmit,
 };
 
 static int dpaa_napi_add(struct net_device *net_dev)
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
index 5c8d52a..daf894a 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
@@ -127,6 +127,7 @@ struct dpaa_napi_portal {
 	struct napi_struct napi;
 	struct qman_portal *p;
 	bool down;
+	int xdp_act;
 };
 
 struct dpaa_percpu_priv {
-- 
1.9.1

Re: [PATCH net-next v4 5/7] dpaa_eth: add XDP_REDIRECT support

[Maciej Fijalkowski]

On Mon, Nov 23, 2020 at 07:36:23PM +0200, Camelia Groza wrote:
> After transmission, the frame is returned on confirmation queues for
> cleanup. For this, store a backpointer to the xdp_frame in the private
> reserved area at the start of the TX buffer.
> 
> No TX batching support is implemented at this time.
> 
> Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 48 +++++++++++++++++++++++++-
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.h |  1 +
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

[PATCH net-next v4 6/7] dpaa_eth: rename current skb A050385 erratum workaround

[Camelia Groza]

Explicitly point that the current workaround addresses skbs. This change is
in preparation for adding a workaround for XDP scenarios.

Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index a96c994..149b549 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2104,7 +2104,7 @@ static inline int dpaa_xmit(struct dpaa_priv *priv,
 }
 
 #ifdef CONFIG_DPAA_ERRATUM_A050385
-static int dpaa_a050385_wa(struct net_device *net_dev, struct sk_buff **s)
+static int dpaa_a050385_wa_skb(struct net_device *net_dev, struct sk_buff **s)
 {
 	struct dpaa_priv *priv = netdev_priv(net_dev);
 	struct sk_buff *new_skb, *skb = *s;
@@ -2220,7 +2220,7 @@ static int dpaa_a050385_wa(struct net_device *net_dev, struct sk_buff **s)
 
 #ifdef CONFIG_DPAA_ERRATUM_A050385
 	if (unlikely(fman_has_errata_a050385())) {
-		if (dpaa_a050385_wa(net_dev, &skb))
+		if (dpaa_a050385_wa_skb(net_dev, &skb))
 			goto enomem;
 		nonlinear = skb_is_nonlinear(skb);
 	}
-- 
1.9.1

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

[Camelia Groza]

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 <madalin.bucur@oss.nxp.com>
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
 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;
+	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)
-- 
1.9.1

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

[Maciej Fijalkowski]

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 <madalin.bucur@oss.nxp.com>
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> ---
>  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
> 

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

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Sent: Tuesday, November 24, 2020 22:51
> To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> davem@davemloft.net; Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> netdev@vger.kernel.org
> 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 <madalin.bucur@oss.nxp.com>
> > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > ---
> >  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
> >

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

[Maciej Fijalkowski]

On Wed, Nov 25, 2020 at 03:52:33PM +0000, Camelia Alexandra Groza wrote:
> > -----Original Message-----
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Sent: Tuesday, November 24, 2020 22:51
> > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> > davem@davemloft.net; Madalin Bucur (OSS)
> > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> > netdev@vger.kernel.org
> > 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 <madalin.bucur@oss.nxp.com>
> > > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > > ---
> > >  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
> > >

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

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Sent: Friday, November 27, 2020 16:44
> To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> davem@davemloft.net; Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> netdev@vger.kernel.org
> 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 <maciej.fijalkowski@intel.com>
> > > Sent: Tuesday, November 24, 2020 22:51
> > > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> > > davem@davemloft.net; Madalin Bucur (OSS)
> > > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> > > netdev@vger.kernel.org
> > > 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 <madalin.bucur@oss.nxp.com>
> > > > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > > > ---
> > > >  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
> > > >

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

[Maciej Fijalkowski]

On Fri, Nov 27, 2020 at 05:35:00PM +0000, Camelia Alexandra Groza wrote:
> > -----Original Message-----
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Sent: Friday, November 27, 2020 16:44
> > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> > davem@davemloft.net; Madalin Bucur (OSS)
> > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> > netdev@vger.kernel.org
> > 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 <maciej.fijalkowski@intel.com>
> > > > Sent: Tuesday, November 24, 2020 22:51
> > > > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > > > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> > > > davem@davemloft.net; Madalin Bucur (OSS)
> > > > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> > > > netdev@vger.kernel.org
> > > > 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 <madalin.bucur@oss.nxp.com>
> > > > > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > > > > ---
> > > > >  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
> > > > >