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
