Date: Tue, 9 May 2023 11:15:57 +0000
From: "Sokolowski, Jan" <jan.sokolowski@...el.com>
To: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: As usage of ethtool private flags is deprecated, what should we use
 from now on?

Hello netdev!

So, as recently I've been trying to upstream a patch that would introduce a new private flag to i40e driver,
I've received a note that, according to this reply from Jakub Kicinski (https://lore.kernel.org/netdev/20230207215643.43f76bdd@kernel.org/),
the use of private flags is deprecated and is something that will not be accepted by upstream anymore. 
As such flags are an easy way to flip driver-specific behavior switches,
and in the future there would be more patches written to allow end-users to change how the driver works,
it appears that a new way forward is required.

Therefore, do you have any tips on what should private flags be replaced with? 

Regards
Jan

-----Original Message-----
From: Nguyen, Anthony L <anthony.l.nguyen@...el.com> 
Sent: Monday, March 13, 2023 9:35 PM
To: Sokolowski, Jan <jan.sokolowski@...el.com>; e1000-patches@...ists.intel.com; Keller, Jacob E <jacob.e.keller@...el.com>; Lobakin, Aleksander <aleksander.lobakin@...el.com>; Brandeburg, Jesse <jesse.brandeburg@...el.com>; Raczynski, Piotr <piotr.raczynski@...el.com>
Subject: Re: [e1000-patches] [PATCH net-next v3 2/2] i40e: add mdd-auto-reset-vf private flag

On 3/10/2023 6:51 AM, Jan Sokolowski wrote:
> Since VF RX MDD events should disable the queue, add ethtool
> private flag mdd-auto-reset-vf to configure VF reset
> to re-enable the queue. This can be used by a system's administrator
> to select the desired level of security in running VF's.

I think this still needs better justification on the private flag. 
Maintainer has already ciriticized our use of private flags so we need 
to give good justification.

"
Intel keeps trickling in private flags to adjust the behavior of legacy
SR-IOV NDOs. No documentation, no well understood semantics.
"
https://lore.kernel.org/netdev/20230207215643.43f76bdd@kernel.org/

I think commit should provide full justification as Jake suggested 
previously:
"
If we want to try and add this flag I think we need to do so openly, and
clearly explain the reasons. Include information about how we're
aligning with another implementation (ice), explain the
security-critical functionality, and probably as RFC with some potential
explanation of alternatives considered.
"
https://eclists.intel.com/sympa/arc/e1000-patches/2023-02/msg00104.html

IMO the MDD reporting (minus the reference to mdd-auto-reset-vfs) should 
be in patch one. If you are breaking out individual MDD events in patch 
one, why not add the reporting as well? If this private flag/patch is 
not accepted, it justifies us splitting the stats out as well as gives 
us the printing functionality without the need for this patch).

Thanks,
Tony

