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

[sashiko-bot@kernel.org]

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