[PATCH 0/3] PDX fixes

[PATCH 0/3] PDX fixes

[Stefano Stabellini]

Hi all,

This series is a small collection of PDX fixes. They are technically
independent but discovered together trying to understand the memory
waste caused by the frametable allocation on Xilinx ZynqMP.

Cheers,

Stefano


The following changes since commit dc497635d93f6672f82727ad97a55205177be2aa:

  build system: make install-stubdom depend on install-tools again (2019-04-23 17:00:08 +0100)

are available in the git repository at:

  http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git pdx-fix-1

for you to fetch changes up to efa7a57449a656a3ffac21e9802b6e5a14a0818a:

  xen/arm: fix mask calculation in init_pdx (2019-05-03 13:16:29 -0700)

----------------------------------------------------------------
Stefano Stabellini (3):
      xen/arm: fix nr_pdxs calculation
      xen: actually skip the first MAX_ORDER bits in pfn_pdx_hole_setup
      xen/arm: fix mask calculation in init_pdx

 xen/arch/arm/mm.c    | 4 ++--
 xen/arch/arm/setup.c | 9 +++++++--
 xen/common/pdx.c     | 6 ++++--
 3 files changed, 13 insertions(+), 6 deletions(-)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 0/3] PDX fixes

[Stefano Stabellini]

Hi all,

This series is a small collection of PDX fixes. They are technically
independent but discovered together trying to understand the memory
waste caused by the frametable allocation on Xilinx ZynqMP.

Cheers,

Stefano


The following changes since commit dc497635d93f6672f82727ad97a55205177be2aa:

  build system: make install-stubdom depend on install-tools again (2019-04-23 17:00:08 +0100)

are available in the git repository at:

  http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git pdx-fix-1

for you to fetch changes up to efa7a57449a656a3ffac21e9802b6e5a14a0818a:

  xen/arm: fix mask calculation in init_pdx (2019-05-03 13:16:29 -0700)

----------------------------------------------------------------
Stefano Stabellini (3):
      xen/arm: fix nr_pdxs calculation
      xen: actually skip the first MAX_ORDER bits in pfn_pdx_hole_setup
      xen/arm: fix mask calculation in init_pdx

 xen/arch/arm/mm.c    | 4 ++--
 xen/arch/arm/setup.c | 9 +++++++--
 xen/common/pdx.c     | 6 ++++--
 3 files changed, 13 insertions(+), 6 deletions(-)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] xen/arm: fix nr_pdxs calculation

[Stefano Stabellini]

pfn_to_pdx expects an address, not a size, as a parameter. It expects
the end address, and the masks calculations compensate for any holes
between start and end. Pass the end address to pfn_to_pdx. Also remove
from the result pfn_to_pdx(start_address) because we know that we
don't need to cover any memory in the range 0-start in the frametable.

Remove the variable `nr_pages' because it is unused.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: JBeulich@suse.com
---
 xen/arch/arm/mm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 01ae2cc..5cb7932 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -874,8 +874,8 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
 /* Map a frame table to cover physical addresses ps through pe */
 void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
 {
-    unsigned long nr_pages = (pe - ps) >> PAGE_SHIFT;
-    unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
+    unsigned long nr_pdxs = pfn_to_pdx((pe >> PAGE_SHIFT) - 1) -
+                            pfn_to_pdx(ps >> PAGE_SHIFT) + 1;
     unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
     mfn_t base_mfn;
     const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32);
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/3] xen/arm: fix nr_pdxs calculation

[Stefano Stabellini]

pfn_to_pdx expects an address, not a size, as a parameter. It expects
the end address, and the masks calculations compensate for any holes
between start and end. Pass the end address to pfn_to_pdx. Also remove
from the result pfn_to_pdx(start_address) because we know that we
don't need to cover any memory in the range 0-start in the frametable.

Remove the variable `nr_pages' because it is unused.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: JBeulich@suse.com
---
 xen/arch/arm/mm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 01ae2cc..5cb7932 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -874,8 +874,8 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
 /* Map a frame table to cover physical addresses ps through pe */
 void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
 {
-    unsigned long nr_pages = (pe - ps) >> PAGE_SHIFT;
-    unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
+    unsigned long nr_pdxs = pfn_to_pdx((pe >> PAGE_SHIFT) - 1) -
+                            pfn_to_pdx(ps >> PAGE_SHIFT) + 1;
     unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
     mfn_t base_mfn;
     const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32);
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] xen: actually skip the first MAX_ORDER bits in pfn_pdx_hole_setup

[Stefano Stabellini]

pfn_pdx_hole_setup is meant to skip the first MAX_ORDER bits, but
actually it only skips the first MAX_ORDER-1 bits. The issue was
probably introduced by bdb5439c3f, when changing to loop to start from
MAX_ORDER-1 an adjustment by 1 was needed in the call to find_next_bit()
but not done.

Fix the issue by passing j+1 and i+1 to find_next_zero_bit and
find_next_bit. Also add a check for i >= BITS_PER_LONG because
find_{,next_}zero_bit() on x86 assume their last argument to be less
than their middle one.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Signed-off-by: Jan Beulich <JBeulich@suse.com>
CC: andrew.cooper3@citrix.com
CC: JBeulich@suse.com
CC: George.Dunlap@eu.citrix.com
CC: ian.jackson@eu.citrix.com
CC: konrad.wilk@oracle.com
CC: tim@xen.org
CC: wei.liu2@citrix.com

