Re: [PATCH v3 08/18] x86/domain_page: prepare the mapcache for per-vCPU accounting

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.10.2026 12:40, George Dunlap wrote:
> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Currently, only PV domains have a mapcache, which is domain-wide one.
> In preparation for enabling per-vCPU mapcaches for HVM domains, split
> the accounting out into a struct mapcache, embedded in struct
> mapcache_domain.
> 
> Introduce has_mapcache(), for whether a vCPU has a mapcache, and
> vcpu_mapcache(), which returns the accounting for a vCPU's mapcache
> and, in *dcache, the domain-wide mapcache whose lock and TLB epoch the
> caller has to take into account.  For now has_mapcache() is
> is_pv_vcpu(), and vcpu_mapcache() always returns the domain's
> accounting.  map_domain_page() and unmap_domain_page() stop repeating
> the is_pv_vcpu() check mapcache_current_vcpu() has made.
> 
> Move map_domain_page()'s handling of the domain-wide lock and TLB epoch
> into three helpers: mapcache_lock() takes the lock and catches up with
> a wrap another vCPU caused, mapcache_new_epoch() starts a new epoch
> after a wrap, and mapcache_unlock() drops the lock.  A per-vCPU
> mapcache can then skip them without map_domain_page() being
> re-indented.
> 
> struct mapcache_domain keeps its size (40 bytes, x86_64 debug build),
> and so do the structures containing it.
> 
> No functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Assisted-by: Claude Code:claude-fable-5, Claude Code:claude-opus-4-8, Claude 
> Code:claude-opus-5-5
> Signed-off-by: George Dunlap <gwd@xxxxxxxxxxxxxx>
> ---
> Changes in v3:
> - New in this version, split out of "x86/hvm: introduce per-vCPU
>   mapcache for vCPU-PT domains" for review.
> ---
>  xen/arch/x86/domain_page.c        | 155 ++++++++++++++++++++----------
>  xen/arch/x86/include/asm/domain.h |  19 ++--
>  2 files changed, 116 insertions(+), 58 deletions(-)
> 
> diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
> index 9119f735f5..5d90f77bed 100644
> --- a/xen/arch/x86/domain_page.c
> +++ b/xen/arch/x86/domain_page.c
> @@ -18,17 +18,23 @@
>  #include <asm/hardirq.h>
>  #include <asm/setup.h>
>  
> +/*
> + * Whether v has a mapcache: PV vCPUs (whose domain-wide mapcache may still 
> be
> + * disabled, see mapcache_domain_init()).  HVM vCPUs reach everything through
> + * the directmap, which covers all physical address space for them.
> + */
> +static bool has_mapcache(const struct vcpu *v)
> +{
> +    return is_pv_vcpu(v);
> +}
> +
>  static inline struct vcpu *mapcache_current_vcpu(void)
>  {
>      struct vcpu *v = this_cpu(pgtable_vcpu);
>      struct vcpu *curr = current;
>  
> -    /*
> -     * During early boot pgtable_vcpu is not set, callers must handle NULL.
> -     * Non-PV domains don't have a mapcache, the directmap covers all 
> physical
> -     * address space.
> -     */
> -    if ( !v || !is_pv_vcpu(v) )
> +    /* During early boot pgtable_vcpu is not set, callers must handle NULL. 
> */
> +    if ( !v || !has_mapcache(v) )
>          return NULL;

(For context below: The replacement of is_pv_vcpu() here is easy to follow.)

> @@ -54,6 +60,55 @@ static inline struct vcpu *mapcache_current_vcpu(void)
>      return ACCESS_ONCE(this_cpu(pgtable_vcpu));
>  }
>  
> +/*
> + * The accounting for v's mapcache: the domain's, with *dcache set so the
> + * caller takes the domain-wide lock and TLB-epoch machinery into account.
> + */
> +static struct mapcache *vcpu_mapcache(struct vcpu *v,
> +                                      struct mapcache_domain **dcache)
> +{
> +    struct domain *d = v->domain;
> +
> +    *dcache = &d->arch.pv.mapcache;
> +    return &(*dcache)->cache;
> +}

This function only retrieves data. Can't its first parameter therefore be
pointer-to-const?

> @@ -74,12 +130,12 @@ void *map_domain_page(mfn_t mfn)
>  #endif
>  
>      v = mapcache_current_vcpu();
> -    if ( !v || !is_pv_vcpu(v) )
> +    if ( !v )
>          return mfn_to_virt(mfn_x(mfn));

I don't quite understand why this is_pv_vcpu() is okay to drop. What
mapcache_current_vcpu() returns isn't what it ran is_pv_vcpu() on. (Same
in unmap_domain_page() then, obviously.)

> @@ -249,15 +294,23 @@ void mapcache_domain_init(struct domain *d)
>  int mapcache_vcpu_init(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
> -    struct mapcache_domain *dcache = &d->arch.pv.mapcache;
> +    struct mapcache_domain *dcache;
> +    struct mapcache *cache;
>      unsigned long i;
> -    unsigned int ents = d->max_vcpus * MAPCACHE_VCPU_ENTRIES;
> -    unsigned int nr = PFN_UP(BITS_TO_LONGS(ents) * sizeof(long));
> +    unsigned int ents, nr;
>  
> -    if ( !is_pv_vcpu(v) || !dcache->inuse )
> +    if ( !has_mapcache(v) )
>          return 0;
>  
> -    if ( ents > dcache->entries )
> +    cache = vcpu_mapcache(v, &dcache);
> +    if ( !cache->inuse )
> +        /* Domain-wide mapcache disabled (fully direct-mapped build). */
> +        return 0;

While technically not wrong, this comment appears to be quite a bit
forward-looking.

> +    ents = d->max_vcpus * MAPCACHE_VCPU_ENTRIES;
> +    nr = PFN_UP(BITS_TO_LONGS(ents) * sizeof(long));

Is there a reason these cannot be the initializers of their variables
anymore?

Jan