[Intel-wired-lan] [PATCH net-next v3 1/2] i40e: Add ability to change VFs default MAC address

[Intel-wired-lan] [PATCH net-next v3 1/2] i40e: Add ability to change VFs default MAC address

[Jan Sokolowski]

From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>

Currently there is no way for a VF driver to specify if it wants to
change it's hardware address. New bits are being added to virtchnl.h
in struct virtchnl_ether_addr that allow the VF to correctly
communicate this information. However, legacy VF drivers that don't
support the new virtchnl.h bits still need to be supported. Make a
best effort attempt at saving the VF's primary/device address in the
legacy case and depend on the VIRTCHNL_ETHER_ADDR_PRIMARY type for
the new case.

Legacy case - If a unicast MAC is being added and the
default_lan_addr.addr is empty, then populate it. This assumes that the
address is the VF's hardware address. If unicast MAC is being deleted
and it matches the default_lan_addr.addr save the time when it happened
and replace it with the last MAC address on the MAC filter list.
If a unicast MAC is being added and the default_lan_addr.addr is not
empty,
then check if default MAC address was deleted shortly before adding
if yes then update the default_lan_addr.addr.
This is done because we cannot guarantee the order of
VIRTCHNL_OP_ADD_ETH_ADDR and VIRTCHNL_OP_DEL_ETH_ADDR.

New case - If a unicast MAC is being added and it's specified as
VIRTCHNL_ETHER_ADDR_PRIMARY, then replace the current
default_lan_addr.addr. If a unicast MAC is being deleted and it's type
is specified as VIRTCHNL_ETHER_ADDR_PRIMARY, then zero the
hw_lan_addr.addr.

Untrusted VFs - Only allow above legacy/new changes to their
hardware address if the PF has not set it administratively via
iproute2.

Trusted VFs - Always allow above legacy/new changes to their
hardware address even if the PF has administratively set it via
iproute2.

Co-Developed-by: Kamil Maziarz <kamil.maziarz@intel.com>
Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
---
v2: previous version had been reported to not build under
some kernel configuration.
v3: fixed minor kerneldoc misspelling
---
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 264 ++++++++++++++----
 .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |   9 +
 2 files changed, 217 insertions(+), 56 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index c2141f0c9adb..6654a230b035 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -2976,26 +2976,112 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 
 /**
  * i40e_vc_add_mac_addr_msg
+ * i40e_vc_ether_addr_type - get type of virtchnl_ether_addr
+ * @vc_ether_addr: used to extract the type
+ **/
+static inline u8
+i40e_vc_ether_addr_type(struct virtchnl_ether_addr *vc_ether_addr)
+{
+	return vc_ether_addr->type & VIRTCHNL_ETHER_ADDR_TYPE_MASK;
+}
+
+/**
+ * i40e_is_vc_addr_legacy
+ * @vc_ether_addr: VIRTCHNL structure that contains MAC and type
+ *
+ * check if the MAC address is from an older VF
+ **/
+static inline bool
+i40e_is_vc_addr_legacy(struct virtchnl_ether_addr __maybe_unused *vc_ether_addr)
+{
+	return i40e_vc_ether_addr_type(vc_ether_addr) ==
+		VIRTCHNL_ETHER_ADDR_LEGACY;
+}
+
+/**
+ * i40e_is_vc_addr_primary
+ * @vc_ether_addr: VIRTCHNL structure that contains MAC and type
+ *
+ * check if the MAC address is the VF's primary MAC
+ * This function should only be called when the MAC address in
+ * virtchnl_ether_addr is a valid unicast MAC
+ **/
+static inline bool
+i40e_is_vc_addr_primary(struct virtchnl_ether_addr
+			__maybe_unused *vc_ether_addr)
+{
+	return i40e_vc_ether_addr_type(vc_ether_addr) ==
+		VIRTCHNL_ETHER_ADDR_PRIMARY;
+}
+
+/**
+ * i40e_is_legacy_umac_expired
+ * @time_last_added_umac: time since the last delete of VFs default MAC
+ *
+ * check if last added legacy unicast MAC expired
+ **/
+static inline bool
+i40e_is_legacy_umac_expired(unsigned long time_last_added_umac)
+{
+#define I40E_LEGACY_VF_MAC_CHANGE_EXPIRE_TIME  msecs_to_jiffies(3000)
+	return time_is_before_jiffies(time_last_added_umac +
+		I40E_LEGACY_VF_MAC_CHANGE_EXPIRE_TIME);
+}
+
+/**
+ * i40e_update_vf_mac_addr
+ * @vf: VF to update
+ * @vc_ether_addr: structure from VIRTCHNL with MAC to add
+ *
+ * update the VF's cached hardware MAC if allowed
+ **/
+static void
+i40e_update_vf_mac_addr(struct i40e_vf *vf,
+			struct virtchnl_ether_addr *vc_ether_addr)
+{
+	u8 *mac_addr = vc_ether_addr->addr;
+
+	if (!is_valid_ether_addr(mac_addr))
+		return;
+
+	/* if request to add MAC filter is a primary request
+	 * update its default MAC address with the requested one
+	 *
+	 * if it is a legacy request then check if current default is empty
+	 * if so update the default MAC
+	 * otherwise save it in case it is followed by a delete request
+	 * meaning VF wants to change its default MAC which will be updated
+	 * in the delete path
+	 */
+	if (i40e_is_vc_addr_primary(vc_ether_addr)) {
+		ether_addr_copy(vf->default_lan_addr.addr, mac_addr);
+	} else {
+		if (is_zero_ether_addr(vf->default_lan_addr.addr)) {
+			ether_addr_copy(vf->default_lan_addr.addr, mac_addr);
+		} else {
+			ether_addr_copy(vf->legacy_last_added_umac.addr,
+					mac_addr);
+			vf->legacy_last_added_umac.time_modified = jiffies;
+		}
+	}
+}
+
+/**
+ * i40e_add_vf_mac_filters
  * @vf: pointer to the VF info
- * @msg: pointer to the msg buffer
+ * @is_quiet: set true for printing msg without opcode info, false otherwise
+ * @al: pointer to the address list of MACs to add
  *
  * add guest mac address filter
  **/
