Re: [net-next 05/16] ice: Add support for tunnel offloads

[Jakub Kicinski <kuba@kernel.org>]

On Wed,  4 Mar 2020 15:21:25 -0800 Jeff Kirsher wrote:
> From: Tony Nguyen <anthony.l.nguyen@intel.com>
> 
> Create a boost TCAM entry for each tunnel port in order to get a tunnel
> PTYPE. Update netdev feature flags and implement the appropriate logic to
> get and set values for hardware offloads.
> 
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> Signed-off-by: Henry Tieman <henry.w.tieman@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

> +/**
> + * ice_create_tunnel
> + * @hw: pointer to the HW structure
> + * @type: type of tunnel
> + * @port: port to use for vxlan tunnel
> + *
> + * Creates a tunnel

I was going to comment how useless this kdoc is, then I realized that
it's not only useless but incorrect - port doesn't have to be vxlan,
you support geneve..

> + */
> +enum ice_status
> +ice_create_tunnel(struct ice_hw *hw, enum ice_tunnel_type type, u16 port)
> +{
> +	struct ice_boost_tcam_section *sect_rx, *sect_tx;
> +	enum ice_status status = ICE_ERR_MAX_LIMIT;
> +	struct ice_buf_build *bld;
> +	u16 index;
> +
> +	if (ice_tunnel_port_in_use(hw, port, NULL))
> +		return ICE_ERR_ALREADY_EXISTS;

Could you explain how ref counting of ports works? It's possible to
have multiple tunnels on the same port. Looks like this is just bailing
without even making a note of the request. So delete will just remove
the port whenever the first tunnel with this port goes down?

> +	if (!ice_find_free_tunnel_entry(hw, type, &index))
> +		return ICE_ERR_OUT_OF_RANGE;
> +
> +	bld = ice_pkg_buf_alloc(hw);
> +	if (!bld)
> +		return ICE_ERR_NO_MEMORY;
> +
> +	/* allocate 2 sections, one for Rx parser, one for Tx parser */
> +	if (ice_pkg_buf_reserve_section(bld, 2))
> +		goto ice_create_tunnel_err;
> +
> +	sect_rx = (struct ice_boost_tcam_section *)
> +		ice_pkg_buf_alloc_section(bld, ICE_SID_RXPARSER_BOOST_TCAM,

this function returns void, the extremely ugly casts are unnecessary

> +					  sizeof(*sect_rx));
> +	if (!sect_rx)
> +		goto ice_create_tunnel_err;
> +	sect_rx->count = cpu_to_le16(1);
> +
> +	sect_tx = (struct ice_boost_tcam_section *)
> +		ice_pkg_buf_alloc_section(bld, ICE_SID_TXPARSER_BOOST_TCAM,

and here

> +					  sizeof(*sect_tx));
> +	if (!sect_tx)
> +		goto ice_create_tunnel_err;
> +	sect_tx->count = cpu_to_le16(1);
> +
> +	/* copy original boost entry to update package buffer */
> +	memcpy(sect_rx->tcam, hw->tnl.tbl[index].boost_entry,
> +	       sizeof(*sect_rx->tcam));
> +
> +	/* over-write the never-match dest port key bits with the encoded port
> +	 * bits
> +	 */
> +	ice_set_key((u8 *)&sect_rx->tcam[0].key, sizeof(sect_rx->tcam[0].key),
> +		    (u8 *)&port, NULL, NULL, NULL,
> +		    offsetof(struct ice_boost_key_value, hv_dst_port_key),
> +		    sizeof(sect_rx->tcam[0].key.key.hv_dst_port_key));
> +
> +	/* exact copy of entry to Tx section entry */
> +	memcpy(sect_tx->tcam, sect_rx->tcam, sizeof(*sect_tx->tcam));
> +
> +	status = ice_update_pkg(hw, ice_pkg_buf(bld), 1);
> +	if (!status) {
> +		hw->tnl.tbl[index].port = port;
> +		hw->tnl.tbl[index].in_use = true;
> +	}
> +
> +ice_create_tunnel_err:
> +	ice_pkg_buf_free(hw, bld);
> +
> +	return status;
> +}

> +/**
> + * ice_udp_tunnel_add - Get notifications about UDP tunnel ports that come up
> + * @netdev: This physical port's netdev
> + * @ti: Tunnel endpoint information
> + */
> +static void
> +ice_udp_tunnel_add(struct net_device *netdev, struct udp_tunnel_info *ti)
> +{
> +	struct ice_netdev_priv *np = netdev_priv(netdev);
> +	struct ice_vsi *vsi = np->vsi;
> +	struct ice_pf *pf = vsi->back;
> +	enum ice_tunnel_type tnl_type;
> +	u16 port = ntohs(ti->port);
> +	enum ice_status status;
> +
> +	switch (ti->type) {
> +	case UDP_TUNNEL_TYPE_VXLAN:
> +		tnl_type = TNL_VXLAN;
> +		break;
> +	case UDP_TUNNEL_TYPE_GENEVE:
> +		tnl_type = TNL_GENEVE;
> +		break;
> +	default:
> +		netdev_err(netdev, "Unknown tunnel type\n");
> +		return;
> +	}
> +
> +	status = ice_create_tunnel(&pf->hw, tnl_type, port);
> +	if (status == ICE_ERR_ALREADY_EXISTS)
> +		dev_dbg(ice_pf_to_dev(pf), "port %d already exists in UDP tunnels list\n",
> +			port);
> +	else if (status == ICE_ERR_OUT_OF_RANGE)
> +		netdev_err(netdev, "Max tunneled UDP ports reached, port %d not added\n",
> +			   port);

error is probably a little much for resource exhaustion since it's not
going to cause any problem other than a slow down?

> +	else if (status)
> +		netdev_err(netdev, "Error adding UDP tunnel - %d\n",
> +			   status);
> +}
> +
> +/**
> + * ice_udp_tunnel_del - Get notifications about UDP tunnel ports that go away
> + * @netdev: This physical port's netdev
> + * @ti: Tunnel endpoint information
> + */
> +static void
> +ice_udp_tunnel_del(struct net_device *netdev, struct udp_tunnel_info *ti)
> +{
> +	struct ice_netdev_priv *np = netdev_priv(netdev);
> +	struct ice_vsi *vsi = np->vsi;
> +	struct ice_pf *pf = vsi->back;
> +	u16 port = ntohs(ti->port);
> +	enum ice_status status;
> +	bool retval;
> +	u16 index;
> +
> +	retval = ice_tunnel_port_in_use(&pf->hw, port, &index);

nit: index is never used

> +	if (!retval) {
> +		netdev_err(netdev, "port %d not found in UDP tunnels list\n",
> +			   port);
> +		return;
> +	}
> +
> +	status = ice_destroy_tunnel(&pf->hw, port, false);
> +	if (status)
> +		netdev_err(netdev, "error deleting port %d from UDP tunnels list\n",
> +			   port);
> +}

> @@ -5294,4 +5380,6 @@ static const struct net_device_ops ice_netdev_ops = {
>  	.ndo_bpf = ice_xdp,
>  	.ndo_xdp_xmit = ice_xdp_xmit,
>  	.ndo_xsk_wakeup = ice_xsk_wakeup,
> +	.ndo_udp_tunnel_add = ice_udp_tunnel_add,
> +	.ndo_udp_tunnel_del = ice_udp_tunnel_del,
>  };