Re: [PATCH v2] vpci/msix: check for BARs enabled in vpci_make_msix_hole

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.02.2026 03:56, Stewart Hildebrand wrote:
> A hotplugged PCI device may be added uninitialized. In particular,
> memory decoding might be disabled and the BARs might be zeroed. In this
> case, the BARs will not be mapped in p2m. However, vpci_make_msix_hole()
> unconditionally attempts to punch holes in p2m, leading to init_msix()
> failing:
> 
> (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 attempt to punch holes if the BARs
> containing the MSI-X/PBA tables are mapped in p2m. Introduce a helper
> for checking if a BAR is enabled, and add a check for the situation
> inside vpci_make_msix_hole().
> 
> Move the vpci_make_msix_hole() call within modify_decoding() to after
> setting ->enabled.
> 
> Fixes: ee2eb6849d50 ("vpci: Refactor REGISTER_VPCI_INIT")
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> ---
> Pipeline: 
> https://gitlab.com/xen-project/people/stewarthildebrand/xen/-/pipelines/2344925375
> 
> v1->v2:
> * new title, was ("vpci/msix: conditionally invoke vpci_make_msix_hole")
> * move BAR enabled check to inside vpci_make_msix_hole()
> * introduce vmsix_table_bar_valid() helper
> * move vpci_make_msix_hole() call within modify_decoding() to after
>   setting ->enabled
> * split typo fixup to separate patch
> 
> v1: 
> https://lore.kernel.org/xen-devel/20250812151744.460953-1-stewart.hildebrand@xxxxxxx/T/#t
> ---
>  xen/drivers/vpci/header.c | 26 +++++++++++++-------------
>  xen/drivers/vpci/msix.c   |  4 ++++
>  xen/include/xen/vpci.h    |  6 ++++++
>  3 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 739a5f610e91..6a28e07a625b 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -122,19 +122,6 @@ static void modify_decoding(const struct pci_dev *pdev, 
> uint16_t cmd,
>      bool map = cmd & PCI_COMMAND_MEMORY;
>      unsigned int i;
>  
> -    /*
> -     * Make sure there are no mappings in the MSIX MMIO areas, so that 
> accesses
> -     * can be trapped (and emulated) by Xen when the memory decoding bit is
> -     * enabled.
> -     *
> -     * FIXME: punching holes after the p2m has been set up might be racy for
> -     * DomU usage, needs to be revisited.
> -     */
> -#ifdef CONFIG_HAS_PCI_MSI
> -    if ( map && !rom_only && vpci_make_msix_hole(pdev) )
> -        return;
> -#endif
> -
>      for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>      {
>          struct vpci_bar *bar = &header->bars[i];
> @@ -164,6 +151,19 @@ static void modify_decoding(const struct pci_dev *pdev, 
> uint16_t cmd,
>              bar->enabled = map;
>      }
>  
> +    /*
> +     * Make sure there are no mappings in the MSIX MMIO areas, so that 
> accesses
> +     * can be trapped (and emulated) by Xen when the memory decoding bit is
> +     * enabled.
> +     *
> +     * FIXME: punching holes after the p2m has been set up might be racy for
> +     * DomU usage, needs to be revisited.
> +     */
> +#ifdef CONFIG_HAS_PCI_MSI
> +    if ( map && !rom_only && vpci_make_msix_hole(pdev) )
> +        return;
> +#endif
> +
>      if ( !rom_only )
>      {
>          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 516282205a53..142cfbae59d5 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -598,6 +598,10 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>      if ( !pdev->vpci->msix )
>          return 0;
>  
> +    if ( !vmsix_table_bar_valid(pdev->vpci, VPCI_MSIX_TABLE) &&
> +         !vmsix_table_bar_valid(pdev->vpci, VPCI_MSIX_PBA) )
> +        return 0;

What if one is enabled and the other isn't? Doesn't the check need to move ...

>      /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>      for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>      {

... into the loop, and then apparently also need mirroring in the hwdom-only
loop further down?

Jan