-static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
+static int i40e_add_vf_mac_filters(struct i40e_vf *vf, bool *is_quiet,
+				   struct virtchnl_ether_addr_list *al)
 {
-	struct virtchnl_ether_addr_list *al =
-	    (struct virtchnl_ether_addr_list *)msg;
 	struct i40e_pf *pf = vf->pf;
 	struct i40e_vsi *vsi = NULL;
-	i40e_status ret = 0;
+	int ret = 0;
 	int i;
 
-	if (!i40e_sync_vf_state(vf, I40E_VF_STATE_ACTIVE) ||
-	    !i40e_vc_isvalid_vsi_id(vf, al->vsi_id)) {
-		ret = I40E_ERR_PARAM;
-		goto error_param;
-	}
-
 	vsi = pf->vsi[vf->lan_vsi_idx];
 
 	/* Lock once, because all function inside for loop accesses VSI's
@@ -3016,20 +3102,23 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 		f = i40e_find_mac(vsi, al->list[i].addr);
 		if (!f) {
 			f = i40e_add_mac_filter(vsi, al->list[i].addr);
-
 			if (!f) {
 				dev_err(&pf->pdev->dev,
 					"Unable to add MAC filter %pM for VF %d\n",
 					al->list[i].addr, vf->vf_id);
-				ret = I40E_ERR_PARAM;
+				ret = -EINVAL;
+				spin_unlock_bh(&vsi->mac_filter_hash_lock);
+				goto error_param;
+			}
+
+			ret = i40e_add_vmmac_to_list(vf, al->list[i].addr);
+			if (ret) {
 				spin_unlock_bh(&vsi->mac_filter_hash_lock);
 				goto error_param;
 			}
-			if (is_valid_ether_addr(al->list[i].addr) &&
-			    is_zero_ether_addr(vf->default_lan_addr.addr))
-				ether_addr_copy(vf->default_lan_addr.addr,
-						al->list[i].addr);
 		}
+
+		i40e_update_vf_mac_addr(vf, &al->list[i]);
 	}
 	spin_unlock_bh(&vsi->mac_filter_hash_lock);
 
@@ -3038,82 +3127,145 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 	if (ret)
 		dev_err(&pf->pdev->dev, "Unable to program VF %d MAC filters, error %d\n",
 			vf->vf_id, ret);
-
 error_param:
-	/* send the response to the VF */
-	return i40e_vc_send_msg_to_vf(vf, VIRTCHNL_OP_ADD_ETH_ADDR,
-				      ret, NULL, 0);
+	return ret;
 }
 
 /**
- * i40e_vc_del_mac_addr_msg
+ * i40e_vc_add_mac_addr_msg
  * @vf: pointer to the VF info
  * @msg: pointer to the msg buffer
  *
- * remove guest mac address filter
+ * add guest mac address filter
  **/
