[PATCH v2 0/3] PDX fixes

[PATCH v2 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 

for you to fetch changes up to 673fb14dc9971198539d02c503abc85d30e95426:

  xen/arm: fix mask calculation in pdx_init_mask (2019-05-08 15:37:03 -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 v2 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 

for you to fetch changes up to 673fb14dc9971198539d02c503abc85d30e95426:

  xen/arm: fix mask calculation in pdx_init_mask (2019-05-08 15:37:03 -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 v2 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
---
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 01ae2cccc0..58d71d3c23 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 = 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 v2 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
---
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 01ae2cccc0..58d71d3c23 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 = 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 v2 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 v2 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 v2 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 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..afaafe7b84 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);
+    /*
+     * Pass 0x0 to pdx_init_mask to get a mask initialized with the
+     * first to 1<<MAX_ORDER pages of RAM left uncompressed.
+     *
+     * 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..268d6f7ec3 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
     return mask;
 }
 
+/*
+ * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
+ * are left uncompressed.
+ */
 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 +84,8 @@ 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 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 v2 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 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..afaafe7b84 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);
+    /*
+     * Pass 0x0 to pdx_init_mask to get a mask initialized with the
+     * first to 1<<MAX_ORDER pages of RAM left uncompressed.
+     *
+     * 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..268d6f7ec3 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
     return mask;
 }
 
+/*
+ * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
+ * are left uncompressed.
+ */
 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 +84,8 @@ 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 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 v2 3/3] xen/arm: fix mask calculation in pdx_init_mask

[Jan Beulich]

>>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote:
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
>      return mask;
>  }
>  
> +/*
> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
> + * are left uncompressed.
> + */
>  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);

Personally I think that despite the surrounding u64 this should be
uint64_t. You could avoid this altogether by using 1ULL.

> @@ -80,6 +84,8 @@ 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 init_pdx too.
>       */
>      for ( j = MAX_ORDER-1; ; )
>      {

At first I was puzzled by a reference to something that I didn't
think would exist, and I was then assuming you mean
pdx_init_mask(). But then I thought I'd use grep nevertheless,
and found the Arm instance of it (which this patch actually
changes, but I'm rarely looking at the "diff -p" symbols when
context is otherwise obvious). If you make such a reference in
common code (I think I'd prefer if it was simply omitted), then
please name the specific architecture as well, or make otherwise
clear that this isn't a symbol that's common or required to be
supplied by each arch.

Jan



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

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

[Jan Beulich]

>>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote:
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
>      return mask;
>  }
>  
> +/*
> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
> + * are left uncompressed.
> + */
>  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);

Personally I think that despite the surrounding u64 this should be
uint64_t. You could avoid this altogether by using 1ULL.

> @@ -80,6 +84,8 @@ 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 init_pdx too.
>       */
>      for ( j = MAX_ORDER-1; ; )
>      {

At first I was puzzled by a reference to something that I didn't
think would exist, and I was then assuming you mean
pdx_init_mask(). But then I thought I'd use grep nevertheless,
and found the Arm instance of it (which this patch actually
changes, but I'm rarely looking at the "diff -p" symbols when
context is otherwise obvious). If you make such a reference in
common code (I think I'd prefer if it was simply omitted), then
please name the specific architecture as well, or make otherwise
clear that this isn't a symbol that's common or required to be
supplied by each arch.

Jan



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

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

[Julien Grall]

Hi Stefano,

On 5/8/19 11:47 PM, Stefano Stabellini wrote:
> 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. 

The wording is a bit difficult to read. Don't you miss a "So" or 
"Therefore" somewhere?


> Also remove

s/remove/substract/?

> 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.

The only reason we can substract pfn_to_pdx(start_address) here is 
because we store the initial PDX in frametable_base_pdx. Without that, 
the frametable would need to cover the range 0 - start.

The code looks good to me.

> 
> Remove the variable `nr_pages' because it is unused.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: JBeulich@suse.com
> ---
> 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 01ae2cccc0..58d71d3c23 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 = 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);
> 

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Stefano,

On 5/8/19 11:47 PM, Stefano Stabellini wrote:
> 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. 

The wording is a bit difficult to read. Don't you miss a "So" or 
"Therefore" somewhere?


> Also remove

s/remove/substract/?

> 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.

The only reason we can substract pfn_to_pdx(start_address) here is 
because we store the initial PDX in frametable_base_pdx. Without that, 
the frametable would need to cover the range 0 - start.

The code looks good to me.

> 
> Remove the variable `nr_pages' because it is unused.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: JBeulich@suse.com
> ---
> 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 01ae2cccc0..58d71d3c23 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 = 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);
> 

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Stefano,

On 5/8/19 11:47 PM, Stefano Stabellini wrote:
> 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 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..afaafe7b84 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);
> +    /*
> +     * Pass 0x0 to pdx_init_mask to get a mask initialized with the
> +     * first to 1<<MAX_ORDER pages of RAM left uncompressed.

What matter is Arm doesn't have any specific restriction on the 
compression, but the common code may have. So how about something:

"Arm does not have any restriction on the bits to compress. Pass 0 to 
let the common code to further restrict".

> +     *
> +     * 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..268d6f7ec3 100644
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
>       return mask;
>   }
>   
> +/*
> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they

I thought I already pointed out in the previous version. At least on 
Arm, we never map the first 1 << MAX_ORDER of RAM. Instead what you want 
to say is that we don't compress the first N bits of the address.

> + * are left uncompressed.
> + */

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Stefano,

