Re: [PATCH] vpci/msix: conditionally invoke vpci_make_msix_hole

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxx>
> ---
>  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.