Re: [PATCH v5 3/3] x86/msi: fix locking for SR-IOV devices

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.10.2024 17:27, Stewart Hildebrand wrote:
> In commit 4f78438b45e2 ("vpci: use per-domain PCI lock to protect vpci
> structure") a lock was moved from allocate_and_map_msi_pirq() to the
> caller and changed from pcidevs_lock() to read_lock(&d->pci_lock).
> However, one call path wasn't updated to reflect the change, leading to
> a failed assertion observed under the following conditions:
> 
> * PV dom0
> * Debug build (CONFIG_DEBUG=y) of Xen
> * There is an SR-IOV device in the system with one or more VFs enabled
> * Dom0 has loaded the driver for the VF and enabled MSI-X
> 
> (XEN) Assertion 'd || pcidevs_locked()' failed at 
> drivers/passthrough/pci.c:535
> (XEN) ----[ Xen-4.20-unstable  x86_64  debug=y  Not tainted ]----
> ...
> (XEN) Xen call trace:
> (XEN)    [<ffff82d040284da8>] R pci_get_pdev+0x4c/0xab
> (XEN)    [<ffff82d040344f5c>] F arch/x86/msi.c#read_pci_mem_bar+0x58/0x272
> (XEN)    [<ffff82d04034530e>] F 
> arch/x86/msi.c#msix_capability_init+0x198/0x755
> (XEN)    [<ffff82d040345dad>] F arch/x86/msi.c#__pci_enable_msix+0x82/0xe8
> (XEN)    [<ffff82d0403463e5>] F pci_enable_msi+0x3f/0x78
> (XEN)    [<ffff82d04034be2b>] F map_domain_pirq+0x2a4/0x6dc
> (XEN)    [<ffff82d04034d4d5>] F allocate_and_map_msi_pirq+0x103/0x262
> (XEN)    [<ffff82d04035da5d>] F physdev_map_pirq+0x210/0x259
> (XEN)    [<ffff82d04035e798>] F do_physdev_op+0x9c3/0x1454
> (XEN)    [<ffff82d040329475>] F pv_hypercall+0x5ac/0x6af
> (XEN)    [<ffff82d0402012d3>] F lstar_enter+0x143/0x150
> 
> In read_pci_mem_bar(), the VF obtains the struct pci_dev pointer for its
> associated PF to access the vf_rlen array. This array is initialized in
> pci_add_device() and is only populated in the associated PF's struct
> pci_dev.
> 
> Access the vf_rlen array via the link to the PF, and remove the
> troublesome call to pci_get_pdev().
> 
> Fixes: 4f78438b45e2 ("vpci: use per-domain PCI lock to protect vpci 
> structure")
> Reported-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -736,7 +736,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>          }
>      }
>  
> -    if ( !pdev->info.is_virtfn && !pdev->vf_rlen[0] )
> +    if ( !pdev->info.is_virtfn && !pdev->physfn.vf_rlen[0] )

Unrelated to your change: Now that I look at this again, it seems slightly
wrong to me to use array slot 0 as "have we populated the array already"
indicator. If BAR0 was unused, we may end up doing this more than once.

Jan