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

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

On Tue, Oct 25, 2022 at 02:57:57PM +0000, Andrew Cooper wrote:
> 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.

I already saw the second on production systems, as is what triggered
the change in 75cc460a1b.  I might not have seen both in conjunction
on the same device on a production system.

Thanks, Roger.