Re: [PATCH] vpci: Add resizable bar support

["Roger Pau Monné" <roger.pau@citrix.com>]

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.

Thanks, Roger.