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 <jbeul...@suse.com>
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
