[RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Vijay Kilari]

In vmap_init, map_pages_to_xen() is called for mapping
vm_bitmap. Initially one page of vm_bitmap is allocated
and mapped using PAGE_HYPERVISOR attribute.
For the rest of vm_bitmap pages, MAP_SMALL_PAGES attribute
is used to map.

In ARM for both PAGE_HYPERVISOR and MAP_SMALL_PAGES, valid bit
is set to 1 in pte entry for these mapping.

In vma_alloc(), map_pages_to_xen() is failing for >128MB because
for this next vm_bitmap page the mapping is already set in vm_init()
with valid bit set in pte entry. So map_pages_to_xen() in
ARM returns error.

On x86, for the pages mapped with MAP_SMALL_PAGES attribute
valid bit is not set. So the common vmap code assumes the
same behaviour from arm. However this is not the case.

This patch will set valid bit to 0 in pte entry when
pages are mapped with MAP_SMALL_PAGES.

Signed-off-by: Vijaya Kumar K<Vijaya.Kumar@caviumnetworks.com>

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c3ae184..f125cf9 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -827,7 +827,8 @@ static int create_xen_table(lpae_t *entry)

 enum xenmap_operation {
     INSERT,
-    REMOVE
+    REMOVE,
+    RESERVE
 };

 static int create_xen_entries(enum xenmap_operation op,
@@ -859,6 +860,7 @@ 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",
@@ -866,6 +868,8 @@ static int create_xen_entries(enum xenmap_operation op,
                     return -EINVAL;
                 }
                 pte = mfn_to_xen_entry(mfn, ai);
+                if ( op == RESERVE )
+                    pte.pt.valid = 0;
                 pte.pt.table = 1;
                 write_pte(&third[third_table_offset(addr)], pte);
                 break;
@@ -896,7 +900,10 @@ int map_pages_to_xen(unsigned long virt,
                      unsigned long nr_mfns,
                      unsigned int flags)
 {
-    return create_xen_entries(INSERT, virt, mfn, nr_mfns, flags);
+    if ( flags  == MAP_SMALL_PAGES )
+        return create_xen_entries(RESERVE, virt, mfn, nr_mfns,
PAGE_HYPERVISOR);
+    else
+        return create_xen_entries(INSERT, virt, mfn, nr_mfns, flags);
 }
 void destroy_xen_mappings(unsigned long v, unsigned long e)
 {
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 53d4b63..e704ac8 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -58,6 +58,7 @@
 #define WRITEBACK     0x3
 #define DEV_SHARED    0x4
 #define WRITEALLOC    0x7
+#define WRITEALLOC_INVALID    0xf
 #define DEV_NONSHARED DEV_SHARED
 #define DEV_WC        BUFFERABLE
 #define DEV_CACHED    WRITEBACK
@@ -65,7 +66,7 @@
 #define PAGE_HYPERVISOR         (WRITEALLOC)
 #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
 #define PAGE_HYPERVISOR_WC      (DEV_WC)
-#define MAP_SMALL_PAGES         PAGE_HYPERVISOR
+#define MAP_SMALL_PAGES         (WRITEALLOC_INVALID)

 /*
  * Stage 2 Memory Type.
-- 
1.7.9.5

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Jan Beulich]

>>> On 17.02.15 at 07:07, <vijay.kilari@gmail.com> wrote:
> In vmap_init, map_pages_to_xen() is called for mapping
> vm_bitmap. Initially one page of vm_bitmap is allocated
> and mapped using PAGE_HYPERVISOR attribute.
> For the rest of vm_bitmap pages, MAP_SMALL_PAGES attribute
> is used to map.
> 
> In ARM for both PAGE_HYPERVISOR and MAP_SMALL_PAGES, valid bit
> is set to 1 in pte entry for these mapping.
> 
> In vma_alloc(), map_pages_to_xen() is failing for >128MB because
> for this next vm_bitmap page the mapping is already set in vm_init()
> with valid bit set in pte entry. So map_pages_to_xen() in
> ARM returns error.
> 
> On x86, for the pages mapped with MAP_SMALL_PAGES attribute
> valid bit is not set. So the common vmap code assumes the
> same behaviour from arm. However this is not the case.
> 
> This patch will set valid bit to 0 in pte entry when
> pages are mapped with MAP_SMALL_PAGES.
> 
> Signed-off-by: Vijaya Kumar K<Vijaya.Kumar@caviumnetworks.com>

Yes, this is precisely the kind of change I expected to address
the previously described problem.

Jan

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Julien Grall]

Hello Vijay,

The title looks more a title for a bug report rather than a patch.

On 17/02/15 06:07, Vijay Kilari wrote:
> In vmap_init, map_pages_to_xen() is called for mapping
> vm_bitmap. Initially one page of vm_bitmap is allocated
> and mapped using PAGE_HYPERVISOR attribute.
> For the rest of vm_bitmap pages, MAP_SMALL_PAGES attribute
> is used to map.
> 
> In ARM for both PAGE_HYPERVISOR and MAP_SMALL_PAGES, valid bit
> is set to 1 in pte entry for these mapping.
> 
> In vma_alloc(), map_pages_to_xen() is failing for >128MB because
> for this next vm_bitmap page the mapping is already set in vm_init()
> with valid bit set in pte entry. So map_pages_to_xen() in
> ARM returns error.
> 
> On x86, for the pages mapped with MAP_SMALL_PAGES attribute
> valid bit is not set. So the common vmap code assumes the
> same behaviour from arm. However this is not the case.
> 
> This patch will set valid bit to 0 in pte entry when
> pages are mapped with MAP_SMALL_PAGES.

Setting the valid bit to 0 or 1, as long as the 128M limit, are just
side-effects of the real problem.

You should explain what does MAP_SMALL_PAGES and PAGE_HYPERVISOR means.
For now, the reader needs to understand by himself that MAP_SMALL_PAGES
is used to pre-allocate non-leaf page table.

[..]

> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 53d4b63..e704ac8 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -58,6 +58,7 @@
>  #define WRITEBACK     0x3
>  #define DEV_SHARED    0x4
>  #define WRITEALLOC    0x7
> +#define WRITEALLOC_INVALID    0xf

Why WRITEALLOC_INVALID? The mapping is not at all invalid and doesn't
exist... We just asked to pre-allocate the non-leaf page.

>  #define DEV_NONSHARED DEV_SHARED
>  #define DEV_WC        BUFFERABLE
>  #define DEV_CACHED    WRITEBACK
> @@ -65,7 +66,7 @@
>  #define PAGE_HYPERVISOR         (WRITEALLOC)
>  #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
>  #define PAGE_HYPERVISOR_WC      (DEV_WC)
> -#define MAP_SMALL_PAGES         PAGE_HYPERVISOR
> +#define MAP_SMALL_PAGES         (WRITEALLOC_INVALID)

The parameter "flags" of map_pages_to_xen is an unsigned int. It would
be better to use the top bits for adding new flags (such as a present flag).

Then you could use this flag in create_xen_entries to know if we need to
set the present bit or not. Or, even better, skip the creation of the PTE.

This would make the code more cleaner and avoid workaround like you did
in map_pages_to_xen.

Regards,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Ian Campbell]

On Tue, 2015-02-17 at 14:50 +0000, Julien Grall wrote:
> Hello Vijay,
> 
> The title looks more a title for a bug report rather than a patch.
> 
> On 17/02/15 06:07, Vijay Kilari wrote:
> > In vmap_init, map_pages_to_xen() is called for mapping
> > vm_bitmap. Initially one page of vm_bitmap is allocated
> > and mapped using PAGE_HYPERVISOR attribute.
> > For the rest of vm_bitmap pages, MAP_SMALL_PAGES attribute
> > is used to map.
> > 
> > In ARM for both PAGE_HYPERVISOR and MAP_SMALL_PAGES, valid bit
> > is set to 1 in pte entry for these mapping.
> > 
> > In vma_alloc(), map_pages_to_xen() is failing for >128MB because
> > for this next vm_bitmap page the mapping is already set in vm_init()
> > with valid bit set in pte entry. So map_pages_to_xen() in
> > ARM returns error.
> > 
> > On x86, for the pages mapped with MAP_SMALL_PAGES attribute
> > valid bit is not set. So the common vmap code assumes the
> > same behaviour from arm. However this is not the case.
> > 
> > This patch will set valid bit to 0 in pte entry when
> > pages are mapped with MAP_SMALL_PAGES.
> 
> Setting the valid bit to 0 or 1, as long as the 128M limit, are just
> side-effects of the real problem.
> 
> You should explain what does MAP_SMALL_PAGES and PAGE_HYPERVISOR means.
> For now, the reader needs to understand by himself that MAP_SMALL_PAGES
> is used to pre-allocate non-leaf page table.
> 
> [..]
> 
> > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > index 53d4b63..e704ac8 100644
> > --- a/xen/include/asm-arm/page.h
> > +++ b/xen/include/asm-arm/page.h
> > @@ -58,6 +58,7 @@
> >  #define WRITEBACK     0x3
> >  #define DEV_SHARED    0x4
> >  #define WRITEALLOC    0x7
> > +#define WRITEALLOC_INVALID    0xf
> 
> Why WRITEALLOC_INVALID? The mapping is not at all invalid and doesn't
> exist... We just asked to pre-allocate the non-leaf page.

> 
> >  #define DEV_NONSHARED DEV_SHARED
> >  #define DEV_WC        BUFFERABLE
> >  #define DEV_CACHED    WRITEBACK
> > @@ -65,7 +66,7 @@
> >  #define PAGE_HYPERVISOR         (WRITEALLOC)
> >  #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
> >  #define PAGE_HYPERVISOR_WC      (DEV_WC)
> > -#define MAP_SMALL_PAGES         PAGE_HYPERVISOR
> > +#define MAP_SMALL_PAGES         (WRITEALLOC_INVALID)
> 
> The parameter "flags" of map_pages_to_xen is an unsigned int. It would
> be better to use the top bits for adding new flags (such as a present flag).
> 
> Then you could use this flag in create_xen_entries to know if we need to
> set the present bit or not. Or, even better, skip the creation of the PTE.

The underlying problem here seems to be that PAGE_FOO on x86 is not just
the attributes but is actually the present bit etc too:
#define __PAGE_HYPERVISOR \
    (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
#define PAGE_HYPERVISOR         (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)

changing ARM similarly (i.e. shifting WRITEALLOC etc to the appropriate
place and adding the present bit) would seem like a good way to both
address this issue and make the arch interfaces more consistent.

MAP_SMALL_PAGES could then be an avail bit or something like on x86.

> 
> This would make the code more cleaner and avoid workaround like you did
> in map_pages_to_xen.
> 
> Regards,
> 

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Julien Grall]

On 23/02/2015 17:07, Ian Campbell wrote:
> On Tue, 2015-02-17 at 14:50 +0000, Julien Grall wrote:
>> Hello Vijay,
>>
>> The title looks more a title for a bug report rather than a patch.
>>
>> On 17/02/15 06:07, Vijay Kilari wrote:
>>> In vmap_init, map_pages_to_xen() is called for mapping
>>> vm_bitmap. Initially one page of vm_bitmap is allocated
>>> and mapped using PAGE_HYPERVISOR attribute.
>>> For the rest of vm_bitmap pages, MAP_SMALL_PAGES attribute
>>> is used to map.
>>>
>>> In ARM for both PAGE_HYPERVISOR and MAP_SMALL_PAGES, valid bit
>>> is set to 1 in pte entry for these mapping.
>>>
>>> In vma_alloc(), map_pages_to_xen() is failing for >128MB because
>>> for this next vm_bitmap page the mapping is already set in vm_init()
>>> with valid bit set in pte entry. So map_pages_to_xen() in
>>> ARM returns error.
>>>
>>> On x86, for the pages mapped with MAP_SMALL_PAGES attribute
>>> valid bit is not set. So the common vmap code assumes the
>>> same behaviour from arm. However this is not the case.
>>>
>>> This patch will set valid bit to 0 in pte entry when
>>> pages are mapped with MAP_SMALL_PAGES.
>>
>> Setting the valid bit to 0 or 1, as long as the 128M limit, are just
>> side-effects of the real problem.
>>
>> You should explain what does MAP_SMALL_PAGES and PAGE_HYPERVISOR means.
>> For now, the reader needs to understand by himself that MAP_SMALL_PAGES
>> is used to pre-allocate non-leaf page table.
>>
>> [..]
>>
>>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>>> index 53d4b63..e704ac8 100644
>>> --- a/xen/include/asm-arm/page.h
>>> +++ b/xen/include/asm-arm/page.h
>>> @@ -58,6 +58,7 @@
>>>   #define WRITEBACK     0x3
>>>   #define DEV_SHARED    0x4
>>>   #define WRITEALLOC    0x7
>>> +#define WRITEALLOC_INVALID    0xf
>>
>> Why WRITEALLOC_INVALID? The mapping is not at all invalid and doesn't
>> exist... We just asked to pre-allocate the non-leaf page.
>
>>
>>>   #define DEV_NONSHARED DEV_SHARED
>>>   #define DEV_WC        BUFFERABLE
>>>   #define DEV_CACHED    WRITEBACK
>>> @@ -65,7 +66,7 @@
>>>   #define PAGE_HYPERVISOR         (WRITEALLOC)
>>>   #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
>>>   #define PAGE_HYPERVISOR_WC      (DEV_WC)
>>> -#define MAP_SMALL_PAGES         PAGE_HYPERVISOR
>>> +#define MAP_SMALL_PAGES         (WRITEALLOC_INVALID)
>>
>> The parameter "flags" of map_pages_to_xen is an unsigned int. It would
>> be better to use the top bits for adding new flags (such as a present flag).
>>
>> Then you could use this flag in create_xen_entries to know if we need to
>> set the present bit or not. Or, even better, skip the creation of the PTE.
>
> The underlying problem here seems to be that PAGE_FOO on x86 is not just
> the attributes but is actually the present bit etc too:
> #define __PAGE_HYPERVISOR \
>      (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
> #define PAGE_HYPERVISOR         (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
>
> changing ARM similarly (i.e. shifting WRITEALLOC etc to the appropriate
> place and adding the present bit) would seem like a good way to both
> address this issue and make the arch interfaces more consistent.
>
> MAP_SMALL_PAGES could then be an avail bit or something like on x86.

Actually the v2 is going on this sense, though I had some comment on it.

Regards,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Ian Campbell]

On Mon, 2015-02-23 at 21:45 +0000, Julien Grall wrote:
> Actually the v2 is going on this sense, though I had some comment on it.

Oh, I seem to have missed v2, I'll try and find it later and have a
look.

Ian.

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Ian Campbell]

On Tue, 2015-02-17 at 11:37 +0530, Vijay Kilari wrote:
> In vmap_init,

Did you mean vm_init? vmap_init is not a function in Xen that I can
find.

>  map_pages_to_xen() is called for mapping
> vm_bitmap. Initially one page of vm_bitmap is allocated
> and mapped using PAGE_HYPERVISOR attribute.
> For the rest of vm_bitmap pages, MAP_SMALL_PAGES attribute
> is used to map.
> 
> In ARM for both PAGE_HYPERVISOR and MAP_SMALL_PAGES, valid bit
> is set to 1 in pte entry for these mapping.
> 
> In vma_alloc()

There is no such function in Xen. (Please take care with such naming,
otherwise I have to waste lots of time trying to find the code you are
talking about).

> , map_pages_to_xen() is failing for >128MB because
> for this next vm_bitmap page the mapping is already set in vm_init()
> with valid bit set in pte entry. So map_pages_to_xen() in
> ARM returns error.
> 
> On x86, for the pages mapped with MAP_SMALL_PAGES attribute
> valid bit is not set. So the common vmap code assumes the
> same behaviour from arm. However this is not the case.
> 
> This patch will set valid bit to 0 in pte entry when
> pages are mapped with MAP_SMALL_PAGES.

MAP_SMALL_PAGES in no way indicates to me "create the PT structure but
do not map any actual data pages". It is documented on x86 as "don't use
superpages mappings", which doesn't imply anything to me about
non-superpage mappings either.

Can we along with this fix perhaps rename to something which does
indicate this. MAP_NON_LEAF_PTS or something perhaps? (This is really a
question for Jan).

Ian.

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Ian Campbell]

On Mon, 2015-02-23 at 17:03 +0000, Ian Campbell wrote:
> MAP_SMALL_PAGES in no way indicates to me "create the PT structure but
> do not map any actual data pages". It is documented on x86 as "don't use
> superpages mappings", which doesn't imply anything to me about
> non-superpage mappings either.

OK I get it now, MAP_SMALL_PAGES is actually a modifier for the PAGE_*
stuff, what's special about this particular usage is that it is a bare
MAP_SMALL_PAGES, i.e. not PAGE_HYPERVISOR|MAP_SMALL_PAGES.

Being bare it then lacks the present bit from PAGE_HYPERVISOR. It's a
bit like saying PAGE_NONE|MAP_SMALL_PAGES (if PAGE_NONE existed)

> Can we along with this fix perhaps rename to something which does
> indicate this. MAP_NON_LEAF_PTS or something perhaps? (This is really a
> question for Jan).

So this then doesn't make any sense, as I'm sure Jan is about to tell
me ;-)

Ian.

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Jan Beulich]

>>> On 23.02.15 at 18:03, <ian.campbell@citrix.com> wrote:
> Can we along with this fix perhaps rename to something which does
> indicate this. MAP_NON_LEAF_PTS or something perhaps? (This is really a
> question for Jan).

Renaming wouldn't be the right thing - we'd need a second flag.
Indeed I've been (ab)using behavior of the x86 implementation
here that's neither documented nor necessarily obvious.

Jan

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Ian Campbell]

On Tue, 2015-02-24 at 10:01 +0000, Jan Beulich wrote:
> >>> On 23.02.15 at 18:03, <ian.campbell@citrix.com> wrote:
> > Can we along with this fix perhaps rename to something which does
> > indicate this. MAP_NON_LEAF_PTS or something perhaps? (This is really a
> > question for Jan).
> 
> Renaming wouldn't be the right thing - we'd need a second flag.
> Indeed I've been (ab)using behavior of the x86 implementation
> here that's neither documented nor necessarily obvious.

How about a new arch interface like create_mapping_pts or
populate_pt_range or some such which provides the semantics you want?

On x86 it could just be implemented using map_pages_to_xen(...,
MAP_SMALL_PAGES) and on ARM we'd figure it out somehow.

Ian.

Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Jan Beulich]

>>> On 24.02.15 at 11:20, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-02-24 at 10:01 +0000, Jan Beulich wrote:
>> >>> On 23.02.15 at 18:03, <ian.campbell@citrix.com> wrote:
>> > Can we along with this fix perhaps rename to something which does
>> > indicate this. MAP_NON_LEAF_PTS or something perhaps? (This is really a
>> > question for Jan).
>> 
>> Renaming wouldn't be the right thing - we'd need a second flag.
>> Indeed I've been (ab)using behavior of the x86 implementation
>> here that's neither documented nor necessarily obvious.
> 
> How about a new arch interface like create_mapping_pts or
> populate_pt_range or some such which provides the semantics you want?
> 
> On x86 it could just be implemented using map_pages_to_xen(...,
> MAP_SMALL_PAGES) and on ARM we'd figure it out somehow.

Fine with me.

Jan