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

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 08/16/2011 07:07 AM, 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).
>
> 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.

I never really understood the rationale for the order stuff; I copied it
across from 2.6.18-xen without really thinking about it.  This looks
sensible.

But...

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

I'd prefer extra parens around the + just to make it very clear.  Is
this kind of overflow check fully defined, or could the compiler find
some way of screwing it up?

> +		machine_to_phys_nr = (unsigned long *)NULL
> +				     - machine_to_phys_mapping;

Is the machine_to_phys_mapping array guaranteed to end at the end of the
address space?  And I think a literal '0' there would make it a bit
clearer what's going on, rather than invoking all the stuff that NULL
implies.

Thanks,
    J