---
 xen/common/pdx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index 50c21b6..bb7e437 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -83,8 +83,10 @@ void __init pfn_pdx_hole_setup(unsigned long mask)
      */
     for ( j = MAX_ORDER-1; ; )
     {
-        i = find_next_zero_bit(&mask, BITS_PER_LONG, j);
-        j = find_next_bit(&mask, BITS_PER_LONG, i);
+        i = find_next_zero_bit(&mask, BITS_PER_LONG, j + 1);
+        if ( i >= BITS_PER_LONG )
+            break;
+        j = find_next_bit(&mask, BITS_PER_LONG, i + 1);
         if ( j >= BITS_PER_LONG )
             break;
         if ( j - i > hole_shift )
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/3] xen: actually skip the first MAX_ORDER bits in pfn_pdx_hole_setup

[Stefano Stabellini]

pfn_pdx_hole_setup is meant to skip the first MAX_ORDER bits, but
actually it only skips the first MAX_ORDER-1 bits. The issue was
probably introduced by bdb5439c3f, when changing to loop to start from
MAX_ORDER-1 an adjustment by 1 was needed in the call to find_next_bit()
but not done.

Fix the issue by passing j+1 and i+1 to find_next_zero_bit and
find_next_bit. Also add a check for i >= BITS_PER_LONG because
find_{,next_}zero_bit() on x86 assume their last argument to be less
than their middle one.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Signed-off-by: Jan Beulich <JBeulich@suse.com>
CC: andrew.cooper3@citrix.com
CC: JBeulich@suse.com
CC: George.Dunlap@eu.citrix.com
CC: ian.jackson@eu.citrix.com
CC: konrad.wilk@oracle.com
CC: tim@xen.org
CC: wei.liu2@citrix.com

---
 xen/common/pdx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index 50c21b6..bb7e437 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -83,8 +83,10 @@ void __init pfn_pdx_hole_setup(unsigned long mask)
      */
     for ( j = MAX_ORDER-1; ; )
     {
-        i = find_next_zero_bit(&mask, BITS_PER_LONG, j);
-        j = find_next_bit(&mask, BITS_PER_LONG, i);
+        i = find_next_zero_bit(&mask, BITS_PER_LONG, j + 1);
+        if ( i >= BITS_PER_LONG )
+            break;
+        j = find_next_bit(&mask, BITS_PER_LONG, i + 1);
         if ( j >= BITS_PER_LONG )
             break;
         if ( j - i > hole_shift )
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Stefano Stabellini]

The mask calculation in init_pdx is wrong when the first bank starts at
address 0x0. The reason is that pdx_init_mask will do '0 - 1' causing an
underflow. As a result, the mask becomes 0xffffffffffffffff which is the
biggest possible mask and ends up causing a significant memory waste in
the frametable size computation.

For instance, on platforms that have a low memory bank and a high memory
bank, the frametable will end up covering all the holes in between.

The purpose of the mask is to be passed as a parameter to
pfn_pdx_hole_setup, which based on the mask parameter caculates
pfn_pdx_hole_shift, pfn_pdx_bottom_mask, etc. which are actually the
important masks for frametable initialization later on.

pfn_pdx_hole_setup never compresses addresses below MAX_ORDER bits (1GB
on ARM). Thus, it is safe to initialize mask passing 1ULL << (MAX_ORDER
+ PAGE_SHIFT) as start address to pdx_init_mask.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: JBeulich@suse.com
---
 xen/arch/arm/setup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ccb0f18..22f20bb 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
 static void __init init_pdx(void)
 {
     paddr_t bank_start, bank_size, bank_end;
-
-    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
+    u64 mask;
     int bank;
 
+    /*
+     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left
+     * uncompressed.
+     */
+    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
+
     for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
     {
         bank_start = bootinfo.mem.bank[bank].start;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Stefano Stabellini]

The mask calculation in init_pdx is wrong when the first bank starts at
address 0x0. The reason is that pdx_init_mask will do '0 - 1' causing an
underflow. As a result, the mask becomes 0xffffffffffffffff which is the
biggest possible mask and ends up causing a significant memory waste in
the frametable size computation.

For instance, on platforms that have a low memory bank and a high memory
bank, the frametable will end up covering all the holes in between.

The purpose of the mask is to be passed as a parameter to
pfn_pdx_hole_setup, which based on the mask parameter caculates
pfn_pdx_hole_shift, pfn_pdx_bottom_mask, etc. which are actually the
important masks for frametable initialization later on.

pfn_pdx_hole_setup never compresses addresses below MAX_ORDER bits (1GB
on ARM). Thus, it is safe to initialize mask passing 1ULL << (MAX_ORDER
+ PAGE_SHIFT) as start address to pdx_init_mask.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: JBeulich@suse.com
---
 xen/arch/arm/setup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ccb0f18..22f20bb 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
 static void __init init_pdx(void)
 {
     paddr_t bank_start, bank_size, bank_end;
-
-    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
+    u64 mask;
     int bank;
 
+    /*
+     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left
+     * uncompressed.
+     */
+    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
+
     for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
     {
         bank_start = bootinfo.mem.bank[bank].start;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Jan Beulich]

>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>  static void __init init_pdx(void)
>  {
>      paddr_t bank_start, bank_size, bank_end;
> -
> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
> +    u64 mask;
>      int bank;
>  
> +    /*
> +     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left

"... pages of RAM." ?

> +     * uncompressed.
> +     */
> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));

PAGE_SIZE << MAX_ORDER?

I wonder whether pdx_init_mask() itself wouldn't better apply this
(lower) cap.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Jan Beulich]

>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>  static void __init init_pdx(void)
>  {
>      paddr_t bank_start, bank_size, bank_end;
> -
> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
> +    u64 mask;
>      int bank;
>  
> +    /*
> +     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left

"... pages of RAM." ?

> +     * uncompressed.
> +     */
> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));

PAGE_SIZE << MAX_ORDER?

I wonder whether pdx_init_mask() itself wouldn't better apply this
(lower) cap.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen: actually skip the first MAX_ORDER bits in pfn_pdx_hole_setup

[Jan Beulich]

>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
> Fix the issue by passing j+1 and i+1 to find_next_zero_bit and
> find_next_bit. Also add a check for i >= BITS_PER_LONG because
> find_{,next_}zero_bit() on x86 assume their last argument to be less
> than their middle one.

I had pointed out x86 since I knew it has this assumption. Now
that you mention it here, I would have expected you've checked
that Arm doesn't make similar assumptions. 32-bit Arm looks to
do, though (while 64-bit has a dedicated if() to deal with the
situation).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] xen: actually skip the first MAX_ORDER bits in pfn_pdx_hole_setup

[Jan Beulich]

>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
> Fix the issue by passing j+1 and i+1 to find_next_zero_bit and
> find_next_bit. Also add a check for i >= BITS_PER_LONG because
> find_{,next_}zero_bit() on x86 assume their last argument to be less
> than their middle one.

I had pointed out x86 since I knew it has this assumption. Now
that you mention it here, I would have expected you've checked
that Arm doesn't make similar assumptions. 32-bit Arm looks to
do, though (while 64-bit has a dedicated if() to deal with the
situation).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Julien Grall]

Hi Jan,

On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>>   static void __init init_pdx(void)
>>   {
>>       paddr_t bank_start, bank_size, bank_end;
>> -
>> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
>> +    u64 mask;
>>       int bank;
>>   
>> +    /*
>> +     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left
> 
> "... pages of RAM." ?
> 
>> +     * uncompressed.
>> +     */
>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
> 
> PAGE_SIZE << MAX_ORDER?

Hmmm, I am not entirely convince this will give the correct value 
because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.

> 
> I wonder whether pdx_init_mask() itself wouldn't better apply this
> (lower) cap

Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the 
init mask?

Note that the later will not produce the same behavior as this patch.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Julien Grall]

Hi Jan,

On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>>   static void __init init_pdx(void)
>>   {
>>       paddr_t bank_start, bank_size, bank_end;
>> -
>> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
>> +    u64 mask;
>>       int bank;
>>   
>> +    /*
>> +     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left
> 
> "... pages of RAM." ?
> 
>> +     * uncompressed.
>> +     */
>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
> 
> PAGE_SIZE << MAX_ORDER?

Hmmm, I am not entirely convince this will give the correct value 
because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.

> 
> I wonder whether pdx_init_mask() itself wouldn't better apply this
> (lower) cap

Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the 
init mask?

Note that the later will not produce the same behavior as this patch.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Jan Beulich]

>>> On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
> On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>>>   static void __init init_pdx(void)
>>>   {
>>>       paddr_t bank_start, bank_size, bank_end;
>>> -
>>> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
>>> +    u64 mask;
>>>       int bank;
>>>   
>>> +    /*
>>> +     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left
>> 
>> "... pages of RAM." ?
>> 
>>> +     * uncompressed.
>>> +     */
>>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
>> 
>> PAGE_SIZE << MAX_ORDER?
> 
> Hmmm, I am not entirely convince this will give the correct value 
> because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.

Oh, indeed, for an abstract 32-bit arch this would de-generate, due
to MAX_ORDER being 20. Nevertheless I think the expression used
invites for "cleaning up" (making the same mistake I've made), and
since this is in Arm-specific code (where MAX_ORDER is 18) I think it
would still be better to use the suggested alternative.

>> I wonder whether pdx_init_mask() itself wouldn't better apply this
>> (lower) cap
> 
> Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the 
> init mask?
> 
> Note that the later will not produce the same behavior as this patch.

Since I'm not sure I understand what you mean with "capping the
init mask", I'm also uncertain what alternative behavior you're
thinking of. What I'm suggesting is

u64 __init pdx_init_mask(u64 base_addr)
{
    return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
}

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Jan Beulich]

>>> On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
> On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>>>   static void __init init_pdx(void)
>>>   {
>>>       paddr_t bank_start, bank_size, bank_end;
>>> -
>>> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
>>> +    u64 mask;
>>>       int bank;
>>>   
>>> +    /*
>>> +     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left
>> 
>> "... pages of RAM." ?
>> 
>>> +     * uncompressed.
>>> +     */
>>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
>> 
>> PAGE_SIZE << MAX_ORDER?
> 
> Hmmm, I am not entirely convince this will give the correct value 
> because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.

Oh, indeed, for an abstract 32-bit arch this would de-generate, due
to MAX_ORDER being 20. Nevertheless I think the expression used
invites for "cleaning up" (making the same mistake I've made), and
since this is in Arm-specific code (where MAX_ORDER is 18) I think it
would still be better to use the suggested alternative.

>> I wonder whether pdx_init_mask() itself wouldn't better apply this
>> (lower) cap
> 
> Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the 
> init mask?
> 
> Note that the later will not produce the same behavior as this patch.

Since I'm not sure I understand what you mean with "capping the
init mask", I'm also uncertain what alternative behavior you're
thinking of. What I'm suggesting is

u64 __init pdx_init_mask(u64 base_addr)
{
    return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
}

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Julien Grall]

Hi Jan,

On 5/7/19 8:40 AM, Jan Beulich wrote:
>>>> On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
>> On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>>>>    static void __init init_pdx(void)
>>>>    {
>>>>        paddr_t bank_start, bank_size, bank_end;
>>>> -
>>>> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
>>>> +    u64 mask;
>>>>        int bank;
>>>>    
>>>> +    /*
>>>> +     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left
>>>
>>> "... pages of RAM." ?
>>>
>>>> +     * uncompressed.
>>>> +     */
>>>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
>>>
>>> PAGE_SIZE << MAX_ORDER?
>>
>> Hmmm, I am not entirely convince this will give the correct value
>> because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
> 
> Oh, indeed, for an abstract 32-bit arch this would de-generate, due
> to MAX_ORDER being 20. Nevertheless I think the expression used
> invites for "cleaning up" (making the same mistake I've made), and
> since this is in Arm-specific code (where MAX_ORDER is 18) I think it
> would still be better to use the suggested alternative.

The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that 
PAGE_SIZE should use signed quantities. So I am not sure whether it is 
safe to switch to UL here.

If we keep PAGE_SIZE as signed quantities, then I would rather not used 
your suggestion because this may introduce buggy code if MAX_ORDER is 
ever updated on Arm.

> 
>>> I wonder whether pdx_init_mask() itself wouldn't better apply this
>>> (lower) cap
>>
>> Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
>> init mask?
>>
>> Note that the later will not produce the same behavior as this patch.
> 
> Since I'm not sure I understand what you mean with "capping the
> init mask", I'm also uncertain what alternative behavior you're
> thinking of. What I'm suggesting is
> 
> u64 __init pdx_init_mask(u64 base_addr)
> {
>      return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
> }

As I pointed out in the original thread, there are a couple of issues 
with this suggestion:
	1) banks are not ordered on Arm, so the pdx mask may get initialized 
with a higher bank address preventing the PDX compression to work
	2) the PDX will not be able to compress any bits between 0 and the MSB 
1' in the base_addr. In other word, for a base address 0x200000000 
(8GB), the initial mask will be  0x1ffffffff. I am aware of some 
platforms with some interesting first bank base address (i.e above 4GB).

2) is not overly critical, but I think 1) should be addressed.

At least on Arm, I don't see any real value to initialize the PDX mask 
with a base address. I would be more keen to implement pdx_init_mask() as:

return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Julien Grall]

Hi Jan,

On 5/7/19 8:40 AM, Jan Beulich wrote:
>>>> On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
>> On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>>>>    static void __init init_pdx(void)
>>>>    {
>>>>        paddr_t bank_start, bank_size, bank_end;
>>>> -
>>>> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
>>>> +    u64 mask;
>>>>        int bank;
>>>>    
>>>> +    /*
>>>> +     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left
>>>
>>> "... pages of RAM." ?
>>>
>>>> +     * uncompressed.
>>>> +     */
>>>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
>>>
>>> PAGE_SIZE << MAX_ORDER?
>>
>> Hmmm, I am not entirely convince this will give the correct value
>> because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
> 
> Oh, indeed, for an abstract 32-bit arch this would de-generate, due
> to MAX_ORDER being 20. Nevertheless I think the expression used
> invites for "cleaning up" (making the same mistake I've made), and
> since this is in Arm-specific code (where MAX_ORDER is 18) I think it
> would still be better to use the suggested alternative.

The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that 
PAGE_SIZE should use signed quantities. So I am not sure whether it is 
safe to switch to UL here.

If we keep PAGE_SIZE as signed quantities, then I would rather not used 
your suggestion because this may introduce buggy code if MAX_ORDER is 
ever updated on Arm.

> 
>>> I wonder whether pdx_init_mask() itself wouldn't better apply this
>>> (lower) cap
>>
>> Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
>> init mask?
>>
>> Note that the later will not produce the same behavior as this patch.
> 
> Since I'm not sure I understand what you mean with "capping the
> init mask", I'm also uncertain what alternative behavior you're
> thinking of. What I'm suggesting is
> 
> u64 __init pdx_init_mask(u64 base_addr)
> {
>      return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
> }

As I pointed out in the original thread, there are a couple of issues 
with this suggestion:
	1) banks are not ordered on Arm, so the pdx mask may get initialized 
with a higher bank address preventing the PDX compression to work
	2) the PDX will not be able to compress any bits between 0 and the MSB 
1' in the base_addr. In other word, for a base address 0x200000000 
(8GB), the initial mask will be  0x1ffffffff. I am aware of some 
platforms with some interesting first bank base address (i.e above 4GB).

2) is not overly critical, but I think 1) should be addressed.

At least on Arm, I don't see any real value to initialize the PDX mask 
with a base address. I would be more keen to implement pdx_init_mask() as:

return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Jan Beulich]

>>> On 07.05.19 at 10:59, <julien.grall@arm.com> wrote:
> On 5/7/19 8:40 AM, Jan Beulich wrote:
>>>>> On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
>>> On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>>>>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
>>>>
>>>> PAGE_SIZE << MAX_ORDER?
>>>
>>> Hmmm, I am not entirely convince this will give the correct value
>>> because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
>> 
>> Oh, indeed, for an abstract 32-bit arch this would de-generate, due
>> to MAX_ORDER being 20. Nevertheless I think the expression used
>> invites for "cleaning up" (making the same mistake I've made), and
>> since this is in Arm-specific code (where MAX_ORDER is 18) I think it
>> would still be better to use the suggested alternative.
> 
> The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that 
> PAGE_SIZE should use signed quantities. So I am not sure whether it is 
> safe to switch to UL here.

It's not (at least when keeping past x86-32 in the picture): Extending
to unsigned long long works differently when the type is "unsigned long".
This matters when using things like ~(PAGE_SIZE - 1).

> If we keep PAGE_SIZE as signed quantities, then I would rather not used 
> your suggestion because this may introduce buggy code if MAX_ORDER is 
> ever updated on Arm.

A BUILD_BUG_ON() could help prevent this.

>>>> I wonder whether pdx_init_mask() itself wouldn't better apply this
>>>> (lower) cap
>>>
>>> Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
>>> init mask?
>>>
>>> Note that the later will not produce the same behavior as this patch.
>> 
>> Since I'm not sure I understand what you mean with "capping the
>> init mask", I'm also uncertain what alternative behavior you're
>> thinking of. What I'm suggesting is
>> 
>> u64 __init pdx_init_mask(u64 base_addr)
>> {
>>      return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
>> }
> 
> As I pointed out in the original thread, there are a couple of issues 
> with this suggestion:
> 	1) banks are not ordered on Arm, so the pdx mask may get initialized 
> with a higher bank address preventing the PDX compression to work

