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@suse.com>
--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -28,8 +28,11 @@ static inline struct vcpu *mapcache_curr
/*
* When current isn't properly set up yet, this is equivalent to
* running in an idle vCPU (callers must check for NULL).
+ *
+ * Non-PV domains don't have any mapcache. For idle domains (which
+ * appear to be PV but also have no mapcache) see below.
*/
- if ( !v )
+ if ( !v || !is_pv_vcpu(v) )
return NULL;
/*
@@ -41,19 +44,22 @@ static inline struct vcpu *mapcache_curr
return NULL;
/*
- * If guest_table is NULL, and we are running a paravirtualised guest,
- * then it means we are running on the idle domain's page table and must
- * therefore use its mapcache.
+ * If guest_table is NULL for a PV domain (which includes IDLE), then it
+ * means we are running on the idle domain's page tables and therefore
+ * must not use any mapcache.
*/
- if ( unlikely(pagetable_is_null(v->arch.guest_table)) && is_pv_vcpu(v) )
+ if ( unlikely(pagetable_is_null(v->arch.guest_table)) )
{
/* If we really are idling, perform lazy context switch now. */
- if ( (v = idle_vcpu[smp_processor_id()]) == current )
+ if ( idle_vcpu[smp_processor_id()] == current )
sync_local_execstate();
/* We must now be running on the idle page table. */
ASSERT(cr3_pa(read_cr3()) == __pa(idle_pg_table));
+ return NULL;
}
+ ASSERT(!is_idle_vcpu(v));
+
return v;
}
@@ -82,13 +88,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));
dcache = &v->domain->arch.pv.mapcache;
vcache = &v->arch.pv.mapcache;
- if ( !dcache->inuse )
- return mfn_to_virt(mfn_x(mfn));
+ ASSERT(dcache->inuse);
perfc_incr(map_domain_page_count);
@@ -187,7 +192,7 @@ void unmap_domain_page(const void *ptr)
ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END);
v = mapcache_current_vcpu();
- ASSERT(v && is_pv_vcpu(v));
+ ASSERT(v);
dcache = &v->domain->arch.pv.mapcache;
ASSERT(dcache->inuse);
Hi Jan, On 05/01/2023 11:09, 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@suse.com> > > --- a/xen/arch/x86/domain_page.c > +++ b/xen/arch/x86/domain_page.c > @@ -28,8 +28,11 @@ static inline struct vcpu *mapcache_curr > /* > * When current isn't properly set up yet, this is equivalent to > * running in an idle vCPU (callers must check for NULL). > + * > + * Non-PV domains don't have any mapcache. For idle domains (which > + * appear to be PV but also have no mapcache) see below. > */ > - if ( !v ) > + if ( !v || !is_pv_vcpu(v) ) > return NULL; I am trying to figure out the implication on this change with my patch [1]. Is this expected to go before hand? If so, is this expectation that I will remove !is_pv_vcpu() and replace with a new check... > > /* > @@ -41,19 +44,22 @@ static inline struct vcpu *mapcache_curr > return NULL; ... right here? if ( !is_pv_vcpu(v) ) return v; > > /* > - * If guest_table is NULL, and we are running a paravirtualised guest, > - * then it means we are running on the idle domain's page table and must > - * therefore use its mapcache. > + * If guest_table is NULL for a PV domain (which includes IDLE), then it > + * means we are running on the idle domain's page tables and therefore > + * must not use any mapcache. > */ > - if ( unlikely(pagetable_is_null(v->arch.guest_table)) && is_pv_vcpu(v) ) > + if ( unlikely(pagetable_is_null(v->arch.guest_table)) ) > { > /* If we really are idling, perform lazy context switch now. */ > - if ( (v = idle_vcpu[smp_processor_id()]) == current ) > + if ( idle_vcpu[smp_processor_id()] == current ) > sync_local_execstate(); > /* We must now be running on the idle page table. */ > ASSERT(cr3_pa(read_cr3()) == __pa(idle_pg_table)); > + return NULL; > } > > + ASSERT(!is_idle_vcpu(v)); > + > return v; > } > > @@ -82,13 +88,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)); > > dcache = &v->domain->arch.pv.mapcache; > vcache = &v->arch.pv.mapcache; > - if ( !dcache->inuse ) > - return mfn_to_virt(mfn_x(mfn)); > + ASSERT(dcache->inuse); > > perfc_incr(map_domain_page_count); > > @@ -187,7 +192,7 @@ void unmap_domain_page(const void *ptr) > ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END); > > v = mapcache_current_vcpu(); > - ASSERT(v && is_pv_vcpu(v)); > + ASSERT(v); > > dcache = &v->domain->arch.pv.mapcache; > ASSERT(dcache->inuse); Cheers, [1] https://lore.kernel.org/xen-devel/20221216114853.8227-15-julien@xen.org -- Julien Grall
On 30.01.2023 20:00, Julien Grall wrote: > On 05/01/2023 11:09, Jan Beulich wrote: >> --- a/xen/arch/x86/domain_page.c >> +++ b/xen/arch/x86/domain_page.c >> @@ -28,8 +28,11 @@ static inline struct vcpu *mapcache_curr >> /* >> * When current isn't properly set up yet, this is equivalent to >> * running in an idle vCPU (callers must check for NULL). >> + * >> + * Non-PV domains don't have any mapcache. For idle domains (which >> + * appear to be PV but also have no mapcache) see below. >> */ >> - if ( !v ) >> + if ( !v || !is_pv_vcpu(v) ) >> return NULL; > > I am trying to figure out the implication on this change with my patch > [1]. Is this expected to go before hand? The change here may not be necessary at all with your change. The main reason I sent this patch was to augment my comments on your patch. If yours gets adjusted to account for the (perhaps just theoretical) issues (if just theoretical, then description adjustments may be all that's needed), then I may be able to drop this instead of seeing whether I can convince myself that some / all of Andrew's requests can actually safely be fulfilled. Jan
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@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
© 2016 - 2024 Red Hat, Inc.