On 2/24/26 05:57, Roger Pau Monné wrote:
> On Mon, Feb 23, 2026 at 09:56:24PM -0500, 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.
>
> I would maybe make it clear that the movement of the
> vpci_make_msix_hole() call in modify_decoding() is due to the extra
> requirement added in this patch that ->enabled must be set before
> calling the function.
>
> "As a result of the newly introduced checks in vpci_make_msix_hole(),
> move the ..."
>
> Or something along the lines.
Will do
>>
>> Fixes: ee2eb6849d50 ("vpci: Refactor REGISTER_VPCI_INIT")
>> Signed-off-by: Stewart Hildebrand <[email protected]>
>> ---
>> 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/[email protected]/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;
>
> As Jan pointed out, this needs to be moved inside the loop.
Will do.
Thanks!
