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

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Mon, Oct 12, 2026 at 03:19:20PM +0100, George Dunlap wrote:
> 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?

It's sometimes hard to know whether a comment is in an intermediate form,
or in the final one, hence it's difficult to trim review to not be
picky about intermittent state once the final form might be
acceptable.  That's why I've suggested to try to add comments in its
final form (or in a form that just need expansion by later changes).

I would maybe use:

/* Whether the vCPU supports mapcache usage, might not be in use. */

Personally: referencing functions by name in code comments is also
something that I usually try to avoid doing if possible.  As the code
evolves it's not unlikely for functions to change names, and we have
(historically) not been very good at keeping the referencing comments
up to date.

Thanks, Roger.