[PATCH V1] PCI/MSI: Don't touch the MSI-X table while the Link is contained

[PATCH V1] PCI/MSI: Don't touch the MSI-X table while the Link is contained

[Vidya Sagar]

The MSI-X table lives in device MMIO space behind a BAR, so it is only
reachable while the Link is up. While a Downstream Port has the Link
contained by DPC it completes accesses to the table with Unsupported
Request, and the read returns all ones.

If the upstream Root Port implements the RP Extensions for DPC, it
additionally reports that UR completion as an RP PIO error and answers
with a DPC of its own, which contains every other device below it. So a
containment event on a single Downstream Port can escalate into one at
the Root Port and take down unrelated devices.

pci_free_irq_vectors() is called from driver error_detected() and
prepare-for-reset callbacks, i.e. while the Link is contained, and it
masks every descriptor. Each mask is an MMIO write followed by a
non-posted flush read, so this is reached on every contained device whose
driver tears down its interrupts before the reset.

Skip the MMIO when the device is not in pci_channel_io_normal, in
addition to the existing surprise removal check. The msix_ctrl cache is
still updated, so __pci_restore_msix_state() replays the intended mask
state once the Link is back up.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/msi/msi.c |  2 +-
 drivers/pci/msi/msi.h | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index 209373c92e9e..5612cf2ff280 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -249,7 +249,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	struct pci_dev *dev = msi_desc_to_pci_dev(entry);
 
