[SRU][Bionic][PATCH 0/1] vfio/pci: Hide broken INTx support from user

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Bionic][PATCH 0/1] vfio/pci: Hide broken INTx support from user

Joseph Salisbury-3
BugLink: http://bugs.launchpad.net/bugs/1779830

== SRU Justification ==
6Wind is requesting this commit in Xenial.  There is a bug where they cannot
assign a i40e pf device to a vm using vfio-pci.  This bug was fixed in
v4.7 by commit 450744051d.

== Fix ==
450744051d ("vfio/pci: Hide broken INTx support from user")

== Regression Potential ==
Low. This commit has been in mainline as of v4.7 with no know
regressions.

== Test Case ==
A test kernel was built with this patch and tested by the original bug reporter.
The bug reporter states the test kernel resolved the bug.

Alex Williamson (1):
  vfio/pci: Hide broken INTx support from user

 drivers/vfio/pci/vfio_pci.c         | 55 ++++++++++++++++++++++++++++++-------
 drivers/vfio/pci/vfio_pci_config.c  |  9 +++++-
 drivers/vfio/pci/vfio_pci_private.h |  1 +
 3 files changed, 54 insertions(+), 11 deletions(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Bionic][PATCH 1/1] vfio/pci: Hide broken INTx support from user

Joseph Salisbury-3
From: Alex Williamson <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1779830

INTx masking has two components, the first is that we need the ability
to prevent the device from continuing to assert INTx.  This is
provided via the DisINTx bit in the command register and is the only
thing we can really probe for when testing if INTx masking is
supported.  The second component is that the device needs to indicate
if INTx is asserted via the interrupt status bit in the device status
register.  With these two features we can generically determine if one
of the devices we own is asserting INTx, signal the user, and mask the
interrupt while the user services the device.

Generally if one or both of these components is broken we resort to
APIC level interrupt masking, which requires an exclusive interrupt
since we have no way to determine the source of the interrupt in a
shared configuration.  This often makes it difficult or impossible to
configure the system for userspace use of the device, for an interrupt
mode that the user may not need.

One possible configuration of broken INTx masking is that the DisINTx
support is fully functional, but the interrupt status bit never
signals interrupt assertion.  In this case we do have the ability to
prevent the device from asserting INTx, but lack the ability to
identify the interrupt source.  For this case we can simply pretend
that the device lacks INTx support entirely, keeping DisINTx set on
the physical device, virtualizing this bit for the user, and
virtualizing the interrupt pin register to indicate no INTx support.
We already support virtualization of the DisINTx bit and already
virtualize the interrupt pin for platforms without INTx support.  By
tying these components together, setting DisINTx on open and reset,
and identifying devices broken in this particular way, we can provide
support for them w/o the handicap of APIC level INTx masking.

Intel i40e (XL710/X710) 10/20/40GbE NICs have been identified as being
broken in this specific way.  We leave the vfio-pci.nointxmask option
as a mechanism to bypass this support, enabling INTx on the device
with all the requirements of APIC level masking.

Signed-off-by: Alex Williamson <[hidden email]>
Cc: John Ronciak <[hidden email]>
Cc: Jesse Brandeburg <[hidden email]>
(cherry picked from commit 450744051d201c4d72436ebf5b04b9a06ba2cf30)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 drivers/vfio/pci/vfio_pci.c         | 55 ++++++++++++++++++++++++++++++-------
 drivers/vfio/pci/vfio_pci_config.c  |  9 +++++-
 drivers/vfio/pci/vfio_pci_private.h |  1 +
 3 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index b31b84f..da5108a 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -112,6 +112,35 @@ static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
 
 static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev);
 
