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
|
Home