Re: [PATCH 03/27] xen/x86: mm: Don't check alloc_boot_pages return

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 14/08/17 15:23, Julien Grall wrote:
> The only way alloc_boot_pages will return 0 is during the error case.

This statement is not true. If alloc_boot_pages() returns, it has
succeeded. Returning 0 is nothing special.

> Although, Xen will panic in the error path. So the check in the caller
> is pointless.
> 
> Looking at the loop, my understanding is it will try to allocate in
> smaller chunk if a bigger chunk fail. Given that alloc_boot_pages can
> never check, the loop seems unecessary.

Agreed, the algorithm doesn't work with (current) implementation of
alloc_boot_pages(), so the patch is valid.

> Signed-off-by: Julien Grall <julien.grall@arm.com>

Given that you adjust the commit message:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> I haven't tested this code, only build test it. I can't see how
> alloc_boot_pages would return 0 other than the error path.
> ---
>  xen/arch/x86/mm.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index f53ca43554..66e337109d 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -200,11 +200,7 @@ static void __init init_frametable_chunk(void *start, void *end)
>           */
>          while ( step && s + (step << PAGE_SHIFT) > e + (4 << PAGE_SHIFT) )
>              step >>= PAGETABLE_ORDER;
> -        do {
> -            mfn = alloc_boot_pages(step, step);
> -        } while ( !mfn && (step >>= PAGETABLE_ORDER) );
> -        if ( !mfn )
> -            panic("Not enough memory for frame table");
> +        mfn = alloc_boot_pages(step, step);
>          map_pages_to_xen(s, mfn, step, PAGE_HYPERVISOR);
>      }
>  
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel