Xen project Mailing List

Re: [Xen-devel] [PATCH] linux-2.6.18/x86: replace order-based range checking of M2P table by linear one

From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Date: Mon, 25 Jul 2011 10:19:03 -0400

Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>

Delivery-date: Mon, 25 Jul 2011 07:19:46 -0700

List-id: Xen developer discussion <xen-devel.lists.xensource.com>

On Mon, Jul 25, 2011 at 11:05:22AM +0100, Jan Beulich wrote: > The order-based approach is not only less efficient (requiring a shift > and a compare, typical generated code looking like this > > mov eax, [machine_to_phys_order] > mov ecx, eax > shr ebx, cl > test ebx, ebx > jnz ... > > whereas a direct check requires just a compare, like in > > cmp ebx, [machine_to_phys_nr] > jae ... > > ), but also slightly dangerous in the 32-on-64 case - the element > address calculation can wrap if the next power of two boundary is > sufficiently far away from the actual upper limit of the table, and > hence can result in user space addresses being accessed (with it being > unknown what may actually be mapped there). You wouldn't have a patch for upstream Linux for this? > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxxxx> > > --- a/arch/i386/mach-xen/setup.c > +++ b/arch/i386/mach-xen/setup.c > @@ -92,13 +92,12 @@ extern void nmi(void); > > unsigned long *machine_to_phys_mapping = (void *)MACH2PHYS_VIRT_START; > EXPORT_SYMBOL(machine_to_phys_mapping); > -unsigned int machine_to_phys_order; > -EXPORT_SYMBOL(machine_to_phys_order); > +unsigned long machine_to_phys_nr; > +EXPORT_SYMBOL(machine_to_phys_nr); > > void __init pre_setup_arch_hook(void) > { > struct xen_machphys_mapping mapping; > - unsigned long machine_to_phys_nr_ents; > struct xen_platform_parameters pp; > > init_mm.pgd = swapper_pg_dir = (pgd_t *)xen_start_info->pt_base; > @@ -112,10 +111,13 @@ void __init pre_setup_arch_hook(void) > > if (HYPERVISOR_memory_op(XENMEM_machphys_mapping, &mapping) == 0) { > machine_to_phys_mapping = (unsigned long *)mapping.v_start; > - machine_to_phys_nr_ents = mapping.max_mfn + 1; > + machine_to_phys_nr = mapping.max_mfn + 1; > } else > - machine_to_phys_nr_ents = MACH2PHYS_NR_ENTRIES; > - machine_to_phys_order = fls(machine_to_phys_nr_ents - 1); > + machine_to_phys_nr = MACH2PHYS_NR_ENTRIES; > + if (machine_to_phys_mapping + machine_to_phys_nr > + < machine_to_phys_mapping) > + machine_to_phys_nr = (unsigned long *)NULL > + - machine_to_phys_mapping; > > if (!xen_feature(XENFEAT_auto_translated_physmap)) > phys_to_machine_mapping = > --- a/arch/x86_64/kernel/head64-xen.c > +++ b/arch/x86_64/kernel/head64-xen.c > @@ -92,13 +92,12 @@ static void __init setup_boot_cpu_data(v > #include <xen/interface/memory.h> > unsigned long *machine_to_phys_mapping; > EXPORT_SYMBOL(machine_to_phys_mapping); > -unsigned int machine_to_phys_order; > -EXPORT_SYMBOL(machine_to_phys_order); > +unsigned long machine_to_phys_nr; > +EXPORT_SYMBOL(machine_to_phys_nr); > > void __init x86_64_start_kernel(char * real_mode_data) > { > struct xen_machphys_mapping mapping; > - unsigned long machine_to_phys_nr_ents; > char *s; > int i; > > @@ -112,13 +111,11 @@ void __init x86_64_start_kernel(char * r > xen_start_info->nr_pt_frames; > > machine_to_phys_mapping = (unsigned long *)MACH2PHYS_VIRT_START; > - machine_to_phys_nr_ents = MACH2PHYS_NR_ENTRIES; > + machine_to_phys_nr = MACH2PHYS_NR_ENTRIES; > if (HYPERVISOR_memory_op(XENMEM_machphys_mapping, &mapping) == 0) { > machine_to_phys_mapping = (unsigned long *)mapping.v_start; > - machine_to_phys_nr_ents = mapping.max_mfn + 1; > + machine_to_phys_nr = mapping.max_mfn + 1; > } > - while ((1UL << machine_to_phys_order) < machine_to_phys_nr_ents ) > - machine_to_phys_order++; > > #if 0 > for (i = 0; i < 256; i++) > --- a/arch/x86_64/mm/init-xen.c > +++ b/arch/x86_64/mm/init-xen.c > @@ -1155,7 +1155,7 @@ int kern_addr_valid(unsigned long addr) > */ > if (addr >= (unsigned long)machine_to_phys_mapping && > addr < (unsigned long)(machine_to_phys_mapping + > - (1UL << machine_to_phys_order))) > + machine_to_phys_nr)) > return 1; > if (addr >= HYPERVISOR_VIRT_START && addr < HYPERVISOR_VIRT_END) > return 0; > --- a/include/asm-i386/mach-xen/asm/maddr.h > +++ b/include/asm-i386/mach-xen/asm/maddr.h > @@ -25,7 +25,7 @@ extern unsigned long max_mapnr; > > #undef machine_to_phys_mapping > extern unsigned long *machine_to_phys_mapping; > -extern unsigned int machine_to_phys_order; > +extern unsigned long machine_to_phys_nr; > > static inline unsigned long pfn_to_mfn(unsigned long pfn) > { > @@ -50,7 +50,7 @@ static inline unsigned long mfn_to_pfn(u > if (xen_feature(XENFEAT_auto_translated_physmap)) > return mfn; > > - if (unlikely((mfn >> machine_to_phys_order) != 0)) > + if (unlikely(mfn >= machine_to_phys_nr)) > return max_mapnr; > > /* The array access can fail (e.g., device space beyond end of RAM). */ > --- a/include/asm-x86_64/mach-xen/asm/maddr.h > +++ b/include/asm-x86_64/mach-xen/asm/maddr.h > @@ -19,7 +19,7 @@ extern unsigned long *phys_to_machine_ma > > #undef machine_to_phys_mapping > extern unsigned long *machine_to_phys_mapping; > -extern unsigned int machine_to_phys_order; > +extern unsigned long machine_to_phys_nr; > > static inline unsigned long pfn_to_mfn(unsigned long pfn) > { > @@ -44,7 +44,7 @@ static inline unsigned long mfn_to_pfn(u > if (xen_feature(XENFEAT_auto_translated_physmap)) > return mfn; > > - if (unlikely((mfn >> machine_to_phys_order) != 0)) > + if (unlikely(mfn >= machine_to_phys_nr)) > return end_pfn; > > /* The array access can fail (e.g., device space beyond end of RAM). */ > > > The order-based approach is not only less efficient (requiring a shift > and a compare, typical generated code looking like this > > mov eax, [machine_to_phys_order] > mov ecx, eax > shr ebx, cl > test ebx, ebx > jnz ... > > whereas a direct check requires just a compare, like in > > cmp ebx, [machine_to_phys_nr] > jae ... > > ), but also slightly dangerous in the 32-on-64 case - the element > address calculation can wrap if the next power of two boundary is > sufficiently far away from the actual upper limit of the table, and > hence can result in user space addresses being accessed (with it being > unknown what may actually be mapped there). > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxxxx> > > --- a/arch/i386/mach-xen/setup.c > +++ b/arch/i386/mach-xen/setup.c > @@ -92,13 +92,12 @@ extern void nmi(void); > > unsigned long *machine_to_phys_mapping = (void *)MACH2PHYS_VIRT_START; > EXPORT_SYMBOL(machine_to_phys_mapping); > -unsigned int machine_to_phys_order; > -EXPORT_SYMBOL(machine_to_phys_order); > +unsigned long machine_to_phys_nr; > +EXPORT_SYMBOL(machine_to_phys_nr); > > void __init pre_setup_arch_hook(void) > { > struct xen_machphys_mapping mapping; > - unsigned long machine_to_phys_nr_ents; > struct xen_platform_parameters pp; > > init_mm.pgd = swapper_pg_dir = (pgd_t *)xen_start_info->pt_base; > @@ -112,10 +111,13 @@ void __init pre_setup_arch_hook(void) > > if (HYPERVISOR_memory_op(XENMEM_machphys_mapping, &mapping) == 0) { > machine_to_phys_mapping = (unsigned long *)mapping.v_start; > - machine_to_phys_nr_ents = mapping.max_mfn + 1; > + machine_to_phys_nr = mapping.max_mfn + 1; > } else > - machine_to_phys_nr_ents = MACH2PHYS_NR_ENTRIES; > - machine_to_phys_order = fls(machine_to_phys_nr_ents - 1); > + machine_to_phys_nr = MACH2PHYS_NR_ENTRIES; > + if (machine_to_phys_mapping + machine_to_phys_nr > + < machine_to_phys_mapping) > + machine_to_phys_nr = (unsigned long *)NULL > + - machine_to_phys_mapping; > > if (!xen_feature(XENFEAT_auto_translated_physmap)) > phys_to_machine_mapping = > --- a/arch/x86_64/kernel/head64-xen.c > +++ b/arch/x86_64/kernel/head64-xen.c > @@ -92,13 +92,12 @@ static void __init setup_boot_cpu_data(v > #include <xen/interface/memory.h> > unsigned long *machine_to_phys_mapping; > EXPORT_SYMBOL(machine_to_phys_mapping); > -unsigned int machine_to_phys_order; > -EXPORT_SYMBOL(machine_to_phys_order); > +unsigned long machine_to_phys_nr; > +EXPORT_SYMBOL(machine_to_phys_nr); > > void __init x86_64_start_kernel(char * real_mode_data) > { > struct xen_machphys_mapping mapping; > - unsigned long machine_to_phys_nr_ents; > char *s; > int i; > > @@ -112,13 +111,11 @@ void __init x86_64_start_kernel(char * r > xen_start_info->nr_pt_frames; > > machine_to_phys_mapping = (unsigned long *)MACH2PHYS_VIRT_START; > - machine_to_phys_nr_ents = MACH2PHYS_NR_ENTRIES; > + machine_to_phys_nr = MACH2PHYS_NR_ENTRIES; > if (HYPERVISOR_memory_op(XENMEM_machphys_mapping, &mapping) == 0) { > machine_to_phys_mapping = (unsigned long *)mapping.v_start; > - machine_to_phys_nr_ents = mapping.max_mfn + 1; > + machine_to_phys_nr = mapping.max_mfn + 1; > } > - while ((1UL << machine_to_phys_order) < machine_to_phys_nr_ents ) > - machine_to_phys_order++; > > #if 0 > for (i = 0; i < 256; i++) > --- a/arch/x86_64/mm/init-xen.c > +++ b/arch/x86_64/mm/init-xen.c > @@ -1155,7 +1155,7 @@ int kern_addr_valid(unsigned long addr) > */ > if (addr >= (unsigned long)machine_to_phys_mapping && > addr < (unsigned long)(machine_to_phys_mapping + > - (1UL << machine_to_phys_order))) > + machine_to_phys_nr)) > return 1; > if (addr >= HYPERVISOR_VIRT_START && addr < HYPERVISOR_VIRT_END) > return 0; > --- a/include/asm-i386/mach-xen/asm/maddr.h > +++ b/include/asm-i386/mach-xen/asm/maddr.h > @@ -25,7 +25,7 @@ extern unsigned long max_mapnr; > > #undef machine_to_phys_mapping > extern unsigned long *machine_to_phys_mapping; > -extern unsigned int machine_to_phys_order; > +extern unsigned long machine_to_phys_nr; > > static inline unsigned long pfn_to_mfn(unsigned long pfn) > { > @@ -50,7 +50,7 @@ static inline unsigned long mfn_to_pfn(u > if (xen_feature(XENFEAT_auto_translated_physmap)) > return mfn; > > - if (unlikely((mfn >> machine_to_phys_order) != 0)) > + if (unlikely(mfn >= machine_to_phys_nr)) > return max_mapnr; > > /* The array access can fail (e.g., device space beyond end of RAM). */ > --- a/include/asm-x86_64/mach-xen/asm/maddr.h > +++ b/include/asm-x86_64/mach-xen/asm/maddr.h > @@ -19,7 +19,7 @@ extern unsigned long *phys_to_machine_ma > > #undef machine_to_phys_mapping > extern unsigned long *machine_to_phys_mapping; > -extern unsigned int machine_to_phys_order; > +extern unsigned long machine_to_phys_nr; > > static inline unsigned long pfn_to_mfn(unsigned long pfn) > { > @@ -44,7 +44,7 @@ static inline unsigned long mfn_to_pfn(u > if (xen_feature(XENFEAT_auto_translated_physmap)) > return mfn; > > - if (unlikely((mfn >> machine_to_phys_order) != 0)) > + if (unlikely(mfn >= machine_to_phys_nr)) > return end_pfn; > > /* The array access can fail (e.g., device space beyond end of RAM). */ > _______________________________________________ > Xen-devel mailing list > Xen-devel@xxxxxxxxxxxxxxxxxxx > http://lists.xensource.com/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.