Re: [PATCH] vpci: Add resizable bar support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2024 12:24, Roger Pau Monné wrote:
> On Wed, Nov 13, 2024 at 12:01:23PM +0100, Jan Beulich wrote:
>> On 13.11.2024 11:56, Roger Pau Monné wrote:
>>> On Wed, Nov 13, 2024 at 11:36:46AM +0100, Jan Beulich wrote:
>>>> On 13.11.2024 11:30, Roger Pau Monné wrote:
>>>>> On Wed, Nov 13, 2024 at 10:00:33AM +0000, Chen, Jiqian wrote:
>>>>>> On 2024/11/13 17:30, Roger Pau Monné wrote:
>>>>>>> On Wed, Nov 13, 2024 at 04:00:27PM +0800, Jiqian Chen wrote:
>>>>>>>> Some devices, like discrete GPU of amd, support resizable bar 
>>>>>>>> capability,
>>>>>>>> but vpci of Xen doesn't support this feature, so they fail to resize 
>>>>>>>> bars
>>>>>>>> and then cause probing failure.
>>>>>>>>
>>>>>>>> According to PCIe spec, each bar that support resizing has two 
>>>>>>>> registers,
>>>>>>>> PCI_REBAR_CAP and PCI_REBAR_CTRL, so add these two registers and their
>>>>>>>> corresponding handler into vpci.
>>>>>>>>
>>>>>>>> PCI_REBAR_CAP is RO, only provide reading.
>>>>>>>>
>>>>>>>> PCI_REBAR_CTRL only has bar size is RW, so add write function to 
>>>>>>>> support
>>>>>>>> setting the new size.
>>>>>>>
>>>>>>> I think the logic to handle resizable BAR could be much simpler.  Some
>>>>>>> time ago I've made a patch to add support for it, but due to lack of
>>>>>>> hardware on my side to test it I've never submitted it.
>>>>>>>
>>>>>>> My approach would be to detect the presence of the
>>>>>>> PCI_EXT_CAP_ID_REBAR capability in init_header(), and if the
>>>>>>> capability is present force the sizing of BARs each time they are
>>>>>>> mapped in modify_bars().  I don't think we need to trap accesses to
>>>>>>> the capability itself, as resizing can only happen when memory
>>>>>>> decoding is not enabled for the device.  It's enough to fetch the size
>>>>>>> of the BARs ahead of each enabling of memory decoding.
>>>>>>>
>>>>>>> Note that memory decoding implies mapping the BARs into the p2m, which
>>>>>>> is already an expensive operation, the extra sizing is unlikely to
>>>>>>> make much of a difference performance wise.
>>>>>>>
>>>>>>> I've found the following on my git tree and rebased on top of staging:
>>>>>> OK.
>>>>>> Do you need me to validate your patch in my environment?
>>>>>
>>>>> Yes please, I have no way to test it.  Let's see what others think
>>>>> about the different approaches.
>>>>
>>>> I'd certainly prefer your simpler form, if it's safe and fits the needs.
>>>>
>>>>>> And I have one question: where does your patch do writing the resizing 
>>>>>> size into hardware?
>>>>>
>>>>> dom0 has unrestricted access to the resize capability, so the value
>>>>> written by dom0 is propagated to the hardware without modification.
>>>>>
>>>>> I would be wary of exposing the resize capability to untrusted
>>>>> domains, as allowing a domU to change the size of BARs can lead to
>>>>> overlapping if the hardware domain hasn't accounted for the increase
>>>>> in BAR size.
>>>>
>>>> Question is how the feature is used in practice: If it was a driver to
>>>> request the re-size, I'd have a hard time seeing how we could make that
>>>> work without intercepting accesses to the capability for DomU-s (implying
>>>> to expose it in the first place, of course).
>>>
>>> Question is also whether the capability is required for guests, as in
>>> OS drivers requesting it to be present for proper operation.
>>>
>>> I haven't given much thought about how to expose to domUs.  The
>>> current patch doesn't attempt to expose to domUs either, as the
>>> capability is not added to the 'supported_caps' array.
>>
>> Hmm, I see. Yet then adding support to vPCI, but limited to Dom0, ends up
>> odd in two ways: Another aspect that'll need dealing with for DomU-s, and
>> the same functionality remaining unavailable (or at least not properly
>> available, with all possible side effects) to PV Dom0.
> 
> I think resizable BARs should just work for PV dom0, as Xen allows PV
> dom0 to map almost all physical memory.  Xen doesn't require knowing
> the BAR positions and sizes like it does for PVH dom0.

Does it really not need to know in any (corner) case? Are there guarantees
that e.g. MSI-X table or PBA can't move when the size of the BAR covering
them changes?

> Note that resizable BAR capability is not exposed to domUs now either
> when using QEMU based pci-passthrough.

Of course.

Jan