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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 2/24/26 02:34, Jan Beulich wrote:
> 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,

Yes, will do.

> and then apparently also need mirroring in the hwdom-only
> loop further down?

I don't think that's necessary, the logic there is idempotent, and it's not
dealing with p2m.

Thanks for taking a look.