[PATCH net-next,v3,1/2] devlink: Add new "max_mac_per_vf" generic device param

[PATCH net-next,v3,1/2] devlink: Add new "max_mac_per_vf" generic device param

[mheib]

From: Mohammad Heib <mheib@redhat.com>

Add a new device generic parameter to controls the maximum
number of MAC filters allowed per VF.

For example, to limit a VF to 3 MAC addresses:
 $ devlink dev param set pci/0000:3b:00.0 name max_mac_per_vf \
        value 3 \
        cmode runtime

Signed-off-by: Mohammad Heib <mheib@redhat.com>
---
 Documentation/networking/devlink/devlink-params.rst | 4 ++++
 include/net/devlink.h                               | 4 ++++
 net/devlink/param.c                                 | 5 +++++
 3 files changed, 13 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
index 211b58177e12..74a35f3b7c9a 100644
--- a/Documentation/networking/devlink/devlink-params.rst
+++ b/Documentation/networking/devlink/devlink-params.rst
@@ -143,3 +143,7 @@ own name.
    * - ``clock_id``
      - u64
      - Clock ID used by the device for registering DPLL devices and pins.
+   * - ``max_mac_per_vf``
+     - u32
+     - Controls the maximum number of MAC address filters that can be assigned
+       to a Virtual Function (VF).
diff --git a/include/net/devlink.h b/include/net/devlink.h
index b32c9ceeb81d..dde5dcbca625 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -530,6 +530,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
 	DEVLINK_PARAM_GENERIC_ID_ENABLE_PHC,
 	DEVLINK_PARAM_GENERIC_ID_CLOCK_ID,
+	DEVLINK_PARAM_GENERIC_ID_MAX_MAC_PER_VF,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -594,6 +595,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_CLOCK_ID_NAME "clock_id"
 #define DEVLINK_PARAM_GENERIC_CLOCK_ID_TYPE DEVLINK_PARAM_TYPE_U64
 
+#define DEVLINK_PARAM_GENERIC_MAX_MAC_PER_VF_NAME "max_mac_per_vf"
+#define DEVLINK_PARAM_GENERIC_MAX_MAC_PER_VF_TYPE DEVLINK_PARAM_TYPE_U32
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/devlink/param.c b/net/devlink/param.c
index 41dcc86cfd94..62fd789ae01c 100644
--- a/net/devlink/param.c
+++ b/net/devlink/param.c
@@ -102,6 +102,11 @@ static const struct devlink_param devlink_param_generic[] = {
 		.name = DEVLINK_PARAM_GENERIC_CLOCK_ID_NAME,
 		.type = DEVLINK_PARAM_GENERIC_CLOCK_ID_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_MAX_MAC_PER_VF,
+		.name = DEVLINK_PARAM_GENERIC_MAX_MAC_PER_VF_NAME,
+		.type = DEVLINK_PARAM_GENERIC_MAX_MAC_PER_VF_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
2.50.1

[PATCH net-next,v3,2/2] i40e: support generic devlink param "max_mac_per_vf"

[mheib]

From: Mohammad Heib <mheib@redhat.com>

Currently the i40e driver enforces its own internally calculated per-VF MAC
filter limit, derived from the number of allocated VFs and available
hardware resources. This limit is not configurable by the administrator,
which makes it difficult to control how many MAC addresses each VF may
use.

This patch adds support for the new generic devlink runtime parameter
"max_mac_per_vf" which provides administrators with a way to cap the
number of MAC addresses a VF can use:

- When the parameter is set to 0 (default), the driver continues to use
  its internally calculated limit.

- When set to a non-zero value, the driver applies this value as a strict
  cap for VFs, overriding the internal calculation.

Important notes:

- The configured value is a theoretical maximum. Hardware limits may
  still prevent additional MAC addresses from being added, even if the
  parameter allows it.

- Since MAC filters are a shared hardware resource across all VFs,
  setting a high value may cause resource contention and starve other
  VFs.

- This change gives administrators predictable and flexible control over
  VF resource allocation, while still respecting hardware limitations.

- Previous discussion about this change:
  https://lore.kernel.org/netdev/20250805134042.2604897-2-dhill@redhat.com
  https://lore.kernel.org/netdev/20250823094952.182181-1-mheib@redhat.com

Signed-off-by: Mohammad Heib <mheib@redhat.com>
---
 Documentation/networking/devlink/i40e.rst     | 32 +++++++++++++
 drivers/net/ethernet/intel/i40e/i40e.h        |  4 ++
 .../net/ethernet/intel/i40e/i40e_devlink.c    | 48 ++++++++++++++++++-
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 31 ++++++++----
 4 files changed, 105 insertions(+), 10 deletions(-)

diff --git a/Documentation/networking/devlink/i40e.rst b/Documentation/networking/devlink/i40e.rst
index d3cb5bb5197e..524524fdd3de 100644
--- a/Documentation/networking/devlink/i40e.rst
+++ b/Documentation/networking/devlink/i40e.rst
@@ -7,6 +7,38 @@ i40e devlink support
 This document describes the devlink features implemented by the ``i40e``
 device driver.
 
+Parameters
+==========
+
+.. list-table:: Generic parameters implemented
+    :widths: 5 5 90
+
+    * - Name
+      - Mode
+      - Notes
+    * - ``max_mac_per_vf``
+      - runtime
+      - Controls the maximum number of MAC addresses a VF can use
+        on i40e devices.
+
+        By default (``0``), the driver enforces its internally calculated per-VF
+        MAC filter limit, which is based on the number of allocated VFS.
+
+        If set to a non-zero value, this parameter acts as a strict cap:
+        the driver will use the user-provided value instead of its internal
+        calculation.
+
+        **Important notes:**
+        - MAC filters are a **shared hardware resource** across all VFs.
+          Setting a high value may cause other VFs to be starved of filters.
+
+        - This value is a **theoretical maximum**. The hardware may return
+          errors when its absolute limit is reached, regardless of the value
+          set here.
+
+        The default value is ``0`` (internal calculation is used).
+
+
 Info versions
 =============
 
diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index 801a57a925da..d2d03db2acec 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -574,6 +574,10 @@ struct i40e_pf {
 	struct i40e_vf *vf;
 	int num_alloc_vfs;	/* actual number of VFs allocated */
 	u32 vf_aq_requests;
+	/* If set to non-zero, the device uses this value
+	 * as maximum number of MAC filters per VF.
+	 */
+	u32 max_mac_per_vf;
 	u32 arq_overflows;	/* Not fatal, possibly indicative of problems */
 	struct ratelimit_state mdd_message_rate_limit;
 	/* DCBx/DCBNL capability for PF that indicates
diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
index cc4e9e2addb7..cd01e35da94e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_devlink.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
@@ -5,6 +5,35 @@
 #include "i40e.h"
 #include "i40e_devlink.h"
 
+static int i40e_max_mac_per_vf_set(struct devlink *devlink,
+				   u32 id,
+				   struct devlink_param_gset_ctx *ctx,
+				   struct netlink_ext_ack *extack)
+{
+	struct i40e_pf *pf = devlink_priv(devlink);
+
+	pf->max_mac_per_vf = ctx->val.vu32;
+	return 0;
+}
+
+static int i40e_max_mac_per_vf_get(struct devlink *devlink,
+				   u32 id,
+				   struct devlink_param_gset_ctx *ctx)
+{
+	struct i40e_pf *pf = devlink_priv(devlink);
+
+	ctx->val.vu32 = pf->max_mac_per_vf;
+	return 0;
+}
+
+static const struct devlink_param i40e_dl_params[] = {
+	DEVLINK_PARAM_GENERIC(MAX_MAC_PER_VF,
+			      BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			      i40e_max_mac_per_vf_get,
+			      i40e_max_mac_per_vf_set,
+			      NULL),
+};
+
 static void i40e_info_get_dsn(struct i40e_pf *pf, char *buf, size_t len)
 {
 	u8 dsn[8];
@@ -165,7 +194,18 @@ void i40e_free_pf(struct i40e_pf *pf)
  **/
 void i40e_devlink_register(struct i40e_pf *pf)
 {
-	devlink_register(priv_to_devlink(pf));
+	struct devlink *dl = priv_to_devlink(pf);
+	struct device *dev = &pf->pdev->dev;
+	int err;
+
+	err = devlink_params_register(dl, i40e_dl_params,
+				      ARRAY_SIZE(i40e_dl_params));
+	if (err)
+		dev_err(dev,
+			"devlink params register failed with error %d", err);
+
+	devlink_register(dl);
+
 }
 
 /**
@@ -176,7 +216,11 @@ void i40e_devlink_register(struct i40e_pf *pf)
  **/
 void i40e_devlink_unregister(struct i40e_pf *pf)
 {
-	devlink_unregister(priv_to_devlink(pf));
+	struct devlink *dl = priv_to_devlink(pf);
+
+	devlink_unregister(dl);
+	devlink_params_unregister(dl, i40e_dl_params,
+				  ARRAY_SIZE(i40e_dl_params));
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 081a4526a2f0..6e154a8aa474 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -2935,33 +2935,48 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 		if (!f)
 			++mac_add_cnt;
 	}
-
-	/* If this VF is not privileged, then we can't add more than a limited
-	 * number of addresses.
+	/* Determine the maximum number of MAC addresses this VF may use.
+	 *
+	 * - For untrusted VFs: use a fixed small limit.
+	 *
+	 * - For trusted VFs: limit is calculated by dividing total MAC
+	 *  filter pool across all VFs/ports.
 	 *
-	 * If this VF is trusted, it can use more resources than untrusted.
-	 * However to ensure that every trusted VF has appropriate number of
-	 * resources, divide whole pool of resources per port and then across
-	 * all VFs.
+	 * - User can override this by devlink param "max_mac_per_vf".
+	 *   If set its value is used as a strict cap for both trusted and
+	 *   untrusted VFs.
+	 *   Note:
+	 *    even when overridden, this is a theoretical maximum; hardware
+	 *    may reject additional MACs if the absolute HW limit is reached.
 	 */
 	if (!vf_trusted)
 		mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
 	else
 		mac_add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs, hw->num_ports);
 
+	if (pf->max_mac_per_vf > 0)
+		mac_add_max = pf->max_mac_per_vf;
+
 	/* VF can replace all its filters in one step, in this case mac_add_max
 	 * will be added as active and another mac_add_max will be in
 	 * a to-be-removed state. Account for that.
 	 */
 	if ((i40e_count_active_filters(vsi) + mac_add_cnt) > mac_add_max ||
 	    (i40e_count_all_filters(vsi) + mac_add_cnt) > 2 * mac_add_max) {
+		if (pf->max_mac_per_vf == mac_add_max && mac_add_max > 0) {
+			dev_err(&pf->pdev->dev,
+				"Cannot add more MAC addresses: VF reached its maximum allowed limit (%d)\n",
+				mac_add_max);
+				return -EPERM;
+		}
 		if (!vf_trusted) {
 			dev_err(&pf->pdev->dev,
 				"Cannot add more MAC addresses, VF is not trusted, switch the VF to trusted to add more functionality\n");
 			return -EPERM;
 		} else {
 			dev_err(&pf->pdev->dev,
-				"Cannot add more MAC addresses, trusted VF exhausted it's resources\n");
+				"Cannot add more MAC addresses: trusted VF reached its maximum allowed limit (%d)\n",
+				mac_add_max);
 			return -EPERM;
 		}
 	}
-- 
2.50.1

Re: [PATCH net-next,v3,2/2] i40e: support generic devlink param "max_mac_per_vf"

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 5549 bytes --]



On 9/3/2025 2:43 PM, mheib@redhat.com wrote:
> From: Mohammad Heib <mheib@redhat.com>
> 
> Currently the i40e driver enforces its own internally calculated per-VF MAC
> filter limit, derived from the number of allocated VFs and available
> hardware resources. This limit is not configurable by the administrator,
> which makes it difficult to control how many MAC addresses each VF may
> use.
> 
> This patch adds support for the new generic devlink runtime parameter
> "max_mac_per_vf" which provides administrators with a way to cap the
> number of MAC addresses a VF can use:
> 
> - When the parameter is set to 0 (default), the driver continues to use
>   its internally calculated limit.
> 
> - When set to a non-zero value, the driver applies this value as a strict
>   cap for VFs, overriding the internal calculation.
> 
> Important notes:
> 
> - The configured value is a theoretical maximum. Hardware limits may
>   still prevent additional MAC addresses from being added, even if the
>   parameter allows it.
> 
> - Since MAC filters are a shared hardware resource across all VFs,
>   setting a high value may cause resource contention and starve other
>   VFs.
> 
> - This change gives administrators predictable and flexible control over
>   VF resource allocation, while still respecting hardware limitations.
> 
> - Previous discussion about this change:
>   https://lore.kernel.org/netdev/20250805134042.2604897-2-dhill@redhat.com
>   https://lore.kernel.org/netdev/20250823094952.182181-1-mheib@redhat.com
> 
> Signed-off-by: Mohammad Heib <mheib@redhat.com>
> ---

This version looks good to me. With or without minor nits relating to
rate limiting and adding mac_add_max to the untrusted message:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index 081a4526a2f0..6e154a8aa474 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -2935,33 +2935,48 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
>  		if (!f)
>  			++mac_add_cnt;
>  	}
> -
> -	/* If this VF is not privileged, then we can't add more than a limited
> -	 * number of addresses.
> +	/* Determine the maximum number of MAC addresses this VF may use.
> +	 *
> +	 * - For untrusted VFs: use a fixed small limit.
> +	 *
> +	 * - For trusted VFs: limit is calculated by dividing total MAC
> +	 *  filter pool across all VFs/ports.
>  	 *
> -	 * If this VF is trusted, it can use more resources than untrusted.
> -	 * However to ensure that every trusted VF has appropriate number of
> -	 * resources, divide whole pool of resources per port and then across
> -	 * all VFs.
> +	 * - User can override this by devlink param "max_mac_per_vf".
> +	 *   If set its value is used as a strict cap for both trusted and
> +	 *   untrusted VFs.
> +	 *   Note:
> +	 *    even when overridden, this is a theoretical maximum; hardware
> +	 *    may reject additional MACs if the absolute HW limit is reached.
>  	 */

Good. I think this is better and allows users to also increase limit for
untrusted VFs without requiring them to become fully "trusted" with the
all-or-nothing approach. Its more flexible in that regard, and avoids
the confusion of the parameter not working because a VF is untrusted.

>  	if (!vf_trusted)
>  		mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
>  	else
>  		mac_add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs, hw->num_ports);
>  
> +	if (pf->max_mac_per_vf > 0)
> +		mac_add_max = pf->max_mac_per_vf;
> +

Nice, a clean way to edit the maximum without needing too much special
casing.

>  	/* VF can replace all its filters in one step, in this case mac_add_max
>  	 * will be added as active and another mac_add_max will be in
>  	 * a to-be-removed state. Account for that.
>  	 */
>  	if ((i40e_count_active_filters(vsi) + mac_add_cnt) > mac_add_max ||
>  	    (i40e_count_all_filters(vsi) + mac_add_cnt) > 2 * mac_add_max) {
> +		if (pf->max_mac_per_vf == mac_add_max && mac_add_max > 0) {
> +			dev_err(&pf->pdev->dev,
> +				"Cannot add more MAC addresses: VF reached its maximum allowed limit (%d)\n",
> +				mac_add_max);
> +				return -EPERM;
> +		}

Good, having the specific error message will aid system administrators
in debugging.

One thought I had, which isn't a knock on your code as we did the same
before.. should these be rate limited to prevent VF spamming MAC filter
adds clogging up the dmesg buffer?

Given that we didn't do it before, I think its reasonable to not hold
this patch up for such a cleanup.

>  		if (!vf_trusted) {
>  			dev_err(&pf->pdev->dev,
>  				"Cannot add more MAC addresses, VF is not trusted, switch the VF to trusted to add more functionality\n");
>  			return -EPERM;
>  		} else {

We didn't rate limit it before. I am not sure how fast the VF can
actually send messages, so I'm not sure if that change would be required.

You could optionally also report the mac_add_max for the untrusted
message as well, but I think its fine to leave as-is in that case as well.

>  			dev_err(&pf->pdev->dev,
> -				"Cannot add more MAC addresses, trusted VF exhausted it's resources\n");
> +				"Cannot add more MAC addresses: trusted VF reached its maximum allowed limit (%d)\n",
> +				mac_add_max);
>  			return -EPERM;
>  		}
>  	}


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net-next,v3,2/2] i40e: support generic devlink param "max_mac_per_vf"

