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

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

On Wed, 2015-03-11 at 10:43 +0530, Vijay Kilari wrote:
> On Tue, Mar 10, 2015 at 5:22 PM, Ian Campbell <ian.campbell@citrix.com> 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@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.
> 
> My initial approach with RFC patch was right.
> 
> http://lists.xen.org/archives/html/xen-devel/2015-02/msg01919.html


You should add to arm:
int populate_pt_range(unsigned long virt, unsigned long mfn,
                      unsigned long nr_mfns)
{
    return create_xen_entries(RESERVE, virt, mfn, nr_mfns, 0);
}

and make create_xen_entries do that, but not worry about making it
handle alternative flags etc, just adding the new case (sharing code
with insert) and adding:

+                if ( op == RESERVE )
+                    break;

at the appropriate place should do it I think.

No need to touch any of the stuff in xen/include/asm-arm/page.h, except
perhaps to delete MAP_SMALL_PAGES if it is now unused.

If you really want to pursue the other changes to page.h for some reason
then it should definitely be in a separate patch.

Ian.