A hotplugged PCI device may be added uninitialized. In particular,
memory decoding may be disabled and the BARs may be zeroed. In this
case, the BARs will not be mapped in p2m. However, currently
vpci_make_msix_hole is called unconditionally in init_msix, and the
initialization fails in this case:

(XEN) d0v0 0000:01:00.0: existing mapping (mfn: 1c1880 type: 0) at 0 clobbers MSIX MMIO area
(XEN) d0 0000:01:00.0: init legacy cap 17 fail rc=-17, mask it

vpci_make_msix_hole should only be called if the BARs containing the
MSI-X/PBA tables are mapped in p2m.

Take the opportunity to fix a typo in the preceding comment.

Fixes: ee2eb6849d50 ("vpci: Refactor REGISTER_VPCI_INIT")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
 xen/drivers/vpci/msix.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 54a5070733aa..39d1c45bd296 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -705,10 +705,16 @@ static int cf_check init_msix(struct pci_dev *pdev)
 
     /*
      * vPCI header initialization will have mapped the whole BAR into the
-     * p2m, as MSI-X capability was not yet initialized.  Crave a hole for
+     * p2m, as MSI-X capability was not yet initialized.  Carve a hole for
      * the MSI-X table here, so that Xen can trap accesses.
      */
-    return vpci_make_msix_hole(pdev);
+    if ( pdev->vpci->header.bars[
+             msix->tables[VPCI_MSIX_TABLE] & PCI_MSIX_BIRMASK].enabled ||
+         pdev->vpci->header.bars[
+             msix->tables[VPCI_MSIX_PBA] & PCI_MSIX_BIRMASK].enabled )
+        return vpci_make_msix_hole(pdev);
+
+    return 0;
 }
 REGISTER_VPCI_CAP(MSIX, init_msix, NULL);
 

base-commit: 4cde4a552ed35f3cc74df877d5a7cfbbfced8fb3
-- 
2.50.1

On Tue, Aug 12, 2025 at 11:17:42AM -0400, Stewart Hildebrand wrote:
> A hotplugged PCI device may be added uninitialized. In particular,
> memory decoding may be disabled and the BARs may be zeroed. In this
> case, the BARs will not be mapped in p2m. However, currently
> vpci_make_msix_hole is called unconditionally in init_msix, and the
> initialization fails in this case:
> 
> (XEN) d0v0 0000:01:00.0: existing mapping (mfn: 1c1880 type: 0) at 0 clobbers MSIX MMIO area
> (XEN) d0 0000:01:00.0: init legacy cap 17 fail rc=-17, mask it
> 
> vpci_make_msix_hole should only be called if the BARs containing the
> MSI-X/PBA tables are mapped in p2m.
> 
> Take the opportunity to fix a typo in the preceding comment.
> 
> Fixes: ee2eb6849d50 ("vpci: Refactor REGISTER_VPCI_INIT")
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
>  xen/drivers/vpci/msix.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 54a5070733aa..39d1c45bd296 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -705,10 +705,16 @@ static int cf_check init_msix(struct pci_dev *pdev)
>  
>      /*
>       * vPCI header initialization will have mapped the whole BAR into the
> -     * p2m, as MSI-X capability was not yet initialized.  Crave a hole for
> +     * p2m, as MSI-X capability was not yet initialized.  Carve a hole for
>       * the MSI-X table here, so that Xen can trap accesses.
>       */
> -    return vpci_make_msix_hole(pdev);
> +    if ( pdev->vpci->header.bars[
> +             msix->tables[VPCI_MSIX_TABLE] & PCI_MSIX_BIRMASK].enabled ||
> +         pdev->vpci->header.bars[
> +             msix->tables[VPCI_MSIX_PBA] & PCI_MSIX_BIRMASK].enabled )
> +        return vpci_make_msix_hole(pdev);

I think it might be better to place this checks inside of
vpci_make_msix_hole() itself, so that in case the guest moves the BAR
to an invalid position vpci_make_msix_hole() doesn't return error
either.

At the same time, you might want to introduce some helper to make this
less cumbersome, for example:

bool vmsix_table_bar_valid(const struct vpci *vpci, unsigned int nr)
{
    return vpci->header.bars[vpci->msix->tables[nr] &
                             PCI_MSIX_BIRMASK].enabled;
}

Note that if you adjust vpci_make_msix_hole() this way you will also
need to move the call in modify_decoding() so it happens strictly
after bar->enabled is set.

Thanks, Roger.