Re: [PATCH] x86: idle domains don't have a domain-page mapcache

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 05/01/2023 11:09 am, Jan Beulich wrote:
> First and foremost correct a comment implying the opposite. Then, to
> make things more clear PV-vs-HVM-wise, move the PV check earlier in the
> function, making it unnecessary for both callers to perform the check
> individually. Finally return NULL from the function when using the idle
> domain's page tables, allowing a dcache->inuse check to also become an
> assertion.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

By and large, I think these changes ok, but I want to make an argument
for going further right away.


Most of mapcache_current_vcpu() is a giant bodge to support the weird
context in dom0_construct_pv(), but we pay the price unilaterally.

By updating current too in that weird context, I think we can drop
mapcache_override_current().  It's also worth noting that the only
callers are __init so having this_cpu(override) as per-cpu is a waste.

But I also think we can drop the pagetable_is_null(v->arch.guest_table)
part too.  If we happen to be half-idle, it doesn't matter if we use the
current mapcache by following cpu_info()->current instead.  That said, I
can't think of any case where we could legally access transient mappings
from an idle context, so I'd be tempted to see if we can simply drop
that clause.


Overall, I think the logic would benefit from being expressed in terms
of short_directmap, just like init_xen_l4_slots().  That is ultimately
the property that we care about, and it's also the property that is
changing in the effort to remove the directmap entirely.

If not short_directmap, then at least having the property expressed
consistently between the two piece of code, seeing as it is the same
property.

~Andrew