[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unprotected `readl()` operations in `pci_msix_write_tph_tag()`, `__pci_read_msi_msg()`, and `msix_prepare_msi_desc()` circumvent the MSI-X MMIO containment fix.
--

--- Patch [1]: [PATCH V1] PCI/MSI: Don't touch the MSI-X table while the Link is contained ---
commit aba17efd26855fb96d48aa82a9f367120d469a3f
Author: Vidya Sagar <vidyas@nvidia.com>

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

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.

> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 209373c92e9e1..5612cf2ff2800 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 0b420b319f50f..e372e9c875087 100644
> --- a/drivers/pci/msi/msi.h
> +++ b/drivers/pci/msi/msi.h
> [ ... ]
> +/*
> + * 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);
> +}

[Severity: High]
This is a pre-existing issue, but could this regression still occur through
other unprotected MSI-X MMIO accesses?

While this patch successfully shields the vector control write inside
pci_msix_write_vector_ctrl() using this new check, there appear to be
unprotected readl() operations that still bypass it.

For example, in drivers/pci/msi/msi.c:pci_msix_write_tph_tag():

    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 */
    readl(pci_msix_desc_addr(msi_desc));

The flush readl() is unconditional. It bypasses the safety check and
would complete with an Unsupported Request if the link is contained.

Similarly, in drivers/pci/msi/msi.c:__pci_read_msi_msg():

    msg->address_lo = readl(base + PCI_MSIX_ENTRY_LOWER_ADDR);
    msg->address_hi = readl(base + PCI_MSIX_ENTRY_UPPER_ADDR);
    msg->data = readl(base + PCI_MSIX_ENTRY_DATA);

And in drivers/pci/msi/msi.c:msix_prepare_msi_desc():

    if (dev->dev_flags & PCI_DEV_FLAGS_MSIX_TOUCH_ENTRY_DATA_FIRST)
        writel(0, addr + PCI_MSIX_ENTRY_DATA);
    desc->pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);

If an IRQ affinity rebalancing event triggers a TPH tag update while
the link is contained, or if the link becomes contained concurrently
during MSI-X initialization, won't these unprotected accesses still
escalate to a Root Port PIO error and trigger a DPC?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817195626.3091331-1-vidyas@nvidia.com?part=1