Re: [PATCH v2 5/7] ath11k: Use VFIO MSI cache when available

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ath11k: early return in ath11k_pci_alloc_msi() skips MSI disable
- [Medium] pci: unsafe linear scan of extended config space in qcom_vfio_find_msi_cache()
--

commit f45d065c25ccffe5ad5bba4c9805876b528061e6
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Subject: ath11k: Use VFIO MSI cache when available

This commit introduces a mechanism to detect and read physical host MSI
addresses from a cached location in the PCI extended config space. This
allows the device to function correctly when passed through to a VM via
VFIO, avoiding interrupt delivery failures caused by guest IOVA addresses.

> diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> --- a/drivers/net/wireless/ath/ath11k/pci.c
> +++ b/drivers/net/wireless/ath/ath11k/pci.c
[ ... ]
> @@ -487,6 +491,36 @@ static int ath11k_pci_alloc_msi(struct ath11k_pci *ab_pci)
>  	}
>  	ath11k_info(ab, "MSI vectors: %d\n", num_vectors);
>  
> +	/*
> +	 * Try to get host MSI address from VFIO cache.
> +	 * When running on a VM, the ath11k firmware requires physical host
> +	 * MSI addresses and cannot handle guest IOVA addresses.
> +	 * qcom-vfio-pci writes host MSI values to extended config space with
> +	 * a magic signature "QMSI" for discovery. Search for it.
> +	 */
> +	msi_offset = qcom_vfio_find_msi_cache(pci_dev);

[Severity: Medium]
Is it safe to unconditionally execute this config space scan during every
bare metal probe?

The qcom_vfio_find_msi_cache() function performs a blind sequential read of
the 4KB extended config space:

        for (offset = PCI_CFG_SPACE_SIZE;
             offset <= PCI_CFG_SPACE_EXP_SIZE - QCOM_VFIO_MSI_SPACE_SIZE;
             offset += 4) {
                if (pci_read_config_dword(pdev, offset, &magic))
                        continue;
                if (magic == QCOM_VFIO_MSI_MAGIC)
                        return offset;
        }

Reading arbitrary or reserved extended config registers without checking
capability headers violates PCIe specifications. Might this trigger
unintended side-effects on physical hardware, such as clearing
clear-on-read registers or triggering Unsupported Requests?

> +	if (msi_offset) {
> +		pci_read_config_dword(pci_dev,
> +				      msi_offset + QCOM_VFIO_MSI_ADDR_LO_OFFSET,
> +				      &msi_addr_lo);
> +		pci_read_config_dword(pci_dev,
> +				      msi_offset + QCOM_VFIO_MSI_ADDR_HI_OFFSET,
> +				      &msi_addr_hi);
> +		pci_read_config_word(pci_dev,
> +				     msi_offset + QCOM_VFIO_MSI_DATA_OFFSET,
> +				     &msi_data);
> +
> +		if (msi_addr_lo && msi_addr_lo != 0xFFFFFFFF) {
> +			ab->pci.msi.addr_lo = msi_addr_lo;
> +			ab->pci.msi.addr_hi = msi_addr_hi;
> +			ab->pci.msi.ep_base_data = msi_data;
> +			ab_pci->vfio_msi_cached = true;
> +			ath11k_info(ab, "VFIO MSI cache at offset 0x%x: addr_lo=0x%x addr_hi=0x%x data=%d\n",
> +				    msi_offset, msi_addr_lo, msi_addr_hi, msi_data);
> +			return 0;

[Severity: High]
Could this early return leave MSI enabled during the remainder of the probe
phase?

Normally, the function calls ath11k_pci_msi_disable(ab_pci) shortly after
this block. By returning early, we skip disabling MSI. As documented in
ath11k_pci_clear_all_intrs():

        /* This is a WAR for PCIE Hotreset.
         * When target receive Hotreset, but will set the interrupt.
         * So when download SBL again, SBL will open Interrupt and
         * receive it, and crash immediately.
         */

If MSI remains enabled here, could the device send a spurious MSI over the
PCIe bus before the IRQ handlers are properly installed, potentially causing
host interrupts or device crashes in a virtualized environment?

> +		}
> +	}
> +
>  	ath11k_pci_msi_disable(ab_pci);
>  
>  	msi_desc = irq_get_msi_desc(ab_pci->pdev->irq);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011074514.515894-1-jtornosm@redhat.com?part=5