[Simon Horman]

On Wed, Sep 03, 2025 at 03:25:40PM -0700, Jacob Keller wrote:
> 
> 
> On 9/3/2025 2:43 PM, mheib@redhat.com wrote:
> > From: Mohammad Heib <mheib@redhat.com>
> > 
> > Currently the i40e driver enforces its own internally calculated per-VF MAC
> > filter limit, derived from the number of allocated VFs and available
> > hardware resources. This limit is not configurable by the administrator,
> > which makes it difficult to control how many MAC addresses each VF may
> > use.
> > 
> > This patch adds support for the new generic devlink runtime parameter
> > "max_mac_per_vf" which provides administrators with a way to cap the
> > number of MAC addresses a VF can use:
> > 
> > - When the parameter is set to 0 (default), the driver continues to use
> >   its internally calculated limit.
> > 
> > - When set to a non-zero value, the driver applies this value as a strict
> >   cap for VFs, overriding the internal calculation.
> > 
> > Important notes:
> > 
> > - The configured value is a theoretical maximum. Hardware limits may
> >   still prevent additional MAC addresses from being added, even if the
> >   parameter allows it.
> > 
> > - Since MAC filters are a shared hardware resource across all VFs,
> >   setting a high value may cause resource contention and starve other
> >   VFs.
> > 
> > - This change gives administrators predictable and flexible control over
> >   VF resource allocation, while still respecting hardware limitations.
> > 
> > - Previous discussion about this change:
> >   https://lore.kernel.org/netdev/20250805134042.2604897-2-dhill@redhat.com
> >   https://lore.kernel.org/netdev/20250823094952.182181-1-mheib@redhat.com
> > 
> > Signed-off-by: Mohammad Heib <mheib@redhat.com>
> > ---
> 
> This version looks good to me. With or without minor nits relating to
> rate limiting and adding mac_add_max to the untrusted message:
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks, I'm very pleased to see this one coming together.

Reviewed-by: Simon Horman <horms@kernel.org>

> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > index 081a4526a2f0..6e154a8aa474 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > @@ -2935,33 +2935,48 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
> >  		if (!f)
> >  			++mac_add_cnt;
> >  	}
> > -
> > -	/* If this VF is not privileged, then we can't add more than a limited
> > -	 * number of addresses.
> > +	/* Determine the maximum number of MAC addresses this VF may use.
> > +	 *
> > +	 * - For untrusted VFs: use a fixed small limit.
> > +	 *
> > +	 * - For trusted VFs: limit is calculated by dividing total MAC
> > +	 *  filter pool across all VFs/ports.
> >  	 *
> > -	 * If this VF is trusted, it can use more resources than untrusted.
> > -	 * However to ensure that every trusted VF has appropriate number of
> > -	 * resources, divide whole pool of resources per port and then across
> > -	 * all VFs.
> > +	 * - User can override this by devlink param "max_mac_per_vf".
> > +	 *   If set its value is used as a strict cap for both trusted and
> > +	 *   untrusted VFs.
> > +	 *   Note:
> > +	 *    even when overridden, this is a theoretical maximum; hardware
> > +	 *    may reject additional MACs if the absolute HW limit is reached.
> >  	 */
> 
> Good. I think this is better and allows users to also increase limit for
> untrusted VFs without requiring them to become fully "trusted" with the
> all-or-nothing approach. Its more flexible in that regard, and avoids
> the confusion of the parameter not working because a VF is untrusted.

+1