This is orthogonal to my suggestion here. It's up to Arm code to
call the function with the lowest bank's base address instead of
the first one.

> 	2) the PDX will not be able to compress any bits between 0 and the MSB 
> 1' in the base_addr. In other word, for a base address 0x200000000 
> (8GB), the initial mask will be  0x1ffffffff. I am aware of some 
> platforms with some interesting first bank base address (i.e above 4GB).

Well, we'd been there before: More "interesting" layouts may
indeed require adjustments to the logic. The particular case
we've been talking about was there not being _any_ RAM
below a certain boundary.

> 2) is not overly critical, but I think 1) should be addressed.
> 
> At least on Arm, I don't see any real value to initialize the PDX mask 
> with a base address. I would be more keen to implement pdx_init_mask() as:
> 
> return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);

But (besides the missing closing parenthese) that's not what x86 wants.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Jan Beulich]

>>> On 07.05.19 at 10:59, <julien.grall@arm.com> wrote:
> On 5/7/19 8:40 AM, Jan Beulich wrote:
>>>>> On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
>>> On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>>>>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
>>>>
>>>> PAGE_SIZE << MAX_ORDER?
>>>
>>> Hmmm, I am not entirely convince this will give the correct value
>>> because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
>> 
>> Oh, indeed, for an abstract 32-bit arch this would de-generate, due
>> to MAX_ORDER being 20. Nevertheless I think the expression used
>> invites for "cleaning up" (making the same mistake I've made), and
>> since this is in Arm-specific code (where MAX_ORDER is 18) I think it
>> would still be better to use the suggested alternative.
> 
> The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that 
> PAGE_SIZE should use signed quantities. So I am not sure whether it is 
> safe to switch to UL here.

It's not (at least when keeping past x86-32 in the picture): Extending
to unsigned long long works differently when the type is "unsigned long".
This matters when using things like ~(PAGE_SIZE - 1).

> If we keep PAGE_SIZE as signed quantities, then I would rather not used 
> your suggestion because this may introduce buggy code if MAX_ORDER is 
> ever updated on Arm.

A BUILD_BUG_ON() could help prevent this.

>>>> I wonder whether pdx_init_mask() itself wouldn't better apply this
>>>> (lower) cap
>>>
>>> Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
>>> init mask?
>>>
>>> Note that the later will not produce the same behavior as this patch.
>> 
>> Since I'm not sure I understand what you mean with "capping the
>> init mask", I'm also uncertain what alternative behavior you're
>> thinking of. What I'm suggesting is
>> 
>> u64 __init pdx_init_mask(u64 base_addr)
>> {
>>      return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
>> }
> 
> As I pointed out in the original thread, there are a couple of issues 
> with this suggestion:
> 	1) banks are not ordered on Arm, so the pdx mask may get initialized 
> with a higher bank address preventing the PDX compression to work

This is orthogonal to my suggestion here. It's up to Arm code to
call the function with the lowest bank's base address instead of
the first one.

> 	2) the PDX will not be able to compress any bits between 0 and the MSB 
> 1' in the base_addr. In other word, for a base address 0x200000000 
> (8GB), the initial mask will be  0x1ffffffff. I am aware of some 
> platforms with some interesting first bank base address (i.e above 4GB).

Well, we'd been there before: More "interesting" layouts may
indeed require adjustments to the logic. The particular case
we've been talking about was there not being _any_ RAM
below a certain boundary.

> 2) is not overly critical, but I think 1) should be addressed.
> 
> At least on Arm, I don't see any real value to initialize the PDX mask 
> with a base address. I would be more keen to implement pdx_init_mask() as:
> 
> return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);

But (besides the missing closing parenthese) that's not what x86 wants.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Julien Grall]

Hi,

