1. Patchew
  2. Xen
  3. [Xen-devel] [PATCH v3 0/3] PDX fixes
  4. View patch

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

Stefano Stabellini posted 3 patches 6 years, 5 months ago
[Xen-devel] [PATCH v3 3/3] xen/arm: fix mask calculation in pdx_init_mask
Posted by Stefano Stabellini 6 years, 5 months ago 
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: [Xen-devel] [PATCH v3 3/3] xen/arm: fix mask calculation in pdx_init_mask
Posted by Julien Grall 6 years, 5 months ago 
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
Posted by Jan Beulich 6 years, 5 months ago 
>>> 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
Posted by Julien Grall 6 years, 5 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.