Re: [PATCH v5 5/5] vPCI: re-init extended-capabilities when MMCFG availability changed

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

On Wed, Mar 04, 2026 at 04:39:00PM +0100, Jan Beulich wrote:
> On 04.03.2026 16:06, Roger Pau Monné wrote:
> > On Wed, Feb 25, 2026 at 12:44:44PM +0100, Jan Beulich wrote:
> >> When Dom0 informs us about MMCFG usability, this may change whether
> >> extended capabilities are available (accessible) for devices. Zap what
> >> might be on record, and re-initialize things.
> >>
> >> No synchronization is added for the case where devices may already be in
> >> use. That'll need sorting when (a) DomU support was added and (b) DomU-s
> >> may run already while Dom0 / hwdom still boots (dom0less, Hyperlaunch).
> >>
> >> vpci_cleanup_capabilities() also shouldn't have used
> >> pci_find_ext_capability(), as already when the function was introduced
> >> extended config space may not have been (properly) accessible anymore,
> >> no matter whether it was during init. Extended capability cleanup hooks
> >> need to cope with being called when the respective capability doesn't
> >> exist (and hence the corresponding ->init() hook was never called).
> >>
> >> Fixes: 70e6dace747e ("vpci: Use cleanup to free capability resource during 
> >> deassign")
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> vpci_reinit_ext_capabilities()'es return value is checked only to log an
> >> error; it doesn't feel quite right to fail the hypercall because of this.
> >> Roger brought up the idea of de-assigning the device in such a case, but
> >> if a driver doesn't use extended capabilities the device would likely
> >> continue to work fine, for Dom0 this probably wouldn't be quite right
> >> anyway, and it's also unclear whether calling deassign_device() could be
> >> done from this context. Something like what pci_check_disable_device()
> >> does may be an option, if we really think we need to "break" the device.
> > 
> > We may want to add a note there, stating that we have considered all
> > possible options, and hiding the capability and hoping the owner
> > domain would continue to work as expected seems the less bad of all of
> > them?
> 
> I'll see what I can do.
> 
> >> The use of is_hardware_domain() in vpci_cleanup_capabilities() was
> >> uncommented and hence is left so. Shouldn't there be a DomU-related TODO
> >> or FIXME?
> > 
> > Hm, yes, possibly.  I think limiting extended space availability to
> > the hardware domain only has been done "just" because we have no
> > extended capabilities to expose to domUs so far, and I don't think we
> > even setup the extended capability list in the domU case.
> 
> Considering how many things there are to be done for DomU support, I
> think it would help if every place where e.g. is_hardware_domain() is
> used only as surrogate would be properly annotated. Or perhaps properly
> named predicates could be introduced right away, so one can actually go
> hunt for all of them. Then again is_hardware_domain() is also something
> one can go hunt for.

I would mind having more concrete predicates, even if right now they
are some kind of dummy checks.  However it might be difficult to
formulate those know without having the full picture of what domU
support requires.

> >> @@ -349,22 +352,23 @@ int vpci_init_capabilities(struct pci_de
> >>      return 0;
> >>  }
> >>  
> >> -void vpci_cleanup_capabilities(struct pci_dev *pdev)
> >> +void vpci_cleanup_capabilities(struct pci_dev *pdev, bool ext_only)
> >>  {
> > 
> > You could short-circuit the function here, ie:
> > 
> > if ( ext_only && !is_hardware_domain(pdev->domain) )
> >     return;
> > 
> > But I'm not sure that would simplify the code of the function much?
> > Likewise for vpci_init_capabilities().
> 
> Such a short-circuit would need replacing / dropping once DomU support is
> added. I was hoping the chosen arrangement would make for a little less
> churn at that time. I'll listen to your advice, though, just that the
> question gives the impression you're not quite sure either.

Yeah, I wasn't fully sure.  IT would be nice if we could add those
short circuits now, and then once domU support is in place we just
remove teh shortcuts and it works for domU also.  But I fear more
changes will be needed anyway, at which point the short-circuit is
not that attractive to use.

> >> +
> >> +    vpci_cleanup_capabilities(pdev, true);
> >> +
> >> +    if ( vpci_remove_registers(pdev->vpci, PCI_CFG_SPACE_SIZE,
> >> +                               PCI_CFG_SPACE_EXP_SIZE - 
> >> PCI_CFG_SPACE_SIZE) )
> >> +        ASSERT_UNREACHABLE();
> > 
> > Ideally this would better be done the other way around.  We first
> > remove the handlers, and the cleanup the capabilities.  Just to ensure
> > no stray handler could end up having cached references to data that's
> > been freed by vpci_cleanup_capabilities().
> 
> And maybe not just that: For the hwdom case cleanup_rebar() adds new handlers,
> which we'd wrongly purge again right away. (Because we pass "false" for 
> "hide",
> this isn't an active issue right now.)
> 
> > And we should take the write_lock(&pdev->domain->pci_lock).
> 
> Now this is a request that I'm struggling with some. I can see that callers
> of vpci_{init,cleanup}_capabilities() assert that the lock is being held, yet
> it's not quite clear to me why that's needed. Shouldn't vPCI internals all
> synchronize on the vPCI lock of the domain?

Right, the callers of the handlers already hold the locks, and the
removal of the handlers should also hold the locks.  The point of
taking the d->pci_lock is to avoid the device from being removed
while there are vPCI accesses against it being done.  The vPCI lock is
fine for vPCI internals, but functions that deal with addition or
removal of devices need the d->pci_lock to avoid races with possibly
freeing pdev->vpci while in use.

I think you are right, and for the usage here (that doesn't add or
remove pdev->vpci itself), the internal vPCI lock should be enough.

Thanks, Roger.