[Intel-wired-lan] [PATCH iwl-next v3] ice: replace ice_vf_recreate_vsi() with ice_vf_reconfig_vsi()

[Intel-wired-lan] [PATCH iwl-next v3] ice: replace ice_vf_recreate_vsi() with ice_vf_reconfig_vsi()

[Jacob Keller]

The ice_vf_create_vsi() function and its VF ops helper introduced by commit
a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") are used
during an individual VF reset to re-create the VSI. This was done in order
to ensure that the VSI gets properly reconfigured within the hardware.

This is somewhat heavy handed as we completely release the VSI memory and
structure, and then create a new VSI. This can also potentially force a
change of the VSI index as we will re-use the first open slot in the VSI
array which may not be the same.

As part of implementing devlink reload, commit 6624e780a577 ("ice: split
ice_vsi_setup into smaller functions") split VSI setup into smaller
functions, introducing both ice_vsi_cfg() and ice_vsi_decfg() which can be
used to configure or deconfigure an existing software VSI structure.

Instead of completely removing the VSI and adding a new one with the
.create_vsi() VF operation, simply perform a reconfiguration of the VSI to
configure its new parameters.

Call ice_vsi_decfg() to remove current VSI configuration data. Call
ice_vsi_cfg() to reconfigure the VSI with all new configuration to match
the modified VF configuration data.

This *does not* remove the VSI from the hardware tables, as that only
happens if the VSI is actually fully removed via the ice_free_vsi()
function. ice_vsi_decfg() only deconfigures but does not remove the VSI
from the switch tables completely.

This new operation does not re-create the VSI, so rename it to
ice_vf_reconfig_vsi().

The new approach can safely share the exact same flow for both SR-IOV VFs
as well as the Scalable IOV VFs being worked on. This uses less code, is a
better abstraction, and is less invasive than the previous remove and
re-add cycle.

Co-developed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v2:
* Switch to ICE_VSI_FLAG_NO_INIT to avoid leaking the VSI in firwmare
* Remove now unnecessary update of the VSI number

Thanks to Michal for pointing this out and suggesting the fix. I've added
him as Co-developed-by for that reason.

 drivers/net/ethernet/intel/ice/ice_sriov.c  | 19 --------------
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 28 +++++++++++++--------
 drivers/net/ethernet/intel/ice/ice_vf_lib.h |  1 -
 3 files changed, 18 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 1f66914c7a20..ab96a5596d09 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -733,24 +733,6 @@ static void ice_sriov_clear_reset_trigger(struct ice_vf *vf)
 	ice_flush(hw);
 }
 
-/**
- * ice_sriov_create_vsi - Create a new VSI for a VF
- * @vf: VF to create the VSI for
- *
- * This is called by ice_vf_recreate_vsi to create the new VSI after the old
- * VSI has been released.
- */
-static int ice_sriov_create_vsi(struct ice_vf *vf)
-{
-	struct ice_vsi *vsi;
-
-	vsi = ice_vf_vsi_setup(vf);
-	if (!vsi)
-		return -ENOMEM;
-
-	return 0;
-}
-
 /**
  * ice_sriov_post_vsi_rebuild - tasks to do after the VF's VSI have been rebuilt
  * @vf: VF to perform tasks on
@@ -770,7 +752,6 @@ static const struct ice_vf_ops ice_sriov_vf_ops = {
 	.poll_reset_status = ice_sriov_poll_reset_status,
 	.clear_reset_trigger = ice_sriov_clear_reset_trigger,
 	.irq_close = NULL,
-	.create_vsi = ice_sriov_create_vsi,
 	.post_vsi_rebuild = ice_sriov_post_vsi_rebuild,
 };
 
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index b26ce4425f45..5fcb48c2dda4 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -265,25 +265,33 @@ static void ice_vf_pre_vsi_rebuild(struct ice_vf *vf)
 }
 
 /**
- * ice_vf_recreate_vsi - Release and re-create the VF's VSI
- * @vf: VF to recreate the VSI for
+ * ice_vf_reconfig_vsi - Reconfigure a VF VSI with the device
+ * @vf: VF to reconfigure the VSI for
  *
- * This is only called when a single VF is being reset (i.e. VVF, VFLR, host
- * VF configuration change, etc)
+ * This is called when a single VF is being reset (i.e. VVF, VFLR, host VF
+ * configuration change, etc).
  *
- * It releases and then re-creates a new VSI.
+ * It brings the VSI down and then reconfigures it with the hardware.
  */