-static int i40e_vc_del_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
+static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 {
 	struct virtchnl_ether_addr_list *al =
 	    (struct virtchnl_ether_addr_list *)msg;
-	bool was_unimac_deleted = false;
-	struct i40e_pf *pf = vf->pf;
-	struct i40e_vsi *vsi = NULL;
-	i40e_status ret = 0;
-	int i;
+	bool is_quiet = false;
+	int ret = 0;
 
 	if (!i40e_sync_vf_state(vf, I40E_VF_STATE_ACTIVE) ||
 	    !i40e_vc_isvalid_vsi_id(vf, al->vsi_id)) {
-		ret = I40E_ERR_PARAM;
+		ret = -EINVAL;
 		goto error_param;
 	}
 
-	for (i = 0; i < al->num_elements; i++) {
-		if (is_broadcast_ether_addr(al->list[i].addr) ||
-		    is_zero_ether_addr(al->list[i].addr)) {
-			dev_err(&pf->pdev->dev, "Invalid MAC addr %pM for VF %d\n",
-				al->list[i].addr, vf->vf_id);
-			ret = I40E_ERR_INVALID_MAC_ADDR;
-			goto error_param;
+	ret = i40e_add_vf_mac_filters(vf, &is_quiet, al);
+
+error_param:
+	/* send the response to the VF */
+	return i40e_vc_send_msg_to_vf(vf, VIRTCHNL_OP_ADD_ETH_ADDR,
+					 ret, NULL, 0);
+}
+
+/**
+ * i40e_vf_clear_default_mac_addr
+ * @vf: pointer to the VF info
+ * @is_legacy_unimac: is request to delete a legacy request
+ *
+ * clear VFs default MAC address
+ **/
+static void i40e_vf_clear_default_mac_addr(struct i40e_vf *vf,
+					   bool is_legacy_unimac)
+{
+	eth_zero_addr(vf->default_lan_addr.addr);
+
+	if (is_legacy_unimac) {
+		unsigned long time_added =
+			vf->legacy_last_added_umac.time_modified;
+
+		if (!i40e_is_legacy_umac_expired(time_added)) {
+			ether_addr_copy(vf->default_lan_addr.addr,
+					vf->legacy_last_added_umac.addr);
 		}
-		if (ether_addr_equal(al->list[i].addr, vf->default_lan_addr.addr))
-			was_unimac_deleted = true;
 	}
+}
+
+/**
+ * i40e_del_vf_mac_filters
+ * @vf: pointer to the VF info
+ * @al: pointer to the address list of MACs to delete
+ *
+ * remove guest mac address filters
+ **/
+static int i40e_del_vf_mac_filters(struct i40e_vf *vf,
+				   struct virtchnl_ether_addr_list *al)
+{
+	bool was_unimac_deleted = false;
+	bool is_legacy_unimac = false;
+	struct i40e_pf *pf = vf->pf;
+	struct i40e_vsi *vsi = NULL;
+	int ret = 0;
+	int i;
+
 	vsi = pf->vsi[vf->lan_vsi_idx];
 
 	spin_lock_bh(&vsi->mac_filter_hash_lock);
 	/* delete addresses from the list */
-	for (i = 0; i < al->num_elements; i++)
-		if (i40e_del_mac_filter(vsi, al->list[i].addr)) {
-			ret = I40E_ERR_INVALID_MAC_ADDR;
+	for (i = 0; i < al->num_elements; i++) {
+		if (ether_addr_equal(al->list[i].addr,
+				     vf->default_lan_addr.addr)) {
+			if (!(vf->trusted || !vf->pf_set_mac)) {
+				dev_err(&pf->pdev->dev,
+					"VF attempting to override administratively set MAC address\n");
+				ret = -EPERM;
+				spin_unlock_bh(&vsi->mac_filter_hash_lock);
+				goto error_param;
+			} else {
+				was_unimac_deleted = true;
+				is_legacy_unimac =
+					i40e_is_vc_addr_legacy(&al->list[i]);
+			}
+		}
+
+		if (is_broadcast_ether_addr(al->list[i].addr) ||
+		    is_zero_ether_addr(al->list[i].addr) ||
+		    i40e_del_mac_filter(vsi, al->list[i].addr)) {
+			dev_err(&pf->pdev->dev, "Invalid MAC addr %pM for VF %d\n",
+				al->list[i].addr, vf->vf_id);
+			ret = -EINVAL;
 			spin_unlock_bh(&vsi->mac_filter_hash_lock);
 			goto error_param;
 		}
 
+		i40e_del_vmmac_from_list(vf, al->list[i].addr);
+	}
 	spin_unlock_bh(&vsi->mac_filter_hash_lock);
 
+	if (was_unimac_deleted)
+		i40e_vf_clear_default_mac_addr(vf, is_legacy_unimac);
+
 	/* program the updated filter list */
 	ret = i40e_sync_vsi_filters(vsi);
 	if (ret)
 		dev_err(&pf->pdev->dev, "Unable to program VF %d MAC filters, error %d\n",
 			vf->vf_id, ret);
+error_param:
+	return ret;
+}
 
-	if (vf->trusted && was_unimac_deleted) {
-		struct i40e_mac_filter *f;
-		struct hlist_node *h;
-		u8 *macaddr = NULL;
-		int bkt;
+/**
+ * i40e_vc_del_mac_addr_msg
+ * @vf: pointer to the VF info
+ * @msg: pointer to the msg buffer
+ *
+ * remove guest mac address filter
+ **/
+static int i40e_vc_del_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
+{
+	struct virtchnl_ether_addr_list *al =
+	    (struct virtchnl_ether_addr_list *)msg;
+	int ret = 0;
 
-		/* set last unicast mac address as default */
-		spin_lock_bh(&vsi->mac_filter_hash_lock);
-		hash_for_each_safe(vsi->mac_filter_hash, bkt, h, f, hlist) {
-			if (is_valid_ether_addr(f->macaddr))
-				macaddr = f->macaddr;
-		}
-		if (macaddr)
-			ether_addr_copy(vf->default_lan_addr.addr, macaddr);
-		spin_unlock_bh(&vsi->mac_filter_hash_lock);
+	if (!i40e_sync_vf_state(vf, I40E_VF_STATE_ACTIVE) ||
+	    !i40e_vc_isvalid_vsi_id(vf, al->vsi_id)) {
+		ret = -EINVAL;
+		goto error_param;
 	}
+
+	ret = i40e_del_vf_mac_filters(vf, al);
+
 error_param:
 	/* send the response to the VF */
 	return i40e_vc_send_resp_to_vf(vf, VIRTCHNL_OP_DEL_ETH_ADDR, ret);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
index 755f29cb0131..a0bd12dd0939 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
@@ -62,6 +62,11 @@ struct i40evf_channel {
 	u64 max_tx_rate; /* bandwidth rate allocation for VSIs */
 };
 
+struct i40e_time_mac {
+	unsigned long time_modified;
+	u8 addr[ETH_ALEN];
+};
+
 /* VF information structure */
 struct i40e_vf {
 	struct i40e_pf *pf;
@@ -77,6 +82,10 @@ struct i40e_vf {
 	u16 stag;
 
 	struct virtchnl_ether_addr default_lan_addr;
+
+	/* keeps last added MAC address */
+	struct i40e_time_mac legacy_last_added_umac;
+
 	u16 port_vlan_id;
 	bool pf_set_mac;	/* The VMM admin set the VF MAC address */
 	bool trusted;
-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next v3 2/2] i40e: Only MACs from host must be preserved

[Jan Sokolowski]

From: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>

Only MACs assigned from host must be preserved after VF reset/reload.
Added vm MAC list to filter vm MAC request from others then
vm MACs do not preserved by host during VF reset/reload.
This list is used to filter the MAC addresses list restored after reset.
Without this patch host automatically restore all the MAC addresses
after VF reset/reload.

Co-Developed-by: Kamil Maziarz <kamil.maziarz@intel.com>
Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
---
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 64 ++++++++++++++++++-
 .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  9 +++
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 6654a230b035..6af627db71a7 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -1045,6 +1045,66 @@ static void i40e_disable_vf_mappings(struct i40e_vf *vf)
 	i40e_flush(hw);
 }
 
+/**
+ * i40e_add_vmmac_to_list
+ * @vf: pointer to the VF info
+ * @macaddr: pointer to the MAC address
+ *
+ * add MAC address into the MAC list for VM
+ **/
+static int i40e_add_vmmac_to_list(struct i40e_vf *vf,
+				  const u8 *macaddr)
+{
+	struct i40e_vm_mac *mac_elem;
+
+	mac_elem = kzalloc(sizeof(*mac_elem), GFP_ATOMIC);
+	if (!mac_elem)
+		return -ENOMEM;
+	ether_addr_copy(mac_elem->macaddr, macaddr);
+	INIT_LIST_HEAD(&mac_elem->list);
+	list_add(&mac_elem->list, &vf->vm_mac_list);
+	return 0;
+}
+
+/**
+ * i40e_del_vmmac_from_list
+ * @vf: pointer to the VF info
+ * @macaddr: pointer to the MAC address
+ *
+ * delete MAC address from the MAC list for VM
+ **/
+static void i40e_del_vmmac_from_list(struct i40e_vf *vf, const u8 *macaddr)
+{
+	struct i40e_vm_mac *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &vf->vm_mac_list, list) {
+		if (ether_addr_equal(macaddr, entry->macaddr)) {
+			list_del(&entry->list);
+			kfree(entry);
+			break;
+		}
+	}
+}
+
+/**
+ * i40e_free_vmmac_list
+ * @vf: pointer to the VF info
+ *
+ * remove whole list of MAC addresses for VM
+ **/
+static void i40e_free_vmmac_list(struct i40e_vf *vf)
+{
+	struct i40e_vm_mac *entry, *tmp;
+
+	if (list_empty(&vf->vm_mac_list))
+		return;
+
+	list_for_each_entry_safe(entry, tmp, &vf->vm_mac_list, list) {
+		list_del(&entry->list);
+		kfree(entry);
+	}
+}
+
 /**
  * i40e_free_vf_res
  * @vf: pointer to the VF info
@@ -1120,6 +1180,9 @@ static void i40e_free_vf_res(struct i40e_vf *vf)
 		wr32(hw, reg_idx, reg);
 		i40e_flush(hw);
 	}
+
+	i40e_free_vmmac_list(vf);
+
 	/* reset some of the state variables keeping track of the resources */
 	vf->num_queue_pairs = 0;
 	clear_bit(I40E_VF_STATE_MC_PROMISC, &vf->vf_states);
@@ -2975,7 +3038,6 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 }
 
 /**
- * i40e_vc_add_mac_addr_msg
  * i40e_vc_ether_addr_type - get type of virtchnl_ether_addr
  * @vc_ether_addr: used to extract the type
  **/
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
index a0bd12dd0939..f0e67a0d4ecf 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
@@ -67,6 +67,12 @@ struct i40e_time_mac {
 	u8 addr[ETH_ALEN];
 };
 
+/* used for MAC list 'vm_mac_list' to recognize MACs added by VM */
+struct i40e_vm_mac {
+	struct list_head list;
+	u8 macaddr[ETH_ALEN];
+};
+
 /* VF information structure */
 struct i40e_vf {
 	struct i40e_pf *pf;
@@ -111,6 +117,9 @@ struct i40e_vf {
 	bool spoofchk;
 	u16 num_vlan;
 
+	/* MAC list created by VM */
+	struct list_head vm_mac_list;
+
 	/* ADq related variables */
 	bool adq_enabled; /* flag to enable adq */
 	u8 num_tc;
-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v3 1/2] i40e: Add ability to change VFs default MAC address

[Tony Nguyen]

On 1/13/2023 5:17 AM, Jan Sokolowski wrote:
> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> 
> Currently there is no way for a VF driver to specify if it wants to
> change it's hardware address. New bits are being added to virtchnl.h
> in struct virtchnl_ether_addr that allow the VF to correctly
> communicate this information. 

This was added with eb550f53099b ("virtchnl: Use pad byte in 
virtchnl_ether_addr to specify MAC type") which landed in 5.14. I'm not 
sure that's "new bits being added".

> However, legacy VF drivers that don't
> support the new virtchnl.h bits still need to be supported. Make a
> best effort attempt at saving the VF's primary/device address in the
> legacy case and depend on the VIRTCHNL_ETHER_ADDR_PRIMARY type for
> the new case.

This whole paragraph probably needs to be reworded.

> Legacy case - If a unicast MAC is being added and the
> default_lan_addr.addr is empty, then populate it. This assumes that the
> address is the VF's hardware address. If unicast MAC is being deleted
> and it matches the default_lan_addr.addr save the time when it happened
> and replace it with the last MAC address on the MAC filter list.
> If a unicast MAC is being added and the default_lan_addr.addr is not
> empty,
> then check if default MAC address was deleted shortly before adding
> if yes then update the default_lan_addr.addr.
> This is done because we cannot guarantee the order of
> VIRTCHNL_OP_ADD_ETH_ADDR and VIRTCHNL_OP_DEL_ETH_ADDR.
> 
> New case - If a unicast MAC is being added and it's specified as
> VIRTCHNL_ETHER_ADDR_PRIMARY, then replace the current
> default_lan_addr.addr. If a unicast MAC is being deleted and it's type
> is specified as VIRTCHNL_ETHER_ADDR_PRIMARY, then zero the
> hw_lan_addr.addr.
> 
> Untrusted VFs - Only allow above legacy/new changes to their
> hardware address if the PF has not set it administratively via
> iproute2.
> 
> Trusted VFs - Always allow above legacy/new changes to their
> hardware address even if the PF has administratively set it via
> iproute2.
> 
> Co-Developed-by: Kamil Maziarz <kamil.maziarz@intel.com>
> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>

Please fix ordering. Author should be first. Immediately following 
Co-developed should be the co-developer's Sign-off. Co-developed-by is 
preferred over Co-Developed-by

> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> ---
> v2: previous version had been reported to not build under
> some kernel configuration.
> v3: fixed minor kerneldoc misspelling

I don't recall seeing v1 and v2 on this list(?)

> ---
>   .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 264 ++++++++++++++----
>   .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |   9 +
>   2 files changed, 217 insertions(+), 56 deletions(-)

Patches don't apply, please rebase.

> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index c2141f0c9adb..6654a230b035 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -2976,26 +2976,112 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
>   
>   /**
>    * i40e_vc_add_mac_addr_msg

Should this be deleted?

> + * i40e_vc_ether_addr_type - get type of virtchnl_ether_addr
> + * @vc_ether_addr: used to extract the type
> + **/
> +static inline u8

No inline in c files.

> +i40e_vc_ether_addr_type(struct virtchnl_ether_addr *vc_ether_addr)
> +{
> +	return vc_ether_addr->type & VIRTCHNL_ETHER_ADDR_TYPE_MASK;
> +}
> +
> +/**
> + * i40e_is_vc_addr_legacy
> + * @vc_ether_addr: VIRTCHNL structure that contains MAC and type
> + *
> + * check if the MAC address is from an older VF
> + **/
> +static inline bool

same here

> +i40e_is_vc_addr_legacy(struct virtchnl_ether_addr __maybe_unused *vc_ether_addr)

Why '__maybe_unused'?

> +{
> +	return i40e_vc_ether_addr_type(vc_ether_addr) ==
> +		VIRTCHNL_ETHER_ADDR_LEGACY;
> +}
> +
> +/**
> + * i40e_is_vc_addr_primary
> + * @vc_ether_addr: VIRTCHNL structure that contains MAC and type
> + *
> + * check if the MAC address is the VF's primary MAC
> + * This function should only be called when the MAC address in
> + * virtchnl_ether_addr is a valid unicast MAC
> + **/
> +static inline bool

again, no inline. Pleas audit the rest of the patch for this.

> +i40e_is_vc_addr_primary(struct virtchnl_ether_addr
> +			__maybe_unused *vc_ether_addr)

__maybe_unused?

> +{
> +	return i40e_vc_ether_addr_type(vc_ether_addr) ==
> +		VIRTCHNL_ETHER_ADDR_PRIMARY;
> +}
> +
> +/**
> + * i40e_is_legacy_umac_expired
> + * @time_last_added_umac: time since the last delete of VFs default MAC
> + *
> + * check if last added legacy unicast MAC expired
> + **/
> +static inline bool
> +i40e_is_legacy_umac_expired(unsigned long time_last_added_umac)
> +{
> +#define I40E_LEGACY_VF_MAC_CHANGE_EXPIRE_TIME  msecs_to_jiffies(3000)

Please move the define out of the function.

> +	return time_is_before_jiffies(time_last_added_umac +
> +		I40E_LEGACY_VF_MAC_CHANGE_EXPIRE_TIME);
> +}
> +
> +/**
> + * i40e_update_vf_mac_addr
> + * @vf: VF to update
> + * @vc_ether_addr: structure from VIRTCHNL with MAC to add
> + *
> + * update the VF's cached hardware MAC if allowed
> + **/
> +static void
> +i40e_update_vf_mac_addr(struct i40e_vf *vf,
> +			struct virtchnl_ether_addr *vc_ether_addr)
> +{
> +	u8 *mac_addr = vc_ether_addr->addr;
> +
> +	if (!is_valid_ether_addr(mac_addr))
> +		return;
> +
> +	/* if request to add MAC filter is a primary request
> +	 * update its default MAC address with the requested one
> +	 *
> +	 * if it is a legacy request then check if current default is empty
> +	 * if so update the default MAC
> +	 * otherwise save it in case it is followed by a delete request
> +	 * meaning VF wants to change its default MAC which will be updated
> +	 * in the delete path
> +	 */
> +	if (i40e_is_vc_addr_primary(vc_ether_addr)) {
> +		ether_addr_copy(vf->default_lan_addr.addr, mac_addr);
> +	} else {
> +		if (is_zero_ether_addr(vf->default_lan_addr.addr)) {
> +			ether_addr_copy(vf->default_lan_addr.addr, mac_addr);
> +		} else {
> +			ether_addr_copy(vf->legacy_last_added_umac.addr,
> +					mac_addr);
> +			vf->legacy_last_added_umac.time_modified = jiffies;
> +		}
> +	}
> +}
> +
> +/**
> + * i40e_add_vf_mac_filters
>    * @vf: pointer to the VF info
> - * @msg: pointer to the msg buffer
> + * @is_quiet: set true for printing msg without opcode info, false otherwise
> + * @al: pointer to the address list of MACs to add
>    *
>    * add guest mac address filter
>    **/
> -static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
> +static int i40e_add_vf_mac_filters(struct i40e_vf *vf, bool *is_quiet,
> +				   struct virtchnl_ether_addr_list *al)
>   {
> -	struct virtchnl_ether_addr_list *al =
> -	    (struct virtchnl_ether_addr_list *)msg;
>   	struct i40e_pf *pf = vf->pf;
>   	struct i40e_vsi *vsi = NULL;
> -	i40e_status ret = 0;

How does this exist after the i40_status removal [1]? Seems like you are 
on the wrong tree, branch, or are not rebased.

Also, looks existing, but some of the vsi and ret initializations look 
to be unneeded (below as well).

> +	int ret = 0;
>   	int i;
>   
> -	if (!i40e_sync_vf_state(vf, I40E_VF_STATE_ACTIVE) ||
> -	    !i40e_vc_isvalid_vsi_id(vf, al->vsi_id)) {
> -		ret = I40E_ERR_PARAM;
> -		goto error_param;
> -	}
> -
>   	vsi = pf->vsi[vf->lan_vsi_idx];
>   
>   	/* Lock once, because all function inside for loop accesses VSI's
> @@ -3016,20 +3102,23 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
>   		f = i40e_find_mac(vsi, al->list[i].addr);
>   		if (!f) {
>   			f = i40e_add_mac_filter(vsi, al->list[i].addr);
> -
>   			if (!f) {
>   				dev_err(&pf->pdev->dev,
>   					"Unable to add MAC filter %pM for VF %d\n",
>   					al->list[i].addr, vf->vf_id);
> -				ret = I40E_ERR_PARAM;
> +				ret = -EINVAL;
> +				spin_unlock_bh(&vsi->mac_filter_hash_lock);
> +				goto error_param;
> +			}
> +
> +			ret = i40e_add_vmmac_to_list(vf, al->list[i].addr);

This function isn't added until patch 2. Does this patch compile?

> +			if (ret) {
>   				spin_unlock_bh(&vsi->mac_filter_hash_lock);
>   				goto error_param;
>   			}
> -			if (is_valid_ether_addr(al->list[i].addr) &&
> -			    is_zero_ether_addr(vf->default_lan_addr.addr))
> -				ether_addr_copy(vf->default_lan_addr.addr,
> -						al->list[i].addr);
>   		}
> +
> +		i40e_update_vf_mac_addr(vf, &al->list[i]);
>   	}
>   	spin_unlock_bh(&vsi->mac_filter_hash_lock);
>   
> @@ -3038,82 +3127,145 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
>   	if (ret)
>   		dev_err(&pf->pdev->dev, "Unable to program VF %d MAC filters, error %d\n",
>   			vf->vf_id, ret);
> -
>   error_param:
> -	/* send the response to the VF */
> -	return i40e_vc_send_msg_to_vf(vf, VIRTCHNL_OP_ADD_ETH_ADDR,
> -				      ret, NULL, 0);
> +	return ret;
>   }
>   
>   /**
> - * i40e_vc_del_mac_addr_msg
> + * i40e_vc_add_mac_addr_msg
>    * @vf: pointer to the VF info
>    * @msg: pointer to the msg buffer
>    *
> - * remove guest mac address filter
> + * add guest mac address filter
>    **/
> -static int i40e_vc_del_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
> +static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
>   {
>   	struct virtchnl_ether_addr_list *al =
>   	    (struct virtchnl_ether_addr_list *)msg;
> -	bool was_unimac_deleted = false;
> -	struct i40e_pf *pf = vf->pf;
> -	struct i40e_vsi *vsi = NULL;
> -	i40e_status ret = 0;
> -	int i;
> +	bool is_quiet = false;
> +	int ret = 0;
>   
>   	if (!i40e_sync_vf_state(vf, I40E_VF_STATE_ACTIVE) ||
>   	    !i40e_vc_isvalid_vsi_id(vf, al->vsi_id)) {
> -		ret = I40E_ERR_PARAM;
> +		ret = -EINVAL;
>   		goto error_param;
>   	}
>   
> -	for (i = 0; i < al->num_elements; i++) {
> -		if (is_broadcast_ether_addr(al->list[i].addr) ||
> -		    is_zero_ether_addr(al->list[i].addr)) {
> -			dev_err(&pf->pdev->dev, "Invalid MAC addr %pM for VF %d\n",
> -				al->list[i].addr, vf->vf_id);
> -			ret = I40E_ERR_INVALID_MAC_ADDR;
> -			goto error_param;
> +	ret = i40e_add_vf_mac_filters(vf, &is_quiet, al);
> +
> +error_param:
> +	/* send the response to the VF */
> +	return i40e_vc_send_msg_to_vf(vf, VIRTCHNL_OP_ADD_ETH_ADDR,
> +					 ret, NULL, 0);
> +}
> +
> +/**
> + * i40e_vf_clear_default_mac_addr
> + * @vf: pointer to the VF info
> + * @is_legacy_unimac: is request to delete a legacy request
> + *
> + * clear VFs default MAC address
> + **/
> +static void i40e_vf_clear_default_mac_addr(struct i40e_vf *vf,
> +					   bool is_legacy_unimac)
> +{
> +	eth_zero_addr(vf->default_lan_addr.addr);
> +
> +	if (is_legacy_unimac) {
> +		unsigned long time_added =
> +			vf->legacy_last_added_umac.time_modified;
> +
> +		if (!i40e_is_legacy_umac_expired(time_added)) {
> +			ether_addr_copy(vf->default_lan_addr.addr,
> +					vf->legacy_last_added_umac.addr);
>   		}
> -		if (ether_addr_equal(al->list[i].addr, vf->default_lan_addr.addr))
> -			was_unimac_deleted = true;
>   	}
> +}
> +
> +/**
> + * i40e_del_vf_mac_filters
> + * @vf: pointer to the VF info
> + * @al: pointer to the address list of MACs to delete
> + *
> + * remove guest mac address filters
> + **/
> +static int i40e_del_vf_mac_filters(struct i40e_vf *vf,
> +				   struct virtchnl_ether_addr_list *al)
> +{
> +	bool was_unimac_deleted = false;
> +	bool is_legacy_unimac = false;
> +	struct i40e_pf *pf = vf->pf;
> +	struct i40e_vsi *vsi = NULL;
> +	int ret = 0;
> +	int i;
> +
>   	vsi = pf->vsi[vf->lan_vsi_idx];
>   
>   	spin_lock_bh(&vsi->mac_filter_hash_lock);
>   	/* delete addresses from the list */
> -	for (i = 0; i < al->num_elements; i++)
> -		if (i40e_del_mac_filter(vsi, al->list[i].addr)) {
> -			ret = I40E_ERR_INVALID_MAC_ADDR;
> +	for (i = 0; i < al->num_elements; i++) {
> +		if (ether_addr_equal(al->list[i].addr,
> +				     vf->default_lan_addr.addr)) {
> +			if (!(vf->trusted || !vf->pf_set_mac)) {
> +				dev_err(&pf->pdev->dev,
> +					"VF attempting to override administratively set MAC address\n");
> +				ret = -EPERM;
> +				spin_unlock_bh(&vsi->mac_filter_hash_lock);
> +				goto error_param;
> +			} else {
> +				was_unimac_deleted = true;
> +				is_legacy_unimac =
> +					i40e_is_vc_addr_legacy(&al->list[i]);
> +			}
> +		}
> +
> +		if (is_broadcast_ether_addr(al->list[i].addr) ||
> +		    is_zero_ether_addr(al->list[i].addr) ||
> +		    i40e_del_mac_filter(vsi, al->list[i].addr)) {
> +			dev_err(&pf->pdev->dev, "Invalid MAC addr %pM for VF %d\n",
> +				al->list[i].addr, vf->vf_id);
> +			ret = -EINVAL;
>   			spin_unlock_bh(&vsi->mac_filter_hash_lock);
>   			goto error_param;
>   		}
>   
> +		i40e_del_vmmac_from_list(vf, al->list[i].addr);
> +	}
>   	spin_unlock_bh(&vsi->mac_filter_hash_lock);
>   
> +	if (was_unimac_deleted)
> +		i40e_vf_clear_default_mac_addr(vf, is_legacy_unimac);
> +
>   	/* program the updated filter list */
>   	ret = i40e_sync_vsi_filters(vsi);
>   	if (ret)
>   		dev_err(&pf->pdev->dev, "Unable to program VF %d MAC filters, error %d\n",
>   			vf->vf_id, ret);
> +error_param:
> +	return ret;
> +}
>   
> -	if (vf->trusted && was_unimac_deleted) {
> -		struct i40e_mac_filter *f;
> -		struct hlist_node *h;
> -		u8 *macaddr = NULL;
> -		int bkt;
> +/**
> + * i40e_vc_del_mac_addr_msg
> + * @vf: pointer to the VF info
> + * @msg: pointer to the msg buffer
> + *
> + * remove guest mac address filter
> + **/
> +static int i40e_vc_del_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
> +{
> +	struct virtchnl_ether_addr_list *al =
> +	    (struct virtchnl_ether_addr_list *)msg;
> +	int ret = 0;
>   
> -		/* set last unicast mac address as default */
> -		spin_lock_bh(&vsi->mac_filter_hash_lock);
> -		hash_for_each_safe(vsi->mac_filter_hash, bkt, h, f, hlist) {
> -			if (is_valid_ether_addr(f->macaddr))
> -				macaddr = f->macaddr;
> -		}
> -		if (macaddr)
> -			ether_addr_copy(vf->default_lan_addr.addr, macaddr);
> -		spin_unlock_bh(&vsi->mac_filter_hash_lock);
> +	if (!i40e_sync_vf_state(vf, I40E_VF_STATE_ACTIVE) ||
> +	    !i40e_vc_isvalid_vsi_id(vf, al->vsi_id)) {
> +		ret = -EINVAL;
> +		goto error_param;
>   	}
> +
> +	ret = i40e_del_vf_mac_filters(vf, al);
> +
>   error_param:
>   	/* send the response to the VF */
>   	return i40e_vc_send_resp_to_vf(vf, VIRTCHNL_OP_DEL_ETH_ADDR, ret);


[1] 
https://lore.kernel.org/intel-wired-lan/20230109141120.3197817-1-jan.sokolowski@intel.com/
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan