Re: [PATCH v6] vPCI: re-init extended-capabilities when MMCFG availability changed

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

On Thu, Mar 05, 2026 at 02:56:32PM +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>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

One comment below.

> ---
> v6: Add comment in physdev_check_pci_extcfg(), while dropping the logging
>     of a(nother) message there. Drop hwdom restriction from
>     vpci_cleanup_capabilities(). In the re-init case don't bail early from
>     vpci_init_capabilities(). Simplify assertion and re-order actions in
>     vpci_reinit_ext_capabilities().
> v5: Don't use pci_find_ext_capability() in vpci_cleanup_capabilities().
>     Add assertion in vpci_reinit_ext_capabilities().
> v4: Make sure ->cleanup() and ->init() are invoked.
> v3: New.
> 
> --- 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,8 +171,19 @@ 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);
>  
> +        /*
> +         * The re-init failing doesn't mean the device becomes entirely non-
> +         * functional.  In case of failure, a message was already logged.
> +         * Hence don't otherwise act upon failure.
> +         *
> +         * FIXME: Re-visit when DomU support is added to vPCI.
> +         */
> +        vpci_reinit_ext_capabilities(pdev);
> +    }
> +
>      return 0;
>  }
>  #endif /* COMPAT */
> --- a/xen/drivers/vpci/cap.c
> +++ b/xen/drivers/vpci/cap.c
> @@ -285,13 +285,16 @@ static int vpci_init_ext_capability_list
>      return 0;
>  }
>  
> -int vpci_init_capabilities(struct pci_dev *pdev)
> +int vpci_init_capabilities(struct pci_dev *pdev, bool ext_only)
>  {
> -    int rc;
> +    int rc, accum_rc = 0;
>  
> -    rc = vpci_init_capability_list(pdev);
> -    if ( rc )
> -        return rc;
> +    if ( !ext_only )
> +    {
> +        rc = vpci_init_capability_list(pdev);
> +        if ( rc )
> +            return rc;
> +    }
>  
>      rc = vpci_init_ext_capability_list(pdev);
>      if ( rc )
> @@ -305,7 +308,7 @@ int vpci_init_capabilities(struct pci_de
>          unsigned int pos = 0;
>  
>          if ( !is_ext )
> -            pos = pci_find_cap_offset(pdev->sbdf, cap);
> +            pos = !ext_only ? pci_find_cap_offset(pdev->sbdf, cap) : 0;
>          else if ( is_hardware_domain(pdev->domain) )
>              pos = pci_find_ext_capability(pdev, cap);
>  
> @@ -341,30 +344,40 @@ int vpci_init_capabilities(struct pci_de
>              {
>                  printk(XENLOG_ERR "%pd %pp: hide %s cap %u fail rc=%d\n",
>                         pdev->domain, &pdev->sbdf, type, cap, rc);
> -                return rc;
> +
> +                /* Best effort for the re-init case. */
> +                if ( !ext_only )
> +                    return rc;
> +
> +                if ( !accum_rc )
> +                    accum_rc = rc;

I've been wondering about this, for the hardware domain case we might
as well do best effort for the init case also.  The other option for
the hardware domain is to allow unmediated access to the device, which
is likely worse than allowing it to work in a possibly degraded
mediated mode.  We already special-case the hardware domain for
failures of capability->cleanup(), and we should likely do the same
here.

Thanks, Roger.