+/*
+ * INTx masking requires the ability to disable INTx signaling via PCI_COMMAND
+ * _and_ the ability detect when the device is asserting INTx via PCI_STATUS.
+ * If a device implements the former but not the latter we would typically
+ * expect broken_intx_masking be set and require an exclusive interrupt.
+ * However since we do have control of the device's ability to assert INTx,
+ * we can instead pretend that the device does not implement INTx, virtualizing
+ * the pin register to report zero and maintaining DisINTx set on the host.
+ */
+static bool vfio_pci_nointx(struct pci_dev *pdev)
+{
+ switch (pdev->vendor) {
+ case PCI_VENDOR_ID_INTEL:
+ switch (pdev->device) {
+ /* All i40e (XL710/X710) 10/20/40GbE NICs */
+ case 0x1572:
+ case 0x1574:
+ case 0x1580 ... 0x1581:
+ case 0x1583 ... 0x1589:
+ case 0x37d0 ... 0x37d2:
+ return true;
+ default:
+ return false;
+ }
+ }
+
+ return false;
+}
+
 static int vfio_pci_enable(struct vfio_pci_device *vdev)
 {
  struct pci_dev *pdev = vdev->pdev;
@@ -135,23 +164,29 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
  pr_debug("%s: Couldn't store %s saved state\n",
  __func__, dev_name(&pdev->dev));
 
- ret = vfio_config_init(vdev);
- if (ret) {
- kfree(vdev->pci_saved_state);
- vdev->pci_saved_state = NULL;
- pci_disable_device(pdev);
- return ret;
+ if (likely(!nointxmask)) {
+ if (vfio_pci_nointx(pdev)) {
+ dev_info(&pdev->dev, "Masking broken INTx support\n");
+ vdev->nointx = true;
+ pci_intx(pdev, 0);
+ } else
+ vdev->pci_2_3 = pci_intx_mask_supported(pdev);
  }
 
- if (likely(!nointxmask))
- vdev->pci_2_3 = pci_intx_mask_supported(pdev);
-
  pci_read_config_word(pdev, PCI_COMMAND, &cmd);
  if (vdev->pci_2_3 && (cmd & PCI_COMMAND_INTX_DISABLE)) {
  cmd &= ~PCI_COMMAND_INTX_DISABLE;
  pci_write_config_word(pdev, PCI_COMMAND, cmd);
  }
 
+ ret = vfio_config_init(vdev);
+ if (ret) {
+ kfree(vdev->pci_saved_state);
+ vdev->pci_saved_state = NULL;
+ pci_disable_device(pdev);
+ return ret;
+ }
+
  msix_pos = pdev->msix_cap;
  if (msix_pos) {
  u16 flags;
@@ -283,7 +318,7 @@ static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
  if (irq_type == VFIO_PCI_INTX_IRQ_INDEX) {
  u8 pin;
  pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN, &pin);
- if (IS_ENABLED(CONFIG_VFIO_PCI_INTX) && pin)
+ if (IS_ENABLED(CONFIG_VFIO_PCI_INTX) && !vdev->nointx && pin)
  return 1;
 
  } else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {
diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index c55c632..7356440 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -387,6 +387,7 @@ static void vfio_bar_restore(struct vfio_pci_device *vdev)
 {
  struct pci_dev *pdev = vdev->pdev;
  u32 *rbar = vdev->rbar;
+ u16 cmd;
  int i;
 
  if (pdev->is_virtfn)
@@ -399,6 +400,12 @@ static void vfio_bar_restore(struct vfio_pci_device *vdev)
  pci_user_write_config_dword(pdev, i, *rbar);
 
  pci_user_write_config_dword(pdev, PCI_ROM_ADDRESS, *rbar);
+
+ if (vdev->nointx) {
+ pci_user_read_config_word(pdev, PCI_COMMAND, &cmd);
+ cmd |= PCI_COMMAND_INTX_DISABLE;
+ pci_user_write_config_word(pdev, PCI_COMMAND, cmd);
+ }
 }
 
 static __le32 vfio_generate_bar_flags(struct pci_dev *pdev, int bar)
@@ -1614,7 +1621,7 @@ int vfio_config_init(struct vfio_pci_device *vdev)
  *(__le16 *)&vconfig[PCI_DEVICE_ID] = cpu_to_le16(pdev->device);
  }
 
- if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX))
+ if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX) || vdev->nointx)
  vconfig[PCI_INTERRUPT_PIN] = 0;
 
  ret = vfio_cap_init(vdev);
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 0e7394f..ee6a3cf 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -57,6 +57,7 @@ struct vfio_pci_device {
  bool bardirty;
  bool has_vga;
  bool needs_reset;
+ bool nointx;
  struct pci_saved_state *pci_saved_state;
  int refcnt;
  struct eventfd_ctx *err_trigger;
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team