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

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

On Thu, Oct 8, 2026 at 9:22 AM Roger Pau Monné <roger@xxxxxxxxxxxxxx> wrote:
>
> 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.

I think describing it the way it is, then changing it, makes the
series or the state thereof more clear.  But I'm not precious about
it; we could make it something like this:

/*
 * Whether v may use a mapcache rather than the directmap (a PV domain's
 * mapcache may still be disabled, see mapcache_domain_init()).
 */

That said, as a meta-comment: We have a limited amount of time on my
contract, and also a limited amount of time before we want to check
this in to the tree.  Is spending our time changing non-functional
intermittent git tree state really what you want to spend that time
on?

>
> > + */
> > +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?

Yup, we can do this.

> > + */
> > +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).

Yeah, can do that.

> > +/* 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.

Great, thanks.
 -George