> >  	if (!vf_trusted)
> >  		mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
> >  	else
> >  		mac_add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs, hw->num_ports);
> >  
> > +	if (pf->max_mac_per_vf > 0)
> > +		mac_add_max = pf->max_mac_per_vf;
> > +
> 
> Nice, a clean way to edit the maximum without needing too much special
> casing.
> 
> >  	/* VF can replace all its filters in one step, in this case mac_add_max
> >  	 * will be added as active and another mac_add_max will be in
> >  	 * a to-be-removed state. Account for that.
> >  	 */
> >  	if ((i40e_count_active_filters(vsi) + mac_add_cnt) > mac_add_max ||
> >  	    (i40e_count_all_filters(vsi) + mac_add_cnt) > 2 * mac_add_max) {
> > +		if (pf->max_mac_per_vf == mac_add_max && mac_add_max > 0) {
> > +			dev_err(&pf->pdev->dev,
> > +				"Cannot add more MAC addresses: VF reached its maximum allowed limit (%d)\n",
> > +				mac_add_max);
> > +				return -EPERM;
> > +		}
> 
> Good, having the specific error message will aid system administrators
> in debugging.

Also, +1.

> One thought I had, which isn't a knock on your code as we did the same
> before.. should these be rate limited to prevent VF spamming MAC filter
> adds clogging up the dmesg buffer?
> 
> Given that we didn't do it before, I think its reasonable to not hold
> this patch up for such a cleanup.
> 
> >  		if (!vf_trusted) {
> >  			dev_err(&pf->pdev->dev,
> >  				"Cannot add more MAC addresses, VF is not trusted, switch the VF to trusted to add more functionality\n");
> >  			return -EPERM;
> >  		} else {
> 
> We didn't rate limit it before. I am not sure how fast the VF can
> actually send messages, so I'm not sure if that change would be required.
> 
> You could optionally also report the mac_add_max for the untrusted
> message as well, but I think its fine to leave as-is in that case as well.

I'm not sure either. I'm more used to rate limits in the datapath,
where network traffic can result in a log.

I think that if we want to go down the path you suggest then we should
look at what other logs fall into the same category: generated by VM admin
actions. And perhaps start by looking in the i40e driver for such cases.

Just my 2c worth on this one.

> 
> >  			dev_err(&pf->pdev->dev,
> > -				"Cannot add more MAC addresses, trusted VF exhausted it's resources\n");
> > +				"Cannot add more MAC addresses: trusted VF reached its maximum allowed limit (%d)\n",
> > +				mac_add_max);
> >  			return -EPERM;
> >  		}
> >  	}
> 

Re: [PATCH net-next,v3,2/2] i40e: support generic devlink param "max_mac_per_vf"

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1108 bytes --]



On 9/5/2025 4:46 AM, Simon Horman wrote:
> On Wed, Sep 03, 2025 at 03:25:40PM -0700, Jacob Keller wrote:
>> We didn't rate limit it before. I am not sure how fast the VF can
>> actually send messages, so I'm not sure if that change would be required.
>>
>> You could optionally also report the mac_add_max for the untrusted
>> message as well, but I think its fine to leave as-is in that case as well.
> 
> I'm not sure either. I'm more used to rate limits in the datapath,
> where network traffic can result in a log.
> 
> I think that if we want to go down the path you suggest then we should
> look at what other logs fall into the same category: generated by VM admin
> actions. And perhaps start by looking in the i40e driver for such cases.
> 
> Just my 2c worth on this one.
> 

I noticed that a VF can cause this message to be spammed indefinitely at
whatever rate the PF processes the virtchnl message, once its MAC cap is
hit. I don't think we really protect against that in any virtchnl
message, so that makes me think its likely not been considered a problem
thus far.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net-next,v3,2/2] i40e: support generic devlink param "max_mac_per_vf"

[mohammad heib]

