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

[Jan Beulich <jbeulich@suse.com>]

On 13.10.2026 11:53, George Dunlap wrote:
> On Tue, Oct 13, 2026 at 7:26 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 12.10.2026 18:07, George Dunlap wrote:
>>> On Mon, Oct 12, 2026 at 2:48 PM Jan Beulich <jbeulich@suse.com> 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.)

Jan