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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan