Re: [PATCH v3 11/18] x86/hvm: introduce per-vCPU mapcache for vCPU-PT domains

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

On Thu, Oct 8, 2026 at 2:56 PM Roger Pau Monné <roger@xxxxxxxxxxxxxx> wrote:
> > Unlike a domain-wide mapcache, a per-vCPU one cannot, by design,
> > borrow slots from other vCPUs: MAPCACHE_VCPU_ENTRIES (16) is the most
> > single-page mappings such a vCPU can hold at once, interrupt handlers'
> > included, and one more hits the BUG_ON() in map_domain_page(). The PoD
> > zero-page sweep held up to 16 on top of its callers' until "x86/PoD: map
> > one page at a time in p2m_pod_zero_check()".
>
> Is it really the best approach to keep the same limit?  As you note,
> with the mapcache being per-vcpu the limit of 16 active mappings is
> going to be strictly enforce, which it wasn't before.
>
> The per-vcpu mapcache area is a whole L1 page-directory (it cannot be
> smaller), so that's 512 entries per-vcpu, of which we limit ourselves
> to 16.
>
> Can we consider using bigger per-vcpu mapcaches, maybe 512 is overkill
> for the bitmap sizes, but using greater than 16 will likely avoid
> frequent TLB flushes to re-use the entries.  Maybe 64 entries, so the
> bitmap is still a single unsigned long on 64bit arches.  Note that
> declare bitmap will already use at least an unsigned long for the
> bitmap itself.

Yes, I think that's a good idea.  I'd thought about extending the
mapcaches since they'd be now per-vCPU, but didn't want to change two
things at once; but we're already changing 2 things at once by taking
away the overflow mechanism.  64 bits is a good number, given that
we've got anunsigned long "bitmap" anyway.

> > @@ -191,7 +207,12 @@ void *map_domain_page(mfn_t mfn)
> >          }
> >          BUG_ON(idx >= cache->entries);
> >
> > -        /* /Second/, flush TLBs. */
> > +        /*
> > +         * /Second/, flush TLBs.  A per-vCPU mapcache needs a local flush
> > +         * only: its entries are never _PAGE_GLOBAL, and this vCPU's
> > +         * page-tables can only have been loaded on another pCPU via
> > +         * switch_cr3_cr4(), which flushes non-global mappings.
> > +         */
>
> Shouldn't the above comment be inside of mapcache_new_epoch()?  It's a
> bit disconnected from the code here, as apparently there's no
> different handling here between per-domain and per-vcpu mapcaches.

Ack

>
> >          perfc_incr(domain_page_tlb_flush);
> >          flush_tlb_local();
> >          mapcache_new_epoch(dcache, vcache);
> > @@ -313,11 +334,19 @@ int mapcache_vcpu_init(struct vcpu *v)
> >          return 0;
> >
> >      cache = vcpu_mapcache(v, &dcache);
> > -    if ( !cache->inuse )
> > +    if ( !dcache )
> > +    {
> > +        /* Per-vCPU bitmaps are a word each: they live in the vCPU itself. 
> > */
> > +        cache->inuse = v->arch.mapcache.inuse;
> > +        cache->garbage = v->arch.mapcache.garbage;
>
> Well, the size of the bitmap is set to 16 bits, but due to the usage of
> DECLARE_BITMAP() the underlying type is unsigned long, so it's
> effectively using 64 bits (a quad).

I'll adjust the comment.

 -George