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

[Ian Campbell <ian.campbell@citrix.com>]

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@linaro.org> wrote:
> >> 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?
> > 
> > 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.

Ian.