On 5/8/19 11:47 PM, Stefano Stabellini wrote:
> 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 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..afaafe7b84 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);
> +    /*
> +     * Pass 0x0 to pdx_init_mask to get a mask initialized with the
> +     * first to 1<<MAX_ORDER pages of RAM left uncompressed.

What matter is Arm doesn't have any specific restriction on the 
compression, but the common code may have. So how about something:

"Arm does not have any restriction on the bits to compress. Pass 0 to 
let the common code to further restrict".

> +     *
> +     * 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..268d6f7ec3 100644
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
>       return mask;
>   }
>   
> +/*
> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they

I thought I already pointed out in the previous version. At least on 
Arm, we never map the first 1 << MAX_ORDER of RAM. Instead what you want 
to say is that we don't compress the first N bits of the address.

> + * are left uncompressed.
> + */

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Thu, 9 May 2019, Jan Beulich wrote:
> >>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote:
> > --- a/xen/common/pdx.c
> > +++ b/xen/common/pdx.c
> > @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
> >      return mask;
> >  }
> >  
> > +/*
> > + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
> > + * are left uncompressed.
> > + */
> >  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);
> 
> Personally I think that despite the surrounding u64 this should be
> uint64_t. You could avoid this altogether by using 1ULL.

I cannot use 1ULL because it would break the build: the reason is that
u64 is defined as unsigned long on some architectures and unsigned long
long on others. The pointers comparison in the max macro will fail to
compile.

I could use uint64_t, that works, but I think is not a good idea to use
potentially different types between the arguments passed to max. If you
still would like me to change (u64)1 to (uint64_t)1 please explain why
in more details.


> > @@ -80,6 +84,8 @@ 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 init_pdx too.
> >       */
> >      for ( j = MAX_ORDER-1; ; )
> >      {
> 
> At first I was puzzled by a reference to something that I didn't
> think would exist, and I was then assuming you mean
> pdx_init_mask(). But then I thought I'd use grep nevertheless,
> and found the Arm instance of it (which this patch actually
> changes, but I'm rarely looking at the "diff -p" symbols when
> context is otherwise obvious). If you make such a reference in
> common code (I think I'd prefer if it was simply omitted), then
> please name the specific architecture as well, or make otherwise
> clear that this isn't a symbol that's common or required to be
> supplied by each arch.

I'll make it clear

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

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

[Stefano Stabellini]

On Thu, 9 May 2019, Jan Beulich wrote:
> >>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote:
> > --- a/xen/common/pdx.c
> > +++ b/xen/common/pdx.c
> > @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
> >      return mask;
> >  }
> >  
> > +/*
> > + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
> > + * are left uncompressed.
> > + */
> >  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);
> 
> Personally I think that despite the surrounding u64 this should be
> uint64_t. You could avoid this altogether by using 1ULL.

I cannot use 1ULL because it would break the build: the reason is that
u64 is defined as unsigned long on some architectures and unsigned long
long on others. The pointers comparison in the max macro will fail to
compile.

I could use uint64_t, that works, but I think is not a good idea to use
potentially different types between the arguments passed to max. If you
still would like me to change (u64)1 to (uint64_t)1 please explain why
in more details.


> > @@ -80,6 +84,8 @@ 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 init_pdx too.
> >       */
> >      for ( j = MAX_ORDER-1; ; )
> >      {
> 
> At first I was puzzled by a reference to something that I didn't
> think would exist, and I was then assuming you mean
> pdx_init_mask(). But then I thought I'd use grep nevertheless,
> and found the Arm instance of it (which this patch actually
> changes, but I'm rarely looking at the "diff -p" symbols when
> context is otherwise obvious). If you make such a reference in
> common code (I think I'd prefer if it was simply omitted), then
> please name the specific architecture as well, or make otherwise
> clear that this isn't a symbol that's common or required to be
> supplied by each arch.

I'll make it clear

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

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

[Julien Grall]

Hi Stefano,

On 6/3/19 10:56 PM, Stefano Stabellini wrote:
> On Thu, 9 May 2019, Jan Beulich wrote:
>>>>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote:
>>> --- a/xen/common/pdx.c
>>> +++ b/xen/common/pdx.c
>>> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
>>>       return mask;
>>>   }
>>>   
>>> +/*
>>> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
>>> + * are left uncompressed.
>>> + */
>>>   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);
>>
>> Personally I think that despite the surrounding u64 this should be
>> uint64_t. You could avoid this altogether by using 1ULL.
> 
> I cannot use 1ULL because it would break the build: the reason is that
> u64 is defined as unsigned long on some architectures and unsigned long
> long on others. The pointers comparison in the max macro will fail to
> compile.
> 
> I could use uint64_t, that works, but I think is not a good idea to use
> potentially different types between the arguments passed to max. If you
> still would like me to change (u64)1 to (uint64_t)1 please explain why
> in more details.

