netdev - Re: [PATCH iwl-net v5] i40e: Restore VF MSI-X state during PCI reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <958b65b3-202b-fa73-8a0e-1f886d55df2f@intel.com>
Date: Thu, 21 Dec 2023 14:49:04 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Andrii Staikov <andrii.staikov@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, Karen Ostrowska <karen.ostrowska@...el.com>,
	Mateusz Palczewski <mateusz.palczewski@...el.com>, Wojciech Drewek
	<wojciech.drewek@...el.com>
Subject: Re: [PATCH iwl-net v5] i40e: Restore VF MSI-X state during PCI reset

On 12/21/23 14:27, Andrii Staikov wrote:
> During a PCI FLR the MSI-X Enable flag in the VF PCI MSI-X capability
> register will be cleared. This can lead to issues when a VF is
> assigned to a VM because in these cases the VF driver receives no
> indication of the PF PCI error/reset and additionally it is incapable
> of restoring the cleared flag in the hypervisor configuration space
> without fully reinitializing the driver interrupt functionality.
> 
> Since the VF driver is unable to easily resolve this condition on its own,
> restore the VF MSI-X flag during the PF PCI reset handling.
> 
> Fixes: 19b7960b2da1 ("i40e: implement split PCI error reset handler")
> Co-developed-by: Karen Ostrowska <karen.ostrowska@...el.com>
> Signed-off-by: Karen Ostrowska <karen.ostrowska@...el.com>
> Co-developed-by: Mateusz Palczewski <mateusz.palczewski@...el.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@...el.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
> Reviewed-by: Przemyslaw Kitszel <przemyslaw.kitszel@...el.com>

should be "Przemek", but no need to send next version for that
(but pls promise it's last time ;))

> Signed-off-by: Andrii Staikov <andrii.staikov@...el.com>
> ---
> v1 -> v2: Fix signed-off tags
> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20231204131041.3369693-1-andrii.staikov@intel.com/
> 
> v2 -> v3: use @vf_dev in pci_get_device() instead of NULL and remove unnecessary call
> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20231206125127.218350-1-andrii.staikov@intel.com/
> 
> v3 -> v4: wrap the added functionality into the CONFIG_PCI_IOV define as
> this is VF-related functionality
> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20231212122452.3250691-1-andrii.staikov@intel.com/
> 
> v4 -> v5: fix RB tags

there was a question in v4 from Jake to "perhaps do it like in ice",
but I think that for a bug fix it's better to keep this patch as-is
(review always welcomed through).

> ---
>   drivers/net/ethernet/intel/i40e/i40e_main.c   |  3 +++
>   .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 26 +++++++++++++++++++
>   .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  3 +++
>   3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 7bb1f64833eb..bbe2d115fb15 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -16513,6 +16513,9 @@ static void i40e_pci_error_reset_done(struct pci_dev *pdev)
>   		return;
>   
>   	i40e_reset_and_rebuild(pf, false, false);
> +#ifdef CONFIG_PCI_IOV
> +	i40e_restore_all_vfs_msi_state(pdev);
> +#endif /* CONFIG_PCI_IOV */
>   }
>   
>   /**
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index 3f99eb198245..d60f5419d6bd 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -154,6 +154,32 @@ void i40e_vc_notify_reset(struct i40e_pf *pf)
>   			     (u8 *)&pfe, sizeof(struct virtchnl_pf_event));
>   }
>   
> +#ifdef CONFIG_PCI_IOV
> +void i40e_restore_all_vfs_msi_state(struct pci_dev *pdev)
> +{
> +	u16 vf_id;

nit: scope of @vf_id could be local to the last if
(but I guess it's too late for such)

> +	u16 pos;
> +
> +	/* Continue only if this is a PF */
> +	if (!pdev->is_physfn)
> +		return;
> +
> +	if (!pci_num_vf(pdev))
> +		return;
> +
> +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
> +	if (pos) {
> +		struct pci_dev *vf_dev = NULL;
> +
> +		pci_read_config_word(pdev, pos + PCI_SRIOV_VF_DID, &vf_id);
> +		while ((vf_dev = pci_get_device(pdev->vendor, vf_id, vf_dev))) {
> +			if (vf_dev->is_virtfn && vf_dev->physfn == pdev)
> +				pci_restore_msi_state(vf_dev);
> +		}
> +	}
> +}
> +#endif /* CONFIG_PCI_IOV */
> +
>   /**
>    * i40e_vc_notify_vf_reset
>    * @vf: pointer to the VF structure
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
> index 2ee0f8a23248..5fd607c0de0a 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
> @@ -137,6 +137,9 @@ int i40e_ndo_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool enable);
>   
>   void i40e_vc_notify_link_state(struct i40e_pf *pf);
>   void i40e_vc_notify_reset(struct i40e_pf *pf);
> +#ifdef CONFIG_PCI_IOV
> +void i40e_restore_all_vfs_msi_state(struct pci_dev *pdev);
> +#endif /* CONFIG_PCI_IOV */
>   int i40e_get_vf_stats(struct net_device *netdev, int vf_id,
>   		      struct ifla_vf_stats *vf_stats);
>