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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Tue, Mar 10, 2015 at 5:22 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> On Tue, 2015-03-10 at 11:45 +0000, Julien Grall wrote:
>> On 09/03/15 16:08, Vijay Kilari wrote:
>> > 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.
>>
>> The only usage was in vmap that you removed in this patch...
>>
>> Furthermore, we decided to use to introduce populate_pt_range in order
>> to avoid using MAP_SMALL_PAGES on ARM...
>>
>> It's pointless to keep to different way to population page table...
>>
>> [..]
>>
>> >>
>> >> 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.
>>
>> If you looked at the code you would have notice that the code is only
>> compiled for x86 and would never work for ARM (_PAGE_PAT, _PAGE_PWT...
>> doesn't exist).
>
> Right, I think we should just remove MAP_SMALL_PAGES on ARM and if/when
> it turns out we need it we can add it back and implement it in the PT
> creation code.
>
> In any case the fix to make vmap_init use the new function should
> certainly be in a separate patch to anything which is fixing
> MAP_SMALL_PAGES.

My initial approach with RFC patch was right.

http://lists.xen.org/archives/html/xen-devel/2015-02/msg01919.html

So I will add populate_pt_range() and remove MAP_SMALL_PAGES.
Let me know your stance.

Regards
Vijay

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