Re: [Xen-devel] [PATCH v3 05/13] xen/arm: early_ioremap: allocate virtual addresses from top to bottom

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-04-24 at 20:07 +0100, Stefano Stabellini wrote:

... because ...

(I know now, but other people don't, and I may not remember in six
months time)

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  xen/arch/arm/mm.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index ba3140d..bd7baaf 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -433,15 +433,17 @@ void __init setup_frametable_mappings(paddr_t ps, 
> paddr_t pe)
>   */
>  void* __init early_ioremap(paddr_t start, size_t len, unsigned attributes)
>  {
> -    static unsigned long virt_start = EARLY_VMAP_VIRT_START;
> -    unsigned long ret_addr = virt_start;
> +    static unsigned long virt_start = EARLY_VMAP_VIRT_END;
>      paddr_t end = start + len;
>  
> +    len = ((len + SECOND_SIZE - 1) >> SECOND_SHIFT) << SECOND_SHIFT;

These shifts are equivalent to & ~SECOND_MASK. Or you could use
DIV_ROUND_UP + one shift.

> +    virt_start -= len;
> +
>      ASSERT(!(start & (~SECOND_MASK)));
>      ASSERT(!(virt_start & (~SECOND_MASK)));
>  
>      /* The range we need to map is too big */
> -    if ( virt_start + len >= EARLY_VMAP_VIRT_END )
> +    if ( virt_start >= EARLY_VMAP_VIRT_START )

Needs to be < doesn't it?

>          return NULL;
>  
>      while ( start < end )
> @@ -453,9 +455,10 @@ void* __init early_ioremap(paddr_t start, size_t len, 
> unsigned attributes)
>          start += SECOND_SIZE;
>          virt_start += SECOND_SIZE;
>      }
> -    flush_xen_data_tlb_range_va(ret_addr, len);
> +    virt_start -= len;

You've done this twice now (once right after my comment about the
shifts).

Perhaps this is because of the virt_start manipulations inside the loop,
but in that case I think that loop needs rewriting with its own variable
etc etc, having virt_start go up and down like a yo-yo in this function
is just begging to confuse people.

Actually, if you drop the first  virt_start -= len and make the range
check
        if ( virt_start - len < EARLY..  )
then you can invert the loop to map backwards and decrement virt_start
and end as you go along. That would work too IMHO (at least so far as I
have one when just imagining what it would look like).

> +    flush_xen_data_tlb_range_va(virt_start, len);
>  
> -    return (void*)ret_addr;
> +    return (void*)virt_start;
>  }
>  
>  enum mg { mg_clear, mg_ro, mg_rw, mg_rx };



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