Re: [PATCH] x86/vpci: fix handling of BAR overlaps with non-hole regions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.05.2025 10:41, Roger Pau Monne wrote:
> For once the message printed when a BAR overlaps with a non-hole regions is
> not accurate on x86.  While the BAR won't be mapped by the vPCI logic, it
> is quite likely overlapping with a reserved region in the memory map, and
> already mapped as by default all reserved regions are identity mapped in
> the p2m.  Fix the message so it just warns about the overlap, without
> mentioning that the BAR won't be mapped, as this has caused confusion in
> the past.
> 
> Secondly, when an overlap is detected the BAR 'enabled' field is not set,
> hence other vPCI code that depends on it like vPCI MSI-X handling won't
> function properly, as it sees the BAR as disabled, even when memory
> decoding is enabled for the device and the BAR is likely mapped in the
> p2m.  Change the handling of BARs that overlap non-hole regions to instead
> remove any overlapped regions from the rangeset, so the resulting ranges to
> map just contain the hole regions.  This requires introducing a new
> pci_sanitize_bar_memory() that's implemented per-arch and sanitizes the
> address range to add to the p2m.
> 
> For x86 pci_sanitize_bar_memory() removes any regions present in the host
> memory map, for ARM this is currently left as a dummy handler to not change
> existing behavior.
> 
> Ultimately the above changes should fix the vPCI MSI-X handlers not working
> correctly when the BAR that contains the MSI-X table overlaps with a
> non-hole region, as then the 'enabled' BAR bit won't be set and the MSI-X
> traps won't handle accesses as expected.

While all of this reads plausible, I may need to look at this again later,
to hopefully grok the connections and implications.

> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -2,6 +2,7 @@
>  #define __X86_PCI_H__
>  
>  #include <xen/mm.h>
> +#include <xen/rangeset.h>

Please don't, ...

> @@ -57,14 +58,7 @@ static always_inline bool is_pci_passthrough_enabled(void)
>  
>  void arch_pci_init_pdev(struct pci_dev *pdev);
>  
> -static inline bool pci_check_bar(const struct pci_dev *pdev,
> -                                 mfn_t start, mfn_t end)
> -{
> -    /*
> -     * Check if BAR is not overlapping with any memory region defined
> -     * in the memory map.
> -     */
> -    return is_memory_hole(start, end);
> -}
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
> +int pci_sanitize_bar_memory(struct rangeset *r);

... all you need is a struct forward decl here.

> --- a/xen/arch/x86/pci.c
> +++ b/xen/arch/x86/pci.c
> @@ -98,3 +98,53 @@ int pci_conf_write_intercept(unsigned int seg, unsigned 
> int bdf,
>  
>      return rc;
>  }
> +
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
> +{
> +    /*
> +     * Check if BAR is not overlapping with any memory region defined
> +     * in the memory map.
> +     */
> +    if ( !is_memory_hole(start, end) )
> +        gdprintk(XENLOG_WARNING,
> +                 "%pp: BAR at [%"PRI_mfn", %"PRI_mfn"] not in memory map 
> hole\n",
> +                 &pdev->sbdf, mfn_x(start), mfn_x(end));
> +
> +    /*
> +     * Unconditionally return true, pci_sanitize_bar_memory() will remove any
> +     * non-hole regions.
> +     */
> +    return true;
> +}
> +
> +/* Remove overlaps with any ranges defined in the host memory map. */
> +int pci_sanitize_bar_memory(struct rangeset *r)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < e820.nr_map; i++ )
> +    {
> +        const struct e820entry *entry = &e820.map[i];
> +        int rc;
> +
> +        if ( !entry->size )
> +            continue;
> +
> +        rc = rangeset_remove_range(r, PFN_DOWN(entry->addr),
> +                                   PFN_UP(entry->addr + entry->size - 1));
> +        if ( rc )
> +            return rc;

Perhaps better continue the loop in a best effort manner, only accumulating
the error to then ...

> +    }
> +
> +    return 0;
> +}

... return here?

Jan