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

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Wed, Oct 07, 2026 at 11:40:41AM +0100, 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.

I've been wondering whether there could be a way to write this comment
so that it doesn't need rewording once we switch to per-vCPU
mapcaches, as to avoid adding a comment here that will very likely be
completely re-worded by a later change in the series.  However I
cannot find anything much better rather than stripping most of the
comment content.

> + */
> +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;
>  
>      /*
> @@ -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;
> +}
> +
> +/*
> + * The domain-wide mapcache's lock and TLB-epoch machinery, taken around
> + * allocating a slot: the domain's vCPUs share the mapcache's slots, and each
> + * catches up with a wrap another one caused, flushing its TLB if it has not
> + * been flushed since.

Thinking forward about how this function will end up looking when we
have per-vCPU map caches, do you maybe want to place this comment
inside the function body itself?

I expect per-vCPU splitting will add a short-circuit at the top of the
function that will skip taking the lock or doing the flushing, and
hence the comment won't need shuffling around?

> + */
> +static void mapcache_lock(struct mapcache_domain *dcache,

Maybe mapcache_enter() would be a better suited name once there's no
longer a need to take a lock for the mapcache?  Just wondering, no
strong opinion really (the more that I don't even know how the code
will end up looking yet).

> +                          struct mapcache_vcpu *vcache)
> +{
> +    spin_lock(&dcache->lock);
> +
> +    /* Has some other CPU caused a wrap? We must flush if so. */
> +    if ( unlikely(dcache->epoch != vcache->shadow_epoch) )
> +    {
> +        vcache->shadow_epoch = dcache->epoch;
> +        if ( NEED_FLUSH(this_cpu(tlbflush_time), dcache->tlbflush_timestamp) 
> )
> +        {
> +            perfc_incr(domain_page_tlb_flush);
> +            flush_tlb_local();
> +        }
> +    }
> +}
> +
> +/* A wrap has just flushed this CPU's TLB: start a new epoch. */
> +static void mapcache_new_epoch(struct mapcache_domain *dcache,
> +                               struct mapcache_vcpu *vcache)
> +{

Since this is now a function on its own, I would possibly assert that
dcache->lock is taken on entry.

Let me know what you think of the suggestions above, overall I think
the logic is fine.

Thanks, Roger.