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 <[email protected]>
Signed-off-by: Karen Ostrowska <[email protected]>
Co-developed-by: Mateusz Palczewski <[email protected]>
Signed-off-by: Mateusz Palczewski <[email protected]>
Reviewed-by: Drewek Wojciech <[email protected]>
Reviewed-by: Kitszel Przemyslaw <[email protected]>
Signed-off-by: Andrii Staikov <[email protected]>
---
v1 -> v2: Fix signed-off tags
https://patchwork.ozlabs.org/project/intel-wired-lan/patch/[email protected]/
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/[email protected]/
v3 -> v4: wrap the added functionality into the CONFIG_PCI_IOV define as
this is VF-related functionality
---
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;
+ 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);
--
2.25.1
On 12/12/2023 4:24 AM, 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 <[email protected]>
> Signed-off-by: Karen Ostrowska <[email protected]>
> Co-developed-by: Mateusz Palczewski <[email protected]>
> Signed-off-by: Mateusz Palczewski <[email protected]>
> Reviewed-by: Drewek Wojciech <[email protected]>
> Reviewed-by: Kitszel Przemyslaw <[email protected]>
> Signed-off-by: Andrii Staikov <[email protected]>
The ice driver recently started caching the PCI device structure
pointers in their VF structure instead of having to do this sort of
lookup on the fly.
See 31642d2854e2 ("ice: store VF's pci_dev ptr in ice_vf") [1][2]
[1]:
https://lore.kernel.org/intel-wired-lan/[email protected]/
[2]:
https://lore.kernel.org/netdev/[email protected]/
Can we do something similar for i40e?
> ---
> v1 -> v2: Fix signed-off tags
> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/[email protected]/
>
> 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/[email protected]/
>
> v3 -> v4: wrap the added functionality into the CONFIG_PCI_IOV define as
> this is VF-related functionality
> ---
> 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
Also noticed that i40e_virtchnl_pf.c is compiled always instead of only
conditionally when CONFIG_PCI_IOV is enabled. That's not really the
fault of this change, but I think it would be good cleanup to avoid
needing to compile any of this code if CONFIG_PCI_IOV is disabled.
> +void i40e_restore_all_vfs_msi_state(struct pci_dev *pdev)
> +{
> + u16 vf_id;
> + u16 pos;
> +
> + /* Continue only if this is a PF */
> + if (!pdev->is_physfn)
> + return;
> +
The function could also just pass the pf instead of pdev, and we'd know
its the physical function.
> + if (!pci_num_vf(pdev))
> + return;
If we switch to saving pdevs in our VF structure, this could be a simple
iteration loop that does nothing if the number of VFs is 0.
> +
> + 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 */
> +
Thanks,
Jake
> The ice driver recently started caching the PCI device structure
> pointers in their VF structure instead of having to do this sort of
> lookup on the fly.
>
> See 31642d2854e2 ("ice: store VF's pci_dev ptr in ice_vf") [1][2]
>
> [1]:
> https://lore.kernel.org/intel-wired-lan/[email protected]/
> [2]:
> https://lore.kernel.org/netdev/[email protected]/
>
> Can we do something similar for i40e?
For now we don't anticipate much benefit of this approach, and we want relatively smaller change for a bugfix.
Regards,
Staikov Andrii
> -----Original Message-----
> From: Staikov, Andrii <[email protected]>
> Sent: Thursday, December 21, 2023 6:00 AM
> To: Keller, Jacob E <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; Ostrowska, Karen
> <[email protected]>; Mateusz Palczewski
> <[email protected]>; Drewek, Wojciech
> <[email protected]>; Kitszel, Przemyslaw
> <[email protected]>
> Subject: RE: [PATCH iwl-net v4] i40e: Restore VF MSI-X state during PCI reset
>
>
> > The ice driver recently started caching the PCI device structure
> > pointers in their VF structure instead of having to do this sort of
> > lookup on the fly.
> >
> > See 31642d2854e2 ("ice: store VF's pci_dev ptr in ice_vf") [1][2]
> >
> > [1]:
> > https://lore.kernel.org/intel-wired-lan/20230912115626.105828-1-
> [email protected]/
> > [2]:
> > https://lore.kernel.org/netdev/20231019173227.3175575-4-
> [email protected]/
> >
> > Can we do something similar for i40e?
>
> For now we don't anticipate much benefit of this approach, and we want relatively
> smaller change for a bugfix.
>
> Regards,
> Staikov Andrii
Sure. If we ever need the VF PCI dev pointer in the future we can look into this. Not a huge deal for this since it’s the only place we use it currently anyways. Thanks for the response!
Thanks,
Jake