[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

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

[Vidya Sagar]

On 18-08-2026 01:42, sashiko-bot@kernel.org wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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.
Thanks for looking at this. The three sites do exist as described, but
they are not equivalent, and only one of them needs a change. Taking
them in turn.
> --
> 
> --- 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.
Agreed, and this one is worth fixing. It is also worse than you state:
it is not purely pre-existing, because this patch is what makes the
write conditional while leaving its paired flush read unconditional. So
after this patch the function can issue a non-posted read to flush a
write that never happened.

pcie_tph_set_st_entry() already propagates an error by disabling TPH, so
the caller can cope. I will bail out early in v2:

	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);

> 
> 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);
The only caller of __pci_read_msi_msg() in the tree is
pseries_msi_compose_msg() on powerpc/pseries, which already selects
between reading the device and using the cached message based on device
state. pseries uses firmware-mediated EEH rather than native DPC, so the
escalation this patch is about does not arise there.

More importantly, this is a read whose value the caller consumes. The
write sites are safe to skip only because msi_desc::msix_ctrl caches the
intent and __pci_restore_msix_state() replays it once the Link is back
up. A read has no such fallback, so "guarding" it would mean returning
uninitialised or all-ones data, which is worse than the UR. Making it
safe would require an error return and propagation through
irq_compose_msi_msg(), which is a different and much larger change.

> 
> 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?
Guarding this one would introduce a bug. That readl() seeds the
msix_ctrl cache from hardware. If it were skipped the cache would read
back as zero, i.e. unmasked, while the hardware vector is masked, which
is the reset default. Every later mask/unmask decision would then work
off an inverted cache, and __pci_restore_msix_state() would replay the
wrong state.

The specific sequence you describe also has no deterministic path.
report_slot_reset() moves the device back to pci_channel_io_normal
before it invokes the driver callback:

	if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
	    !pdrv || !pdrv->err_handler || !pdrv->err_handler->slot_reset)
		goto out;

	vote = err_handler->slot_reset(dev);

So a driver re-enabling MSI-X from .slot_reset() runs with error_state
already normal and nothing is skipped. That same ordering is why this
patch does not break pci_restore_state() ->
__pci_restore_msix_state() during recovery.

In principle yes, and that is inherent rather than something this patch
regresses. error_state is a notification set by the DPC/AER handler
after containment has already happened; it is not a lock. Containment is
asynchronous, so any MMIO to any device can race with it, and no
placement of these checks changes that. Closing the race properly would
require serialising every MSI-X mask against containment, i.e. a lock in
the interrupt masking path.

What the patch does close is the deterministic window, which is the
interval between report_frozen_detected() and report_slot_reset(). In
that window the kernel already knows the Link is contained, and driver
.error_detected() and prepare-for-reset callbacks nonetheless call
pci_free_irq_vectors(), which masks every descriptor and flushes each
mask with a non-posted read. That is reproducible on every contained
device that tears down its interrupts before the reset, and it is what I
observed escalating a Downstream Port containment event into one at the
Root Port.

For v2 I will also rename the helper to pci_msi_dev_inaccessible(),
since in __pci_write_msi_msg() it gates the Configuration Space MSI path
as well, and the previous name implied MSI-X MMIO only.

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

[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

Re: [PATCH V2] 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 2 potential issue(s) to consider:
- [High] The early `pci_msi_dev_inaccessible()` check in `pci_msix_write_tph_tag()` leaves a large race window across lock acquisitions, leading to an unconditional flush read on an inaccessible device.
- [High] Silently skipping writes in `pci_msix_write_vector_ctrl()` exposes callers like `pci_write_msg_msix()` to partial MMIO updates and unconditional flush reads.
--

--- Patch [1]: [PATCH V2] PCI/MSI: Don't touch the MSI-X table while the Link is contained ---
commit 170aedad1f36f8c51175a3888ba22b67ee4a98ef
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. This patch skips hardware accesses to
the MSI-X table when the device is not in pci_channel_io_normal to prevent
DPC escalations caused by Unsupported Requests. It also modifies
pci_msix_write_tph_tag() to return -EIO to let the caller disable TPH.

> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 80a9db417dc87..0ebe153937ccc 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
[ ... ]
> @@ -954,6 +954,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;
> +

[Severity: High]
Since this check happens before acquiring msi_descs_lock and irq_desc->lock
in pci_msix_write_tph_tag(), does it leave a race window where the device
could become inaccessible after the check?

If a DPC event occurs while waiting for the locks, it looks like the
subsequent unconditional flush read:

drivers/pci/msi/msi.c:pci_msix_write_tph_tag() {
    ...
	/* Flush the write */
	readl(pci_msix_desc_addr(msi_desc));
    ...
}

would still execute on an inaccessible device, potentially triggering the
exact DPC escalation this patch aims to prevent.

>  	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 0b420b319f50f..c3194d8425c88 100644
> --- a/drivers/pci/msi/msi.h
> +++ b/drivers/pci/msi/msi.h
[ ... ]
> @@ -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;
> +

[Severity: High]
If this silently skips the vector control write, does it expose callers like
pci_write_msg_msix() to partial MMIO updates and unconditional flush reads?

Looking at pci_write_msg_msix(), if the mask write is silently skipped,
would the subsequent address and data writes, as well as the flush read,
still execute unconditionally:

drivers/pci/msi/msi.c:pci_write_msg_msix() {
    ...
	writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
	writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
	writel(msg->data, base + PCI_MSIX_ENTRY_DATA);

	if (unmasked)
		pci_msix_write_vector_ctrl(desc, ctrl);

	/* Ensure that the writes are visible in the device */
	readl(base + PCI_MSIX_ENTRY_DATA);
    ...
}

Could this trigger an Unsupported Request and the DPC escalation if the link
is contained?

>  	if (desc->pci.msi_attrib.can_mask)
>  		writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>  }

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

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

[Vidya Sagar]

On 25-08-2026 19:55, sashiko-bot@kernel.org wrote:
> External email: Use caution opening links or attachments
> 
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The early `pci_msi_dev_inaccessible()` check in `pci_msix_write_tph_tag()` leaves a large race window across lock acquisitions, leading to an unconditional flush read on an inaccessible device.
The window is real and the placement was poor, so I will move the check.
Thanks.

msi_descs_lock is a mutex, so the caller can sleep between the check and
the flush read, which makes the window far wider than it needs to be.
Moving the check under irq_desc::lock, immediately before the tag update,
reduces it to a few instructions with interrupts disabled:

	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);

To be clear about what this does and does not achieve: it narrows the
window, it does not close it. error_state is updated by
pci_dev_set_io_state() with a bare cmpxchg from the DPC handler, which
holds none of these locks, so containment can still begin between the
check and the access even when they are adjacent. This check is a filter
for the case where the kernel already knows the Link is down, not a
mutual exclusion primitive, and no placement can make it one. Closing the
race would require serialising every MSI-X access against containment.

> - [High] Silently skipping writes in `pci_msix_write_vector_ctrl()` exposes callers like `pci_write_msg_msix()` to partial MMIO updates and unconditional flush reads.
No. pci_write_msg_msix() has exactly one caller in the tree, and it is
already gated on the same predicate, so the function is not entered at
all when the device is inaccessible:

	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);

The inner check in pci_msix_write_vector_ctrl() is not redundant, because
that helper is also reached from pci_msix_unmask(),
__pci_restore_msix_state() and pci_msix_write_tph_tag(). But along the
pci_write_msg_msix() path it can only fire if error_state changes between
the outer and the inner check, which is the same unavoidable race as
above rather than anything this patch introduces.

Even then there is nothing to be partially updated. A partial update
would mean some writes reach the device and others do not, leaving the
Table inconsistent. Containment discards accesses uniformly: while the
Link is up every write lands, and once contained none of them do. The
transition only goes one way during an event, so there is no half-written
entry. On recovery __pci_restore_msix_state() rewrites address low,
address high, data and Vector Control from the cached descriptor, so the
entry is reconstructed in full:

	write_msg = arch_restore_msi_irqs(dev);

	scoped_guard (msi_descs_lock, &dev->dev) {
		msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
			if (write_msg)
				__pci_write_msi_msg(entry, &entry->msg);
			pci_msix_write_vector_ctrl(entry, entry->pci.msix_ctrl);
		}
	}

arch_restore_msi_irqs() returns true in the weak default and on s390, and
on x86 it is xen_initdom_restore_msi(), which only returns false under Xen
dom0 where Xen owns the restore. So on the platforms where native DPC
applies, write_msg is true and the message is rewritten along with the
control word.

I will send v3 with the TPH check moved.

Thanks,
Vidya Sagar
> --
> 
> --- Patch [1]: [PATCH V2] PCI/MSI: Don't touch the MSI-X table while the Link is contained ---
> commit 170aedad1f36f8c51175a3888ba22b67ee4a98ef
> 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. This patch skips hardware accesses to
> the MSI-X table when the device is not in pci_channel_io_normal to prevent
> DPC escalations caused by Unsupported Requests. It also modifies
> pci_msix_write_tph_tag() to return -EIO to let the caller disable TPH.
> 
>> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
>> index 80a9db417dc87..0ebe153937ccc 100644
>> --- a/drivers/pci/msi/msi.c
>> +++ b/drivers/pci/msi/msi.c
> [ ... ]
>> @@ -954,6 +954,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;
>> +
> 
> [Severity: High]
> Since this check happens before acquiring msi_descs_lock and irq_desc->lock
> in pci_msix_write_tph_tag(), does it leave a race window where the device
> could become inaccessible after the check?
> 
> If a DPC event occurs while waiting for the locks, it looks like the
> subsequent unconditional flush read:
> 
> drivers/pci/msi/msi.c:pci_msix_write_tph_tag() {
>     ...
>         /* Flush the write */
>         readl(pci_msix_desc_addr(msi_desc));
>     ...
> }
> 
> would still execute on an inaccessible device, potentially triggering the
> exact DPC escalation this patch aims to prevent.
> 
>>       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 0b420b319f50f..c3194d8425c88 100644
>> --- a/drivers/pci/msi/msi.h
>> +++ b/drivers/pci/msi/msi.h
> [ ... ]
>> @@ -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;
>> +
> 
> [Severity: High]
> If this silently skips the vector control write, does it expose callers like
> pci_write_msg_msix() to partial MMIO updates and unconditional flush reads?
> 
> Looking at pci_write_msg_msix(), if the mask write is silently skipped,
> would the subsequent address and data writes, as well as the flush read,
> still execute unconditionally:
> 
> drivers/pci/msi/msi.c:pci_write_msg_msix() {
>     ...
>         writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
>         writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
>         writel(msg->data, base + PCI_MSIX_ENTRY_DATA);
> 
>         if (unmasked)
>                 pci_msix_write_vector_ctrl(desc, ctrl);
> 
>         /* Ensure that the writes are visible in the device */
>         readl(base + PCI_MSIX_ENTRY_DATA);
>     ...
> }
> 
> Could this trigger an Unsupported Request and the DPC escalation if the link
> is contained?
> 
>>       if (desc->pci.msi_attrib.can_mask)
>>               writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>>  }
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260825140952.4066140-1-vidyas@nvidia.com?part=1

[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

Re: [PATCH V3] 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 3 potential issue(s) to consider:

New issues:
- [High] Unintended Side-effect / Triggered DPC Escalation in Error Path
- [Medium] Race condition and lockless access of pdev->error_state

Pre-existing issues:
- [High] Incomplete Fix / Unhandled Configuration Space Access during DPC Containment
--

commit 1da77665cdebed45957e3d9c2c58ae00d2dbc9b0
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 reads return all ones.

[ ... ]

> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 80a9db417dc87..975948502f8e8 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c

[ ... ]

> @@ -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;

[Severity: High]
Does returning -EIO here actively trigger a fatal Configuration Space write
that escalates DPC containment?

When the link is contained by DPC, returning -EIO forces the caller
(pcie_tph_set_st_entry()) into an error path that invokes pcie_disable_tph().
This function unconditionally writes to the device's Configuration Space:

drivers/pci/tph.c:pcie_disable_tph() {
    ...
    pci_write_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, 0);
}

Additionally, the caller performs an unguarded config write before the MMIO
access via set_ctrl_reg_req_en(..., PCI_TPH_REQ_DISABLE).

Does this newly introduced early error return actively force the caller to
perform actions that reach the hardware and escalate the DPC event?

> +
>  	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 0b420b319f50f..c3194d8425c88 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.
> + */

[Severity: High]
This isn't a bug introduced by this patch, but because the patch attempts to
fix DPC escalation by guarding MMIO, should it also guard Configuration Space
accesses during teardown?

For example, when pci_free_irq_vectors() initiates teardown during DPC
containment (error_state == pci_channel_io_frozen), it calls
pci_msix_shutdown(), pci_msi_shutdown(), or pci_msi_update_mask(). These
perform unguarded Configuration Space accesses. In pci_msi_update_mask():

drivers/pci/msi/msi.c:pci_msi_update_mask() {
    ...
    pci_write_config_dword(dev, desc->pci.mask_pos, desc->pci.msi_mask);
}

Crucially, Configuration Space accesses are not blocked when
error_state == pci_channel_io_frozen. As a result, the accesses reach the
hardware, trigger Unsupported Request (UR) completions, and escalate the DPC
event.

Is this fix fatally incomplete without similarly guarding Configuration Space
accesses?

> +static inline bool pci_msi_dev_inaccessible(struct pci_dev *pdev)
> +{
> +	return pdev->error_state != pci_channel_io_normal ||

[Severity: Medium]
Could this lockless read of pdev->error_state be subject to compiler
optimizations like load tearing or caching, leading to missed DPC
containment detection?

Concurrent atomic updates to pdev->error_state are performed in
pci_dev_set_io_state() via xchg()/cmpxchg(). However,
pci_msi_dev_inaccessible() reads it locklessly without using READ_ONCE().

According to the Linux kernel memory model, does this constitute a data race
where the compiler could cache the value, falsely returning that the device
is accessible and leading to fatal MMIO accesses during DPC containment?

> +	       pci_dev_is_disconnected(pdev);
> +}

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