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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Mon, Mar 9, 2015 at 5:46 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Vijay,
>
> Given the introduction of the new helper, the title looks wrong to me.
>
>
> On 09/03/2015 08:59, vijay.kilari@xxxxxxxxx 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?

map_pages_to_xen() can still take MAP_SMALL_PAGES as flags.
In future if any common code requires  MAP_SMALL_PAGES then,
this can be used.

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

We can just drop WRITEALLOC to MAP_SMALL_PAGES but still keep
PAGE_NOT_PRESENT.

>
> And, therefore, MAP_SMALL_PAGES could be dropped.

MAP_SMALL_PAGES is still used in common code esp. EFI code.
We can remove this provided if we clean up this. But I still think
MAP_SMALL_PAGES is required to keep equivalent functionality of x86.

Regards,
Vijay

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