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

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

On Fri, Feb 20, 2026 at 11:20:52AM +0100, Jan Beulich wrote:
> On 20.02.2026 10:28, Roger Pau Monné wrote:
> > On Tue, Feb 10, 2026 at 11:55:34AM +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).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> vpci_reinit_ext_capabilities()'es return value isn't checked, as it
> >> doesn't feel quite right to fail the hypercall because of this. At the
> >> same time it also doesn't feel quite right to have the function return
> >> "void". Thoughts?
> > 
> > For the non hardwware domain case we could deassign the device from
> > the domain?
> 
> Will need to check. De-assigning is generally done only from domctl context,
> I think. I'm also uncertain what other things may break (in Xen or the
> toolstacks) if we take away a device in such a pretty much uncontrolled way.
> 
> > And print a warning message for both cases.
> 
> Can do, albeit I'm unsure what "both" refers to - I see only ...

Oh, I mean print a warning message for both the hardware domain and
the domU case.

> >> --- a/xen/arch/x86/physdev.c
> >> +++ b/xen/arch/x86/physdev.c
> >> @@ -8,6 +8,8 @@
> >>  #include <xen/guest_access.h>
> >>  #include <xen/iocap.h>
> >>  #include <xen/serial.h>
> >> +#include <xen/vpci.h>
> >> +
> >>  #include <asm/current.h>
> >>  #include <asm/io_apic.h>
> >>  #include <asm/msi.h>
> >> @@ -169,7 +171,10 @@ int cf_check physdev_check_pci_extcfg(st
> >>  
> >>      ASSERT(pdev->seg == info->segment);
> >>      if ( pdev->bus >= info->start_bus && pdev->bus <= info->end_bus )
> >> +    {
> >>          pci_check_extcfg(pdev);
> >> +        vpci_reinit_ext_capabilities(pdev);
> >> +    }
> 
> ... this.
> 
> >> @@ -376,6 +379,20 @@ void vpci_cleanup_capabilities(struct pc
> >>      }
> >>  }
> >>  
> >> +int vpci_reinit_ext_capabilities(struct pci_dev *pdev)
> >> +{
> >> +    if ( !pdev->vpci )
> >> +        return 0;
> >> +
> >> +    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();
> >> +
> >> +    return vpci_init_capabilities(pdev, true);
> > 
> > I wonder here, in the context here, where the device is already
> > assigned to a domain you likely need to take the vPCI lock to safely
> > perform (parts of?) the cleanup and reinit.  Otherwise you could free
> > capability data while it's being accessed by the handlers.
> 
> The lock isn't recursive, so I fear we'd deadlock if it was taken here.

Yeah, that's why I've put the "parts of" remark above, some of those
functions already take the lock themselves.

> Furthermore this falls into "DomU support needs dealing with"; right
> now we assume Dom0 tells us about its final MCFG verdict ahead of
> putting devices in use. Once we need to consider devices already in
> use, I think we would further need to pause the owning domain. Also ...

Agreed.  So that this is clear, you might add an ASSERT() that the
device is not assigned to a non-hardware domain when this reinit
happens?

> > The only current extended capability (reBAR) doesn't have any internal
> > state to free on cleanup, so it's all safe.  But a cleanup like the
> > MSI(-X) ones would be racy, as they free the structure without holding
> > the vPCI lock.  I think we need to be careful, and possibly adjust the
> > cleanup functions so they can tolerate cleanup with possible
> > concurrent accesses.
> 
> ... to cover such. (For something like MSI(-X) it might then further be
> necessary to mask/disable interrupts, but hopefully we'll never have to
> deal with extended capabilities that would require this.)

Indeed.  Something like MSI-X would be extra fun to deal with, but
sooner or later we will need to handle it if we ever plan to support
hot-unplug from domUs.

In fact, the current cleanup_msi{,x}() helpers should already remove any
interrupt bindings and free any mapped pIRQs, otherwise we rely on
the PVH hardware domain having cleaned up the device properly before it
being assigned to another domain?  (Not that you need to do it here,
just realized from your comment)

Thanks, Roger