Re: [PATCH] vpci: Add resizable bar support

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

On Wed, Nov 13, 2024 at 12:29:11PM +0100, Jan Beulich wrote:
> 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?

Those are the same guarantees that Xen has from a PV dom0 not moving
the position of the BARs after having MSI-X enabled for example.  IOW:
dom0 should set the BAR size before enabling MSI-X, just like it does
for the BAR positions currently.

The specification doesn't mention anything about MSI-X table or PBA
explicitly not changing it's offset if the BAR where it resides is
resized, so I don't think we have any guarantees.  Albeit I would
think it's unexpected for the MSI-X BIR or offset to change as a
result of a BAR resize.

There's an implementation note in the PCI specification that the BAR
should be resized during resource allocation, prior to assigning the
base address to the BAR.

Thanks, Roger.