> Signed-off-by: Jan Sokolowski <jan.sokolowski@...el.com>
> ---
> v2: Replaced custom ratelimiting to printk_ratelimit
> v3: Reworded commit message.
> ---
>   drivers/net/ethernet/intel/i40e/i40e.h        |  2 +-
>   .../net/ethernet/intel/i40e/i40e_ethtool.c    |  1 +
>   drivers/net/ethernet/intel/i40e/i40e_main.c   | 75 ++++++++++++++++---
>   .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  2 +
>   4 files changed, 70 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
> index 6e310a539467..72bd45c4f9ba 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> @@ -603,7 +603,7 @@ struct i40e_pf {
>    *   in abilities field of i40e_aq_set_phy_config structure
>    */
>   #define I40E_FLAG_TOTAL_PORT_SHUTDOWN_ENABLED	BIT(27)
> -
> +#define I40E_FLAG_MDD_AUTO_RESET_VF		BIT(28)
>   	struct i40e_client_instance *cinst;
>   	bool stat_offsets_loaded;
>   	struct i40e_hw_port_stats stats;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 4934ff58332c..1ed1df620ae4 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -457,6 +457,7 @@ static const struct i40e_priv_flags i40e_gstrings_priv_flags[] = {
>   	I40E_PRIV_FLAG("base-r-fec", I40E_FLAG_BASE_R_FEC, 0),
>   	I40E_PRIV_FLAG("vf-vlan-pruning",
>   		       I40E_FLAG_VF_VLAN_PRUNING, 0),
> +	I40E_PRIV_FLAG("mdd-auto-reset-vf", I40E_FLAG_MDD_AUTO_RESET_VF, 0),
>   };
>   
>   #define I40E_PRIV_FLAGS_STR_LEN ARRAY_SIZE(i40e_gstrings_priv_flags)
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 9bab68f0a618..f00d8b674687 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -11150,6 +11150,52 @@ static void i40e_handle_reset_warning(struct i40e_pf *pf, bool lock_acquired)
>   	i40e_reset_and_rebuild(pf, false, lock_acquired);
>   }
>   
> +/**
> + * i40e_print_vf_rx_mdd_event - print VF Rx malicious driver detect event
> + * @vf: pointer to the VF structure
> + */
> +static void i40e_print_vf_rx_mdd_event(struct i40e_pf *pf, struct i40e_vf *vf)
> +{
> +	dev_err_ratelimited(&pf->pdev->dev, "%lld Rx Malicious Driver Detection events detected on PF %d VF %d MAC %pm. mdd-auto-reset-vfs=%s\n",
> +		vf->mdd_rx_events.count,
> +		pf->hw.pf_id,
> +		vf->vf_id,
> +		vf->default_lan_addr.addr,
> +		(I40E_FLAG_MDD_AUTO_RESET_VF & pf->flags) ? "on" : "off");
> +}
> +
> +/**
> + * i40e_print_vfs_mdd_events - print VFs malicious driver detect event
> + * @pf: pointer to the PF structure
> + *
> + * Called from i40e_handle_mdd_event to rate limit and print VFs MDD events.
> + */
> +static void i40e_print_vfs_mdd_events(struct i40e_pf *pf)
> +{
> +	struct i40e_vf *vf;
> +	unsigned int i;
> +
> +	for (i = 0; i < pf->num_alloc_vfs; i++) {
> +		vf = &pf->vf[i];
> +		/* only print Rx MDD event message if there are new events */
> +		if (vf->mdd_rx_events.count != vf->mdd_rx_events.last_printed) {
> +			vf->mdd_rx_events.last_printed = vf->mdd_rx_events.count;
> +			i40e_print_vf_rx_mdd_event(pf, vf);
> +		}
> +
> +		/* only print Tx MDD event message if there are new events */
> +		if (vf->mdd_tx_events.count != vf->mdd_tx_events.last_printed) {
> +			vf->mdd_tx_events.last_printed = vf->mdd_tx_events.count;
> +			dev_err_ratelimited(&pf->pdev->dev, "%lld Tx Malicious Driver Detection events detected on PF %d VF %d MAC %pM.\n",
> +				vf->mdd_tx_events.count,
> +				pf->hw.pf_id,
> +				vf->vf_id,
> +				vf->default_lan_addr.addr);
> +		}
> +	}
> +}
> +
> +
>   /**
>    * i40e_handle_mdd_event
>    * @pf: pointer to the PF structure
> @@ -11164,8 +11210,13 @@ static void i40e_handle_mdd_event(struct i40e_pf *pf)
>   	u32 reg;
>   	int i;
>   
> -	if (!test_bit(__I40E_MDD_EVENT_PENDING, pf->state))
> +	if (!test_and_clear_bit(__I40E_MDD_EVENT_PENDING, pf->state)) {
> +		/* Since the VF MDD event logging is rate limited, check if
> +		 * there are pending MDD events.
> +		 */
> +		i40e_print_vfs_mdd_events(pf);
>   		return;
> +	}
>   
>   	/* find what triggered the MDD event */
>   	reg = rd32(hw, I40E_GL_MDET_TX);
> @@ -11221,10 +11272,6 @@ static void i40e_handle_mdd_event(struct i40e_pf *pf)
>   		if (reg & I40E_VP_MDET_TX_VALID_MASK) {
>   			wr32(hw, I40E_VP_MDET_TX(i), 0xFFFF);
>   			vf->mdd_tx_events.count++;
> -			dev_info(&pf->pdev->dev, "TX driver issue detected on VF %d\n",
> -				 i);
> -			dev_info(&pf->pdev->dev,
> -				 "Use PF Control I/F to re-enable the VF\n");
>   			set_bit(I40E_VF_STATE_DISABLED, &vf->vf_states);
>   		}
>   
> @@ -11232,11 +11279,19 @@ static void i40e_handle_mdd_event(struct i40e_pf *pf)
>   		if (reg & I40E_VP_MDET_RX_VALID_MASK) {
>   			wr32(hw, I40E_VP_MDET_RX(i), 0xFFFF);
>   			vf->mdd_rx_events.count++;
> -			dev_info(&pf->pdev->dev, "RX driver issue detected on VF %d\n",
> -				 i);
> -			dev_info(&pf->pdev->dev,
> -				 "Use PF Control I/F to re-enable the VF\n");
>   			set_bit(I40E_VF_STATE_DISABLED, &vf->vf_states);
> +
> +			if (pf->flags & I40E_FLAG_MDD_AUTO_RESET_VF) {
> +				/* VF MDD event counters will be cleared by
> +				 * reset, so print the event prior to reset.
> +				 */
> +				i40e_print_vf_rx_mdd_event(pf, vf);
> +				i40e_vc_notify_vf_reset(vf);
> +				/* Allow VF to process pending reset notification */
> +				msleep(20);
> +
> +				i40e_reset_vf(vf, false);
> +			}
>   		}
>   	}
>   
> @@ -11246,6 +11301,8 @@ static void i40e_handle_mdd_event(struct i40e_pf *pf)
>   	reg |=  I40E_PFINT_ICR0_ENA_MAL_DETECT_MASK;
>   	wr32(hw, I40E_PFINT_ICR0_ENA, reg);
>   	i40e_flush(hw);
> +
> +	i40e_print_vfs_mdd_events(pf);
>   }
>   
>   /**
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
> index 9e701ff09e37..c3bae16cc679 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
> @@ -75,6 +75,8 @@ struct i40e_vm_mac {
>   
>   struct i40e_mdd_vf_events {
>   	u64 count; /* total count of Rx|Tx events */
> +	/* count number of the last printed event */
> +	u64 last_printed;
>   };
>   
>   /* VF information structure */
> 
> 
> ------------------------------------------------------------------------------
> e1000-patches mailing list:   e1000-patches@...ists.intel.com
> 

Powered by blists - more mailing lists