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

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

On Tue, Oct 13, 2026 at 12:57 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 13.10.2026 11:53, George Dunlap wrote:
> > On Tue, Oct 13, 2026 at 7:26 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 12.10.2026 18:07, George Dunlap wrote:
> >>> On Mon, Oct 12, 2026 at 2:48 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>>> @@ -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,
> >>
> >> I was going to say "But there's no returning of NULL there if
> >> pgtable_vcpu is HVM, at least not at this point of the series." Only
> >> then I realized that I was being confused by there being both v and
> >> curr, and neither of them being pointer-to-const (i.e. both being
> >> equal in terms of which of the two might be returned from the function,
> >> yet as a result of earlier changes neither is being returned anymore).
> >>
> >>> 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.
> >>
> >> Yet then - aren't the is_pv_vcpu() you drop here redundant already
> >> before this patch/series? In which case (since it's not entirely
> >> straightforward to reason about and beneficial in its own right),
> >> might it make sense to have this be a separate change, with (in
> >> particular) dedicated justification? (ISTR having thought about
> >> making a patch to this effect a little while ago - perhaps around when
> >> Roger last touched this logic -, but then deciding that something was
> >> wrong about my reasoning. That may have been a mistake, as it looks
> >> now.)
> >>
> >>> 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.
> >>
> >> Sounds entirely plausible, but can as well be done as cleanup later.
> >
> > I'm a bit confused about what, if any, change you're requesting here.
> > Are you requesting a preparatory patch just removing the is_pv_vcpu(),
> > with an argument as to why it's redundant?  Or are you OK with having
> > this cleanup work be put until later?  Or do you want me to put the
> > is_pv_vcpu() back until we feel like cleaning it up properly?
>
> Now the confusion is on my side: Aren't what you suggested as further
> cleanup and the dropping of the is_pv_vcpu() independent? If it had
> been just your first question, I would have said "yes, if possible".
> But the following question starting with "Or" is irritating. Yes,
> that _other_ cleanup work can (as I thought you also suggested) be
> deferred.
>
> Putting back the is_pv_vcpu() isn't really necessary, as long as
> their removal can be properly justified. (Hence why I think that would
> be better as a separate change.)

I've added a new patch.

 -George