-	if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) {
+	if (dev->current_state != PCI_D0 || pci_msix_mmio_unsafe(dev)) {
 		/* Don't touch the hardware now */
 	} else if (entry->pci.msi_attrib.is_msix) {
 		pci_write_msg_msix(entry, msg);
diff --git a/drivers/pci/msi/msi.h b/drivers/pci/msi/msi.h
index 0b420b319f50..e372e9c87508 100644
--- a/drivers/pci/msi/msi.h
+++ b/drivers/pci/msi/msi.h
@@ -26,6 +26,19 @@ static inline void __iomem *pci_msix_desc_addr(struct msi_desc *desc)
 	return desc->pci.mask_base + desc->msi_index * PCI_MSIX_ENTRY_SIZE;
 }
 
+/*
+ * The MSI-X Table lives in device MMIO space, so it is only reachable while
+ * the Link is usable. While a Downstream Port has the Link contained by DPC
+ * it completes these accesses with Unsupported Request, which the Root Port
+ * in turn reports as an RP PIO error and answers with a DPC of its own,
+ * taking down every other device below it.
+ */
+static inline bool pci_msix_mmio_unsafe(struct pci_dev *pdev)
+{
+	return pdev->error_state != pci_channel_io_normal ||
+	       pci_dev_is_disconnected(pdev);
+}
+
 /*
  * This internal function does not flush PCI writes to the device.  All
  * users must ensure that they read from the device before either assuming
@@ -36,6 +49,9 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
 {
 	void __iomem *desc_addr = pci_msix_desc_addr(desc);
 
+	if (pci_msix_mmio_unsafe(msi_desc_to_pci_dev(desc)))
+		return;
+
 	if (desc->pci.msi_attrib.can_mask)
 		writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
 }
@@ -43,6 +59,10 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
 static inline void pci_msix_mask(struct msi_desc *desc)
 {
 	desc->pci.msix_ctrl |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
+
+	if (pci_msix_mmio_unsafe(msi_desc_to_pci_dev(desc)))
+		return;
+
 	pci_msix_write_vector_ctrl(desc, desc->pci.msix_ctrl);
 	/* Flush write to device */
 	readl(desc->pci.mask_base);
-- 
2.43.0

[PATCH V2] PCI/MSI: Don't touch the MSI-X table while the Link is contained

[Vidya Sagar]

The MSI-X Table lives in device MMIO space behind a BAR, so it is only
reachable while the Link is up. While a Downstream Port has the Link
contained by DPC it completes accesses to the Table with Unsupported
Request, and reads return all ones.

If the upstream Root Port implements the RP Extensions for DPC, it
additionally reports that UR completion as an RP PIO error and answers
with a DPC of its own, which contains every other device below it. So a
containment event on a single Downstream Port can escalate into one at
the Root Port and take down unrelated devices.

pci_free_irq_vectors() is called from driver error_detected() and
prepare-for-reset callbacks, i.e. while the Link is contained, and it
masks every descriptor. Each mask is an MMIO write followed by a
non-posted flush read, so this is reached on every contained device whose
driver tears down its interrupts before the reset.

Skip the hardware access when the device is not in pci_channel_io_normal,
in addition to the existing surprise removal check. The msix_ctrl cache
is still updated, so __pci_restore_msix_state() replays the intended mask
state once the Link is back up. report_slot_reset() returns the device to
pci_channel_io_normal before invoking the driver callback, so re-enabling
and restoring MSI-X during recovery is unaffected.

pci_msix_write_tph_tag() flushes its Vector Control update with an
unconditional read, which would otherwise be issued for a write that was
skipped, so return -EIO there instead. pcie_tph_set_st_entry() responds
by disabling TPH, which is preferable to reporting a Steering Tag update
that never reached the device.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
Changes in v2:
- Return -EIO from pci_msix_write_tph_tag() so its unconditional flush
  read is not issued for a skipped write (reported by Sashiko AI review).
- Rename pci_msix_mmio_unsafe() to pci_msi_dev_inaccessible(), since in
  __pci_write_msi_msg() it also gates the Configuration Space MSI path.
- Note in the log why MSI-X restore during recovery is unaffected.

 drivers/pci/msi/msi.c | 10 +++++++++-
 drivers/pci/msi/msi.h | 21 +++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index 209373c92e9e..579f471ab86a 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -249,7 +249,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	struct pci_dev *dev = msi_desc_to_pci_dev(entry);
 
-	if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) {
+	if (dev->current_state != PCI_D0 || pci_msi_dev_inaccessible(dev)) {
 		/* Don't touch the hardware now */
 	} else if (entry->pci.msi_attrib.is_msix) {
 		pci_write_msg_msix(entry, msg);
@@ -944,6 +944,14 @@ int pci_msix_write_tph_tag(struct pci_dev *pdev, unsigned int index, u16 tag)
 	if (!pdev->msix_enabled)
 		return -ENXIO;
 
+	/*
+	 * The tag update below is a write to the MSI-X Table followed by a
+	 * flush read, neither of which can be completed while the Link is
+	 * contained. Let the caller disable TPH instead.
+	 */
+	if (pci_msi_dev_inaccessible(pdev))
+		return -EIO;
+
 	virq = msi_get_virq(&pdev->dev, index);
 	if (!virq)
 		return -ENXIO;
diff --git a/drivers/pci/msi/msi.h b/drivers/pci/msi/msi.h
index 0b420b319f50..c3194d8425c8 100644
--- a/drivers/pci/msi/msi.h
+++ b/drivers/pci/msi/msi.h
@@ -26,6 +26,20 @@ static inline void __iomem *pci_msix_desc_addr(struct msi_desc *desc)
 	return desc->pci.mask_base + desc->msi_index * PCI_MSIX_ENTRY_SIZE;
 }
 
+/*
+ * The MSI-X Table lives in device MMIO space and the MSI Capability in
+ * Configuration Space, so both are only reachable while the Link is usable.
+ * While a Downstream Port has the Link contained by DPC it completes these
+ * accesses with Unsupported Request. If the upstream Root Port implements the
+ * RP Extensions for DPC, it reports that completion as an RP PIO error and
+ * answers with a DPC of its own, taking down every other device below it.
+ */
+static inline bool pci_msi_dev_inaccessible(struct pci_dev *pdev)
+{
+	return pdev->error_state != pci_channel_io_normal ||
+	       pci_dev_is_disconnected(pdev);
+}
+
 /*
  * This internal function does not flush PCI writes to the device.  All
  * users must ensure that they read from the device before either assuming
@@ -36,6 +50,9 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
 {
 	void __iomem *desc_addr = pci_msix_desc_addr(desc);
 
+	if (pci_msi_dev_inaccessible(msi_desc_to_pci_dev(desc)))
+		return;
+
 	if (desc->pci.msi_attrib.can_mask)
 		writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
 }
@@ -43,6 +60,10 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
 static inline void pci_msix_mask(struct msi_desc *desc)
 {
 	desc->pci.msix_ctrl |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
+
+	if (pci_msi_dev_inaccessible(msi_desc_to_pci_dev(desc)))
+		return;
+
 	pci_msix_write_vector_ctrl(desc, desc->pci.msix_ctrl);
 	/* Flush write to device */
 	readl(desc->pci.mask_base);
-- 
2.43.0

[PATCH V3] PCI/MSI: Don't touch the MSI-X table while the Link is contained

[Vidya Sagar]

The MSI-X Table lives in device MMIO space behind a BAR, so it is only
reachable while the Link is up. While a Downstream Port has the Link
contained by DPC it completes accesses to the Table with Unsupported
Request, and reads return all ones.

If the upstream Root Port implements the RP Extensions for DPC, it
additionally reports that UR completion as an RP PIO error and answers
with a DPC of its own, which contains every other device below it. So a
containment event on a single Downstream Port can escalate into one at
the Root Port and take down unrelated devices.

pci_free_irq_vectors() is called from driver error_detected() and
prepare-for-reset callbacks, i.e. while the Link is contained, and it
masks every descriptor. Each mask is an MMIO write followed by a
non-posted flush read, so this is reached on every contained device whose
driver tears down its interrupts before the reset.

Skip the hardware access when the device is not in pci_channel_io_normal,
in addition to the existing surprise removal check. The msix_ctrl cache
is still updated, so __pci_restore_msix_state() replays the intended mask
state once the Link is back up. report_slot_reset() returns the device to
pci_channel_io_normal before invoking the driver callback, so re-enabling
and restoring MSI-X during recovery is unaffected.

error_state is set once containment has already occurred, so these checks
cover the case where the kernel knows the Link is down. They are not
mutual exclusion against a containment event that begins concurrently.

pci_msix_write_tph_tag() flushes its Vector Control update with an
unconditional read, which would otherwise be issued for a write that was
skipped, so return -EIO there instead. pcie_tph_set_st_entry() responds
by disabling TPH, which is preferable to reporting a Steering Tag update
that never reached the device.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
Changes in v3:
- Move the pci_msi_dev_inaccessible() check in pci_msix_write_tph_tag()
  under irq_desc::lock, next to the accesses it guards, instead of before
  msi_descs_lock which can sleep (reported by Sashiko AI review).

Changes in v2:
- Return -EIO from pci_msix_write_tph_tag() so its unconditional flush
  read is not issued for a skipped write (reported by Sashiko AI review).
- Rename pci_msix_mmio_unsafe() to pci_msi_dev_inaccessible(), since in
  __pci_write_msi_msg() it also gates the Configuration Space MSI path.
- Note in the log why MSI-X restore during recovery is unaffected.

 drivers/pci/msi/msi.c | 11 ++++++++++-
 drivers/pci/msi/msi.h | 21 +++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index 80a9db417dc8..975948502f8e 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -249,7 +249,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	struct pci_dev *dev = msi_desc_to_pci_dev(entry);
 
-	if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) {
+	if (dev->current_state != PCI_D0 || pci_msi_dev_inaccessible(dev)) {
 		/* Don't touch the hardware now */
 	} else if (entry->pci.msi_attrib.is_msix) {
 		pci_write_msg_msix(entry, msg);
@@ -976,6 +976,15 @@ int pci_msix_write_tph_tag(struct pci_dev *pdev, unsigned int index, u16 tag)
 	if (!msi_desc || msi_desc->pci.msi_attrib.is_virtual)
 		return -ENXIO;
 
+	/*
+	 * The tag update below is a write to the MSI-X Table followed by a
+	 * flush read, neither of which can be completed while the Link is
+	 * contained. Check as late as possible, i.e. under irq_desc::lock, as
+	 * containment can begin at any point. Let the caller disable TPH.
+	 */
+	if (pci_msi_dev_inaccessible(pdev))
+		return -EIO;
+
 	FIELD_MODIFY(PCI_MSIX_ENTRY_CTRL_ST, &msi_desc->pci.msix_ctrl, tag);
 	pci_msix_write_vector_ctrl(msi_desc, msi_desc->pci.msix_ctrl);
 	/* Flush the write */
diff --git a/drivers/pci/msi/msi.h b/drivers/pci/msi/msi.h
index 0b420b319f50..c3194d8425c8 100644
--- a/drivers/pci/msi/msi.h
+++ b/drivers/pci/msi/msi.h
@@ -26,6 +26,20 @@ static inline void __iomem *pci_msix_desc_addr(struct msi_desc *desc)
 	return desc->pci.mask_base + desc->msi_index * PCI_MSIX_ENTRY_SIZE;
 }
 
+/*
+ * The MSI-X Table lives in device MMIO space and the MSI Capability in
+ * Configuration Space, so both are only reachable while the Link is usable.
+ * While a Downstream Port has the Link contained by DPC it completes these
+ * accesses with Unsupported Request. If the upstream Root Port implements the
+ * RP Extensions for DPC, it reports that completion as an RP PIO error and
+ * answers with a DPC of its own, taking down every other device below it.
+ */
+static inline bool pci_msi_dev_inaccessible(struct pci_dev *pdev)
+{
+	return pdev->error_state != pci_channel_io_normal ||
+	       pci_dev_is_disconnected(pdev);
+}
+
 /*
  * This internal function does not flush PCI writes to the device.  All
  * users must ensure that they read from the device before either assuming
@@ -36,6 +50,9 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
 {
 	void __iomem *desc_addr = pci_msix_desc_addr(desc);
 
+	if (pci_msi_dev_inaccessible(msi_desc_to_pci_dev(desc)))
+		return;
+
 	if (desc->pci.msi_attrib.can_mask)
 		writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
 }
@@ -43,6 +60,10 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
 static inline void pci_msix_mask(struct msi_desc *desc)
 {
 	desc->pci.msix_ctrl |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
+
+	if (pci_msi_dev_inaccessible(msi_desc_to_pci_dev(desc)))
+		return;
+
 	pci_msix_write_vector_ctrl(desc, desc->pci.msix_ctrl);
 	/* Flush write to device */
 	readl(desc->pci.mask_base);
-- 
2.43.0

[PATCH V4] PCI/MSI: Skip MSI/MSI-X programming while the channel is offline

[Vidya Sagar]

The MSI-X Table lives in device MMIO space behind a BAR and the MSI Mask
register in Configuration Space, so neither is reachable while the Link
is down. While a Downstream Port has the Link contained by DPC it
completes these accesses with Unsupported Request, and reads return all
ones.

If the upstream Root Port implements the RP Extensions for DPC, it
additionally reports that UR completion as an RP PIO error and triggers a
second containment event, this time at the Root Port, which contains
every device below it. So a contained Link on one Downstream Port turns
into a far wider outage that takes down unrelated devices.

pci_free_irq_vectors() is called from driver error_detected() and
prepare-for-reset callbacks, i.e. while the Link is contained and before
the reset and the pci_restore_state() that follows it, and it masks every
descriptor. Skip the programming when pci_channel_offline(), which also
covers surprise removal. The msix_ctrl and msi_mask caches are still
updated, so the restore paths replay the intended state once the Link is
back up, and report_slot_reset() clears the offline state before the
driver callback runs, so recovery is unaffected.
pci_msix_write_tph_tag() flushes its Vector Control update with an
unconditional read, so return -EIO there rather than issue it for a write
that was skipped; the caller disables TPH in response. error_state is
only set once containment has occurred, so this covers the case where the
kernel knows the Link is down; it is not mutual exclusion against a
containment event that begins concurrently.

This does not attempt to make every Configuration Space access safe
while the Link is contained.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
Changes in v4:
- Drop the pci_msi_dev_inaccessible() helper and use the existing
  pci_channel_offline() instead. It is the same predicate, and the
  pci_dev_is_disconnected() half was redundant because error_state !=
  pci_channel_io_normal already covers pci_channel_io_perm_failure.
- Also skip the Mask register write in pci_msi_update_mask(), so legacy
  MSI below a contained Downstream Port is covered and not just MSI-X.
  Subject and log updated accordingly.

Changes in v3:
- Move the pci_msi_dev_inaccessible() check in pci_msix_write_tph_tag()
  under irq_desc::lock, next to the accesses it guards, instead of before
  msi_descs_lock which can sleep (reported by Sashiko AI review).

Changes in v2:
- Return -EIO from pci_msix_write_tph_tag() so its unconditional flush
  read is not issued for a skipped write (reported by Sashiko AI review).
- Rename pci_msix_mmio_unsafe() to pci_msi_dev_inaccessible(), since in
  __pci_write_msi_msg() it also gates the Configuration Space MSI path.
- Note in the log why MSI-X restore during recovery is unaffected.

 drivers/pci/msi/msi.c | 15 +++++++++++++--
 drivers/pci/msi/msi.h |  8 ++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index 80a9db417dc8..8a0133a6a286 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -133,7 +133,9 @@ void pci_msi_update_mask(struct msi_desc *desc, u32 clear, u32 set)
 	raw_spin_lock_irqsave(lock, flags);
 	desc->pci.msi_mask &= ~clear;
 	desc->pci.msi_mask |= set;
-	pci_write_config_dword(dev, desc->pci.mask_pos, desc->pci.msi_mask);
+	/* Cached either way, for __pci_restore_msi_state() to replay */
+	if (!pci_channel_offline(dev))
+		pci_write_config_dword(dev, desc->pci.mask_pos, desc->pci.msi_mask);
 	raw_spin_unlock_irqrestore(lock, flags);
 }
 
@@ -249,7 +251,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	struct pci_dev *dev = msi_desc_to_pci_dev(entry);
 
-	if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) {
+	if (dev->current_state != PCI_D0 || pci_channel_offline(dev)) {
 		/* Don't touch the hardware now */
 	} else if (entry->pci.msi_attrib.is_msix) {
 		pci_write_msg_msix(entry, msg);
@@ -976,6 +978,15 @@ int pci_msix_write_tph_tag(struct pci_dev *pdev, unsigned int index, u16 tag)
 	if (!msi_desc || msi_desc->pci.msi_attrib.is_virtual)
 		return -ENXIO;
 
+	/*
+	 * The tag update below is a write to the MSI-X Table followed by a
+	 * flush read, neither of which can be completed while the Link is
+	 * down. Check as late as possible, as the Link can go down at any
+	 * point. Let the caller disable TPH.
+	 */
+	if (pci_channel_offline(pdev))
+		return -EIO;
+
 	FIELD_MODIFY(PCI_MSIX_ENTRY_CTRL_ST, &msi_desc->pci.msix_ctrl, tag);
 	pci_msix_write_vector_ctrl(msi_desc, msi_desc->pci.msix_ctrl);
 	/* Flush the write */
diff --git a/drivers/pci/msi/msi.h b/drivers/pci/msi/msi.h
index 0b420b319f50..f987cf897264 100644
--- a/drivers/pci/msi/msi.h
+++ b/drivers/pci/msi/msi.h
@@ -36,6 +36,10 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
 {
 	void __iomem *desc_addr = pci_msix_desc_addr(desc);
 
+	/* The Table is unreachable while the Link is down */
+	if (pci_channel_offline(msi_desc_to_pci_dev(desc)))
+		return;
+
 	if (desc->pci.msi_attrib.can_mask)
 		writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
 }
@@ -43,6 +47,10 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
 static inline void pci_msix_mask(struct msi_desc *desc)
 {
 	desc->pci.msix_ctrl |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
+
+	if (pci_channel_offline(msi_desc_to_pci_dev(desc)))
+		return;
+
 	pci_msix_write_vector_ctrl(desc, desc->pci.msix_ctrl);
 	/* Flush write to device */
 	readl(desc->pci.mask_base);
-- 
2.43.0