On 5/7/19 10:35 AM, Jan Beulich wrote:
>>>> On 07.05.19 at 10:59, <julien.grall@arm.com> wrote:
>> On 5/7/19 8:40 AM, Jan Beulich wrote:
>>>>>> On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
>>>> On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>>>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>>>>>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
>>>>>
>>>>> PAGE_SIZE << MAX_ORDER?
>>>>
>>>> Hmmm, I am not entirely convince this will give the correct value
>>>> because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
>>>
>>> Oh, indeed, for an abstract 32-bit arch this would de-generate, due
>>> to MAX_ORDER being 20. Nevertheless I think the expression used
>>> invites for "cleaning up" (making the same mistake I've made), and
>>> since this is in Arm-specific code (where MAX_ORDER is 18) I think it
>>> would still be better to use the suggested alternative.
>>
>> The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that
>> PAGE_SIZE should use signed quantities. So I am not sure whether it is
>> safe to switch to UL here.
> 
> It's not (at least when keeping past x86-32 in the picture): Extending
> to unsigned long long works differently when the type is "unsigned long".
> This matters when using things like ~(PAGE_SIZE - 1).
> 
>> If we keep PAGE_SIZE as signed quantities, then I would rather not used
>> your suggestion because this may introduce buggy code if MAX_ORDER is
>> ever updated on Arm.
> 
> A BUILD_BUG_ON() could help prevent this.

Good point.

> 
>>>>> I wonder whether pdx_init_mask() itself wouldn't better apply this
>>>>> (lower) cap
>>>>
>>>> Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
>>>> init mask?
>>>>
>>>> Note that the later will not produce the same behavior as this patch.
>>>
>>> Since I'm not sure I understand what you mean with "capping the
>>> init mask", I'm also uncertain what alternative behavior you're
>>> thinking of. What I'm suggesting is
>>>
>>> u64 __init pdx_init_mask(u64 base_addr)
>>> {
>>>       return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
>>> }
>>
>> As I pointed out in the original thread, there are a couple of issues
>> with this suggestion:
>> 	1) banks are not ordered on Arm, so the pdx mask may get initialized
>> with a higher bank address preventing the PDX compression to work
> 
> This is orthogonal to my suggestion here. It's up to Arm code to
> call the function with the lowest bank's base address instead of
> the first one. >
>> 	2) the PDX will not be able to compress any bits between 0 and the MSB
>> 1' in the base_addr. In other word, for a base address 0x200000000
>> (8GB), the initial mask will be  0x1ffffffff. I am aware of some
>> platforms with some interesting first bank base address (i.e above 4GB).
> 
> Well, we'd been there before: More "interesting" layouts may
> indeed require adjustments to the logic. The particular case
> we've been talking about was there not being _any_ RAM
> below a certain boundary.
Yes this is unrelated to the case Stefano is trying to fix, however 
Stefano & I have also been discussing of other potential issues with PDX.

I would rather try to address the most important/concerning one at the 
same time. Stefano's patch is actually fixing all the known issues with 
PDX on Arm.

>> 2) is not overly critical, but I think 1) should be addressed.
>>
>> At least on Arm, I don't see any real value to initialize the PDX mask
>> with a base address. I would be more keen to implement pdx_init_mask() as:
>>
>> return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);
> 
> But (besides the missing closing parenthese) that's not what x86 wants.

Could you remind me why?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Julien Grall]

Hi,

On 5/7/19 10:35 AM, Jan Beulich wrote:
>>>> On 07.05.19 at 10:59, <julien.grall@arm.com> wrote:
>> On 5/7/19 8:40 AM, Jan Beulich wrote:
>>>>>> On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
>>>> On 5/6/19 10:06 AM, Jan Beulich wrote:
>>>>>>>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
>>>>>> +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
>>>>>
>>>>> PAGE_SIZE << MAX_ORDER?
>>>>
>>>> Hmmm, I am not entirely convince this will give the correct value
>>>> because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
>>>
>>> Oh, indeed, for an abstract 32-bit arch this would de-generate, due
>>> to MAX_ORDER being 20. Nevertheless I think the expression used
>>> invites for "cleaning up" (making the same mistake I've made), and
>>> since this is in Arm-specific code (where MAX_ORDER is 18) I think it
>>> would still be better to use the suggested alternative.
>>
>> The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that
>> PAGE_SIZE should use signed quantities. So I am not sure whether it is
>> safe to switch to UL here.
> 
> It's not (at least when keeping past x86-32 in the picture): Extending
> to unsigned long long works differently when the type is "unsigned long".
> This matters when using things like ~(PAGE_SIZE - 1).
> 
>> If we keep PAGE_SIZE as signed quantities, then I would rather not used
>> your suggestion because this may introduce buggy code if MAX_ORDER is
>> ever updated on Arm.
> 
> A BUILD_BUG_ON() could help prevent this.

Good point.

> 
>>>>> I wonder whether pdx_init_mask() itself wouldn't better apply this
>>>>> (lower) cap
>>>>
>>>> Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
>>>> init mask?
>>>>
>>>> Note that the later will not produce the same behavior as this patch.
>>>
>>> Since I'm not sure I understand what you mean with "capping the
>>> init mask", I'm also uncertain what alternative behavior you're
>>> thinking of. What I'm suggesting is
>>>
>>> u64 __init pdx_init_mask(u64 base_addr)
>>> {
>>>       return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
>>> }
>>
>> As I pointed out in the original thread, there are a couple of issues
>> with this suggestion:
>> 	1) banks are not ordered on Arm, so the pdx mask may get initialized
>> with a higher bank address preventing the PDX compression to work
> 
> This is orthogonal to my suggestion here. It's up to Arm code to
> call the function with the lowest bank's base address instead of
> the first one. >
>> 	2) the PDX will not be able to compress any bits between 0 and the MSB
>> 1' in the base_addr. In other word, for a base address 0x200000000
>> (8GB), the initial mask will be  0x1ffffffff. I am aware of some
>> platforms with some interesting first bank base address (i.e above 4GB).
> 
> Well, we'd been there before: More "interesting" layouts may
> indeed require adjustments to the logic. The particular case
> we've been talking about was there not being _any_ RAM
> below a certain boundary.
Yes this is unrelated to the case Stefano is trying to fix, however 
Stefano & I have also been discussing of other potential issues with PDX.

I would rather try to address the most important/concerning one at the 
same time. Stefano's patch is actually fixing all the known issues with 
PDX on Arm.

>> 2) is not overly critical, but I think 1) should be addressed.
>>
>> At least on Arm, I don't see any real value to initialize the PDX mask
>> with a base address. I would be more keen to implement pdx_init_mask() as:
>>
>> return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);
> 
> But (besides the missing closing parenthese) that's not what x86 wants.

Could you remind me why?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Jan Beulich]

>>> On 07.05.19 at 15:20, <julien.grall@arm.com> wrote:
> On 5/7/19 10:35 AM, Jan Beulich wrote:
>>>>> On 07.05.19 at 10:59, <julien.grall@arm.com> wrote:
>>> At least on Arm, I don't see any real value to initialize the PDX mask
>>> with a base address. I would be more keen to implement pdx_init_mask() as:
>>>
>>> return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);
>> 
>> But (besides the missing closing parenthese) that's not what x86 wants.
> 
> Could you remind me why?

Because we don't want to compress on the low 32 bits, for there
being "interesting" things below 4Gb.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Jan Beulich]

>>> On 07.05.19 at 15:20, <julien.grall@arm.com> wrote:
> On 5/7/19 10:35 AM, Jan Beulich wrote:
>>>>> On 07.05.19 at 10:59, <julien.grall@arm.com> wrote:
>>> At least on Arm, I don't see any real value to initialize the PDX mask
>>> with a base address. I would be more keen to implement pdx_init_mask() as:
>>>
>>> return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);
>> 
>> But (besides the missing closing parenthese) that's not what x86 wants.
> 
> Could you remind me why?

Because we don't want to compress on the low 32 bits, for there
being "interesting" things below 4Gb.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen: actually skip the first MAX_ORDER bits in pfn_pdx_hole_setup

[Stefano Stabellini]

On Mon, 6 May 2019, Jan Beulich wrote:
> >>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
> > Fix the issue by passing j+1 and i+1 to find_next_zero_bit and
> > find_next_bit. Also add a check for i >= BITS_PER_LONG because
> > find_{,next_}zero_bit() on x86 assume their last argument to be less
> > than their middle one.
> 
> I had pointed out x86 since I knew it has this assumption. Now
> that you mention it here, I would have expected you've checked
> that Arm doesn't make similar assumptions. 32-bit Arm looks to
> do, though (while 64-bit has a dedicated if() to deal with the
> situation).

I think that either way we want to say that those functions are not
supposed to be called that way. I'll update the commit message.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] xen: actually skip the first MAX_ORDER bits in pfn_pdx_hole_setup

[Stefano Stabellini]

On Mon, 6 May 2019, Jan Beulich wrote:
> >>> On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
> > Fix the issue by passing j+1 and i+1 to find_next_zero_bit and
> > find_next_bit. Also add a check for i >= BITS_PER_LONG because
> > find_{,next_}zero_bit() on x86 assume their last argument to be less
> > than their middle one.
> 
> I had pointed out x86 since I knew it has this assumption. Now
> that you mention it here, I would have expected you've checked
> that Arm doesn't make similar assumptions. 32-bit Arm looks to
> do, though (while 64-bit has a dedicated if() to deal with the
> situation).

I think that either way we want to say that those functions are not
supposed to be called that way. I'll update the commit message.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Stefano Stabellini]

On Tue, 7 May 2019, Julien Grall wrote:
> Hi,
> 
> On 5/7/19 10:35 AM, Jan Beulich wrote:
> > > > > On 07.05.19 at 10:59, <julien.grall@arm.com> wrote:
> > > On 5/7/19 8:40 AM, Jan Beulich wrote:
> > > > > > > On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
> > > > > On 5/6/19 10:06 AM, Jan Beulich wrote:
> > > > > > > > > On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
> > > > > > > +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
> > > > > > 
> > > > > > PAGE_SIZE << MAX_ORDER?
> > > > > 
> > > > > Hmmm, I am not entirely convince this will give the correct value
> > > > > because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
> > > > 
> > > > Oh, indeed, for an abstract 32-bit arch this would de-generate, due
> > > > to MAX_ORDER being 20. Nevertheless I think the expression used
> > > > invites for "cleaning up" (making the same mistake I've made), and
> > > > since this is in Arm-specific code (where MAX_ORDER is 18) I think it
> > > > would still be better to use the suggested alternative.
> > > 
> > > The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that
> > > PAGE_SIZE should use signed quantities. So I am not sure whether it is
> > > safe to switch to UL here.
> > 
> > It's not (at least when keeping past x86-32 in the picture): Extending
> > to unsigned long long works differently when the type is "unsigned long".
> > This matters when using things like ~(PAGE_SIZE - 1).
> > 
> > > If we keep PAGE_SIZE as signed quantities, then I would rather not used
> > > your suggestion because this may introduce buggy code if MAX_ORDER is
> > > ever updated on Arm.
> > 
> > A BUILD_BUG_ON() could help prevent this.
> 
> Good point.

Fair enough, but I would rather keep the original version because I don't see
how PAGE_SIZE << MAX_ORDER could be an improvement. It is also more
understandable I think.


> > > > > > I wonder whether pdx_init_mask() itself wouldn't better apply this
> > > > > > (lower) cap
> > > > > 
> > > > > Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
> > > > > init mask?
> > > > > 
> > > > > Note that the later will not produce the same behavior as this patch.
> > > > 
> > > > Since I'm not sure I understand what you mean with "capping the
> > > > init mask", I'm also uncertain what alternative behavior you're
> > > > thinking of. What I'm suggesting is
> > > > 
> > > > u64 __init pdx_init_mask(u64 base_addr)
> > > > {
> > > >       return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER)
> > > > - 1);
> > > > }
> > > 
> > > As I pointed out in the original thread, there are a couple of issues
> > > with this suggestion:
> > > 	1) banks are not ordered on Arm, so the pdx mask may get initialized
> > > with a higher bank address preventing the PDX compression to work
> > 
> > This is orthogonal to my suggestion here. It's up to Arm code to
> > call the function with the lowest bank's base address instead of
> > the first one. >
> > > 	2) the PDX will not be able to compress any bits between 0 and the MSB
> > > 1' in the base_addr. In other word, for a base address 0x200000000
> > > (8GB), the initial mask will be  0x1ffffffff. I am aware of some
> > > platforms with some interesting first bank base address (i.e above 4GB).
> > 
> > Well, we'd been there before: More "interesting" layouts may
> > indeed require adjustments to the logic. The particular case
> > we've been talking about was there not being _any_ RAM
> > below a certain boundary.
> Yes this is unrelated to the case Stefano is trying to fix, however Stefano &
> I have also been discussing of other potential issues with PDX.
> 
> I would rather try to address the most important/concerning one at the same
> time. Stefano's patch is actually fixing all the known issues with PDX on Arm.
> 
> > > 2) is not overly critical, but I think 1) should be addressed.
> > > 
> > > At least on Arm, I don't see any real value to initialize the PDX mask
> > > with a base address. I would be more keen to implement pdx_init_mask() as:
> > > 
> > > return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);
> > 
> > But (besides the missing closing parenthese) that's not what x86 wants.