On 9/5/25 2:46 PM, Simon Horman wrote:
> On Wed, Sep 03, 2025 at 03:25:40PM -0700, Jacob Keller wrote:
>>
>>
>> On 9/3/2025 2:43 PM, mheib@redhat.com wrote:
>>> From: Mohammad Heib <mheib@redhat.com>
>>>
>>> Currently the i40e driver enforces its own internally calculated per-VF MAC
>>> filter limit, derived from the number of allocated VFs and available
>>> hardware resources. This limit is not configurable by the administrator,
>>> which makes it difficult to control how many MAC addresses each VF may
>>> use.
>>>
>>> This patch adds support for the new generic devlink runtime parameter
>>> "max_mac_per_vf" which provides administrators with a way to cap the
>>> number of MAC addresses a VF can use:
>>>
>>> - When the parameter is set to 0 (default), the driver continues to use
>>>    its internally calculated limit.
>>>
>>> - When set to a non-zero value, the driver applies this value as a strict
>>>    cap for VFs, overriding the internal calculation.
>>>
>>> Important notes:
>>>
>>> - The configured value is a theoretical maximum. Hardware limits may
>>>    still prevent additional MAC addresses from being added, even if the
>>>    parameter allows it.
>>>
>>> - Since MAC filters are a shared hardware resource across all VFs,
>>>    setting a high value may cause resource contention and starve other
>>>    VFs.
>>>
>>> - This change gives administrators predictable and flexible control over
>>>    VF resource allocation, while still respecting hardware limitations.
>>>
>>> - Previous discussion about this change:
>>>    https://lore.kernel.org/netdev/20250805134042.2604897-2-dhill@redhat.com
>>>    https://lore.kernel.org/netdev/20250823094952.182181-1-mheib@redhat.com
>>>
>>> Signed-off-by: Mohammad Heib <mheib@redhat.com>
>>> ---
>>
>> This version looks good to me. With or without minor nits relating to
>> rate limiting and adding mac_add_max to the untrusted message:
>>
>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Thanks, I'm very pleased to see this one coming together.
Me too :L)
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>> index 081a4526a2f0..6e154a8aa474 100644
>>> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>>> @@ -2935,33 +2935,48 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
>>>   		if (!f)
>>>   			++mac_add_cnt;
>>>   	}
>>> -
>>> -	/* If this VF is not privileged, then we can't add more than a limited
>>> -	 * number of addresses.
>>> +	/* Determine the maximum number of MAC addresses this VF may use.
>>> +	 *
>>> +	 * - For untrusted VFs: use a fixed small limit.
>>> +	 *
>>> +	 * - For trusted VFs: limit is calculated by dividing total MAC
>>> +	 *  filter pool across all VFs/ports.
>>>   	 *
>>> -	 * If this VF is trusted, it can use more resources than untrusted.
>>> -	 * However to ensure that every trusted VF has appropriate number of
>>> -	 * resources, divide whole pool of resources per port and then across
>>> -	 * all VFs.
>>> +	 * - User can override this by devlink param "max_mac_per_vf".
>>> +	 *   If set its value is used as a strict cap for both trusted and
>>> +	 *   untrusted VFs.
>>> +	 *   Note:
>>> +	 *    even when overridden, this is a theoretical maximum; hardware
>>> +	 *    may reject additional MACs if the absolute HW limit is reached.
>>>   	 */
>>
>> Good. I think this is better and allows users to also increase limit for
>> untrusted VFs without requiring them to become fully "trusted" with the
>> all-or-nothing approach. Its more flexible in that regard, and avoids
>> the confusion of the parameter not working because a VF is untrusted.
> 
> +1
> 
>>>   	if (!vf_trusted)
>>>   		mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
>>>   	else
>>>   		mac_add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs, hw->num_ports);
>>>   
>>> +	if (pf->max_mac_per_vf > 0)
>>> +		mac_add_max = pf->max_mac_per_vf;
>>> +
>>
>> Nice, a clean way to edit the maximum without needing too much special
>> casing.
>>
>>>   	/* VF can replace all its filters in one step, in this case mac_add_max
>>>   	 * will be added as active and another mac_add_max will be in
>>>   	 * a to-be-removed state. Account for that.
>>>   	 */
>>>   	if ((i40e_count_active_filters(vsi) + mac_add_cnt) > mac_add_max ||
>>>   	    (i40e_count_all_filters(vsi) + mac_add_cnt) > 2 * mac_add_max) {
>>> +		if (pf->max_mac_per_vf == mac_add_max && mac_add_max > 0) {
>>> +			dev_err(&pf->pdev->dev,
>>> +				"Cannot add more MAC addresses: VF reached its maximum allowed limit (%d)\n",
>>> +				mac_add_max);
>>> +				return -EPERM;
>>> +		}
>>
>> Good, having the specific error message will aid system administrators
>> in debugging.
> 
> Also, +1.
> 
>> One thought I had, which isn't a knock on your code as we did the same
>> before.. should these be rate limited to prevent VF spamming MAC filter
>> adds clogging up the dmesg buffer?
>>
>> Given that we didn't do it before, I think its reasonable to not hold
>> this patch up for such a cleanup.
>>
>>>   		if (!vf_trusted) {
>>>   			dev_err(&pf->pdev->dev,
>>>   				"Cannot add more MAC addresses, VF is not trusted, switch the VF to trusted to add more functionality\n");
>>>   			return -EPERM;
>>>   		} else {
>>
>> We didn't rate limit it before. I am not sure how fast the VF can
>> actually send messages, so I'm not sure if that change would be required.
>>
>> You could optionally also report the mac_add_max for the untrusted
>> message as well, but I think its fine to leave as-is in that case as well.
> 
> I'm not sure either. I'm more used to rate limits in the datapath,
> where network traffic can result in a log.
> 
> I think that if we want to go down the path you suggest then we should
> look at what other logs fall into the same category: generated by VM admin
> actions. And perhaps start by looking in the i40e driver for such cases.
> 
> Just my 2c worth on this one.
> 
>>
>>>   			dev_err(&pf->pdev->dev,
>>> -				"Cannot add more MAC addresses, trusted VF exhausted it's resources\n");
>>> +				"Cannot add more MAC addresses: trusted VF reached its maximum allowed limit (%d)\n",
>>> +				mac_add_max);
>>>   			return -EPERM;
>>>   		}
>>>   	}
>>
> 
> 
> 

Thank you, Jacob, Simon, and Aleksandr, for reviewing this. I really 
appreciate your time.

RE: [PATCH net-next,v3,2/2] i40e: support generic devlink param "max_mac_per_vf"

[Loktionov, Aleksandr]

> -----Original Message-----
> From: mheib@redhat.com <mheib@redhat.com>
> Sent: Wednesday, September 3, 2025 11:43 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: przemyslawx.patynowski@intel.com; jiri@resnulli.us;
> netdev@vger.kernel.org; horms@kernel.org; Keller, Jacob E
> <jacob.e.keller@intel.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Mohammad Heib <mheib@redhat.com>
> Subject: [PATCH net-next,v3,2/2] i40e: support generic devlink param
> "max_mac_per_vf"
> 
> From: Mohammad Heib <mheib@redhat.com>
> 
> Currently the i40e driver enforces its own internally calculated per-
> VF MAC filter limit, derived from the number of allocated VFs and
> available hardware resources. This limit is not configurable by the
> administrator, which makes it difficult to control how many MAC
> addresses each VF may use.
> 
> This patch adds support for the new generic devlink runtime parameter
> "max_mac_per_vf" which provides administrators with a way to cap the
> number of MAC addresses a VF can use:
> 
> - When the parameter is set to 0 (default), the driver continues to
> use
>   its internally calculated limit.
> 
> - When set to a non-zero value, the driver applies this value as a
> strict
>   cap for VFs, overriding the internal calculation.
> 
> Important notes:
> 
> - The configured value is a theoretical maximum. Hardware limits may
>   still prevent additional MAC addresses from being added, even if the
>   parameter allows it.
> 
> - Since MAC filters are a shared hardware resource across all VFs,
>   setting a high value may cause resource contention and starve other
>   VFs.
> 
> - This change gives administrators predictable and flexible control
> over
>   VF resource allocation, while still respecting hardware limitations.
> 
> - Previous discussion about this change:
>   https://lore.kernel.org/netdev/20250805134042.2604897-2-
> dhill@redhat.com
>   https://lore.kernel.org/netdev/20250823094952.182181-1-
> mheib@redhat.com
> 
> Signed-off-by: Mohammad Heib <mheib@redhat.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>


