Re: [PATCH v5 4/9] vpci/msix: move MSI-X hole punching as a result of memory decoding enable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.07.2026 23:02, Stewart Hildebrand wrote:
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> 
> Deferring the p2m unpopulate of the MSI-X table region to after the BARs are
> mapped in the p2m is not needed.  The aim of vpci_make_msix_hole() is to
> cope with the BAR containing the MSI-X table being positioned over a
> reserved e820 region, by making sure the MSI-X table region is
> unpopulated on the p2m. Note that reserved e820 regions are identity mapped
> in the hardware domain p2m by default.
> 
> Doing it before or after the BAR p2m changes are done is irrelevant, hence
> do it ahead of the p2m changes, as that's simpler.
> 
> Note that vpci_make_msix_hole() should be invoked strictly after setting
> bar->enabled. The movement of vpci_make_msix_hole() is OK since
> bar->enabled is set in modify_bars().
> 
> Take the opportunity to store the memory decoding enabled bit in a local
> variable.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> ---
> Note that this should not be committed until after the bar->enabled
> setting movement in ("xen/vpci: only check BAR validity once")
> 
> I tested this with hot-plug as described in 7c24d58a6e64 ("vpci/msix:
> check for BARs enabled in vpci_make_msix_hole").
> 
> The return at the end of the function will gain a purpose again after a
> later code movement patch in the series.
> 
> The new_enabled variable will gain 2 additional uses later in the series.
> 
> v1->v5:
> * order after ("xen/vpci: only check BAR validity once")
> * preserve behavior of only invoking vpci_make_msix_hole() when memory
>   decoding toggles from 0 to 1
> * add note about vpci_make_msix_hole() invocation after setting bar->enabled
> * use local variable
> * Jan offered an R-b tag in [1], though I hesitated to pick it up due to
>   changes, and it was offered prior to 7c24d58a6e64 going upstream
> [1] 
> https://lore.kernel.org/xen-devel/e2c4944d-d089-490c-879d-b6baec5e4cae@xxxxxxxx/

If there wasn't ...

> @@ -556,6 +544,12 @@ static void cf_check cmd_write(
>          modify_bars(pdev, cmd, false);
>      else
>          pci_conf_write16(pdev->sbdf, reg, cmd);
> +
> +#ifdef CONFIG_HAS_PCI_MSI
> +    /* Unpopulate MSI-X table region, so accesses trap into Xen. */
> +    if ( !header->bars_mapped && new_enabled && vpci_make_msix_hole(pdev) )
> +        return;
> +#endif

... the new first part of the conditional, I'd re-offer R-b right away. With
that check added, aren't we at risk of leaving the range mapped when it
shouldn't be?

Jan