Re: [PATCH for-4.5 v2 2/2] xen: arm: Enable physical address space compression (PDX) on arm32

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

Don't you implement PDX for both arm32 and arm64? If so, you may need to 
update the commit title.

The patch looks good to me. I have only few comments on it.

On 12/09/14 08:15, Ian Campbell wrote:
> This allows us to support sparse physical address maps which we previously
> could not because the frametable would end up taking up an enourmous fraction

enormous

[..]

> -    memset(&frame_table[0], 0, nr_pages * sizeof(struct page_info));
> -    memset(&frame_table[nr_pages], -1,
> -           frametable_size - (nr_pages * sizeof(struct page_info)));
> +    memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
> +    memset(&frame_table[nr_pdxs], -1,
> +           frametable_size - (nr_pdxs * sizeof(struct page_info)));
> +
> +    frametable_virt_end = FRAMETABLE_VIRT_START + (nr_pdxs * sizeof(struct page_info));
>

NIT: I don't think it's necessary to have a blank line before the end of 
the block. I would drop it.

[..]

> +/* Sets all bits from the most-significant 1-bit down to the LSB */
> +static u64 __init fill_mask(u64 mask)
> +{
> +    while (mask & (mask + 1))
> +        mask |= mask + 1;
> +    return mask;
> +}

The function is the same on x86. Shall we create an helper?

[..]

>           setup_xenheap_mappings(bank_start>>PAGE_SHIFT, bank_size>>PAGE_SHIFT);

You forgot to drop check if ( bank != bootinfo.mem.nr_banks ) which, I 
think, is not relevant anymore.


> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index 1c3abcf..59b2887 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -126,7 +126,12 @@
>   #define CONFIG_SEPARATE_XENHEAP 1
>
>   #define FRAMETABLE_VIRT_START  _AT(vaddr_t,0x02000000)
> -#define VMAP_VIRT_START  _AT(vaddr_t,0x10000000)
> +#define FRAMETABLE_SIZE        MB(128-32)

I would add a comment about why the frametable size is "MB(128-32)".

Regards,

-- 
Julien Grall