On Wed, Sep 03, 2025 at 03:25:40PM -0700, Jacob Keller wrote: > > > On 9/3/2025 2:43 PM, [email protected] wrote: > > From: Mohammad Heib <[email protected]> > > > > 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/[email protected] > > https://lore.kernel.org/netdev/[email protected] > > > > Signed-off-by: Mohammad Heib <[email protected]> > > --- > > 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 <[email protected]>
Thanks, I'm very pleased to see this one coming together. Reviewed-by: Simon Horman <[email protected]> > > 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; > > } > > } >
