[PATCH RFC net-next 0/2] pds_core: add switchdev and tc for vlan offload

[PATCH RFC net-next 0/2] pds_core: add switchdev and tc for vlan offload

[Shannon Nelson]

This is an RFC for adding to the pds_core driver some very simple support
for VF representors and a tc command for offloading VF port vlans.

The problem to solve is how to request that a NIC do the push/pop of port
vlans on a VF.  The initial pds_core patchset[0] included this support
through the legacy ip-link methods with a PF netdev that had no datapath,
simply existing to enable commands such as
    ip link set <pf> vf <vfid> vlan <vid>
This was soundly squashed with a request to create proper VF representors.
The pds_core driver has since been reworked and merged without this feature.

This pair of patches is a first attempt at adding support for a simple
VF representor and tc offload which I've been tinkering with off and
on over the last few weeks.  I will acknowledge that we have no proper
filtering offload language in our firmware's adminq interface yet.
This has been mentioned internally and is a "future project" with no
actual schedule yet.  Given that, I have worked here with what I have,
using the existing vf_setattr function.

An alternative that later occured to me is to make this a "devlink port
function" thing, similar to the existing port mac.  This would have the
benefit of using a familiar concept from and similar single command as
the legacy method, would allow early port setup as with setting the mac
and other port features, and would not need to create a lot of mostly
empty netdevs for the VF representors.  I don't know if this would then
lead to adding "trust" and "spoofcheck" as well, but I'm not aware of any
other solutions for them, either.  This also might make more sense for
devices that don't end up as user network interfaces, such as a virtio
block device that runs over ethernet on the back end.  I don't have RFC
code for this idea, but thought I would toss it out for discussion -
I didn't see any previous related discussion in a (rather quick) search.

I welcome your comments and suggestions.

Thanks,
sln

[0]: https://lore.kernel.org/netdev/20221118225656.48309-1-snelson@pensando.io/

Shannon Nelson (2):
  pds_core: netdev representors for each VF
  pds_core: tc command handling for vlan push-pop

 drivers/net/ethernet/amd/pds_core/Makefile |   1 +
 drivers/net/ethernet/amd/pds_core/core.h   |  12 +
 drivers/net/ethernet/amd/pds_core/main.c   |  28 +-
 drivers/net/ethernet/amd/pds_core/rep.c    | 322 +++++++++++++++++++++
 4 files changed, 361 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/amd/pds_core/rep.c

-- 
2.17.1

[PATCH RFC net-next 1/2] pds_core: netdev representors for each VF

[Shannon Nelson]

When using pds_core VFs, we want to do some amount of control
and configuration - specifically, we need the ability to
configure port vlans.  Here we add switchdev and create
representor netdevs for the VFs.

The pds_core switchdev can be enabled before or after the
VFs have been enabled.  This means that they can be brought
up specifically for configuring the VF vlan, then can be
immediately turned back off.  Disabling the switchdev will
not affect VF vlan configuration.

Due to the design of this particular device, there is no packet
switching to the PF: the only datapaths available are through
the VFs.  Therefore, neither the PF nor the VF representors
have packet processing queues.  For now, these representors
are just enough to send a few tc configuration commands to
the FW for setting port vlans on each VF.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/Makefile |   1 +
 drivers/net/ethernet/amd/pds_core/core.h   |  10 ++
 drivers/net/ethernet/amd/pds_core/main.c   |  28 ++++-
 drivers/net/ethernet/amd/pds_core/rep.c    | 140 +++++++++++++++++++++
 4 files changed, 177 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/amd/pds_core/rep.c

diff --git a/drivers/net/ethernet/amd/pds_core/Makefile b/drivers/net/ethernet/amd/pds_core/Makefile
index 0abc33ce826c..3bfcfa4eda42 100644
--- a/drivers/net/ethernet/amd/pds_core/Makefile
+++ b/drivers/net/ethernet/amd/pds_core/Makefile
@@ -9,6 +9,7 @@ pds_core-y := main.o \
 	      dev.o \
 	      adminq.o \
 	      core.o \
+	      rep.o \
 	      fw.o
 
 pds_core-$(CONFIG_DEBUG_FS) += debugfs.o
diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
index e545fafc4819..2f38143dd5c2 100644
--- a/drivers/net/ethernet/amd/pds_core/core.h
+++ b/drivers/net/ethernet/amd/pds_core/core.h
@@ -171,6 +171,7 @@ struct pdsc {
 	unsigned int wdtimer_period;
 	struct work_struct health_work;
 	struct devlink_health_reporter *fw_reporter;
+	struct devlink_port dl_port;
 	u32 fw_recoveries;
 
 	struct pdsc_devinfo dev_info;
@@ -196,6 +197,9 @@ struct pdsc {
 	struct pdsc_qcq notifyqcq;
 	u64 last_eid;
 	struct pdsc_viftype *viftype_status;
+
+	struct net_device *netdev;
+	enum devlink_eswitch_mode eswitch_mode;
 };
 
 /** enum pds_core_dbell_bits - bitwise composition of dbell values.
@@ -309,4 +313,10 @@ irqreturn_t pdsc_adminq_isr(int irq, void *data);
 
 int pdsc_firmware_update(struct pdsc *pdsc, const struct firmware *fw,
 			 struct netlink_ext_ack *extack);
+
+int pdsc_add_rep(struct pdsc *vf, struct pdsc *pf);
+void pdsc_del_rep(struct pdsc *vf);
+int pdsc_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode);
+int pdsc_dl_eswitch_mode_set(struct devlink *devlink, u16 mode,
+			     struct netlink_ext_ack *extack);
 #endif /* _PDSC_H_ */
diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index e2d14b1ca471..272a8979e53d 100644
--- a/drivers/net/ethernet/amd/pds_core/main.c
+++ b/drivers/net/ethernet/amd/pds_core/main.c
@@ -185,12 +185,27 @@ static int pdsc_init_vf(struct pdsc *vf)
 
 	pf->vfs[vf->vf_id].vf = vf;
 	err = pdsc_auxbus_dev_add(vf, pf);
-	if (err) {
+	if (err)
+		goto err_dl_unreg;
+
+	if (pf->eswitch_mode == DEVLINK_ESWITCH_MODE_SWITCHDEV) {
 		devl_lock(dl);
-		devl_unregister(dl);
+		err = pdsc_add_rep(vf, pf);
 		devl_unlock(dl);
+		if (err)
+			goto err_del_aux;
 	}
 
+	return 0;
+
+err_del_aux:
+	pdsc_auxbus_dev_del(vf, pf);
+	pf->vfs[vf->vf_id].vf = NULL;
+err_dl_unreg:
+	devl_lock(dl);
+	devl_unregister(dl);
+	devl_unlock(dl);
+
 	return err;
 }
 
@@ -262,6 +277,8 @@ static int pdsc_init_pf(struct pdsc *pdsc)
 		goto err_out_unlock_dl;
 	}
 
+	pdsc->eswitch_mode = DEVLINK_ESWITCH_MODE_LEGACY;
+
 	hr = devl_health_reporter_create(dl, &pdsc_fw_reporter_ops, 0, pdsc);
 	if (IS_ERR(hr)) {
 		dev_warn(pdsc->dev, "Failed to create fw reporter: %pe\n", hr);
@@ -304,6 +321,8 @@ static int pdsc_init_pf(struct pdsc *pdsc)
 static const struct devlink_ops pdsc_dl_ops = {
 	.info_get	= pdsc_dl_info_get,
 	.flash_update	= pdsc_dl_flash_update,
+	.eswitch_mode_set = pdsc_dl_eswitch_mode_set,
+	.eswitch_mode_get = pdsc_dl_eswitch_mode_get,
 };
 
 static const struct devlink_ops pdsc_dl_vf_ops = {
@@ -406,6 +425,11 @@ static void pdsc_remove(struct pci_dev *pdev)
 
 		pf = pdsc_get_pf_struct(pdsc->pdev);
 		if (!IS_ERR(pf)) {
+			if (pf->eswitch_mode == DEVLINK_ESWITCH_MODE_SWITCHDEV) {
+				devl_lock(dl);
+				pdsc_del_rep(pdsc);
+				devl_unlock(dl);
+			}
 			pdsc_auxbus_dev_del(pdsc, pf);
 			pf->vfs[pdsc->vf_id].vf = NULL;
 		}
diff --git a/drivers/net/ethernet/amd/pds_core/rep.c b/drivers/net/ethernet/amd/pds_core/rep.c
new file mode 100644
index 000000000000..297d9e2bac31
--- /dev/null
+++ b/drivers/net/ethernet/amd/pds_core/rep.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2023 Advanced Micro Devices, Inc */
+
+#include <linux/pci.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+
+#include "core.h"
+
+struct pds_rep {
+	struct pdsc *vf;
+	struct pdsc *pf;
+};
+
+static const struct net_device_ops pdsc_rep_netdev_ops = {
+};
+
+static void pdsc_get_rep_drvinfo(struct net_device *netdev,
+				 struct ethtool_drvinfo *drvinfo)
+{
+	struct pds_rep *rep = netdev_priv(netdev);
+
+	strscpy(drvinfo->driver, PDS_CORE_DRV_NAME,
+		sizeof(drvinfo->driver));
+	strscpy(drvinfo->bus_info, pci_name(rep->vf->pdev),
+		sizeof(drvinfo->bus_info));
+	strscpy(drvinfo->fw_version, rep->pf->dev_info.fw_version,
+		sizeof(drvinfo->fw_version));
+}
+
+static const struct ethtool_ops pdsc_rep_ethtool_ops = {
+	.get_drvinfo = pdsc_get_rep_drvinfo,
+};
+
+int pdsc_add_rep(struct pdsc *vf, struct pdsc *pf)
+{
+	struct pds_rep *rep;
+	int err = 0;
+
+	memset(&vf->dl_port, 0, sizeof(vf->dl_port));
+	devlink_port_attrs_pci_vf_set(&vf->dl_port, 0, PCI_FUNC(pf->pdev->devfn),
+				      vf->vf_id, false);
+	err = devl_port_register(priv_to_devlink(vf), &vf->dl_port, vf->vf_id);
+	if (err) {
+		dev_err(vf->dev, "devlink_port_register failed: %pe\n",
+			ERR_PTR(err));
+		return err;
+	}
+
+	vf->netdev = alloc_etherdev(sizeof(struct pds_rep));
+	if (!vf->netdev) {
+		err = -ENOMEM;
+		goto err_unreg_port;
+	}
+	SET_NETDEV_DEV(vf->netdev, vf->dev);
+
+	rep = netdev_priv(vf->netdev);
+	rep->pf = pf;
+	rep->vf = vf;
+
+	vf->netdev->netdev_ops = &pdsc_rep_netdev_ops;
+	vf->netdev->ethtool_ops = &pdsc_rep_ethtool_ops;
+	netif_carrier_off(vf->netdev);
+
+	SET_NETDEV_DEVLINK_PORT(vf->netdev,  &vf->dl_port);
+	err = register_netdev(vf->netdev);
+	if (err) {
+		dev_err(vf->dev, "register_netdev failed: %pe\n",
+			ERR_PTR(err));
+		goto err_free_netdev;
+	}
+
+	return 0;
+
+err_free_netdev:
+	free_netdev(vf->netdev);
+	vf->netdev = NULL;
+err_unreg_port:
+	devl_port_unregister(&vf->dl_port);
+	return err;
+}
+
+void pdsc_del_rep(struct pdsc *vf)
+{
+	unregister_netdev(vf->netdev);
+	free_netdev(vf->netdev);
+	vf->netdev = NULL;
+	devl_port_unregister(&vf->dl_port);
+}
+
+int pdsc_dl_eswitch_mode_get(struct devlink *dl, u16 *mode)
+{
+	struct pdsc *pf = devlink_priv(dl);
+
+	*mode = pf->eswitch_mode;
+	return 0;
+}
+
+int pdsc_dl_eswitch_mode_set(struct devlink *dl, u16 mode,
+			     struct netlink_ext_ack *extack)
+{
+	struct pdsc *pf = devlink_priv(dl);
+	char *msg;
+	int ret = 0;
+	int i;
+
+	if (pf->eswitch_mode == mode)
+		return 0;
+
+	switch (mode) {
+	case DEVLINK_ESWITCH_MODE_LEGACY:
+		for (i = pf->num_vfs - 1; i >= 0; i--)
+			pdsc_del_rep(pf->vfs[i].vf);
+		msg = "Changed eswitch mode to legacy";
+		dev_info(pf->dev, msg);
+		NL_SET_ERR_MSG_FMT_MOD(extack, "%s", msg);
+		break;
+
+	case DEVLINK_ESWITCH_MODE_SWITCHDEV:
+		for (i = 0; i < pf->num_vfs && !ret; i++)
+			ret = pdsc_add_rep(pf->vfs[i].vf, pf);
+		if (ret) {
+			for (i-- ; i >= 0; i--)
+				pdsc_del_rep(pf->vfs[i].vf);
+			return ret;
+		}
+
+		msg = "Changed eswitch mode to switchdev";
+		dev_info(pf->dev, msg);
+		NL_SET_ERR_MSG_FMT_MOD(extack, "%s", msg);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	pf->eswitch_mode = mode;
+
+	return 0;
+}
-- 
2.17.1

[PATCH RFC net-next 2/2] pds_core: tc command handling for vlan push-pop

[Shannon Nelson]

Set up handling of tc commands for adding VF vlan push-pop HW
offload through the VF's representor.  The FW doesn't currently
have a proper set of adminq commands for any tc filtering,
that's a future discussion.  For now we have to get by with
using the existing VF_SETATTR command that will add the push
and pop in a single request.

Example commands for use:

    # add a qdisc for context
    tc qdisc add dev eth0 handle ffff: ingress

    # add a rule to wrap outgoing traffic with vlan id 124
    tc filter add dev eth0 parent ffff: pref 11 protocol all u32 skip_sw \
	   match u32 0 0 flowid 1:1 action vlan push id 124

    # add a rule to pop the tag from incoming traffic
    # this is required normally, but redundant with current FW
    tc filter add dev eth0 parent ffff: pref 1 protocol 802.1Q u32 skip_sw \
	   match u32 0 0 action vlan pop

    # remove rules
    tc filter del dev eth0 parent ffff:

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/core.h |   2 +
 drivers/net/ethernet/amd/pds_core/rep.c  | 182 +++++++++++++++++++++++
 2 files changed, 184 insertions(+)

diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
index 2f38143dd5c2..ebd55bc0a7dc 100644
--- a/drivers/net/ethernet/amd/pds_core/core.h
+++ b/drivers/net/ethernet/amd/pds_core/core.h
@@ -37,6 +37,8 @@ struct pdsc_vf {
 	struct pdsc *vf;
 	u16     index;
 	__le16  vif_types[PDS_DEV_TYPE_MAX];
+
+	u16     vlanid;
 };
 
 struct pdsc_devinfo {
diff --git a/drivers/net/ethernet/amd/pds_core/rep.c b/drivers/net/ethernet/amd/pds_core/rep.c
index 297d9e2bac31..265b3be5b9d3 100644
--- a/drivers/net/ethernet/amd/pds_core/rep.c
+++ b/drivers/net/ethernet/amd/pds_core/rep.c
@@ -4,15 +4,194 @@
 #include <linux/pci.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
+#include <net/pkt_cls.h>
+#include <net/tc_act/tc_vlan.h>
 
 #include "core.h"
 
 struct pds_rep {
 	struct pdsc *vf;
 	struct pdsc *pf;
+	u32 vlan_offload_handle;
+	u16 vlan_id;
 };
 
+static int pdsc_set_vf_vlan(struct net_device *netdev, int vf_id,
+			    u16 vid, u8 qos, __be16 proto)
+{
+	struct pds_rep *vf_rep = netdev_priv(netdev);
+	union pds_core_dev_comp comp = { 0 };
+	union pds_core_dev_cmd cmd = {
+		.vf_setattr.opcode = PDS_CORE_CMD_VF_SETATTR,
+		.vf_setattr.attr = PDS_CORE_VF_ATTR_VLAN,
+		.vf_setattr.vf_index = cpu_to_le16(vf_id),
+		.vf_setattr.vlanid = cpu_to_le16(vid),
+	};
+	struct pdsc *pf = vf_rep->pf;
+	int err;
+
+	netdev_info(netdev, "%s: vf %d vlan %d\n", __func__, vf_id, vid);
+
+	err = pdsc_devcmd(pf, &cmd, &comp, pf->devcmd_timeout);
+	if (!err)
+		pf->vfs[vf_id].vlanid = vid;
+
+	return err;
+}
+
+static void print_cls(struct net_device *netdev,
+		      struct tc_cls_u32_offload *cls)
+{
+	netdev_info(netdev, "cmd %d chain_i %d proto %#x prio %d\n",
+		    cls->command, cls->common.chain_index,
+		    cls->common.protocol, cls->common.prio);
+	netdev_info(netdev, "  handle %#x val %#x mask %#x link_handle %#x fshift %d\n",
+		    cls->knode.handle, cls->knode.val, cls->knode.mask,
+		    cls->knode.link_handle, cls->knode.fshift);
+	netdev_info(netdev, "  exts %p res %p sel %p\n",
+		    cls->knode.exts,  cls->knode.res,  cls->knode.sel);
+}
+
+static int pdsc_configure_clsu32(struct net_device *netdev,
+				 struct tc_cls_u32_offload *cls)
+{
+	struct pds_rep *vf_rep = netdev_priv(netdev);
+	const struct tc_action *a, *act = NULL;
+	int err = 0;
+	u16 vid;
+	int i;
+
+	netdev_info(netdev, "%s: top handle %#x\n",
+		    __func__, vf_rep->vlan_offload_handle);
+	print_cls(netdev, cls);
+
+	if (!tcf_exts_has_actions(cls->knode.exts))
+		return -EINVAL;
+
+	/* only one action TCA_ID_VLAN is supported */
+	tcf_exts_for_each_action(i, a, cls->knode.exts) {
+		if (!is_tcf_vlan(a)) {
+			netdev_err(netdev, "%s: unsupported action %d\n",
+				   __func__, a->ops->id);
+			return -EOPNOTSUPP;
+		} else if (act) {
+			netdev_err(netdev, "%s: multiple vlan actions?\n",
+				   __func__);
+			return -EOPNOTSUPP;
+		}
+
+		act = a;
+	}
+
+	switch (tcf_vlan_action(act)) {
+	case TCA_VLAN_ACT_PUSH:
+		vid = tcf_vlan_push_vid(act);
+		if (!vid)
+			return -EINVAL;
+
+		/* only one allowed at a time */
+		if (vf_rep->vlan_id)
+			return -EBUSY;
+
+		/* with existing FW this will set up both push and pop */
+		err = pdsc_set_vf_vlan(netdev, vf_rep->vf->vf_id, vid, 0,
+				       tcf_vlan_push_proto(act));
+		if (!err) {
+			vf_rep->vlan_id = vid;
+			vf_rep->vlan_offload_handle = TC_U32_USERHTID(cls->knode.handle);
+		}
+		break;
+	case TCA_VLAN_ACT_POP:
+		/* with existing FW this is redundant */
+		err = 0;
+		break;
+	default:
+		netdev_err(netdev, "%s: tcf_vlan_action %d unsupported\n",
+			   __func__, tcf_vlan_action(act));
+		return -EOPNOTSUPP;
+	}
+
+	return err;
+}
+
+static int pdsc_delete_clsu32(struct net_device *netdev,
+			      struct tc_cls_u32_offload *cls)
+{
+	struct pds_rep *vf_rep = netdev_priv(netdev);
+	int err;
+
+	netdev_info(netdev, "%s: top handle %#x\n",
+		    __func__, vf_rep->vlan_offload_handle);
+	print_cls(netdev, cls);
+
+	if (vf_rep->vlan_offload_handle != TC_U32_USERHTID(cls->knode.handle))
+		return -EINVAL;
+
+	netdev_info(netdev, "%s: vf_id %d vlan_id %d delete\n",
+		    __func__, vf_rep->vf->vf_id, vf_rep->vlan_id);
+
+	/* with existing FW this will remove both push and pop */
+	err = pdsc_set_vf_vlan(netdev, vf_rep->vf->vf_id, 0, 0, 0);
+	vf_rep->vlan_id = 0;
+	vf_rep->vlan_offload_handle = 0;
+
+	return err;
+}
+
+static int pdsc_setup_tc_cls_u32(struct net_device *netdev,
+				 struct tc_cls_u32_offload *cls_u32)
+{
+	switch (cls_u32->command) {
+	case TC_CLSU32_NEW_KNODE:
+	case TC_CLSU32_REPLACE_KNODE:
+		return pdsc_configure_clsu32(netdev, cls_u32);
+	case TC_CLSU32_DELETE_KNODE:
+		return pdsc_delete_clsu32(netdev, cls_u32);
+
+	case TC_CLSU32_NEW_HNODE:
+	case TC_CLSU32_REPLACE_HNODE:
+	case TC_CLSU32_DELETE_HNODE:
+		return 0;
+	default:
+		netdev_info(netdev, "%s: unhandled cls_u32->command = %d\n",
+			    __func__, cls_u32->command);
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pdsc_setup_tc_block_cb(enum tc_setup_type tc_type, void *type_data,
+				  void *cb_priv)
+{
+	struct net_device *netdev = cb_priv;
+
+	if (!tc_cls_can_offload_and_chain0(netdev, type_data))
+		return -EOPNOTSUPP;
+
+	switch (tc_type) {
+	case TC_SETUP_CLSU32:
+		return pdsc_setup_tc_cls_u32(netdev, type_data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static LIST_HEAD(pdsc_block_cb_list);
+
+static int pdsc_setup_tc(struct net_device *netdev, enum tc_setup_type tc_type, void *type_data)
+{
+	switch (tc_type) {
+	case TC_SETUP_BLOCK:
+		return flow_block_cb_setup_simple(type_data,
+						  &pdsc_block_cb_list,
+						  pdsc_setup_tc_block_cb,
+						  netdev, netdev, true);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static const struct net_device_ops pdsc_rep_netdev_ops = {
+	.ndo_setup_tc		= pdsc_setup_tc,
 };
 
 static void pdsc_get_rep_drvinfo(struct net_device *netdev,
@@ -60,6 +239,9 @@ int pdsc_add_rep(struct pdsc *vf, struct pdsc *pf)
 
 	vf->netdev->netdev_ops = &pdsc_rep_netdev_ops;
 	vf->netdev->ethtool_ops = &pdsc_rep_ethtool_ops;
+	vf->netdev->features |= NETIF_F_HW_TC;
+	vf->netdev->hw_features |= NETIF_F_HW_TC;
+
 	netif_carrier_off(vf->netdev);
 
 	SET_NETDEV_DEVLINK_PORT(vf->netdev,  &vf->dl_port);
-- 
2.17.1

Re: [PATCH RFC net-next 0/2] pds_core: add switchdev and tc for vlan offload

[Samudrala, Sridhar]

On 4/27/2023 11:45 AM, Shannon Nelson wrote:
> This is an RFC for adding to the pds_core driver some very simple support
> for VF representors and a tc command for offloading VF port vlans.
> 
> The problem to solve is how to request that a NIC do the push/pop of port
> vlans on a VF.  The initial pds_core patchset[0] included this support
> through the legacy ip-link methods with a PF netdev that had no datapath,
> simply existing to enable commands such as
>      ip link set <pf> vf <vfid> vlan <vid>
> This was soundly squashed with a request to create proper VF representors.
> The pds_core driver has since been reworked and merged without this feature.
> 
> This pair of patches is a first attempt at adding support for a simple
> VF representor and tc offload which I've been tinkering with off and
> on over the last few weeks.  I will acknowledge that we have no proper
> filtering offload language in our firmware's adminq interface yet.
> This has been mentioned internally and is a "future project" with no
> actual schedule yet.  Given that, I have worked here with what I have,
> using the existing vf_setattr function.
> 
> An alternative that later occured to me is to make this a "devlink port
> function" thing, similar to the existing port mac.  This would have the
> benefit of using a familiar concept from and similar single command as
> the legacy method, would allow early port setup as with setting the mac
> and other port features, and would not need to create a lot of mostly
> empty netdevs for the VF representors.  I don't know if this would then
> lead to adding "trust" and "spoofcheck" as well, but I'm not aware of any
> other solutions for them, either.  This also might make more sense for
> devices that don't end up as user network interfaces, such as a virtio
> block device that runs over ethernet on the back end.  I don't have RFC
> code for this idea, but thought I would toss it out for discussion -
> I didn't see any previous related discussion in a (rather quick) search.
> 
> I welcome your comments and suggestions.

Adding a tc rule doesn't seem to be the right interface to
configure port-vlan for all packets sent/received on a port.
tc would be appropriate if we want to do push/pop vlan on certain flows.

   bridge vlan add dev $VF_REP vid 200 pvid untagged master
would be a better option. This may require adding vf reps to a bridge
and enable vlan_filtering on the bridge. The driver needs to register
for SWITCHDEV_PORT_OBJ_ADD events.

Extending devlink port function to support port-vlan similar to setting 
a mac address also looks like a good option without the requirement of
having to add port reps to a bridge.

> 
> Thanks,
> sln
> 
> [0]: https://lore.kernel.org/netdev/20221118225656.48309-1-snelson@pensando.io/
> 
> Shannon Nelson (2):
>    pds_core: netdev representors for each VF
>    pds_core: tc command handling for vlan push-pop
> 
>   drivers/net/ethernet/amd/pds_core/Makefile |   1 +
>   drivers/net/ethernet/amd/pds_core/core.h   |  12 +
>   drivers/net/ethernet/amd/pds_core/main.c   |  28 +-
>   drivers/net/ethernet/amd/pds_core/rep.c    | 322 +++++++++++++++++++++
>   4 files changed, 361 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/net/ethernet/amd/pds_core/rep.c
> 

Re: [PATCH RFC net-next 0/2] pds_core: add switchdev and tc for vlan offload

[Shannon Nelson]

On 4/28/23 3:52 PM, Samudrala, Sridhar wrote:
> 
> On 4/27/2023 11:45 AM, Shannon Nelson wrote:
>> This is an RFC for adding to the pds_core driver some very simple support
>> for VF representors and a tc command for offloading VF port vlans.
>>
>> The problem to solve is how to request that a NIC do the push/pop of port
>> vlans on a VF.  The initial pds_core patchset[0] included this support
>> through the legacy ip-link methods with a PF netdev that had no datapath,
>> simply existing to enable commands such as
>>      ip link set <pf> vf <vfid> vlan <vid>
>> This was soundly squashed with a request to create proper VF 
>> representors.
>> The pds_core driver has since been reworked and merged without this 
>> feature.
>>
>> This pair of patches is a first attempt at adding support for a simple
>> VF representor and tc offload which I've been tinkering with off and
>> on over the last few weeks.  I will acknowledge that we have no proper
>> filtering offload language in our firmware's adminq interface yet.
>> This has been mentioned internally and is a "future project" with no
>> actual schedule yet.  Given that, I have worked here with what I have,
>> using the existing vf_setattr function.
>>
>> An alternative that later occured to me is to make this a "devlink port
>> function" thing, similar to the existing port mac.  This would have the
>> benefit of using a familiar concept from and similar single command as
>> the legacy method, would allow early port setup as with setting the mac
>> and other port features, and would not need to create a lot of mostly
>> empty netdevs for the VF representors.  I don't know if this would then
>> lead to adding "trust" and "spoofcheck" as well, but I'm not aware of any
>> other solutions for them, either.  This also might make more sense for
>> devices that don't end up as user network interfaces, such as a virtio
>> block device that runs over ethernet on the back end.  I don't have RFC
>> code for this idea, but thought I would toss it out for discussion -
>> I didn't see any previous related discussion in a (rather quick) search.
>>
>> I welcome your comments and suggestions.
> 
> Adding a tc rule doesn't seem to be the right interface to
> configure port-vlan for all packets sent/received on a port.
> tc would be appropriate if we want to do push/pop vlan on certain flows.

This is something that had occurred to me.  The tc commands seem to be 
aimed at a finer grain control than what I need for this case.

> 
>    bridge vlan add dev $VF_REP vid 200 pvid untagged master
> would be a better option. This may require adding vf reps to a bridge
> and enable vlan_filtering on the bridge. The driver needs to register
> for SWITCHDEV_PORT_OBJ_ADD events.

I thought about this as well, but this also seems to be abusing the 
model when there are no data streams and no representor for the PF.

> 
> Extending devlink port function to support port-vlan similar to setting
> a mac address also looks like a good option without the requirement of
> having to add port reps to a bridge.

I'm hoping that others will chime in on this idea.  Meanwhile, I'll see 
if I can work up an RFC for this thought later this week.

sln

> 
>>
>> Thanks,
>> sln
>>
>> [0]: 
>> https://lore.kernel.org/netdev/20221118225656.48309-1-snelson@pensando.io/
>>
>> Shannon Nelson (2):
>>    pds_core: netdev representors for each VF
>>    pds_core: tc command handling for vlan push-pop
>>
>>   drivers/net/ethernet/amd/pds_core/Makefile |   1 +
>>   drivers/net/ethernet/amd/pds_core/core.h   |  12 +
>>   drivers/net/ethernet/amd/pds_core/main.c   |  28 +-
>>   drivers/net/ethernet/amd/pds_core/rep.c    | 322 +++++++++++++++++++++
>>   4 files changed, 361 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/net/ethernet/amd/pds_core/rep.c
>>

Re: [PATCH RFC net-next 0/2] pds_core: add switchdev and tc for vlan offload

[Jakub Kicinski]

On Thu, 27 Apr 2023 09:45:44 -0700 Shannon Nelson wrote:
> This is an RFC for adding to the pds_core driver some very simple support
> for VF representors and a tc command for offloading VF port vlans.
> 
> The problem to solve is how to request that a NIC do the push/pop of port
> vlans on a VF.  The initial pds_core patchset[0] included this support
> through the legacy ip-link methods with a PF netdev that had no datapath,
> simply existing to enable commands such as
>     ip link set <pf> vf <vfid> vlan <vid>
> This was soundly squashed with a request to create proper VF representors.
> The pds_core driver has since been reworked and merged without this feature.

Have you read the representors documentation? Passing traffic is
crucial.

> This pair of patches is a first attempt at adding support for a simple
> VF representor and tc offload which I've been tinkering with off and
> on over the last few weeks.  I will acknowledge that we have no proper
> filtering offload language in our firmware's adminq interface yet.
> This has been mentioned internally and is a "future project" with no
> actual schedule yet.  Given that, I have worked here with what I have,
> using the existing vf_setattr function.
> 
> An alternative that later occured to me is to make this a "devlink port
> function" thing, similar to the existing port mac.  This would have the
> benefit of using a familiar concept from and similar single command as
> the legacy method, would allow early port setup as with setting the mac
> and other port features, and would not need to create a lot of mostly
> empty netdevs for the VF representors.  I don't know if this would then
> lead to adding "trust" and "spoofcheck" as well, but I'm not aware of any
> other solutions for them, either.  This also might make more sense for
> devices that don't end up as user network interfaces, such as a virtio
> block device that runs over ethernet on the back end.  I don't have RFC
> code for this idea, but thought I would toss it out for discussion -
> I didn't see any previous related discussion in a (rather quick) search.

No, no -- the problem is not rtnetlink vs devlink but the fact 
that the old API was inventing its own parallel way of configuring
forwarding outside of normal/SW netdev concepts.

Re: [PATCH RFC net-next 0/2] pds_core: add switchdev and tc for vlan offload

[Shannon Nelson]

On 5/2/23 4:43 PM, Jakub Kicinski wrote:
> 
> On Thu, 27 Apr 2023 09:45:44 -0700 Shannon Nelson wrote:
>> This is an RFC for adding to the pds_core driver some very simple support
>> for VF representors and a tc command for offloading VF port vlans.
>>
>> The problem to solve is how to request that a NIC do the push/pop of port
>> vlans on a VF.  The initial pds_core patchset[0] included this support
>> through the legacy ip-link methods with a PF netdev that had no datapath,
>> simply existing to enable commands such as
>>      ip link set <pf> vf <vfid> vlan <vid>
>> This was soundly squashed with a request to create proper VF representors.
>> The pds_core driver has since been reworked and merged without this feature.
> 
> Have you read the representors documentation? Passing traffic is
> crucial.

Given that there is no traffic to the host PF in this case, and I can't 
do anything more than wave my hands at vague promises for the future, 
perhaps tc and representors is the wrong path for now.

But there remains a need for some port related configuration.  I can 
take a stab at adding this concept to "devlink port function" and see 
what discussion follows from there.

sln

Re: [PATCH RFC net-next 0/2] pds_core: add switchdev and tc for vlan offload

[Jakub Kicinski]

On Wed, 3 May 2023 15:49:27 -0700 Shannon Nelson wrote:
> Given that there is no traffic to the host PF in this case, and I can't 
> do anything more than wave my hands at vague promises for the future, 
> perhaps tc and representors is the wrong path for now.

If it's not a priority for AMD to have an upstream implementation
that's fine. There's not point over-complicating the discussion.

> But there remains a need for some port related configuration.  I can 
> take a stab at adding this concept to "devlink port function" and see 
> what discussion follows from there.

You mean setting vlan encap via devlink?
I don't know why you'd do that. It will certainly aggravate me,
and I doubt anyone will care/support you.

Re: [PATCH RFC net-next 0/2] pds_core: add switchdev and tc for vlan offload

[Shannon Nelson]

On 5/3/23 6:27 PM, Jakub Kicinski wrote:
> 
> On Wed, 3 May 2023 15:49:27 -0700 Shannon Nelson wrote:
>> Given that there is no traffic to the host PF in this case, and I can't
>> do anything more than wave my hands at vague promises for the future,
>> perhaps tc and representors is the wrong path for now.
> 
> If it's not a priority for AMD to have an upstream implementation
> that's fine. There's not point over-complicating the discussion.
> 
>> But there remains a need for some port related configuration.  I can
>> take a stab at adding this concept to "devlink port function" and see
>> what discussion follows from there.
> 
> You mean setting vlan encap via devlink?
> I don't know why you'd do that. It will certainly aggravate me,
> and I doubt anyone will care/support you.

We're trying to solve what would seem to be a simple problem for our 
customer: how to do basic vlan encap/decap on all traffic going in and 
of a VF.  With no host PF traffic, the legacy ip-link and the newer 
switchdev+tc solutions don't fit.  As this is VF port setup, and devlink 
is meant for device setup, it would seem to fit in the devlink port 
function model similar to the setting of the port hw_addr.  What am I 
missing that makes this an unacceptable answer?

I understand you don't like this devlink port function suggestion, but 
when I go back to negotiate with internal management, architects, etc, I 
can get a lot further with them if I have a technical explanation of why 
this is not acceptable.  So, at the risk of further aggravating you, can 
I request a little more detail on why this is a bad idea?

Thanks,
sln

Re: [PATCH RFC net-next 0/2] pds_core: add switchdev and tc for vlan offload

[Jakub Kicinski]

On Thu, 4 May 2023 13:51:13 -0700 Shannon Nelson wrote:
> On 5/3/23 6:27 PM, Jakub Kicinski wrote:
> > You mean setting vlan encap via devlink?
> > I don't know why you'd do that. It will certainly aggravate me,
> > and I doubt anyone will care/support you.  
> 
> We're trying to solve what would seem to be a simple problem for our 
> customer: how to do basic vlan encap/decap on all traffic going in and 
> of a VF. 

Offload bridge.

If your customer doesn't want the bridge offload you can decide 
to ship an out of tree driver for them with whatever deprecated 
APIs you please.

> With no host PF traffic, the legacy ip-link and the newer 
> switchdev+tc solutions don't fit.  As this is VF port setup, and devlink 
> is meant for device setup, it would seem to fit in the devlink port 
> function model similar to the setting of the port hw_addr.  What am I 
> missing that makes this an unacceptable answer?
> 
> I understand you don't like this devlink port function suggestion, but 
> when I go back to negotiate with internal management, architects, etc, I 
> can get a lot further with them if I have a technical explanation of why 
> this is not acceptable.  So, at the risk of further aggravating you, can 
> I request a little more detail on why this is a bad idea?

The direction of the community was to use offload of standard networking
concepts like switching, routing and flow matching for probably a decade
now.