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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.05.2025 12:11, Roger Pau Monné wrote:
> 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.

Right - the code change itself looks technically fine to me.

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

Yet any user of the header not calling this function won't need the full
definition of the struct, nor (perhaps) any other of the contents of
xen/rangeset.h.

Jan