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

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

On Thu, Mar 05, 2026 at 10:45:29AM +0100, Jan Beulich wrote:
> On 05.03.2026 10:22, Roger Pau Monné wrote:
> > On Thu, Mar 05, 2026 at 10:00:13AM +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@suse.com>
> >>>> ---
> >>>> 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?
> >>
> >> While adding that note it occurred to me that in order to keep the device
> >> as functioning as possible, in the re-init case vpci_init_capabilities()
> >> might better not bail upon encountering a failure, but accumulate the
> >> error while continuing its loop in a best-effort manner. Thoughts? (One
> >> of the two return-s is already guarded by !is_hardware_domain(), so that
> >> could be left alone for the immediate purpose.)
> > 
> > Right, yes, that would be preferable.  We already print a message for
> > the failed to init capabilities, so there's no need to print another
> > one in the caller.
> 
> Hmm, that's another aspect I didn't consider. Yes, the log message in the
> caller is redundant with the present code structure. If we expect that to
> remain like that, I can drop logging anything from
> physdev_check_pci_extcfg(). Which then re-raises the question whether
> vpci_reinit_ext_capabilities() might better return void. At which point
> the comment I put in physdev_check_pci_extcfg() (upon your request) would
> want to move there.
> 
> But my earlier question went in a different direction, and you didn't
> comment on that at all.

Yes, I think we should accumulate errors.  One device failing doesn't
mean the rest will also fail.  We should continue the loop.

Thanks, Roger.