[v4,7/9] igc: Add initial XDP support

This patch adds the initial XDP support to the igc driver. For now,
only XDP_PASS, XDP_DROP, XDP_ABORTED actions are supported. Upcoming
patches will add support for the remaining XDP actions.

XDP configuration helpers are defined in a new file, igc_xdp.c. These
helpers are utilized in igc_main.c to implement the ndo_bpf callback.
XDP-related code that belongs to the driver's hot path is landed in
igc_main.c.

By default, the driver uses rx buffers with 2 KB size. When XDP is
enabled, it uses larger buffers so we have enough space to accommodate
the headroom and tailroom required by XDP infrastructure. Also, the
driver doesn't support XDP functionality with frames that span over
multiple buffers so jumbo frames are not allowed for now.

The approach implemented by this patch follows the approach implemented
in other Intel drivers as much as possible for the sake of consistency
across the drivers.

Quick comment regarding igc_build_skb(): this patch doesn't touch it
because the function is never called. It seems its support is
incomplete/in progress. The function was added by commit 0507ef8a0372b
("igc: Add transmit and receive fastpath and interrupt handlers") but
ring_uses_build_skb() always return False since the IGC_RING_FLAG_RX_
BUILD_SKB_ENABLED isn't set anywhere in the driver code.

This patch has been tested with the sample app "xdp1" located in
samples/bpf/ dir.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/igc/Makefile   |   2 +-
 drivers/net/ethernet/intel/igc/igc.h      |   2 +
 drivers/net/ethernet/intel/igc/igc_main.c | 118 ++++++++++++++++++++--
 drivers/net/ethernet/intel/igc/igc_xdp.c  |  33 ++++++
 drivers/net/ethernet/intel/igc/igc_xdp.h  |  10 ++
 5 files changed, 153 insertions(+), 12 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igc/igc_xdp.c
 create mode 100644 drivers/net/ethernet/intel/igc/igc_xdp.h

