Re: [PATCH v3] xen/arm: Do not allocate pte entries for MAP_SMALL_PAGES

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

Hi Vijay,

Given the introduction of the new helper, the title looks wrong to me.

On 09/03/2015 08:59, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7d4ba0c..e0be36b 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -827,14 +827,15 @@ static int create_xen_table(lpae_t *entry)
>
>   enum xenmap_operation {
>       INSERT,
> -    REMOVE
> +    REMOVE,
> +    RESERVE
>   };
>
>   static int create_xen_entries(enum xenmap_operation op,
>                                 unsigned long virt,
>                                 unsigned long mfn,
>                                 unsigned long nr_mfns,
> -                              unsigned int ai)
> +                              unsigned int flags)
>   {
>       int rc;
>       unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> @@ -859,13 +860,17 @@ static int create_xen_entries(enum xenmap_operation op,
>
>           switch ( op ) {
>               case INSERT:
> +            case RESERVE:
>                   if ( third[third_table_offset(addr)].pt.valid )
>                   {
>                       printk("create_xen_entries: trying to replace an existing mapping addr=%lx mfn=%lx\n",
>                              addr, mfn);
>                       return -EINVAL;
>                   }
> -                pte = mfn_to_xen_entry(mfn, ai);
> +                if ( op == RESERVE || !is_pte_present(flags) )

As you have a new operation (only used by populate_pt_range), why do you 
need to check is_pte_present?

>   void *vm_alloc(unsigned int nr, unsigned int align)
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 3e7b0ae..f743003 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -61,10 +61,27 @@
>   #define DEV_WC        BUFFERABLE
>   #define DEV_CACHED    WRITEBACK
>
> -#define PAGE_HYPERVISOR         (WRITEALLOC)
> -#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
> -#define PAGE_HYPERVISOR_WC      (DEV_WC)
> -#define MAP_SMALL_PAGES         PAGE_HYPERVISOR
> +#define PAGE_PRESENT       (0x1 << 16)
> +#define PAGE_NOT_PRESENT   (0x0)
> +
> +/* bit[16] in the below representation can be used to know if
> + * PTE entry should be added or not. This is useful
> + * when ONLY non-leaf page table entries need to allocated.
> + *
> + * bits[2:0] of be below represent correponds to AttrIndx[2:0]
> + * i.e lpae_t.pt.ai[2:4]
> + *
> + * For readability purpose MAP_SMALL_PAGES is set with PAGE_NOT_PRESENT
> + * though PAGE_NOT_PRESENT is 0.
> + */
> +
> +#define PAGE_HYPERVISOR         (WRITEALLOC | PAGE_PRESENT)
> +#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED | PAGE_PRESENT)
> +#define PAGE_HYPERVISOR_WC      (DEV_WC     | PAGE_PRESENT)
> +#define MAP_SMALL_PAGES         (WRITEALLOC | PAGE_NOT_PRESENT)

Again, using WRITEALLOC for MAP_SMALL_PAGES doesn't make any sense.

Given that you introduced a new helper populate_pt_range and a new 
operation (RESERVE), the flags PAGE{,_NOT}_PRESENT seems useless.

And, therefore, MAP_SMALL_PAGES could be dropped.

Regards,

-- 
Julien Grall