[PATCH v3 0/3] PDX fixes

[PATCH v3 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 844aa0a13d34e9a341a8374119d2ed67d4dcd6bb:

  sched_null: superficial clean-ups (2019-06-03 17:56:20 +0200)

are available in the Git repository at:

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

for you to fetch changes up to c92f589e7cf66fc9c3ad8a812cdbc69214a812d1:

  xen/arm: fix mask calculation in pdx_init_mask (2019-06-03 14:42:10 -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 pdx_init_mask

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

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

[Xen-devel] [PATCH v3 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 844aa0a13d34e9a341a8374119d2ed67d4dcd6bb:

  sched_null: superficial clean-ups (2019-06-03 17:56:20 +0200)

are available in the Git repository at:

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

for you to fetch changes up to c92f589e7cf66fc9c3ad8a812cdbc69214a812d1:

  xen/arm: fix mask calculation in pdx_init_mask (2019-06-03 14:42:10 -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 pdx_init_mask

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

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

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

[Stefano Stabellini]

pfn_to_pdx expects an address, not a size, as a parameter. Specifically,
it expects the end address, then the masks calculations compensate for
any holes between start and end. Thus, we should pass the end address to
pfn_to_pdx.

The initial pdx is stored in frametable_base_pdx, so we can subtract the
result of pfn_to_pdx(start_address) from nr_pdxs; 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
---
Changes in v3:
- improve commit message

Changes in v2:
- use mfn_to_pdx and maddr_to_mfn along with mfn_add()
---
 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 6ac169ce27..22ed91b5c9 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -886,8 +886,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 = mfn_to_pdx(mfn_add(maddr_to_mfn(pe), -1)) -
+                            mfn_to_pdx(maddr_to_mfn(ps)) + 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);
-- 
2.17.1


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

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

[Stefano Stabellini]

pfn_to_pdx expects an address, not a size, as a parameter. Specifically,
it expects the end address, then the masks calculations compensate for
any holes between start and end. Thus, we should pass the end address to
pfn_to_pdx.

The initial pdx is stored in frametable_base_pdx, so we can subtract the
result of pfn_to_pdx(start_address) from nr_pdxs; 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
---
Changes in v3:
- improve commit message

Changes in v2:
- use mfn_to_pdx and maddr_to_mfn along with mfn_add()
---
 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 6ac169ce27..22ed91b5c9 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -886,8 +886,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 = mfn_to_pdx(mfn_add(maddr_to_mfn(pe), -1)) -
+                            mfn_to_pdx(maddr_to_mfn(ps)) + 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);
-- 
2.17.1


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

[PATCH v3 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 ("x86_64: Ensure frame-table
compression leaves MAX_ORDER aligned"), 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() are free to assume that their last argument is
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
---
Changes in v2:
- add commit title of bdb5439c3f
- more CC
- update commit message
---
 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 50c21b6bf8..bb7e437049 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 )
-- 
2.17.1


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

[Xen-devel] [PATCH v3 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 ("x86_64: Ensure frame-table
compression leaves MAX_ORDER aligned"), 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() are free to assume that their last argument is
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
---
Changes in v2:
- add commit title of bdb5439c3f
- more CC
- update commit message
---
 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 50c21b6bf8..bb7e437049 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 )
-- 
2.17.1


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

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

[Stefano Stabellini]

The mask calculation in pdx_init_mask 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 starting at 0x0
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 calculates
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
CC: andrew.cooper3@citrix.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
---

Changes in v3:
- improve in-code comments

Changes in v2:
- update commit message
- add in-code comments regarding update sites
- improve in-code comments
- move the mask initialization changes to pdx_init_mask
---
 xen/arch/arm/setup.c | 9 ++++++++-
 xen/common/pdx.c     | 8 +++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ccb0f181ea..45312df006 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -482,7 +482,14 @@ static void __init init_pdx(void)
 {
     paddr_t bank_start, bank_size, bank_end;
 
-    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
+    /*
+     * Arm does not have any restrictions on the bits to compress. Pass 0 to
+     * let the common code further restrict the mask.
+     *
+     * If the logic changes in pfn_pdx_hole_setup we might have to
+     * update this function too.
+     */
+    u64 mask = pdx_init_mask(0x0);
     int bank;
 
     for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index bb7e437049..a3c6f4c1ee 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -50,9 +50,12 @@ static u64 __init fill_mask(u64 mask)
     return mask;
 }
 
+/*
+ * We don't compress the first MAX_ORDER bit of the addresses.
+ */
 u64 __init pdx_init_mask(u64 base_addr)
 {
-    return fill_mask(base_addr - 1);
+    return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1);
 }
 
 u64 __init pdx_region_mask(u64 base, u64 len)
@@ -80,6 +83,9 @@ void __init pfn_pdx_hole_setup(unsigned long mask)
      * This guarantees that page-pointer arithmetic remains valid within
      * contiguous aligned ranges of 2^MAX_ORDER pages. Among others, our
      * buddy allocator relies on this assumption.
+     *
+     * If the logic changes here, we might have to update the ARM specific
+     * init_pdx too.
      */
     for ( j = MAX_ORDER-1; ; )
     {
-- 
2.17.1


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

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

[Stefano Stabellini]

The mask calculation in pdx_init_mask 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 starting at 0x0
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 calculates
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
CC: andrew.cooper3@citrix.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
---

Changes in v3:
- improve in-code comments

Changes in v2:
- update commit message
- add in-code comments regarding update sites
- improve in-code comments
- move the mask initialization changes to pdx_init_mask
---
 xen/arch/arm/setup.c | 9 ++++++++-
 xen/common/pdx.c     | 8 +++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ccb0f181ea..45312df006 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -482,7 +482,14 @@ static void __init init_pdx(void)
 {
     paddr_t bank_start, bank_size, bank_end;
 
-    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
+    /*
+     * Arm does not have any restrictions on the bits to compress. Pass 0 to
+     * let the common code further restrict the mask.
+     *
+     * If the logic changes in pfn_pdx_hole_setup we might have to
+     * update this function too.
+     */
+    u64 mask = pdx_init_mask(0x0);
     int bank;
 
     for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index bb7e437049..a3c6f4c1ee 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -50,9 +50,12 @@ static u64 __init fill_mask(u64 mask)
     return mask;
 }
 
+/*
+ * We don't compress the first MAX_ORDER bit of the addresses.
+ */
 u64 __init pdx_init_mask(u64 base_addr)
 {
-    return fill_mask(base_addr - 1);
+    return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1);
 }
 
 u64 __init pdx_region_mask(u64 base, u64 len)
@@ -80,6 +83,9 @@ void __init pfn_pdx_hole_setup(unsigned long mask)
      * This guarantees that page-pointer arithmetic remains valid within
      * contiguous aligned ranges of 2^MAX_ORDER pages. Among others, our
      * buddy allocator relies on this assumption.
+     *
+     * If the logic changes here, we might have to update the ARM specific
+     * init_pdx too.
      */
     for ( j = MAX_ORDER-1; ; )
     {
-- 
2.17.1


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

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

[Julien Grall]

Hi,

On 6/3/19 11:02 PM, Stefano Stabellini wrote:
> diff --git a/xen/common/pdx.c b/xen/common/pdx.c
> index bb7e437049..a3c6f4c1ee 100644
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -50,9 +50,12 @@ static u64 __init fill_mask(u64 mask)
>       return mask;
>   }
>   
> +/*
> + * We don't compress the first MAX_ORDER bit of the addresses.
> + */
>   u64 __init pdx_init_mask(u64 base_addr)
>   {
> -    return fill_mask(base_addr - 1);
> +    return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1);

See my comment on v2 regarding u64 vs uint64_t.

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi,

On 6/3/19 11:02 PM, Stefano Stabellini wrote:
> diff --git a/xen/common/pdx.c b/xen/common/pdx.c
> index bb7e437049..a3c6f4c1ee 100644
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -50,9 +50,12 @@ static u64 __init fill_mask(u64 mask)
>       return mask;
>   }
>   
> +/*
> + * We don't compress the first MAX_ORDER bit of the addresses.
> + */
>   u64 __init pdx_init_mask(u64 base_addr)
>   {
> -    return fill_mask(base_addr - 1);
> +    return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1);

See my comment on v2 regarding u64 vs uint64_t.

Cheers,

-- 
Julien Grall

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

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

[Jan Beulich]

>>> On 04.06.19 at 00:02, <sstabellini@kernel.org> wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -482,7 +482,14 @@ static void __init init_pdx(void)
>  {
>      paddr_t bank_start, bank_size, bank_end;
>  
> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
> +    /*
> +     * Arm does not have any restrictions on the bits to compress. Pass 0 to
> +     * let the common code further restrict the mask.
> +     *
> +     * If the logic changes in pfn_pdx_hole_setup we might have to
> +     * update this function too.
> +     */
> +    u64 mask = pdx_init_mask(0x0);

Seeing Julien's clarification on the previous version's discussion,
how about switching this one to uint64_t as well at this occasion?

> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -50,9 +50,12 @@ static u64 __init fill_mask(u64 mask)
>      return mask;
>  }
>  
> +/*
> + * We don't compress the first MAX_ORDER bit of the addresses.
> + */

This is a single line comment.

>  u64 __init pdx_init_mask(u64 base_addr)

It wouldn't hamper patch readability much if even this one was
switched to uint64_t at the same time, thus restoring consistency
with ...

>  {
> -    return fill_mask(base_addr - 1);
> +    return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1);

... the requested use of uint64_t here.

Jan



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

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

[Jan Beulich]

>>> On 04.06.19 at 00:02, <sstabellini@kernel.org> wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -482,7 +482,14 @@ static void __init init_pdx(void)
>  {
>      paddr_t bank_start, bank_size, bank_end;
>  
> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
> +    /*
> +     * Arm does not have any restrictions on the bits to compress. Pass 0 to
> +     * let the common code further restrict the mask.
> +     *
> +     * If the logic changes in pfn_pdx_hole_setup we might have to
> +     * update this function too.
> +     */
> +    u64 mask = pdx_init_mask(0x0);

Seeing Julien's clarification on the previous version's discussion,
how about switching this one to uint64_t as well at this occasion?

> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -50,9 +50,12 @@ static u64 __init fill_mask(u64 mask)
>      return mask;
>  }
>  
> +/*
> + * We don't compress the first MAX_ORDER bit of the addresses.
> + */

This is a single line comment.

>  u64 __init pdx_init_mask(u64 base_addr)

It wouldn't hamper patch readability much if even this one was
switched to uint64_t at the same time, thus restoring consistency
with ...

>  {
> -    return fill_mask(base_addr - 1);
> +    return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1);

... the requested use of uint64_t here.

Jan



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

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

[Julien Grall]

Hi Stefano,

On 6/3/19 11:02 PM, Stefano Stabellini wrote:
> pfn_to_pdx expects an address, not a size, as a parameter. Specifically,
> it expects the end address, then the masks calculations compensate for
> any holes between start and end. Thus, we should pass the end address to
> pfn_to_pdx.
> 
> The initial pdx is stored in frametable_base_pdx, so we can subtract the
> result of pfn_to_pdx(start_address) from nr_pdxs; 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>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Stefano,

On 6/3/19 11:02 PM, Stefano Stabellini wrote:
> 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 ("x86_64: Ensure frame-table
> compression leaves MAX_ORDER aligned"), 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() are free to assume that their last argument is
> less than their middle one.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> Signed-off-by: Jan Beulich <JBeulich@suse.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Jan,

On 6/4/19 7:56 AM, Jan Beulich wrote:
>>>> On 04.06.19 at 00:02, <sstabellini@kernel.org> wrote:
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -482,7 +482,14 @@ static void __init init_pdx(void)
>>   {
>>       paddr_t bank_start, bank_size, bank_end;
>>   
>> -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
>> +    /*
>> +     * Arm does not have any restrictions on the bits to compress. Pass 0 to
>> +     * let the common code further restrict the mask.
>> +     *
>> +     * If the logic changes in pfn_pdx_hole_setup we might have to
>> +     * update this function too.
>> +     */
>> +    u64 mask = pdx_init_mask(0x0);
> 
> Seeing Julien's clarification on the previous version's discussion,
> how about switching this one to uint64_t as well at this occasion?
> 
>> --- a/xen/common/pdx.c
>> +++ b/xen/common/pdx.c
>> @@ -50,9 +50,12 @@ static u64 __init fill_mask(u64 mask)
>>       return mask;
>>   }
>>   
>> +/*
>> + * We don't compress the first MAX_ORDER bit of the addresses.
>> + */
> 
> This is a single line comment.
> 
>>   u64 __init pdx_init_mask(u64 base_addr)
> 
> It wouldn't hamper patch readability much if even this one was
> switched to uint64_t at the same time, thus restoring consistency
> with ...

On Arm we don't tend to mix clean-up and fix. It would be preferable if 
the switch to uint64_t is done in a separate 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 v3 0/3] PDX fixes

[Julien Grall]

Hi,

On 6/3/19 11:01 PM, Stefano Stabellini wrote:
> 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 844aa0a13d34e9a341a8374119d2ed67d4dcd6bb:
> 
>    sched_null: superficial clean-ups (2019-06-03 17:56:20 +0200)
> 
> are available in the Git repository at:
> 
>    http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git
> 
> for you to fetch changes up to c92f589e7cf66fc9c3ad8a812cdbc69214a812d1:
> 
>    xen/arm: fix mask calculation in pdx_init_mask (2019-06-03 14:42:10 -0700)
> 
> ----------------------------------------------------------------
> Stefano Stabellini (3):
>        xen/arm: fix nr_pdxs calculation
>        xen: actually skip the first MAX_ORDER bits in pfn_pdx_hole_setup

I have pushed the 2 patches...

>        xen/arm: fix mask calculation in pdx_init_mask

... this one require input from you.

Cheers,

-- 
Julien Grall

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