Re: [PATCH] xen/x86: replace order-based range checking of M2P table by linear one

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Aug 16, 2011 at 03:07:41PM +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).

Ugh. OK, let me queue it up for 3.1 - this also sounds like a good
candidate for stable trees.

> 
> Additionally, the elimination of the mistaken use of fls() here (should
> have been __fls()) fixes a latent issue on x86-64 that would trigger
> if the code was run on a system with memory extending beyond the 44-bit
> boundary.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> ---
>  arch/x86/include/asm/xen/page.h |    4 ++--
>  arch/x86/xen/enlighten.c        |    4 ++--
>  arch/x86/xen/mmu.c              |   12 ++++++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> --- 3.1-rc2/arch/x86/include/asm/xen/page.h
> +++ 3.1-rc2-x86-xen-p2m-nr/arch/x86/include/asm/xen/page.h
> @@ -39,7 +39,7 @@ typedef struct xpaddr {
>      ((unsigned long)((u64)CONFIG_XEN_MAX_DOMAIN_MEMORY * 1024 * 1024 * 1024 / PAGE_SIZE))
>  
>  extern unsigned long *machine_to_phys_mapping;
> -extern unsigned int   machine_to_phys_order;
> +extern unsigned long  machine_to_phys_nr;
>  
>  extern unsigned long get_phys_to_machine(unsigned long pfn);
>  extern bool set_phys_to_machine(unsigned long pfn, unsigned long mfn);
> @@ -87,7 +87,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)) {
>  		pfn = ~0;
>  		goto try_override;
>  	}
> --- 3.1-rc2/arch/x86/xen/enlighten.c
> +++ 3.1-rc2-x86-xen-p2m-nr/arch/x86/xen/enlighten.c
> @@ -77,8 +77,8 @@ EXPORT_SYMBOL_GPL(xen_domain_type);
>  
>  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);
>  
>  struct start_info *xen_start_info;
>  EXPORT_SYMBOL_GPL(xen_start_info);
> --- 3.1-rc2/arch/x86/xen/mmu.c
> +++ 3.1-rc2-x86-xen-p2m-nr/arch/x86/xen/mmu.c
> @@ -1713,15 +1713,19 @@ static void __init xen_map_identity_earl
>  void __init xen_setup_machphys_mapping(void)
>  {
>  	struct xen_machphys_mapping mapping;
> -	unsigned long machine_to_phys_nr_ents;
>  
>  	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_nr = MACH2PHYS_NR_ENTRIES;
>  	}
> -	machine_to_phys_order = fls(machine_to_phys_nr_ents - 1);
> +#ifdef CONFIG_X86_32
> +	if (machine_to_phys_mapping + machine_to_phys_nr
> +	    < machine_to_phys_mapping)
> +		machine_to_phys_nr = (unsigned long *)NULL
> +				     - machine_to_phys_mapping;
> +#endif
>  }
>  
>  #ifdef CONFIG_X86_64
> 
> 
> 

> 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).
> 
> Additionally, the elimination of the mistaken use of fls() here (should
> have been __fls()) fixes a latent issue on x86-64 that would trigger
> if the code was run on a system with memory extending beyond the 44-bit
> boundary.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> ---
>  arch/x86/include/asm/xen/page.h |    4 ++--
>  arch/x86/xen/enlighten.c        |    4 ++--
>  arch/x86/xen/mmu.c              |   12 ++++++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> --- 3.1-rc2/arch/x86/include/asm/xen/page.h
> +++ 3.1-rc2-x86-xen-p2m-nr/arch/x86/include/asm/xen/page.h
> @@ -39,7 +39,7 @@ typedef struct xpaddr {
>      ((unsigned long)((u64)CONFIG_XEN_MAX_DOMAIN_MEMORY * 1024 * 1024 * 1024 / PAGE_SIZE))
>  
>  extern unsigned long *machine_to_phys_mapping;
> -extern unsigned int   machine_to_phys_order;
> +extern unsigned long  machine_to_phys_nr;
>  
>  extern unsigned long get_phys_to_machine(unsigned long pfn);
>  extern bool set_phys_to_machine(unsigned long pfn, unsigned long mfn);
> @@ -87,7 +87,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)) {
>  		pfn = ~0;
>  		goto try_override;
>  	}
> --- 3.1-rc2/arch/x86/xen/enlighten.c
> +++ 3.1-rc2-x86-xen-p2m-nr/arch/x86/xen/enlighten.c
> @@ -77,8 +77,8 @@ EXPORT_SYMBOL_GPL(xen_domain_type);
>  
>  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);
>  
>  struct start_info *xen_start_info;
>  EXPORT_SYMBOL_GPL(xen_start_info);
> --- 3.1-rc2/arch/x86/xen/mmu.c
> +++ 3.1-rc2-x86-xen-p2m-nr/arch/x86/xen/mmu.c
> @@ -1713,15 +1713,19 @@ static void __init xen_map_identity_earl
>  void __init xen_setup_machphys_mapping(void)
>  {
>  	struct xen_machphys_mapping mapping;
> -	unsigned long machine_to_phys_nr_ents;
>  
>  	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_nr = MACH2PHYS_NR_ENTRIES;
>  	}
> -	machine_to_phys_order = fls(machine_to_phys_nr_ents - 1);
> +#ifdef CONFIG_X86_32
> +	if (machine_to_phys_mapping + machine_to_phys_nr
> +	    < machine_to_phys_mapping)
> +		machine_to_phys_nr = (unsigned long *)NULL
> +				     - machine_to_phys_mapping;
> +#endif
>  }
>  
>  #ifdef CONFIG_X86_64

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel