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

[Petr Oros <poros@redhat.com>]

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