> ---
...
> --
> 2.50.1

Re: [PATCH net-next,v3,2/2] i40e: support generic devlink param "max_mac_per_vf"

[Simon Horman]

On Thu, Sep 04, 2025 at 12:43:05AM +0300, mheib@redhat.com wrote:
> From: Mohammad Heib <mheib@redhat.com>
> 
> Currently the i40e driver enforces its own internally calculated per-VF MAC
> filter limit, derived from the number of allocated VFs and available
> hardware resources. This limit is not configurable by the administrator,
> which makes it difficult to control how many MAC addresses each VF may
> use.
> 
> This patch adds support for the new generic devlink runtime parameter
> "max_mac_per_vf" which provides administrators with a way to cap the
> number of MAC addresses a VF can use:
> 
> - When the parameter is set to 0 (default), the driver continues to use
>   its internally calculated limit.
> 
> - When set to a non-zero value, the driver applies this value as a strict
>   cap for VFs, overriding the internal calculation.
> 
> Important notes:
> 
> - The configured value is a theoretical maximum. Hardware limits may
>   still prevent additional MAC addresses from being added, even if the
>   parameter allows it.
> 
> - Since MAC filters are a shared hardware resource across all VFs,
>   setting a high value may cause resource contention and starve other
>   VFs.
> 
> - This change gives administrators predictable and flexible control over
>   VF resource allocation, while still respecting hardware limitations.
> 
> - Previous discussion about this change:
>   https://lore.kernel.org/netdev/20250805134042.2604897-2-dhill@redhat.com
>   https://lore.kernel.org/netdev/20250823094952.182181-1-mheib@redhat.com
> 
> Signed-off-by: Mohammad Heib <mheib@redhat.com>
> ---
>  Documentation/networking/devlink/i40e.rst     | 32 +++++++++++++
>  drivers/net/ethernet/intel/i40e/i40e.h        |  4 ++
>  .../net/ethernet/intel/i40e/i40e_devlink.c    | 48 ++++++++++++++++++-
>  .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 31 ++++++++----
>  4 files changed, 105 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/networking/devlink/i40e.rst b/Documentation/networking/devlink/i40e.rst
> index d3cb5bb5197e..524524fdd3de 100644
> --- a/Documentation/networking/devlink/i40e.rst
> +++ b/Documentation/networking/devlink/i40e.rst
> @@ -7,6 +7,38 @@ i40e devlink support
>  This document describes the devlink features implemented by the ``i40e``
>  device driver.
>  
> +Parameters
> +==========
> +
> +.. list-table:: Generic parameters implemented
> +    :widths: 5 5 90
> +
> +    * - Name
> +      - Mode
> +      - Notes
> +    * - ``max_mac_per_vf``
> +      - runtime
> +      - Controls the maximum number of MAC addresses a VF can use
> +        on i40e devices.
> +
> +        By default (``0``), the driver enforces its internally calculated per-VF
> +        MAC filter limit, which is based on the number of allocated VFS.
> +
> +        If set to a non-zero value, this parameter acts as a strict cap:
> +        the driver will use the user-provided value instead of its internal
> +        calculation.
> +
> +        **Important notes:**
> +        - MAC filters are a **shared hardware resource** across all VFs.

Sorry for not noticing this before sending my previous response.

make htmldocs is unhappy about the line above. Could you look into it?

.../i40e.rst:33: ERROR: Unexpected indentation. [docutils]

> +          Setting a high value may cause other VFs to be starved of filters.
> +
> +        - This value is a **theoretical maximum**. The hardware may return
> +          errors when its absolute limit is reached, regardless of the value
> +          set here.
> +
> +        The default value is ``0`` (internal calculation is used).
> +
> +

...

Re: [PATCH net-next,v3,2/2] i40e: support generic devlink param "max_mac_per_vf"

[mohammad heib]

Hi Simon,

Thanks for the review i fixed it in v4.