-static int ice_vf_recreate_vsi(struct ice_vf *vf)
+static int ice_vf_reconfig_vsi(struct ice_vf *vf)
 {
+	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
+	struct ice_vsi_cfg_params params = {};
 	struct ice_pf *pf = vf->pf;
 	int err;
 
-	ice_vf_vsi_release(vf);
+	if (WARN_ON(!vsi))
+		return -EINVAL;
 
-	err = vf->vf_ops->create_vsi(vf);
+	params = ice_vsi_to_params(vsi);
+	params.flags = ICE_VSI_FLAG_NO_INIT;
+
+	ice_vsi_decfg(vsi);
+
+	err = ice_vsi_cfg(vsi, &params);
 	if (err) {
 		dev_err(ice_pf_to_dev(pf),
-			"Failed to recreate the VF%u's VSI, error %d\n",
+			"Failed to reconfigure the VF%u's VSI, error %d\n",
 			vf->vf_id, err);
 		return err;
 	}
@@ -699,7 +707,7 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
 
 	ice_vf_pre_vsi_rebuild(vf);
 
-	if (ice_vf_recreate_vsi(vf)) {
+	if (ice_vf_reconfig_vsi(vf)) {
 		dev_err(dev, "Failed to release and setup the VF%u's VSI\n",
 			vf->vf_id);
 		err = -EFAULT;
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
index 67172fdd9bc2..ad648009a238 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
@@ -62,7 +62,6 @@ struct ice_vf_ops {
 	bool (*poll_reset_status)(struct ice_vf *vf);
 	void (*clear_reset_trigger)(struct ice_vf *vf);
 	void (*irq_close)(struct ice_vf *vf);
-	int (*create_vsi)(struct ice_vf *vf);
 	void (*post_vsi_rebuild)(struct ice_vf *vf);
 };
 

base-commit: 8d5bc02673a4fa9f5db4e9e949f41cfc68ae1758
-- 
2.41.0.1.g9857a21e0017.dirty

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

Re: [Intel-wired-lan] [PATCH iwl-next v3] ice: replace ice_vf_recreate_vsi() with ice_vf_reconfig_vsi()

[Michal Swiatkowski]

On Wed, Jul 12, 2023 at 03:29:36PM -0700, Jacob Keller wrote:
> The ice_vf_create_vsi() function and its VF ops helper introduced by commit
> a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") are used
> during an individual VF reset to re-create the VSI. This was done in order
> to ensure that the VSI gets properly reconfigured within the hardware.
> 
> This is somewhat heavy handed as we completely release the VSI memory and
> structure, and then create a new VSI. This can also potentially force a
> change of the VSI index as we will re-use the first open slot in the VSI
> array which may not be the same.
> 
> As part of implementing devlink reload, commit 6624e780a577 ("ice: split
> ice_vsi_setup into smaller functions") split VSI setup into smaller
> functions, introducing both ice_vsi_cfg() and ice_vsi_decfg() which can be
> used to configure or deconfigure an existing software VSI structure.
> 
> Instead of completely removing the VSI and adding a new one with the
> .create_vsi() VF operation, simply perform a reconfiguration of the VSI to
> configure its new parameters.
> 
> Call ice_vsi_decfg() to remove current VSI configuration data. Call
> ice_vsi_cfg() to reconfigure the VSI with all new configuration to match
> the modified VF configuration data.
> 
> This *does not* remove the VSI from the hardware tables, as that only
> happens if the VSI is actually fully removed via the ice_free_vsi()
> function. ice_vsi_decfg() only deconfigures but does not remove the VSI
> from the switch tables completely.
> 
> This new operation does not re-create the VSI, so rename it to
> ice_vf_reconfig_vsi().
> 
> The new approach can safely share the exact same flow for both SR-IOV VFs
> as well as the Scalable IOV VFs being worked on. This uses less code, is a
> better abstraction, and is less invasive than the previous remove and
> re-add cycle.
> 
> Co-developed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> Changes since v2:
> * Switch to ICE_VSI_FLAG_NO_INIT to avoid leaking the VSI in firwmare
> * Remove now unnecessary update of the VSI number
> 
> Thanks to Michal for pointing this out and suggesting the fix. I've added
> him as Co-developed-by for that reason.
> 
>  drivers/net/ethernet/intel/ice/ice_sriov.c  | 19 --------------
>  drivers/net/ethernet/intel/ice/ice_vf_lib.c | 28 +++++++++++++--------
>  drivers/net/ethernet/intel/ice/ice_vf_lib.h |  1 -
>  3 files changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 1f66914c7a20..ab96a5596d09 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -733,24 +733,6 @@ static void ice_sriov_clear_reset_trigger(struct ice_vf *vf)
>  	ice_flush(hw);
>  }

Looks good, thanks
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next v3] ice: replace ice_vf_recreate_vsi() with ice_vf_reconfig_vsi()

[Przemek Kitszel]

On 7/13/23 00:29, Jacob Keller wrote:
> The ice_vf_create_vsi() function and its VF ops helper introduced by commit
> a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") are used
> during an individual VF reset to re-create the VSI. This was done in order
> to ensure that the VSI gets properly reconfigured within the hardware.
> 
> This is somewhat heavy handed as we completely release the VSI memory and
> structure, and then create a new VSI. This can also potentially force a
> change of the VSI index as we will re-use the first open slot in the VSI
> array which may not be the same.
> 
> As part of implementing devlink reload, commit 6624e780a577 ("ice: split
> ice_vsi_setup into smaller functions") split VSI setup into smaller
> functions, introducing both ice_vsi_cfg() and ice_vsi_decfg() which can be
> used to configure or deconfigure an existing software VSI structure.
> 
> Instead of completely removing the VSI and adding a new one with the
> .create_vsi() VF operation, simply perform a reconfiguration of the VSI to
> configure its new parameters.
> 
> Call ice_vsi_decfg() to remove current VSI configuration data. Call
> ice_vsi_cfg() to reconfigure the VSI with all new configuration to match
> the modified VF configuration data.
> 
> This *does not* remove the VSI from the hardware tables, as that only
> happens if the VSI is actually fully removed via the ice_free_vsi()
> function. ice_vsi_decfg() only deconfigures but does not remove the VSI
> from the switch tables completely.
> 
> This new operation does not re-create the VSI, so rename it to
> ice_vf_reconfig_vsi().
> 
> The new approach can safely share the exact same flow for both SR-IOV VFs
> as well as the Scalable IOV VFs being worked on. This uses less code, is a
> better abstraction, and is less invasive than the previous remove and
> re-add cycle.
> 
> Co-developed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> Changes since v2:
> * Switch to ICE_VSI_FLAG_NO_INIT to avoid leaking the VSI in firwmare
> * Remove now unnecessary update of the VSI number
> 
> Thanks to Michal for pointing this out and suggesting the fix. I've added
> him as Co-developed-by for that reason.
> 
>   drivers/net/ethernet/intel/ice/ice_sriov.c  | 19 --------------
>   drivers/net/ethernet/intel/ice/ice_vf_lib.c | 28 +++++++++++++--------
>   drivers/net/ethernet/intel/ice/ice_vf_lib.h |  1 -
>   3 files changed, 18 insertions(+), 30 deletions(-)
> 

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>



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

Re: [Intel-wired-lan] [PATCH iwl-next v3] ice: replace ice_vf_recreate_vsi() with ice_vf_reconfig_vsi()

[Petr Oros]

Jacob Keller píše v Čt 01. 01. 1970 v 00:00 +0000:
> The ice_vf_create_vsi() function and its VF ops helper introduced by
> commit
> a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") are
> used
> during an individual VF reset to re-create the VSI. This was done in
> order
> to ensure that the VSI gets properly reconfigured within the
> hardware.
> 
> This is somewhat heavy handed as we completely release the VSI memory
> and
> structure, and then create a new VSI. This can also potentially force
> a
> change of the VSI index as we will re-use the first open slot in the
> VSI
> array which may not be the same.
> 
> As part of implementing devlink reload, commit 6624e780a577 ("ice:
> split
> ice_vsi_setup into smaller functions") split VSI setup into smaller
> functions, introducing both ice_vsi_cfg() and ice_vsi_decfg() which
> can be
> used to configure or deconfigure an existing software VSI structure.
> 
> Instead of completely removing the VSI and adding a new one with the
> .create_vsi() VF operation, simply perform a reconfiguration of the
> VSI to
> configure its new parameters.
> 
> Call ice_vsi_decfg() to remove current VSI configuration data. Call
> ice_vsi_cfg() to reconfigure the VSI with all new configuration to
> match
> the modified VF configuration data.
> 
> This *does not* remove the VSI from the hardware tables, as that only
> happens if the VSI is actually fully removed via the ice_free_vsi()
> function. ice_vsi_decfg() only deconfigures but does not remove the
> VSI
> from the switch tables completely.
> 
> This new operation does not re-create the VSI, so rename it to
> ice_vf_reconfig_vsi().
> 
> The new approach can safely share the exact same flow for both SR-IOV
> VFs
> as well as the Scalable IOV VFs being worked on. This uses less code,
> is a
> better abstraction, and is less invasive than the previous remove and
> re-add cycle.
> 
> Co-developed-by: Michal Swiatkowski
> <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Michal Swiatkowski
> <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> Changes since v2:
> * Switch to ICE_VSI_FLAG_NO_INIT to avoid leaking the VSI in firwmare
> * Remove now unnecessary update of the VSI number
> 
> Thanks to Michal for pointing this out and suggesting the fix. I've
> added
> him as Co-developed-by for that reason.
> 
>  drivers/net/ethernet/intel/ice/ice_sriov.c  | 19 --------------
>  drivers/net/ethernet/intel/ice/ice_vf_lib.c | 28 +++++++++++++------
> --
>  drivers/net/ethernet/intel/ice/ice_vf_lib.h |  1 -
>  3 files changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c
> b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 1f66914c7a20..ab96a5596d09 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -733,24 +733,6 @@ static void ice_sriov_clear_reset_trigger(struct
> ice_vf *vf)
>         ice_flush(hw);
>  }
>  
> -/**
> - * ice_sriov_create_vsi - Create a new VSI for a VF
> - * @vf: VF to create the VSI for
> - *
> - * This is called by ice_vf_recreate_vsi to create the new VSI after
> the old
> - * VSI has been released.
> - */
> -static int ice_sriov_create_vsi(struct ice_vf *vf)
> -{
> -       struct ice_vsi *vsi;
> -
> -       vsi = ice_vf_vsi_setup(vf);
> -       if (!vsi)
> -               return -ENOMEM;
> -
> -       return 0;
> -}
> -
>  /**
>   * ice_sriov_post_vsi_rebuild - tasks to do after the VF's VSI have
> been rebuilt
>   * @vf: VF to perform tasks on
> @@ -770,7 +752,6 @@ static const struct ice_vf_ops ice_sriov_vf_ops =
> {
>         .poll_reset_status = ice_sriov_poll_reset_status,
>         .clear_reset_trigger = ice_sriov_clear_reset_trigger,
>         .irq_close = NULL,
> -       .create_vsi = ice_sriov_create_vsi,
>         .post_vsi_rebuild = ice_sriov_post_vsi_rebuild,
>  };
>  
> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> index b26ce4425f45..5fcb48c2dda4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> @@ -265,25 +265,33 @@ static void ice_vf_pre_vsi_rebuild(struct
> ice_vf *vf)
>  }
>  
>  /**
> - * ice_vf_recreate_vsi - Release and re-create the VF's VSI
> - * @vf: VF to recreate the VSI for
> + * ice_vf_reconfig_vsi - Reconfigure a VF VSI with the device
> + * @vf: VF to reconfigure the VSI for
>   *
> - * This is only called when a single VF is being reset (i.e. VVF,
> VFLR, host
> - * VF configuration change, etc)
> + * This is called when a single VF is being reset (i.e. VVF, VFLR,
> host VF
> + * configuration change, etc).
>   *
> - * It releases and then re-creates a new VSI.
> + * It brings the VSI down and then reconfigures it with the
> hardware.
>   */
> -static int ice_vf_recreate_vsi(struct ice_vf *vf)
> +static int ice_vf_reconfig_vsi(struct ice_vf *vf)
>  {
> +       struct ice_vsi *vsi = ice_get_vf_vsi(vf);
> +       struct ice_vsi_cfg_params params = {};
>         struct ice_pf *pf = vf->pf;
>         int err;
>  
> -       ice_vf_vsi_release(vf);
> +       if (WARN_ON(!vsi))
> +               return -EINVAL;
>  
> -       err = vf->vf_ops->create_vsi(vf);
> +       params = ice_vsi_to_params(vsi);
> +       params.flags = ICE_VSI_FLAG_NO_INIT;
> +
> +       ice_vsi_decfg(vsi);
> +
> +       err = ice_vsi_cfg(vsi, &params);
>         if (err) {
>                 dev_err(ice_pf_to_dev(pf),
> -                       "Failed to recreate the VF%u's VSI, error
> %d\n",
> +                       "Failed to reconfigure the VF%u's VSI, error
> %d\n",
>                         vf->vf_id, err);
>                 return err;
>         }
> @@ -699,7 +707,7 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
>  
>         ice_vf_pre_vsi_rebuild(vf);
>  
> -       if (ice_vf_recreate_vsi(vf)) {
> +       if (ice_vf_reconfig_vsi(vf)) {
>                 dev_err(dev, "Failed to release and setup the VF%u's
> VSI\n",
>                         vf->vf_id);
>                 err = -EFAULT;
> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> index 67172fdd9bc2..ad648009a238 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> @@ -62,7 +62,6 @@ struct ice_vf_ops {
>         bool (*poll_reset_status)(struct ice_vf *vf);
>         void (*clear_reset_trigger)(struct ice_vf *vf);
>         void (*irq_close)(struct ice_vf *vf);
> -       int (*create_vsi)(struct ice_vf *vf);
>         void (*post_vsi_rebuild)(struct ice_vf *vf);
>  };
>  
> 
> base-commit: 8d5bc02673a4fa9f5db4e9e949f41cfc68ae1758

Hi Jacob,

I found a problem while testing this commit.
When I attach the VF to the VM I see this error in dmesg:
# virsh attach-interface v0 hostdev --managed 0000:41:01.0 --mac
52:54:00:b4:aa:bb ice 
0000:41:00.0: failed to add default unicast MAC filter
42:d5:f1:b5:17:33 for VF 0, error -17
ice 0000:41:00.0: failed to rebuild default MAC configuration for VF 0

This is probably another issue after commit 6624e780a577 ("ice:
split ice_vsi_setup into smaller functions") and when you try to do a
lightweight vsi recreate something is missing. I hate this commit, it
introduced inconsistencies in PF/VF init/deinit and caused a lot of
problems.

Regards,
Petr

 


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