On 11/4/2020 05:12, Andre Guedes wrote:
> This patch adds the initial XDP support to the igc driver. For now,
> only XDP_PASS, XDP_DROP, XDP_ABORTED actions are supported. Upcoming
> patches will add support for the remaining XDP actions.
> 
> XDP configuration helpers are defined in a new file, igc_xdp.c. These
> helpers are utilized in igc_main.c to implement the ndo_bpf callback.
> XDP-related code that belongs to the driver's hot path is landed in
> igc_main.c.
> 
> By default, the driver uses rx buffers with 2 KB size. When XDP is
> enabled, it uses larger buffers so we have enough space to accommodate
> the headroom and tailroom required by XDP infrastructure. Also, the
> driver doesn't support XDP functionality with frames that span over
> multiple buffers so jumbo frames are not allowed for now.
> 
> The approach implemented by this patch follows the approach implemented
> in other Intel drivers as much as possible for the sake of consistency
> across the drivers.
> 
> Quick comment regarding igc_build_skb(): this patch doesn't touch it
> because the function is never called. It seems its support is
> incomplete/in progress. The function was added by commit 0507ef8a0372b
> ("igc: Add transmit and receive fastpath and interrupt handlers") but
> ring_uses_build_skb() always return False since the IGC_RING_FLAG_RX_
> BUILD_SKB_ENABLED isn't set anywhere in the driver code.
> 
> This patch has been tested with the sample app "xdp1" located in
> samples/bpf/ dir.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/Makefile   |   2 +-
>   drivers/net/ethernet/intel/igc/igc.h      |   2 +
>   drivers/net/ethernet/intel/igc/igc_main.c | 118 ++++++++++++++++++++--
>   drivers/net/ethernet/intel/igc/igc_xdp.c  |  33 ++++++
>   drivers/net/ethernet/intel/igc/igc_xdp.h  |  10 ++
>   5 files changed, 153 insertions(+), 12 deletions(-)
>   create mode 100644 drivers/net/ethernet/intel/igc/igc_xdp.c
>   create mode 100644 drivers/net/ethernet/intel/igc/igc_xdp.h
> 
> diff --git a/drivers/net/ethernet/intel/igc/Makefile b/drivers/net/ethernet/intel/igc/Makefile
> index 1c3051db9085..95d1e8c490a4 100644
> --- a/drivers/net/ethernet/intel/igc/Makefile
> +++ b/drivers/net/ethernet/intel/igc/Makefile
> @@ -8,4 +8,4 @@
>   obj-$(CONFIG_IGC) += igc.o
>   
>   igc-objs := igc_main.o igc_mac.o igc_i225.o igc_base.o igc_nvm.o igc_phy.o \
> -igc_diag.o igc_ethtool.o igc_ptp.o igc_dump.o igc_tsn.o
> +igc_diag.o igc_ethtool.o igc_ptp.o igc_dump.o igc_tsn.o igc_xdp.o
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index e72f1fc772aa..5c2f363106ae 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -224,6 +224,8 @@ struct igc_adapter {
>   	struct mutex ptm_time_lock; /* protects host and device timestamps */
>   	ktime_t ptm_device_time;
>   	struct system_counterval_t ptm_host_time;
> +
> +	struct bpf_prog *xdp_prog;
>   };
>   
>   void igc_up(struct igc_adapter *adapter);
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 84ffde75e968..734a570bbadb 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -11,17 +11,22 @@
>   #include <linux/pm_runtime.h>
>   #include <net/pkt_sched.h>
>   #include <linux/pci.h>
> +#include <linux/bpf_trace.h>
>   
>   #include <net/ipv6.h>
>   
>   #include "igc.h"
>   #include "igc_hw.h"
>   #include "igc_tsn.h"
> +#include "igc_xdp.h"
>   
>   #define DRV_SUMMARY	"Intel(R) 2.5G Ethernet Linux Driver"
>   
>   #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK)
>   
> +#define IGC_XDP_PASS		0
> +#define IGC_XDP_CONSUMED	BIT(0)
> +
Hello Andre, please, move these defines to igc_defines.h to bottom the 
file under /* XDP defines */ comment.
>   static int debug = -1;
>   
>   MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
> @@ -346,6 +351,8 @@ static void igc_clean_rx_ring(struct igc_ring *rx_ring)
>   {
>   	u16 i = rx_ring->next_to_clean;
>   
> +	clear_ring_uses_large_buffer(rx_ring);
> +
>   	dev_kfree_skb(rx_ring->skb);
>   	rx_ring->skb = NULL;
>   
> @@ -498,6 +505,11 @@ static int igc_setup_all_rx_resources(struct igc_adapter *adapter)
>   	return err;
>   }
>   
> +static bool igc_xdp_is_enabled(struct igc_adapter *adapter)
> +{
> +	return !!adapter->xdp_prog;
> +}
> +
>   /**
>    * igc_configure_rx_ring - Configure a receive ring after Reset
>    * @adapter: board private structure
> @@ -514,6 +526,9 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
>   	u32 srrctl = 0, rxdctl = 0;
>   	u64 rdba = ring->dma;
>   
> +	if (igc_xdp_is_enabled(adapter))
> +		set_ring_uses_large_buffer(ring);
> +
>   	/* disable the queue */
>   	wr32(IGC_RXDCTL(reg_idx), 0);
>   
> @@ -1594,12 +1609,12 @@ static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring,
>   
>   static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
>   					 struct igc_rx_buffer *rx_buffer,
> -					 unsigned int size, int pkt_offset,
> +					 struct xdp_buff *xdp,
>   					 ktime_t timestamp)
>   {
> -	void *va = page_address(rx_buffer->page) + rx_buffer->page_offset +
> -		   pkt_offset;
> +	unsigned int size = xdp->data_end - xdp->data;
>   	unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size);
> +	void *va = xdp->data;
>   	unsigned int headlen;
>   	struct sk_buff *skb;
>   
> @@ -1748,6 +1763,10 @@ static bool igc_cleanup_headers(struct igc_ring *rx_ring,
>   				union igc_adv_rx_desc *rx_desc,
>   				struct sk_buff *skb)
>   {
> +	/* XDP packets use error pointer so abort at this point */
> +	if (IS_ERR(skb))
> +		return true;
> +
>   	if (unlikely(igc_test_staterr(rx_desc, IGC_RXDEXT_STATERR_RXE))) {
>   		struct net_device *netdev = rx_ring->netdev;
>   
> @@ -1787,7 +1806,14 @@ static void igc_put_rx_buffer(struct igc_ring *rx_ring,
>   
>   static inline unsigned int igc_rx_offset(struct igc_ring *rx_ring)
>   {
> -	return ring_uses_build_skb(rx_ring) ? IGC_SKB_PAD : 0;
> +	struct igc_adapter *adapter = rx_ring->q_vector->adapter;
> +
> +	if (ring_uses_build_skb(rx_ring))
> +		return IGC_SKB_PAD;
> +	if (igc_xdp_is_enabled(adapter))
> +		return XDP_PACKET_HEADROOM;
> +
> +	return 0;
>   }
>   
>   static bool igc_alloc_mapped_page(struct igc_ring *rx_ring,
> @@ -1901,6 +1927,42 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
>   	}
>   }
>   
> +static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter,
> +					struct xdp_buff *xdp)
> +{
> +	struct bpf_prog *prog;
> +	int res;
> +	u32 act;
> +
> +	rcu_read_lock();
> +
> +	prog = READ_ONCE(adapter->xdp_prog);
> +	if (!prog) {
> +		res = IGC_XDP_PASS;
> +		goto unlock;
> +	}
> +
> +	act = bpf_prog_run_xdp(prog, xdp);
> +	switch (act) {
> +	case XDP_PASS:
> +		res = IGC_XDP_PASS;
> +		break;
> +	default:
> +		bpf_warn_invalid_xdp_action(act);
> +		fallthrough;
> +	case XDP_ABORTED:
> +		trace_xdp_exception(adapter->netdev, prog, act);
> +		fallthrough;
> +	case XDP_DROP:
> +		res = IGC_XDP_CONSUMED;
> +		break;
> +	}
> +
> +unlock:
> +	rcu_read_unlock();
> +	return ERR_PTR(-res);
> +}
> +
>   static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>   {
>   	unsigned int total_bytes = 0, total_packets = 0;
> @@ -1912,8 +1974,10 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>   		union igc_adv_rx_desc *rx_desc;
>   		struct igc_rx_buffer *rx_buffer;
>   		ktime_t timestamp = 0;
> +		struct xdp_buff xdp;
>   		int pkt_offset = 0;
>   		unsigned int size;
> +		void *pktbuf;
>   
>   		/* return some buffers to hardware, one at a time is too slow */
>   		if (cleaned_count >= IGC_RX_BUFFER_WRITE) {
> @@ -1934,24 +1998,38 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>   
>   		rx_buffer = igc_get_rx_buffer(rx_ring, size);
>   
> -		if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
> -			void *pktbuf = page_address(rx_buffer->page) +
> -				       rx_buffer->page_offset;
> +		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
>   
> +		if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
>   			timestamp = igc_ptp_rx_pktstamp(q_vector->adapter,
>   							pktbuf);
>   			pkt_offset = IGC_TS_HDR_LEN;
>   			size -= IGC_TS_HDR_LEN;
>   		}
>   
> -		/* retrieve a buffer from the ring */
> -		if (skb)
> +		if (!skb) {
> +			struct igc_adapter *adapter = q_vector->adapter;
> +
> +			xdp.data = pktbuf + pkt_offset;
> +			xdp.data_end = xdp.data + size;
> +			xdp.data_hard_start = pktbuf - igc_rx_offset(rx_ring);
> +			xdp_set_data_meta_invalid(&xdp);
> +			xdp.frame_sz = igc_get_rx_frame_truesize(rx_ring, size);
> +
> +			skb = igc_xdp_run_prog(adapter, &xdp);
> +		}
> +
> +		if (IS_ERR(skb)) {
> +			rx_buffer->pagecnt_bias++;
> +			total_packets++;
> +			total_bytes += size;
> +		} else if (skb)
>   			igc_add_rx_frag(rx_ring, rx_buffer, skb, size);
>   		else if (ring_uses_build_skb(rx_ring))
>   			skb = igc_build_skb(rx_ring, rx_buffer, rx_desc, size);
>   		else
> -			skb = igc_construct_skb(rx_ring, rx_buffer, size,
> -						pkt_offset, timestamp);
> +			skb = igc_construct_skb(rx_ring, rx_buffer, &xdp,
> +						timestamp);
>   
>   		/* exit if we failed to retrieve a buffer */
>   		if (!skb) {
> @@ -3893,6 +3971,11 @@ static int igc_change_mtu(struct net_device *netdev, int new_mtu)
>   	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
>   	struct igc_adapter *adapter = netdev_priv(netdev);
>   
> +	if (igc_xdp_is_enabled(adapter) && new_mtu > ETH_DATA_LEN) {
> +		netdev_dbg(netdev, "Jumbo frames not supported with XDP");
> +		return -EINVAL;
> +	}
> +
>   	/* adjust max frame to be at least the size of a standard frame */
>   	if (max_frame < (ETH_FRAME_LEN + ETH_FCS_LEN))
>   		max_frame = ETH_FRAME_LEN + ETH_FCS_LEN;
> @@ -4881,6 +4964,18 @@ static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type,
>   	}
>   }
>   
> +static int igc_bpf(struct net_device *dev, struct netdev_bpf *bpf)
> +{
> +	struct igc_adapter *adapter = netdev_priv(dev);
> +
> +	switch (bpf->command) {
> +	case XDP_SETUP_PROG:
> +		return igc_xdp_set_prog(adapter, bpf->prog, bpf->extack);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>   static const struct net_device_ops igc_netdev_ops = {
>   	.ndo_open		= igc_open,
>   	.ndo_stop		= igc_close,
> @@ -4894,6 +4989,7 @@ static const struct net_device_ops igc_netdev_ops = {
>   	.ndo_features_check	= igc_features_check,
>   	.ndo_do_ioctl		= igc_ioctl,
>   	.ndo_setup_tc		= igc_setup_tc,
> +	.ndo_bpf		= igc_bpf,
>   };
>   
>   /* PCIe configuration access */
> diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c
> new file mode 100644
> index 000000000000..27c886a254f1
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2020, Intel Corporation. */
> +
> +#include "igc.h"
> +#include "igc_xdp.h"
> +
> +int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
> +		     struct netlink_ext_ack *extack)
> +{
> +	struct net_device *dev = adapter->netdev;
> +	bool if_running = netif_running(dev);
> +	struct bpf_prog *old_prog;
> +
> +	if (dev->mtu > ETH_DATA_LEN) {
> +		/* For now, the driver doesn't support XDP functionality with
> +		 * jumbo frames so we return error.
> +		 */
> +		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (if_running)
> +		igc_close(dev);
> +
> +	old_prog = xchg(&adapter->xdp_prog, prog);
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +
> +	if (if_running)
> +		igc_open(dev);
> +
> +	return 0;
> +}
> diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.h b/drivers/net/ethernet/intel/igc/igc_xdp.h
> new file mode 100644
> index 000000000000..8a410bcefe1a
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/igc/igc_xdp.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2020, Intel Corporation. */
> +
> +#ifndef _IGC_XDP_H_
> +#define _IGC_XDP_H_
> +
> +int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
> +		     struct netlink_ext_ack *extack);
> +
> +#endif /* _IGC_XDP_H_ */
> 
Thanks,
Sasha

Quoting Neftin, Sasha (2020-11-12 01:13:27)
> > +#define IGC_XDP_PASS         0
> > +#define IGC_XDP_CONSUMED     BIT(0)
> > +
> Hello Andre, please, move these defines to igc_defines.h to bottom the 
> file under /* XDP defines */ comment.

Hi Sasha, looking at the macros defined in igc_defines.h, they seem to be
hardware-related (registers, fields, masks, etc) which is not the case of
the XDP macros defined here. In addition to that, note that these macros
are used only in igc_main.c so maybe it makes sense to leave them here?

Andre Guedes wrote:

> Quoting Neftin, Sasha (2020-11-12 01:13:27)
> > > +#define IGC_XDP_PASS         0
> > > +#define IGC_XDP_CONSUMED     BIT(0)
> > > +
> > Hello Andre, please, move these defines to igc_defines.h to bottom the 
> > file under /* XDP defines */ comment.
> 
> Hi Sasha, looking at the macros defined in igc_defines.h, they seem to be
> hardware-related (registers, fields, masks, etc) which is not the case of
> the XDP macros defined here. In addition to that, note that these macros
> are used only in igc_main.c so maybe it makes sense to leave them here?

FWIW, I agree with Andre here, I'd just leave them in the igc_main.c
file.