Re: [PATCH v3 7/8] ath12k: use KVM MSI domain for VM passthrough

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] wifi: ath12k: stale msi_ep_base_data not updated in ath12k_pci_power_up()
- [High] wifi: ath12k: module load failure on baremetal when PCI_KVM_MSI_DOMAIN=m
- [Medium] wifi: ath12k: vmlinux link failure when ATH12K=y and PCI_KVM_MSI_DOMAIN=m
--

commit ea9e51c37aa4f1f3d5125a423b52732232e12c47
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

ath12k: use KVM MSI domain for VM passthrough

This commit adds support for synchronizing and retrieving the host physical
message signaled interrupt address via the kvm pci msi domain in
ath12k_pci_power_up() and ath12k_pci_get_msi_address() when running in a
virtual machine.

> diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
> index f7879fb48d32e..dbd960f88d386 100644
> --- a/drivers/net/wireless/ath/ath12k/pci.c
> +++ b/drivers/net/wireless/ath/ath12k/pci.c
> @@ -8,6 +8,9 @@
>  #include <linux/interrupt.h>
>  #include <linux/msi.h>
>  #include <linux/pci.h>
> +#if IS_ENABLED(CONFIG_PCI_KVM_MSI_DOMAIN)
> +#include <linux/pci-kvm-msi.h>
> +#endif
>  #include <linux/time.h>
>  #include <linux/vmalloc.h>
>  
> @@ -1082,6 +1085,11 @@ void ath12k_pci_get_msi_address(struct ath12k_base *ab, u32 *msi_addr_lo,
>  	struct ath12k_pci *ab_pci = ath12k_pci_priv(ab);
>  	struct pci_dev *pci_dev = to_pci_dev(ab->dev);
>  
> +#if IS_ENABLED(CONFIG_PCI_KVM_MSI_DOMAIN)
> +	if (kvm_pci_msi_get_address(pci_dev, msi_addr_lo, msi_addr_hi, NULL))
> +		return;
> +#endif

[Severity: Medium]
Will this cause a vmlinux link failure when ath12k is built-in and the kvm
msi domain driver is built as a loadable module?

Both Kconfig options are defined as tristate without a dependency between
them:

drivers/net/wireless/ath/ath12k/Kconfig:
    config ATH12K
        tristate "Qualcomm Wi-Fi 7 support (ath12k)"
        depends on MAC80211 && HAS_DMA && PCI

drivers/pci/controller/Kconfig:
    config PCI_KVM_MSI_DOMAIN
        tristate "KVM PCI passthrough MSI domain"
        depends on X86_64 && KVM_GUEST && PCI_MSI

Because the calls to kvm_pci_msi_get_address() in
ath12k_pci_get_msi_address() and kvm_pci_msi_wait_ready() in
ath12k_pci_power_up() are guarded with:

    #if IS_ENABLED(CONFIG_PCI_KVM_MSI_DOMAIN)

building with ath12k=y and pci_kvm_msi_domain=m causes built-in
drivers/net/wireless/ath/ath12k/pci.o to emit direct calls to symbols in
pci-kvm-msi.ko, failing at the vmlinux link stage. Should this use
IS_REACHABLE() or enforce a Kconfig dependency?

[Severity: High]
What happens on baremetal when both ath12k and pci-kvm-msi are built as
modules?

The direct calls to kvm_pci_msi_get_address() and kvm_pci_msi_wait_ready()
create a hard symbol dependency from ath12k.ko onto pci-kvm-msi.ko. When
modprobe loads pci-kvm-msi.ko first, kvm_pci_msi_init() aborts module
loading on baremetal or when no device in kvm_msi_devids is present on the
bus:

drivers/pci/controller/pci-kvm-msi.c:kvm_pci_msi_init() {
    if (!kvm_para_available())
        return -ENODEV;

    for_each_pci_dev(pdev) {
        if (pci_match_id(kvm_msi_devids, pdev)) {
            first = pdev;
            break;
        }
    }

    if (!first) {
        pr_info("kvm-pci-msi: no matching devices found\n");
        return -ENODEV;
    }

Note that kvm_msi_devids only matches 0x1103 and 0x1107 here, and even
with later commit 1d17e12c16ad ("PCI: add QCN9274 to reset quirks and KVM
MSI domain") adding 0x1109, baremetal systems, guests with only other
ath12k devices such as 0x1112, or guests prior to hotplug still return
-ENODEV from kvm_pci_msi_init().

When pci-kvm-msi.ko fails to load with -ENODEV, its exported symbols are
never registered, preventing ath12k.ko from loading on baremetal. Could
that break ath12k module loading on distribution kernels where both options
are enabled as modules?

>  
>  	pci_read_config_dword(pci_dev, pci_dev->msi_cap + PCI_MSI_ADDRESS_LO,
>  			      msi_addr_lo);
>  
> @@ -1485,6 +1493,10 @@ int ath12k_pci_power_up(struct ath12k_base *ab)
>  
>  	ath12k_pci_msi_enable(ab_pci);
>  
> +#if IS_ENABLED(CONFIG_PCI_KVM_MSI_DOMAIN)
> +	kvm_pci_msi_wait_ready(ab_pci->pdev, 500);
> +#endif

[Severity: High]
Does ab_pci->msi_ep_base_data remain stale with the guest virtual message
data after kvm_pci_msi_wait_ready() completes in ath12k_pci_power_up()?

During ath12k_pci_probe(), before ath12k_pci_power_up() runs,
ath12k_pci_config_msi_data() latches msi_desc->msg.data into
ab_pci->msi_ep_base_data:

drivers/net/wireless/ath/ath12k/pci.c:ath12k_pci_config_msi_data() {
    ...
    ab_pci->msi_ep_base_data = msi_desc->msg.data;

At that point kvm_pci_msi_wait_ready() has not yet been called, so
msi_desc->msg.data contains the guest virtual vector data rather than the
host physical base data.

Later, when ath12k_pci_power_up() calls kvm_pci_msi_wait_ready(), the host
address and data are fetched and cached in kvm_data->msg and desc->msg,
but ab_pci->msi_ep_base_data is never updated here, and
ath12k_pci_get_msi_address() passes NULL for the data argument of
kvm_pci_msi_get_address().

When copy engine and data path rings are subsequently configured via
ath12k_ce_srng_msi_ring_params_setup() and ath12k_dp_srng_msi_setup(),
ath12k_hif_get_user_msi_vector() calls ath12k_pci_get_user_msi_assignment():

drivers/net/wireless/ath/ath12k/pci.c:ath12k_pci_get_user_msi_assignment() {
    ...
    *num_vectors = msi_config->users[idx].num_vectors;
    *base_vector =  msi_config->users[idx].base_vector;
    *user_base_data = *base_vector + ab_pci->msi_ep_base_data;

Wouldn't this program the device registers and firmware ring setup messages
with the stale guest message data instead of the host base data retrieved
by kvm_pci_msi_wait_ready()?

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