Re: [PATCH for-4.17 v2 4/5] pci: do not disable memory decoding for devices

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 25/10/2022 15:44, Roger Pau Monne wrote:
> Commit 75cc460a1b added checks to ensure the position of the BARs from
> PCI devices don't overlap with regions defined on the memory map.
> When there's a collision memory decoding is left disabled for the
> device, assuming that dom0 will reposition the BAR if necessary and
> enable memory decoding.
>
> While this would be the case for devices being used by dom0, devices
> being used by the firmware itself that have no driver would usually be
> left with memory decoding disabled by dom0 if that's the state dom0
> found them in, and thus firmware trying to make use of them will not
> function correctly.
>
> The initial intent of 75cc460a1b was to prevent vPCI from creating
> MMIO mappings on the dom0 p2m over regions that would otherwise
> already have mappings established.  It's my view now that we likely
> went too far with 75cc460a1b, and Xen disabling memory decoding of
> devices (as buggy as they might be) is harmful, and reduces the set of
> hardware on which Xen works.
>
> This commits reverts most of 75cc460a1b, and instead adds checks to
> vPCI in order to prevent misplaced BARs from being added to the
> hardware domain p2m.  Signaling on whether BARs are mapped is tracked
> in the vpci structure, so that misplaced BARs are not mapped, and thus
> Xen won't attempt to unmap them when memory decoding is disabled.
>
> This restores the behavior of Xen for PV dom0 to the state it was
> previous to 75cc460a1b, while also introducing a more contained fix
> for the vPCI BAR mapping issues.
>
> Fixes: 75cc460a1b ('xen/pci: detect when BARs are not suitably positioned')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> AT Citrix we have a system with a device with the following BARs:
>
> BAR [0xfe010, 0xfe010] -> in a EfiMemoryMappedIO region
> BAR [0, 0x1fff] -> not positioned, outside host bridge window

This needs a bit more explanation (even if only in the email thread). 
The first item here is permitted under the UEFI spec, and exists on
production systems.  We've got several examples.

The second has only been seen on an SDP, and is hopefully a firmware bug
that doesn't get out to production, but we should boot nevertheless.

~Andrew