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

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

On Mon, Oct 12, 2026 at 2:48 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > @@ -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?

Currently that's true; but in a further patch, it may be
&v->arch.mapcache.cache, which is in the vcpu structure itself,
effectively stripping off the const-ness by pointing to individual
fields in a struct.

>
> > @@ -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.)

So mapcache_current_vcpu() is a bit weird; the actual invariant is,
"Return the vcpu whose cache structure we should use if present; or
NULL if there's no cache structure to check and no need to sync."  So
if pgtable_vcpu is HVM, return NULL and just use the raw address,
because at this point in the series, both HVM and the idle domain have
full directmaps; if pgtable_vcpu is PV, then maybe sync, and then
return the result of the sync.  That may be a PV domain with a
mapcache, or an idle domain with no mapcache, but in either case you
can then read cache.inuse to see if there's something there.

So yes, it's a different vCPU; but it's still defined in such a way to
be safe to check cache.inuse.  The only possible value *change* after
a sync is the idle domain; I don't think we need to do another check
to make sure it's still a PV domain.

In fact, there's an argument that we should modify
mapcache_current_vcpu() to take pointers to dcache, and vcache, and
return just cache; that gets rid of v as a local variable of
map_domain_page entirely, and makes the logic ("get the cache we
should use") more clean, and gets rid of one mfn_to_virt() instance.
That would require some non-trivial changes, so I'd rather avoid that,
but I can do it if you want.

> > @@ -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.

Yes; I'll drop the last bit.

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

No reason.  I'll put them back.

 -George