It is not a problem to move the change into pdx_init_mask, and it could
make sense to do so. From init_pdx we'll just pass 0x0 to get the
behavior of the current patch.

I'll send an update

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Stefano Stabellini]

On Tue, 7 May 2019, Julien Grall wrote:
> Hi,
> 
> On 5/7/19 10:35 AM, Jan Beulich wrote:
> > > > > On 07.05.19 at 10:59, <julien.grall@arm.com> wrote:
> > > On 5/7/19 8:40 AM, Jan Beulich wrote:
> > > > > > > On 06.05.19 at 17:26, <julien.grall@arm.com> wrote:
> > > > > On 5/6/19 10:06 AM, Jan Beulich wrote:
> > > > > > > > > On 03.05.19 at 22:50, <sstabellini@kernel.org> wrote:
> > > > > > > +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
> > > > > > 
> > > > > > PAGE_SIZE << MAX_ORDER?
> > > > > 
> > > > > Hmmm, I am not entirely convince this will give the correct value
> > > > > because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
> > > > 
> > > > Oh, indeed, for an abstract 32-bit arch this would de-generate, due
> > > > to MAX_ORDER being 20. Nevertheless I think the expression used
> > > > invites for "cleaning up" (making the same mistake I've made), and
> > > > since this is in Arm-specific code (where MAX_ORDER is 18) I think it
> > > > would still be better to use the suggested alternative.
> > > 
> > > The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that
> > > PAGE_SIZE should use signed quantities. So I am not sure whether it is
> > > safe to switch to UL here.
> > 
> > It's not (at least when keeping past x86-32 in the picture): Extending
> > to unsigned long long works differently when the type is "unsigned long".
> > This matters when using things like ~(PAGE_SIZE - 1).
> > 
> > > If we keep PAGE_SIZE as signed quantities, then I would rather not used
> > > your suggestion because this may introduce buggy code if MAX_ORDER is
> > > ever updated on Arm.
> > 
> > A BUILD_BUG_ON() could help prevent this.
> 
> Good point.

Fair enough, but I would rather keep the original version because I don't see
how PAGE_SIZE << MAX_ORDER could be an improvement. It is also more
understandable I think.


> > > > > > I wonder whether pdx_init_mask() itself wouldn't better apply this
> > > > > > (lower) cap
> > > > > 
> > > > > Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
> > > > > init mask?
> > > > > 
> > > > > Note that the later will not produce the same behavior as this patch.
> > > > 
> > > > Since I'm not sure I understand what you mean with "capping the
> > > > init mask", I'm also uncertain what alternative behavior you're
> > > > thinking of. What I'm suggesting is
> > > > 
> > > > u64 __init pdx_init_mask(u64 base_addr)
> > > > {
> > > >       return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER)
> > > > - 1);
> > > > }
> > > 
> > > As I pointed out in the original thread, there are a couple of issues
> > > with this suggestion:
> > > 	1) banks are not ordered on Arm, so the pdx mask may get initialized
> > > with a higher bank address preventing the PDX compression to work
> > 
> > This is orthogonal to my suggestion here. It's up to Arm code to
> > call the function with the lowest bank's base address instead of
> > the first one. >
> > > 	2) the PDX will not be able to compress any bits between 0 and the MSB
> > > 1' in the base_addr. In other word, for a base address 0x200000000
> > > (8GB), the initial mask will be  0x1ffffffff. I am aware of some
> > > platforms with some interesting first bank base address (i.e above 4GB).
> > 
> > Well, we'd been there before: More "interesting" layouts may
> > indeed require adjustments to the logic. The particular case
> > we've been talking about was there not being _any_ RAM
> > below a certain boundary.
> Yes this is unrelated to the case Stefano is trying to fix, however Stefano &
> I have also been discussing of other potential issues with PDX.
> 
> I would rather try to address the most important/concerning one at the same
> time. Stefano's patch is actually fixing all the known issues with PDX on Arm.
> 
> > > 2) is not overly critical, but I think 1) should be addressed.
> > > 
> > > At least on Arm, I don't see any real value to initialize the PDX mask
> > > with a base address. I would be more keen to implement pdx_init_mask() as:
> > > 
> > > return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);
> > 
> > But (besides the missing closing parenthese) that's not what x86 wants.

It is not a problem to move the change into pdx_init_mask, and it could
make sense to do so. From init_pdx we'll just pass 0x0 to get the
behavior of the current patch.

I'll send an update

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel