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

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

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

On x86, for the pages mapped with PAGE_HYPERVISOR attribute
non-leaf page tables are allocated with valid pte entries.
and with MAP_SMALL_PAGES attribute only non-leaf page tables are
allocated with invalid (valid bit set to 0) pte entries.
However on arm this is not the case. On arm for the pages
mapped with PAGE_HYPERVISOR and MAP_SMALL_PAGES both
non-leaf and leaf level page table are allocated with valid bit
in pte entries.

This behaviour in arm makes common vmap code fail to
allocate memory beyond 128MB as described below.

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.

With this patch, MAP_SMALL_PAGES attribute will only allocate
non-leaf page tables only.

Here we use bit[16] in the attribute flag to know if leaf page
tables should be allocated or not.

This bit is set only for MAP_SMALL_PAGES attribute.

Signed-off-by: Vijaya Kumar K<Vijaya.Kumar@caviumnetworks.com>
---
 xen/arch/arm/mm.c          |    9 ++++++---
 xen/include/asm-arm/page.h |    8 +++++++-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7d4ba0c..a12f3f5 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -865,9 +865,12 @@ static int create_xen_entries(enum xenmap_operation op,
                            addr, mfn);
                     return -EINVAL;
                 }
-                pte = mfn_to_xen_entry(mfn, ai);
-                pte.pt.table = 1;
-                write_pte(&third[third_table_offset(addr)], pte);
+                if ( !(ai & PTE_INVALID) )
+                {
+                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
+                    pte.pt.table = 1;
+                    write_pte(&third[third_table_offset(addr)], pte);
+                }
                 break;
             case REMOVE:
                 if ( !third[third_table_offset(addr)].pt.valid )
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 3e7b0ae..80415b3 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -61,10 +61,16 @@
 #define DEV_WC        BUFFERABLE
 #define DEV_CACHED    WRITEBACK
 
+/* bit 16 in the Attribute index 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
+ */
+#define PTE_INVALID   (0x1 << 16)
+
 #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         (PAGE_HYPERVISOR | (PTE_INVALID))
 
 /*
  * Stage 2 Memory Type.
-- 
1.7.9.5

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

[Julien Grall]

Hello Vijay,

On 18/02/2015 12:56, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> On x86, for the pages mapped with PAGE_HYPERVISOR attribute
> non-leaf page tables are allocated with valid pte entries.
> and with MAP_SMALL_PAGES attribute only non-leaf page tables are
> allocated with invalid (valid bit set to 0) pte entries.
> However on arm this is not the case. On arm for the pages
> mapped with PAGE_HYPERVISOR and MAP_SMALL_PAGES both
> non-leaf and leaf level page table are allocated with valid bit
> in pte entries.
>
> This behaviour in arm makes common vmap code fail to
> allocate memory beyond 128MB as described below.
>
> 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.
>
> With this patch, MAP_SMALL_PAGES attribute will only allocate
> non-leaf page tables only.
>
> Here we use bit[16] in the attribute flag to know if leaf page
> tables should be allocated or not.
>
> This bit is set only for MAP_SMALL_PAGES attribute.
>
> Signed-off-by: Vijaya Kumar K<Vijaya.Kumar@caviumnetworks.com>
> ---
>   xen/arch/arm/mm.c          |    9 ++++++---
>   xen/include/asm-arm/page.h |    8 +++++++-
>   2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7d4ba0c..a12f3f5 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -865,9 +865,12 @@ static int create_xen_entries(enum xenmap_operation op,
>                              addr, mfn);
>                       return -EINVAL;
>                   }
> -                pte = mfn_to_xen_entry(mfn, ai);
> -                pte.pt.table = 1;
> -                write_pte(&third[third_table_offset(addr)], pte);
> +                if ( !(ai & PTE_INVALID) )

you could do if ( ai & PTE_INVALID ) break; It would avoid a new level 
of indentation.

Also, I would rename ai to flags as the variable is not anymore an 
attribute index.

> +                {
> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));

Please introduce a new macro for the mask.

> +                    pte.pt.table = 1;
> +                    write_pte(&third[third_table_offset(addr)], pte);
> +                }
>                   break;
>               case REMOVE:
>                   if ( !third[third_table_offset(addr)].pt.valid )
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 3e7b0ae..80415b3 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -61,10 +61,16 @@
>   #define DEV_WC        BUFFERABLE
>   #define DEV_CACHED    WRITEBACK
>
> +/* bit 16 in the Attribute index 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
> + */
> +#define PTE_INVALID   (0x1 << 16)

It makes more sense to introduce a PTE_PRESENT flags compare to 
PTE_INVALID. The former has more meaning that the latter.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
> > +                {
> > +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
> 
> Please introduce a new macro for the mask.

Better would be a pte_foo accessor, similar (if not identical) to x86's
pte_get_flags. So pte_get_flags(ai) or so.

> 
> > +                    pte.pt.table = 1;
> > +                    write_pte(&third[third_table_offset(addr)], pte);
> > +                }
> >                   break;
> >               case REMOVE:
> >                   if ( !third[third_table_offset(addr)].pt.valid )
> > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > index 3e7b0ae..80415b3 100644
> > --- a/xen/include/asm-arm/page.h
> > +++ b/xen/include/asm-arm/page.h
> > @@ -61,10 +61,16 @@
> >   #define DEV_WC        BUFFERABLE
> >   #define DEV_CACHED    WRITEBACK
> >
> > +/* bit 16 in the Attribute index 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
> > + */
> > +#define PTE_INVALID   (0x1 << 16)
> 
> It makes more sense to introduce a PTE_PRESENT flags compare to 
> PTE_INVALID. The former has more meaning that the latter.

Agreed that PTE_PRESENT is the way to go. Ideally this could all be done
via the lpae_pt_t type, but I suspect that might turn out to be tricky.

#define PTE_PRESENT ((struct lpae_t){ .pt.present = 1 }).bits

probably doesn't work, I'm not even sure if this sort of thing is
possible. If not then "#define PTE_PRESET (1ULL<<0)".

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

[Julien Grall]

Hi Ian,

On 24/02/2015 09:31, Ian Campbell wrote:
> On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
>>> +                {
>>> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
>>
>> Please introduce a new macro for the mask.
>
> Better would be a pte_foo accessor, similar (if not identical) to x86's
> pte_get_flags. So pte_get_flags(ai) or so.

I'm not able to find a such function in x86. Did you intend to mean 
pte_flags_to_cacheattr?

>
>>
>>> +                    pte.pt.table = 1;
>>> +                    write_pte(&third[third_table_offset(addr)], pte);
>>> +                }
>>>                    break;
>>>                case REMOVE:
>>>                    if ( !third[third_table_offset(addr)].pt.valid )
>>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>>> index 3e7b0ae..80415b3 100644
>>> --- a/xen/include/asm-arm/page.h
>>> +++ b/xen/include/asm-arm/page.h
>>> @@ -61,10 +61,16 @@
>>>    #define DEV_WC        BUFFERABLE
>>>    #define DEV_CACHED    WRITEBACK
>>>
>>> +/* bit 16 in the Attribute index 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
>>> + */
>>> +#define PTE_INVALID   (0x1 << 16)
>>
>> It makes more sense to introduce a PTE_PRESENT flags compare to
>> PTE_INVALID. The former has more meaning that the latter.
>
> Agreed that PTE_PRESENT is the way to go. Ideally this could all be done
> via the lpae_pt_t type, but I suspect that might turn out to be tricky.

I don't think it's a good idea to re-use lpae_pt_t type for this 
purpose. We might decide to introduce flags which won't be set in the 
final PTE.

In another side, using PTE_PRESENT would require to introduce a 
PAGE_AVAIL0 (or smth similar).

> #define PTE_PRESENT ((struct lpae_t){ .pt.present = 1 }).bits
>
> probably doesn't work, I'm not even sure if this sort of thing is
> possible. If not then "#define PTE_PRESET (1ULL<<0)".

