* As usage of ethtool private flags is deprecated, what should we use from now on?
[not found] ` <99823d11-4c46-d105-aaa5-2d5da113627d@intel.com>
@ 2023-05-09 11:15 ` Sokolowski, Jan
2023-05-09 13:26 ` Andrew Lunn
2023-05-09 15:50 ` Jakub Kicinski
0 siblings, 2 replies; 3+ messages in thread
From: Sokolowski, Jan @ 2023-05-09 11:15 UTC (permalink / raw)
To: netdev@vger.kernel.org
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@intel.com>
Sent: Monday, March 13, 2023 9:35 PM
To: Sokolowski, Jan <jan.sokolowski@intel.com>; e1000-patches@eclists.intel.com; Keller, Jacob E <jacob.e.keller@intel.com>; Lobakin, Aleksander <aleksander.lobakin@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Raczynski, Piotr <piotr.raczynski@intel.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@intel.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@eclists.intel.com
>
^ permalink raw reply [flat|nested] 3+ messages in thread