[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH V3 (resend) 09/19] x86/domain_page: Remove the fast paths when mfn is not in the directmap
On Mon, May 13, 2024 at 01:40:36PM +0000, Elias El Yandouzi wrote: > From: Hongyan Xia <hongyxia@xxxxxxxxxx> > > When mfn is not in direct map, never use mfn_to_virt for any mappings. > > We replace mfn_x(mfn) <= PFN_DOWN(__pa(HYPERVISOR_VIRT_END - 1)) with > arch_mfns_in_direct_map(mfn, 1) because these two are equivalent. The > extra comparison in arch_mfns_in_direct_map() looks different but because > DIRECTMAP_VIRT_END is always higher, it does not make any difference. > > Lastly, domain_page_map_to_mfn() needs to gain to a special case for > the PMAP. > > Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx> > > ---- > > Changes since Hongyan's version: > * arch_mfn_in_direct_map() was renamed to arch_mfns_in_directmap() > * add a special case for the PMAP in domain_page_map_to_mfn() > > diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c > index 55e337aaf7..89caefc8a2 100644 > --- a/xen/arch/x86/domain_page.c > +++ b/xen/arch/x86/domain_page.c > @@ -14,8 +14,10 @@ > #include <xen/sched.h> > #include <xen/vmap.h> > #include <asm/current.h> > +#include <asm/fixmap.h> > #include <asm/flushtlb.h> > #include <asm/hardirq.h> > +#include <asm/pmap.h> > #include <asm/setup.h> > > static DEFINE_PER_CPU(struct vcpu *, override); > @@ -35,10 +37,11 @@ static inline struct vcpu *mapcache_current_vcpu(void) > /* > * When using efi runtime page tables, we have the equivalent of the idle > * domain's page tables but current may point at another domain's VCPU. > - * Return NULL as though current is not properly set up yet. > + * Return the idle domains's vcpu on that core because the efi per-domain > + * region (where the mapcache is) is in-sync with the idle domain. > */ > if ( efi_rs_using_pgtables() ) > - return NULL; > + return idle_vcpu[smp_processor_id()]; There's already an existing instance of idle_vcpu[smp_processor_id()] down in the function, it might make sense to put this in a local variable. > > /* > * If guest_table is NULL, and we are running a paravirtualised guest, > @@ -77,18 +80,24 @@ void *map_domain_page(mfn_t mfn) > struct vcpu_maphash_entry *hashent; > > #ifdef NDEBUG > - if ( mfn_x(mfn) <= PFN_DOWN(__pa(HYPERVISOR_VIRT_END - 1)) ) > + if ( arch_mfns_in_directmap(mfn_x(mfn), 1) ) > return mfn_to_virt(mfn_x(mfn)); > #endif > > v = mapcache_current_vcpu(); > - if ( !v ) > - return mfn_to_virt(mfn_x(mfn)); > + if ( !v || !v->domain->arch.mapcache.inuse ) > + { > + if ( arch_mfns_in_directmap(mfn_x(mfn), 1) ) > + return mfn_to_virt(mfn_x(mfn)); > + else > + { > + BUG_ON(system_state >= SYS_STATE_smp_boot); > + return pmap_map(mfn); > + } > + } > > dcache = &v->domain->arch.mapcache; > vcache = &v->arch.mapcache; > - if ( !dcache->inuse ) > - return mfn_to_virt(mfn_x(mfn)); > > perfc_incr(map_domain_page_count); > > @@ -184,6 +193,12 @@ void unmap_domain_page(const void *ptr) > if ( !va || va >= DIRECTMAP_VIRT_START ) > return; > > + if ( va >= FIXADDR_START && va < FIXADDR_TOP ) This should be a fixmap helper IMO. virt_is_fixmap(addr) or similar. There's already an existing instance in virt_to_fix(). > + { > + pmap_unmap((void *)ptr); > + return; > + } > + > ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END); > > v = mapcache_current_vcpu(); > @@ -237,7 +252,7 @@ int mapcache_domain_init(struct domain *d) > unsigned int bitmap_pages; > > #ifdef NDEBUG > - if ( !mem_hotplug && max_page <= PFN_DOWN(__pa(HYPERVISOR_VIRT_END - 1)) > ) > + if ( !mem_hotplug && arch_mfn_in_directmap(0, max_page) ) > return 0; > #endif > > @@ -308,7 +323,7 @@ void *map_domain_page_global(mfn_t mfn) > local_irq_is_enabled())); > > #ifdef NDEBUG > - if ( mfn_x(mfn) <= PFN_DOWN(__pa(HYPERVISOR_VIRT_END - 1)) ) > + if ( arch_mfn_in_directmap(mfn_x(mfn, 1)) ) > return mfn_to_virt(mfn_x(mfn)); > #endif > > @@ -335,6 +350,23 @@ mfn_t domain_page_map_to_mfn(const void *ptr) > if ( va >= DIRECTMAP_VIRT_START ) > return _mfn(virt_to_mfn(ptr)); > > + /* > + * The fixmap is stealing the top-end of the VMAP. So the check for > + * the PMAP *must* happen first. > + * > + * Also, the fixmap translate a slot to an address backwards. The > + * logic will rely on it to avoid any complexity. So check at > + * compile time this will always hold. > + */ > + BUILD_BUG_ON(fix_to_virt(FIX_PMAP_BEGIN) < fix_to_virt(FIX_PMAP_END)); > + > + if ( ((unsigned long)fix_to_virt(FIX_PMAP_END) <= va) && > + ((va & PAGE_MASK) <= (unsigned long)fix_to_virt(FIX_PMAP_BEGIN)) ) > + { Can we place this as some kind of helper in fixmap.h? It's already quite ugly, and could be useful in other places. bool virt_in_fixmap_range(addr, start idx, end idx) Or something similar. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |