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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.10.2022 14:45, Roger Pau Monné wrote:
> On Mon, Oct 24, 2022 at 01:19:22PM +0200, Jan Beulich wrote:
>> On 20.10.2022 11:46, 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.
>>
>> Which makes me wonder: How do things work then? Dom0 then still can't
>> access the BAR address range, can it?
> 
> It does allow access on some situations where the previous arrangement
> didn't work because it wholesale disabled memory decoding for the
> device.
> 
> So if it's only one BAR that's misplaced the rest will still get added
> to the dom0 p2m and be accessible, because memory decoding won't be
> turned off for the device.

Right - without a per-BAR disable there can only be all or nothing. In
the end if things work with this adjustment, the problem BAR cannot
really be in use aiui. I wonder what you would propose we do if on
another system such a BAR is actually in use.

>> Plus with this adjustment, is
>> ...
>>
>>>  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.
>>
>> ... this (in particular "restores the behavior") a valid description
>> of this change?
> 
> Yes, it restores the previous behavior for PV dom0, as memory decoding
> is no longer turned off for any devices regardless of where the BARs
> are positioned.

It restores one aspect of behavior but then puts in place another
restriction.

>>> 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
>>>
>>> And memory decoding enabled by the firmware.  With the current code
>>> (or any of the previous fix proposals), Xen would still disable memory
>>> decoding for the device, and the system will freeze when attempting to
>>> set EFI vars.
>>
>> Isn't the latter (BAR at address 0) yet another problem?
> 
> It's a BAR that hasn't been positioned by the firmware AFAICT.  Which
> is a bug in the firmware but shouldn't prevent Xen from booting.
> 
> In the above system address 0 is outside of the PCI host bridge
> window, so even if we mapped the BAR and memory decoding for the
> device was enabled accessing such BAR wouldn't work.

It's mere luck I would say that in this case the BAR is outside the
bridge's window. What if this was a device integrated in the root
complex?

>> I have to admit
>> that I'm uncertain in how far it is a good idea to try to make Xen look
>> to work on such a system ...
> 
> PV dom0 works on a system like the above prior to c/s 75cc460a1b, so I
> would consider 75cc460a1b to be a regression for PV dom0 setups.

Agreed, in a way it is a regression. In another way it is deliberate
behavior to not accept bogus configurations. The difficulty is to
find a reasonable balance between allowing Xen to work in such cases
and guarding Xen from suffering follow-on issues resulting from such
misconfiguration. After all if this system later was impacted by the
bad BAR(s), connecting the misbehavior to the root cause might end
up quite a bit more difficult.

Jan