The attribute index (write-alloc, buferrable...) is using the less 
significant 3 bits. So I was suggesting to use the top of the word.

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 24.02.15 at 10:38, <julien.grall@linaro.org> wrote:
> On 24/02/2015 09:31, Ian Campbell wrote:
>> On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
>>>> +                {
>>>> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
>>>
>>> Please introduce a new macro for the mask.
>>
>> Better would be a pte_foo accessor, similar (if not identical) to x86's
>> pte_get_flags. So pte_get_flags(ai) or so.
> 
> I'm not able to find a such function in x86. Did you intend to mean 
> pte_flags_to_cacheattr?

get_pte_flags()

Jan

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

[Ian Campbell]

On Tue, 2015-02-24 at 09:38 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 24/02/2015 09:31, Ian Campbell wrote:
> > On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
> >>> +                {
> >>> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
> >>
> >> Please introduce a new macro for the mask.
> >
> > Better would be a pte_foo accessor, similar (if not identical) to x86's
> > pte_get_flags. So pte_get_flags(ai) or so.
> 
> I'm not able to find a such function in x86. Did you intend to mean 
> pte_flags_to_cacheattr?

It's actually get_pte_flags.

> In another side, using PTE_PRESENT would require to introduce a 
> PAGE_AVAIL0 (or smth similar).

Why?

> > #define PTE_PRESENT ((struct lpae_t){ .pt.present = 1 }).bits
> >
> > probably doesn't work, I'm not even sure if this sort of thing is
> > possible. If not then "#define PTE_PRESET (1ULL<<0)".
> 
> The attribute index (write-alloc, buferrable...) is using the less 
> significant 3 bits. So I was suggesting to use the top of the word.

I was suggesting to use bits 2..4 as in the real PTE, to be more similar
to the x86 interpretation of this argument.

Ian.

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

[Julien Grall]

On 24/02/15 10:26, Ian Campbell wrote:
> On Tue, 2015-02-24 at 09:38 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 24/02/2015 09:31, Ian Campbell wrote:
>>> On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
>>>>> +                {
>>>>> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
>>>>
>>>> Please introduce a new macro for the mask.
>>>
>>> Better would be a pte_foo accessor, similar (if not identical) to x86's
>>> pte_get_flags. So pte_get_flags(ai) or so.
>>
>> I'm not able to find a such function in x86. Did you intend to mean 
>> pte_flags_to_cacheattr?
> 
> It's actually get_pte_flags.
> 
>> In another side, using PTE_PRESENT would require to introduce a 
>> PAGE_AVAIL0 (or smth similar).
> 
> Why?

If we have only a bit PTE_PRESENT, how do you define MAP_SMALL_PAGES?

Anyway, I guess introduce a separate helper would help here.

>>> #define PTE_PRESENT ((struct lpae_t){ .pt.present = 1 }).bits
>>>
>>> probably doesn't work, I'm not even sure if this sort of thing is
>>> possible. If not then "#define PTE_PRESET (1ULL<<0)".
>>
>> The attribute index (write-alloc, buferrable...) is using the less 
>> significant 3 bits. So I was suggesting to use the top of the word.
> 
> I was suggesting to use bits 2..4 as in the real PTE, to be more similar
> to the x86 interpretation of this argument.

I don't think we have to follow how x86 interpret this argument. This is
just a series of flags and may or may not match a bit in the PTE.

For instance, in the case of MAP_SMALL_PAGES, we don't have to write the
final PTE.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2015-02-24 at 12:59 +0000, Julien Grall wrote:
> On 24/02/15 10:26, Ian Campbell wrote:
> > On Tue, 2015-02-24 at 09:38 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 24/02/2015 09:31, Ian Campbell wrote:
> >>> On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
> >>>>> +                {
> >>>>> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
> >>>>
> >>>> Please introduce a new macro for the mask.
> >>>
> >>> Better would be a pte_foo accessor, similar (if not identical) to x86's
> >>> pte_get_flags. So pte_get_flags(ai) or so.
> >>
> >> I'm not able to find a such function in x86. Did you intend to mean 
> >> pte_flags_to_cacheattr?
> > 
> > It's actually get_pte_flags.
> > 
> >> In another side, using PTE_PRESENT would require to introduce a 
> >> PAGE_AVAIL0 (or smth similar).
> > 
> > Why?
> 
> If we have only a bit PTE_PRESENT, how do you define MAP_SMALL_PAGES?

Oh right, yes, I was thinking that would use some other bit, either an
avail bit or reusing the table bit or something.

> Anyway, I guess introduce a separate helper would help here.
> 
> >>> #define PTE_PRESENT ((struct lpae_t){ .pt.present = 1 }).bits
> >>>
> >>> probably doesn't work, I'm not even sure if this sort of thing is
> >>> possible. If not then "#define PTE_PRESET (1ULL<<0)".
> >>
> >> The attribute index (write-alloc, buferrable...) is using the less 
> >> significant 3 bits. So I was suggesting to use the top of the word.
> > 
> > I was suggesting to use bits 2..4 as in the real PTE, to be more similar
> > to the x86 interpretation of this argument.
> 
> I don't think we have to follow how x86 interpret this argument. This is
> just a series of flags and may or may not match a bit in the PTE.

Not matching x86 here has already led to one set of confusion.

I'm not saying with have to match x86, but we should strongly consider
it and not just run with what we have now because it is a smaller
change.

Ian.

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

[Julien Grall]

On 24/02/15 13:13, Ian Campbell wrote:
>>>>> #define PTE_PRESENT ((struct lpae_t){ .pt.present = 1 }).bits
>>>>>
>>>>> probably doesn't work, I'm not even sure if this sort of thing is
>>>>> possible. If not then "#define PTE_PRESET (1ULL<<0)".
>>>>
>>>> The attribute index (write-alloc, buferrable...) is using the less 
>>>> significant 3 bits. So I was suggesting to use the top of the word.
>>>
>>> I was suggesting to use bits 2..4 as in the real PTE, to be more similar
>>> to the x86 interpretation of this argument.
>>
>> I don't think we have to follow how x86 interpret this argument. This is
>> just a series of flags and may or may not match a bit in the PTE.
> 
> Not matching x86 here has already led to one set of confusion.

That was a misunderstanding of the define. Without Jan's explanation I
would not have understand the purpose of this define.

> I'm not saying with have to match x86, but we should strongly consider
> it and not just run with what we have now because it is a smaller
> change.

lpae_t is an uint64_t and flags an unsigned int, so we will have to
check that any bits we have to modified is effectively living on the
less significant word.

Also, a smaller change would allow us to backport the patch to Xen 4.5.

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 24.02.15 at 14:47, <julien.grall@linaro.org> wrote:
> lpae_t is an uint64_t and flags an unsigned int, so we will have to
> check that any bits we have to modified is effectively living on the
> less significant word.

Or, upon extraction, fold all flags into something fitting into 32 bits,
like we do on x86.

Jan

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

[Vijay Kilari]

On Tue, Feb 24, 2015 at 6:29 PM, Julien Grall <julien.grall@linaro.org> wrote:
> On 24/02/15 10:26, Ian Campbell wrote:
>> On Tue, 2015-02-24 at 09:38 +0000, Julien Grall wrote:
>>> Hi Ian,
>>>
>>> On 24/02/2015 09:31, Ian Campbell wrote:
>>>> On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
>>>>>> +                {
>>>>>> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
>>>>>
>>>>> Please introduce a new macro for the mask.
>>>>
>>>> Better would be a pte_foo accessor, similar (if not identical) to x86's
>>>> pte_get_flags. So pte_get_flags(ai) or so.
>>>
>>> I'm not able to find a such function in x86. Did you intend to mean
>>> pte_flags_to_cacheattr?
>>
>> It's actually get_pte_flags.
>>
>>> In another side, using PTE_PRESENT would require to introduce a
>>> PAGE_AVAIL0 (or smth similar).
>>
>> Why?
>
> If we have only a bit PTE_PRESENT, how do you define MAP_SMALL_PAGES?

MAP_SMALL_PAGES is already defined as WRITE_ALLOC which occupies lower 3 bits
what is need for PAGE_AVAIL0?

Below definitions should suffice?

-#define PTE_INVALID   (0x1 << 16)
+#define PTE_PRESENT   (0x1 << 16)

-#define PAGE_HYPERVISOR         (WRITEALLOC)
-#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
-#define PAGE_HYPERVISOR_WC      (DEV_WC)
-#define MAP_SMALL_PAGES         (PAGE_HYPERVISOR | (PTE_INVALID))
+#define PAGE_HYPERVISOR         ((WRITEALLOC) | PTE_PRESENT )
+#define PAGE_HYPERVISOR_NOCACHE ((DEV_SHARED) | PTE_PRESENT )
+#define PAGE_HYPERVISOR_WC      ((DEV_WC) | PTE_PRESENT )
+#define MAP_SMALL_PAGES         (WRITEALLOC)
+
+#define is_pte_present(x) ((x) & PTE_PRESENT)
+#define get_pte_flags(x)  ((x) & 0x7)


>
> Anyway, I guess introduce a separate helper would help here.
>
>>>> #define PTE_PRESENT ((struct lpae_t){ .pt.present = 1 }).bits
>>>>
>>>> probably doesn't work, I'm not even sure if this sort of thing is
>>>> possible. If not then "#define PTE_PRESET (1ULL<<0)".
>>>
>>> The attribute index (write-alloc, buferrable...) is using the less
>>> significant 3 bits. So I was suggesting to use the top of the word.
>>
>> I was suggesting to use bits 2..4 as in the real PTE, to be more similar
>> to the x86 interpretation of this argument.
>
> I don't think we have to follow how x86 interpret this argument. This is
> just a series of flags and may or may not match a bit in the PTE.
>
> For instance, in the case of MAP_SMALL_PAGES, we don't have to write the
> final PTE.
>
> Regards,
>
> --
> Julien Grall

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

[Ian Campbell]

On Tue, 2015-03-03 at 13:28 +0530, Vijay Kilari wrote:
> On Tue, Feb 24, 2015 at 6:29 PM, Julien Grall <julien.grall@linaro.org> wrote:
> > On 24/02/15 10:26, Ian Campbell wrote:
> >> On Tue, 2015-02-24 at 09:38 +0000, Julien Grall wrote:
> >>> Hi Ian,
> >>>
> >>> On 24/02/2015 09:31, Ian Campbell wrote:
> >>>> On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
> >>>>>> +                {
> >>>>>> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
> >>>>>
> >>>>> Please introduce a new macro for the mask.
> >>>>
> >>>> Better would be a pte_foo accessor, similar (if not identical) to x86's
> >>>> pte_get_flags. So pte_get_flags(ai) or so.
> >>>
> >>> I'm not able to find a such function in x86. Did you intend to mean
> >>> pte_flags_to_cacheattr?
> >>
> >> It's actually get_pte_flags.
> >>
> >>> In another side, using PTE_PRESENT would require to introduce a
> >>> PAGE_AVAIL0 (or smth similar).
> >>
> >> Why?
> >
> > If we have only a bit PTE_PRESENT, how do you define MAP_SMALL_PAGES?
> 
> MAP_SMALL_PAGES is already defined as WRITE_ALLOC which occupies lower 3 bits
> what is need for PAGE_AVAIL0?
> 
> Below definitions should suffice?

I think so. You don't need the ()s around DEV_* and WRITE_ALLOC etc in
the PAGE_* definitions.

A comment on the format of the bit packing done here would also be
useful e.g. "16: present\n2:0: pte attributes field" in a little block
comment above this stuff, below the DEV_* stuff.

PAGE_PRESENT might be more in keepign that PTE_PRESENT, since this isn't
a PTE bit pattern.

> -#define PTE_INVALID   (0x1 << 16)
> +#define PTE_PRESENT   (0x1 << 16)
> 
> -#define PAGE_HYPERVISOR         (WRITEALLOC)
> -#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
> -#define PAGE_HYPERVISOR_WC      (DEV_WC)
> -#define MAP_SMALL_PAGES         (PAGE_HYPERVISOR | (PTE_INVALID))
> +#define PAGE_HYPERVISOR         ((WRITEALLOC) | PTE_PRESENT )
> +#define PAGE_HYPERVISOR_NOCACHE ((DEV_SHARED) | PTE_PRESENT )
> +#define PAGE_HYPERVISOR_WC      ((DEV_WC) | PTE_PRESENT )
> +#define MAP_SMALL_PAGES         (WRITEALLOC)
> +
> +#define is_pte_present(x) ((x) & PTE_PRESENT)
> +#define get_pte_flags(x)  ((x) & 0x7)
> 
> 
> >
> > Anyway, I guess introduce a separate helper would help here.
> >
> >>>> #define PTE_PRESENT ((struct lpae_t){ .pt.present = 1 }).bits
> >>>>
> >>>> probably doesn't work, I'm not even sure if this sort of thing is
> >>>> possible. If not then "#define PTE_PRESET (1ULL<<0)".
> >>>
> >>> The attribute index (write-alloc, buferrable...) is using the less
> >>> significant 3 bits. So I was suggesting to use the top of the word.
> >>
> >> I was suggesting to use bits 2..4 as in the real PTE, to be more similar
> >> to the x86 interpretation of this argument.
> >
> > I don't think we have to follow how x86 interpret this argument. This is
> > just a series of flags and may or may not match a bit in the PTE.
> >
> > For instance, in the case of MAP_SMALL_PAGES, we don't have to write the
> > final PTE.
> >
> > Regards,
> >
> > --
> > Julien Grall

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

[Julien Grall]

Hi Ian,

On 03/03/2015 10:27, Ian Campbell wrote:
> On Tue, 2015-03-03 at 13:28 +0530, Vijay Kilari wrote:
>> On Tue, Feb 24, 2015 at 6:29 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>> On 24/02/15 10:26, Ian Campbell wrote:
>>>> On Tue, 2015-02-24 at 09:38 +0000, Julien Grall wrote:
>>>>> Hi Ian,
>>>>>
>>>>> On 24/02/2015 09:31, Ian Campbell wrote:
>>>>>> On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
>>>>>>>> +                {
>>>>>>>> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
>>>>>>>
>>>>>>> Please introduce a new macro for the mask.
>>>>>>
>>>>>> Better would be a pte_foo accessor, similar (if not identical) to x86's
>>>>>> pte_get_flags. So pte_get_flags(ai) or so.
>>>>>
>>>>> I'm not able to find a such function in x86. Did you intend to mean
>>>>> pte_flags_to_cacheattr?
>>>>
>>>> It's actually get_pte_flags.
>>>>
>>>>> In another side, using PTE_PRESENT would require to introduce a
>>>>> PAGE_AVAIL0 (or smth similar).
>>>>
>>>> Why?
>>>
>>> If we have only a bit PTE_PRESENT, how do you define MAP_SMALL_PAGES?
>>
>> MAP_SMALL_PAGES is already defined as WRITE_ALLOC which occupies lower 3 bits
>> what is need for PAGE_AVAIL0?
>>
>> Below definitions should suffice?
>
> I think so. You don't need the ()s around DEV_* and WRITE_ALLOC etc in
> the PAGE_* definitions.

I'm not sure to follow here. Do you think MAP_SMALL_PAGES should be 
defined as WRITE_ALLOC?


> A comment on the format of the bit packing done here would also be
> useful e.g. "16: present\n2:0: pte attributes field" in a little block
> comment above this stuff, below the DEV_* stuff.
>
> PAGE_PRESENT might be more in keepign that PTE_PRESENT, since this isn't
> a PTE bit pattern.

Sounds good to me.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2015-03-03 at 11:17 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/03/2015 10:27, Ian Campbell wrote:
> > On Tue, 2015-03-03 at 13:28 +0530, Vijay Kilari wrote:
> >> On Tue, Feb 24, 2015 at 6:29 PM, Julien Grall <julien.grall@linaro.org> wrote:
> >>> On 24/02/15 10:26, Ian Campbell wrote:
> >>>> On Tue, 2015-02-24 at 09:38 +0000, Julien Grall wrote:
> >>>>> Hi Ian,
> >>>>>
> >>>>> On 24/02/2015 09:31, Ian Campbell wrote:
> >>>>>> On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote:
> >>>>>>>> +                {
> >>>>>>>> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));
> >>>>>>>
> >>>>>>> Please introduce a new macro for the mask.
> >>>>>>
> >>>>>> Better would be a pte_foo accessor, similar (if not identical) to x86's
> >>>>>> pte_get_flags. So pte_get_flags(ai) or so.
> >>>>>
> >>>>> I'm not able to find a such function in x86. Did you intend to mean
> >>>>> pte_flags_to_cacheattr?
> >>>>
> >>>> It's actually get_pte_flags.
> >>>>
> >>>>> In another side, using PTE_PRESENT would require to introduce a
> >>>>> PAGE_AVAIL0 (or smth similar).
> >>>>
> >>>> Why?
> >>>
> >>> If we have only a bit PTE_PRESENT, how do you define MAP_SMALL_PAGES?
> >>
> >> MAP_SMALL_PAGES is already defined as WRITE_ALLOC which occupies lower 3 bits
> >> what is need for PAGE_AVAIL0?
> >>
> >> Below definitions should suffice?
> >
> > I think so. You don't need the ()s around DEV_* and WRITE_ALLOC etc in
> > the PAGE_* definitions.
> 
> I'm not sure to follow here. Do you think MAP_SMALL_PAGES should be 
> defined as WRITE_ALLOC?

Sorry I just meant that:
        +#define PAGE_HYPERVISOR_WC      ((DEV_WC) | PTE_PRESENT )
should be just:
        +#define PAGE_HYPERVISOR_WC      (DEV_WC | PTE_PRESENT )
etc, no need for the inner-()s.

MAP_SMALL_PAGES was correct as 
        +#define MAP_SMALL_PAGES         (WRITEALLOC)

Ian

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

[Julien Grall]

On 03/03/2015 11:47, Ian Campbell wrote:
>> I'm not sure to follow here. Do you think MAP_SMALL_PAGES should be
>> defined as WRITE_ALLOC?
>
> Sorry I just meant that:
>          +#define PAGE_HYPERVISOR_WC      ((DEV_WC) | PTE_PRESENT )
> should be just:
>          +#define PAGE_HYPERVISOR_WC      (DEV_WC | PTE_PRESENT )
> etc, no need for the inner-()s.
>
> MAP_SMALL_PAGES was correct as
>          +#define MAP_SMALL_PAGES         (WRITEALLOC)

I wanted to check the definition of MAP_SMALL_PAGES you were agreed.

I think using WRITEALLOC here is confusing. There is no resulting PTE 
and therefore WRITEALLOC will never be set.

I think it would be better to have a separate helper (as you suggested 
on a previous mail).

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2015-03-03 at 11:51 +0000, Julien Grall wrote:
> 
> On 03/03/2015 11:47, Ian Campbell wrote:
> >> I'm not sure to follow here. Do you think MAP_SMALL_PAGES should be
> >> defined as WRITE_ALLOC?
> >
> > Sorry I just meant that:
> >          +#define PAGE_HYPERVISOR_WC      ((DEV_WC) | PTE_PRESENT )
> > should be just:
> >          +#define PAGE_HYPERVISOR_WC      (DEV_WC | PTE_PRESENT )
> > etc, no need for the inner-()s.
> >
> > MAP_SMALL_PAGES was correct as
> >          +#define MAP_SMALL_PAGES         (WRITEALLOC)
> 
> I wanted to check the definition of MAP_SMALL_PAGES you were agreed.
> 
> I think using WRITEALLOC here is confusing. There is no resulting PTE 
> and therefore WRITEALLOC will never be set.
> 
> I think it would be better to have a separate helper (as you suggested 
> on a previous mail).

Ah yes, I'd forgotten I'd thought of that. A separate helper to
provision an empty PT range would indeed be the best thing here.


> 
> Regards,
>