On 9/5/25 3:25 PM, Simon Horman wrote:
> On Thu, Sep 04, 2025 at 12:43:05AM +0300, mheib@redhat.com wrote:
>> From: Mohammad Heib <mheib@redhat.com>
>>
>> Currently the i40e driver enforces its own internally calculated per-VF MAC
>> filter limit, derived from the number of allocated VFs and available
>> hardware resources. This limit is not configurable by the administrator,
>> which makes it difficult to control how many MAC addresses each VF may
>> use.
>>
>> This patch adds support for the new generic devlink runtime parameter
>> "max_mac_per_vf" which provides administrators with a way to cap the
>> number of MAC addresses a VF can use:
>>
>> - When the parameter is set to 0 (default), the driver continues to use
>>    its internally calculated limit.
>>
>> - When set to a non-zero value, the driver applies this value as a strict
>>    cap for VFs, overriding the internal calculation.
>>
>> Important notes:
>>
>> - The configured value is a theoretical maximum. Hardware limits may
>>    still prevent additional MAC addresses from being added, even if the
>>    parameter allows it.
>>
>> - Since MAC filters are a shared hardware resource across all VFs,
>>    setting a high value may cause resource contention and starve other
>>    VFs.
>>
>> - This change gives administrators predictable and flexible control over
>>    VF resource allocation, while still respecting hardware limitations.
>>
>> - Previous discussion about this change:
>>    https://lore.kernel.org/netdev/20250805134042.2604897-2-dhill@redhat.com
>>    https://lore.kernel.org/netdev/20250823094952.182181-1-mheib@redhat.com
>>
>> Signed-off-by: Mohammad Heib <mheib@redhat.com>
>> ---
>>   Documentation/networking/devlink/i40e.rst     | 32 +++++++++++++
>>   drivers/net/ethernet/intel/i40e/i40e.h        |  4 ++
>>   .../net/ethernet/intel/i40e/i40e_devlink.c    | 48 ++++++++++++++++++-
>>   .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 31 ++++++++----
>>   4 files changed, 105 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/networking/devlink/i40e.rst b/Documentation/networking/devlink/i40e.rst
>> index d3cb5bb5197e..524524fdd3de 100644
>> --- a/Documentation/networking/devlink/i40e.rst
>> +++ b/Documentation/networking/devlink/i40e.rst
>> @@ -7,6 +7,38 @@ i40e devlink support
>>   This document describes the devlink features implemented by the ``i40e``
>>   device driver.
>>   
>> +Parameters
>> +==========
>> +
>> +.. list-table:: Generic parameters implemented
>> +    :widths: 5 5 90
>> +
>> +    * - Name
>> +      - Mode
>> +      - Notes
>> +    * - ``max_mac_per_vf``
>> +      - runtime
>> +      - Controls the maximum number of MAC addresses a VF can use
>> +        on i40e devices.
>> +
>> +        By default (``0``), the driver enforces its internally calculated per-VF
>> +        MAC filter limit, which is based on the number of allocated VFS.
>> +
>> +        If set to a non-zero value, this parameter acts as a strict cap:
>> +        the driver will use the user-provided value instead of its internal
>> +        calculation.
>> +
>> +        **Important notes:**
>> +        - MAC filters are a **shared hardware resource** across all VFs.
> 
> Sorry for not noticing this before sending my previous response.
> 
> make htmldocs is unhappy about the line above. Could you look into it?
> 
> .../i40e.rst:33: ERROR: Unexpected indentation. [docutils]
> 
>> +          Setting a high value may cause other VFs to be starved of filters.
>> +
>> +        - This value is a **theoretical maximum**. The hardware may return
>> +          errors when its absolute limit is reached, regardless of the value
>> +          set here.
>> +
>> +        The default value is ``0`` (internal calculation is used).
>> +
>> +
> 
> ...
> 

Re: [PATCH net-next,v3,1/2] devlink: Add new "max_mac_per_vf" generic device param

[Simon Horman]

On Thu, Sep 04, 2025 at 12:43:04AM +0300, mheib@redhat.com wrote:
> From: Mohammad Heib <mheib@redhat.com>
> 
> Add a new device generic parameter to controls the maximum
> number of MAC filters allowed per VF.
> 
> For example, to limit a VF to 3 MAC addresses:
>  $ devlink dev param set pci/0000:3b:00.0 name max_mac_per_vf \
>         value 3 \
>         cmode runtime
> 
> Signed-off-by: Mohammad Heib <mheib@redhat.com>

Overall this looks good to me, thanks.

Reviewed-by: Simon Horman <horms@kernel.org>

One point: This patch-set applies cleanly to iwl but not net-next.
If it is to be picked up by Tony and go via the iwl tree, then all good
on my side. But if it is targeted at net-next then you'll need to
rebase and repost.

...

Re: [PATCH net-next,v3,1/2] devlink: Add new "max_mac_per_vf" generic device param

[mohammad heib]

Hi Simon,

This patch set targets the iwl tree because the second patch depends on 
changes [1] that are already in the tnguy/net-queue.git tree but have 
not yet reached net/next.

[1] e08bca25bd7f ("i40e: improve VF MAC filters accounting")

Thanks,

On 9/5/25 3:22 PM, Simon Horman wrote:
> On Thu, Sep 04, 2025 at 12:43:04AM +0300, mheib@redhat.com wrote:
>> From: Mohammad Heib <mheib@redhat.com>
>>
>> Add a new device generic parameter to controls the maximum
>> number of MAC filters allowed per VF.
>>
>> For example, to limit a VF to 3 MAC addresses:
>>   $ devlink dev param set pci/0000:3b:00.0 name max_mac_per_vf \
>>          value 3 \
>>          cmode runtime
>>
>> Signed-off-by: Mohammad Heib <mheib@redhat.com>
> 
> Overall this looks good to me, thanks.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> One point: This patch-set applies cleanly to iwl but not net-next.
> If it is to be picked up by Tony and go via the iwl tree, then all good
> on my side. But if it is targeted at net-next then you'll need to
> rebase and repost.
> 
> ...
>