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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 15, 2025 at 11:24:59AM +0200, Jan Beulich wrote:
> 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.

Thanks, it's indeed not trivial to follow.  I've attempted to write
this as best as I could.

I think the fix is far easier to understand than the description of
the issue and the connection with vPCI MSI-X handling.

> > --- 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.

Hm, but any user that makes use of pci_sanitize_bar_memory() will need
to include rangeset.h anyway, hence it seemed better to just include
the header rather that pollute the current one by adding forward
declarations.

> 
> > --- 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?

I think for the usage here, any error should be fatal, and should
result in the contents of the rangeset not being mapped.  Failure to
remove sensitive ranges from the rangeset cannot be worked around IMO,
hence accumulating errors doesn't seem helpful.

Thanks, Roger.