We are phasing out uXX in favor of uintXX_t so I agree with Jan that we 
want to avoid introducing more here.

Except the way they are defined, u64 and uint64_t will always be equal 
to 64-bits. So you can easily update the interface to use uint64_t 
instead of u64 without worrying about type issue.

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Stefano,

On 6/3/19 10:56 PM, Stefano Stabellini wrote:
> On Thu, 9 May 2019, Jan Beulich wrote:
>>>>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote:
>>> --- a/xen/common/pdx.c
>>> +++ b/xen/common/pdx.c
>>> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
>>>       return mask;
>>>   }
>>>   
>>> +/*
>>> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
>>> + * are left uncompressed.
>>> + */
>>>   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);
>>
>> Personally I think that despite the surrounding u64 this should be
>> uint64_t. You could avoid this altogether by using 1ULL.
> 
> I cannot use 1ULL because it would break the build: the reason is that
> u64 is defined as unsigned long on some architectures and unsigned long
> long on others. The pointers comparison in the max macro will fail to
> compile.
> 
> I could use uint64_t, that works, but I think is not a good idea to use
> potentially different types between the arguments passed to max. If you
> still would like me to change (u64)1 to (uint64_t)1 please explain why
> in more details.

We are phasing out uXX in favor of uintXX_t so I agree with Jan that we 
want to avoid introducing more here.

Except the way they are defined, u64 and uint64_t will always be equal 
to 64-bits. So you can easily update the interface to use uint64_t 
instead of u64 without worrying about type issue.

Cheers,

-- 
Julien Grall

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

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

[Jan Beulich]

>>> On 04.06.19 at 00:02, <julien.grall@arm.com> wrote:
> On 6/3/19 10:56 PM, Stefano Stabellini wrote:
>> On Thu, 9 May 2019, Jan Beulich wrote:
>>>>>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote:
>>>> --- a/xen/common/pdx.c
>>>> +++ b/xen/common/pdx.c
>>>> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
>>>>       return mask;
>>>>   }
>>>>   
>>>> +/*
>>>> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
>>>> + * are left uncompressed.
>>>> + */
>>>>   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);
>>>
>>> Personally I think that despite the surrounding u64 this should be
>>> uint64_t. You could avoid this altogether by using 1ULL.
>> 
>> I cannot use 1ULL because it would break the build: the reason is that
>> u64 is defined as unsigned long on some architectures and unsigned long
>> long on others. The pointers comparison in the max macro will fail to
>> compile.

Hmm, ugly - we indeed have UINT64_C() only in crypto code right now.

>> I could use uint64_t, that works, but I think is not a good idea to use
>> potentially different types between the arguments passed to max. If you
>> still would like me to change (u64)1 to (uint64_t)1 please explain why
>> in more details.
> 
> We are phasing out uXX in favor of uintXX_t so I agree with Jan that we 
> want to avoid introducing more here.

Is this sufficient detail for you? (Honestly I wouldn't what else to add.)

Jan



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

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

[Jan Beulich]

>>> On 04.06.19 at 00:02, <julien.grall@arm.com> wrote:
> On 6/3/19 10:56 PM, Stefano Stabellini wrote:
>> On Thu, 9 May 2019, Jan Beulich wrote:
>>>>>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote:
>>>> --- a/xen/common/pdx.c
>>>> +++ b/xen/common/pdx.c
>>>> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
>>>>       return mask;
>>>>   }
>>>>   
>>>> +/*
>>>> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
>>>> + * are left uncompressed.
>>>> + */
>>>>   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);
>>>
>>> Personally I think that despite the surrounding u64 this should be
>>> uint64_t. You could avoid this altogether by using 1ULL.
>> 
>> I cannot use 1ULL because it would break the build: the reason is that
>> u64 is defined as unsigned long on some architectures and unsigned long
>> long on others. The pointers comparison in the max macro will fail to
>> compile.

Hmm, ugly - we indeed have UINT64_C() only in crypto code right now.

>> I could use uint64_t, that works, but I think is not a good idea to use
>> potentially different types between the arguments passed to max. If you
>> still would like me to change (u64)1 to (uint64_t)1 please explain why
>> in more details.
> 
> We are phasing out uXX in favor of uintXX_t so I agree with Jan that we 
> want to avoid introducing more here.

Is this sufficient detail for you? (Honestly I wouldn't what else to add.)

Jan



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