1. Patchew
  2. Xen
  3. [PATCH 0/2] xen/arm: Creating base for future PDX changes
  4. View patch

[PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used

Michal Orzel posted 2 patches 3 months, 4 weeks ago
[PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Michal Orzel 3 months, 4 weeks ago 
When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
sense to keep the two loops iterating over all the memory banks.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/setup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 93b730ffb5fb..12b76a0a9837 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -255,7 +255,9 @@ void __init init_pdx(void)
 {
     const struct membanks *mem = bootinfo_get_mem();
     paddr_t bank_start, bank_size, bank_end, ram_end = 0;
+    int bank;
 
+#ifdef CONFIG_PDX_COMPRESSION
     /*
      * Arm does not have any restrictions on the bits to compress. Pass 0 to
      * let the common code further restrict the mask.
@@ -264,7 +266,6 @@ void __init init_pdx(void)
      * update this function too.
      */
     uint64_t mask = pdx_init_mask(0x0);
-    int bank;
 
     for ( bank = 0 ; bank < mem->nr_banks; bank++ )
     {
@@ -284,6 +285,7 @@ void __init init_pdx(void)
     }
 
     pfn_pdx_hole_setup(mask >> PAGE_SHIFT);
+#endif
 
     for ( bank = 0 ; bank < mem->nr_banks; bank++ )
     {
-- 
2.25.1
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Julien Grall 3 months, 2 weeks ago 
Hi Michal,

On 04/07/2025 08:54, Michal Orzel wrote:
> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
> sense to keep the two loops iterating over all the memory banks.

I saw this was already committed. But I have a question. Wouldn't the 
compiler be able to optimize and remove the loops? Asking because we are 
trying to limit the number of #ifdef in the code hence why we have stubs.

Cheers,

-- 
Julien Grall
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Orzel, Michal 3 months, 2 weeks ago 

On 12/07/2025 12:29, Julien Grall wrote:
> Hi Michal,
> 
> On 04/07/2025 08:54, Michal Orzel wrote:
>> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
>> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
>> sense to keep the two loops iterating over all the memory banks.
> 
> I saw this was already committed. But I have a question. Wouldn't the 
> compiler be able to optimize and remove the loops? Asking because we are 
> trying to limit the number of #ifdef in the code hence why we have stubs.
Before submitting a patch I did disassembled init_pdx() with and without my
patch and in the latter case the compiler did not optimize out the loops.

~Michal
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Julien Grall 3 months, 2 weeks ago 
Hi Michal,

On 14/07/2025 08:37, Orzel, Michal wrote:
> 
> 
> On 12/07/2025 12:29, Julien Grall wrote:
>> Hi Michal,
>>
>> On 04/07/2025 08:54, Michal Orzel wrote:
>>> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
>>> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
>>> sense to keep the two loops iterating over all the memory banks.
>>
>> I saw this was already committed. But I have a question. Wouldn't the
>> compiler be able to optimize and remove the loops? Asking because we are
>> trying to limit the number of #ifdef in the code hence why we have stubs.
> Before submitting a patch I did disassembled init_pdx() with and without my
> patch and in the latter case the compiler did not optimize out the loops.

One more question. Was this in debug or non-debug build?

Cheers,

-- 
Julien Grall
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Orzel, Michal 3 months, 2 weeks ago 

On 14/07/2025 11:45, Julien Grall wrote:
> Hi Michal,
> 
> On 14/07/2025 08:37, Orzel, Michal wrote:
>>
>>
>> On 12/07/2025 12:29, Julien Grall wrote:
>>> Hi Michal,
>>>
>>> On 04/07/2025 08:54, Michal Orzel wrote:
>>>> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
>>>> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
>>>> sense to keep the two loops iterating over all the memory banks.
>>>
>>> I saw this was already committed. But I have a question. Wouldn't the
>>> compiler be able to optimize and remove the loops? Asking because we are
>>> trying to limit the number of #ifdef in the code hence why we have stubs.
>> Before submitting a patch I did disassembled init_pdx() with and without my
>> patch and in the latter case the compiler did not optimize out the loops.
> 
> One more question. Was this in debug or non-debug build?
It was in debug build.

~Michal
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Julien Grall 3 months, 2 weeks ago 
Hi Michal,

On 14/07/2025 10:50, Orzel, Michal wrote:
> 
> 
> On 14/07/2025 11:45, Julien Grall wrote:
>> Hi Michal,
>>
>> On 14/07/2025 08:37, Orzel, Michal wrote:
>>>
>>>
>>> On 12/07/2025 12:29, Julien Grall wrote:
>>>> Hi Michal,
>>>>
>>>> On 04/07/2025 08:54, Michal Orzel wrote:
>>>>> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
>>>>> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
>>>>> sense to keep the two loops iterating over all the memory banks.
>>>>
>>>> I saw this was already committed. But I have a question. Wouldn't the
>>>> compiler be able to optimize and remove the loops? Asking because we are
>>>> trying to limit the number of #ifdef in the code hence why we have stubs.
>>> Before submitting a patch I did disassembled init_pdx() with and without my
>>> patch and in the latter case the compiler did not optimize out the loops.
>>
>> One more question. Was this in debug or non-debug build?
> It was in debug build.

Ok. I would be interested to know if this change in non-debug build 
because we have quite a few places in Xen relying on code elimination.

Cheers,

-- 
Julien Grall
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Orzel, Michal 3 months, 2 weeks ago 

On 14/07/2025 12:09, Julien Grall wrote:
> Hi Michal,
> 
> On 14/07/2025 10:50, Orzel, Michal wrote:
>>
>>
>> On 14/07/2025 11:45, Julien Grall wrote:
>>> Hi Michal,
>>>
>>> On 14/07/2025 08:37, Orzel, Michal wrote:
>>>>
>>>>
>>>> On 12/07/2025 12:29, Julien Grall wrote:
>>>>> Hi Michal,
>>>>>
>>>>> On 04/07/2025 08:54, Michal Orzel wrote:
>>>>>> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
>>>>>> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
>>>>>> sense to keep the two loops iterating over all the memory banks.
>>>>>
>>>>> I saw this was already committed. But I have a question. Wouldn't the
>>>>> compiler be able to optimize and remove the loops? Asking because we are
>>>>> trying to limit the number of #ifdef in the code hence why we have stubs.
>>>> Before submitting a patch I did disassembled init_pdx() with and without my
>>>> patch and in the latter case the compiler did not optimize out the loops.
>>>
>>> One more question. Was this in debug or non-debug build?
>> It was in debug build.
> 
> Ok. I would be interested to know if this change in non-debug build 
> because we have quite a few places in Xen relying on code elimination.
I did a test and with -O2 (non-debug), the loops are removed as oppose to -O1
(debug). That said, -fdce is part of -O1, so it's difficult to say what option
made impact here.

~Michal
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Julien Grall 3 months, 2 weeks ago 
Hi Michal,

On 14/07/2025 13:37, Orzel, Michal wrote:
> 
> 
> On 14/07/2025 12:09, Julien Grall wrote:
>> Hi Michal,
>>
>> On 14/07/2025 10:50, Orzel, Michal wrote:
>>>
>>>
>>> On 14/07/2025 11:45, Julien Grall wrote:
>>>> Hi Michal,
>>>>
>>>> On 14/07/2025 08:37, Orzel, Michal wrote:
>>>>>
>>>>>
>>>>> On 12/07/2025 12:29, Julien Grall wrote:
>>>>>> Hi Michal,
>>>>>>
>>>>>> On 04/07/2025 08:54, Michal Orzel wrote:
>>>>>>> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
>>>>>>> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
>>>>>>> sense to keep the two loops iterating over all the memory banks.
>>>>>>
>>>>>> I saw this was already committed. But I have a question. Wouldn't the
>>>>>> compiler be able to optimize and remove the loops? Asking because we are
>>>>>> trying to limit the number of #ifdef in the code hence why we have stubs.
>>>>> Before submitting a patch I did disassembled init_pdx() with and without my
>>>>> patch and in the latter case the compiler did not optimize out the loops.
>>>>
>>>> One more question. Was this in debug or non-debug build?
>>> It was in debug build.
>>
>> Ok. I would be interested to know if this change in non-debug build
>> because we have quite a few places in Xen relying on code elimination.
> I did a test and with -O2 (non-debug), the loops are removed as oppose to -O1
> (debug). That said, -fdce is part of -O1, so it's difficult to say what option
> made impact here.

Ok. If the compiler is able to remove the code in non-debug build, then 
I think we should avoid using #ifdef. This is matching the policy we 
have for the rest of the code base.

IIRC, the idea was to be able to catch compile error in advance (we had 
some cases where we forgot to update code within the #ifdef).

Cheers,

-- 
Julien Grall
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Stefano Stabellini 3 months, 3 weeks ago 
On Fri, 4 Jul 2025, Michal Orzel wrote:
> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
> sense to keep the two loops iterating over all the memory banks.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/setup.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 93b730ffb5fb..12b76a0a9837 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -255,7 +255,9 @@ void __init init_pdx(void)
>  {
>      const struct membanks *mem = bootinfo_get_mem();
>      paddr_t bank_start, bank_size, bank_end, ram_end = 0;
> +    int bank;
>  
> +#ifdef CONFIG_PDX_COMPRESSION
>      /*
>       * Arm does not have any restrictions on the bits to compress. Pass 0 to
>       * let the common code further restrict the mask.
> @@ -264,7 +266,6 @@ void __init init_pdx(void)
>       * update this function too.
>       */
>      uint64_t mask = pdx_init_mask(0x0);
> -    int bank;
>  
>      for ( bank = 0 ; bank < mem->nr_banks; bank++ )
>      {
> @@ -284,6 +285,7 @@ void __init init_pdx(void)
>      }
>  
>      pfn_pdx_hole_setup(mask >> PAGE_SHIFT);
> +#endif
>  
>      for ( bank = 0 ; bank < mem->nr_banks; bank++ )
>      {
> -- 
> 2.25.1
>
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Hari Limaye 3 months, 3 weeks ago 
Apologies for noise, my previous message was missing the intended tags:

On Fri, Jul 04, 2025 at 09:54:28AM +0000, Michal Orzel wrote:
> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
> sense to keep the two loops iterating over all the memory banks.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
Reviewed-by: Hari Limaye <hari.limaye@arm.com>
Tested-by: Hari Limaye <hari.limaye@arm.com>

Cheers,
Hari
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Hari Limaye 3 months, 3 weeks ago 
Hi Michal,

> On Fri, Jul 04, 2025 at 09:54:28AM +0000, Michal Orzel wrote:
> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
> sense to keep the two loops iterating over all the memory banks.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>  xen/arch/arm/setup.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 93b730ffb5fb..12b76a0a9837 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -255,7 +255,9 @@ void __init init_pdx(void)
>  {
>      const struct membanks *mem = bootinfo_get_mem();
>      paddr_t bank_start, bank_size, bank_end, ram_end = 0;
> +    int bank;

Minor/question: Would we potentially prefer to move the declaration of
the loop counter `bank` variables to each for loop? As the variable is
not used outside of the loops and is always initialized to 0, this seems
perhaps better from a variable scope perspective?

>  
> +#ifdef CONFIG_PDX_COMPRESSION
>      /*
>       * Arm does not have any restrictions on the bits to compress. Pass 0 to
>       * let the common code further restrict the mask.
> @@ -264,7 +266,6 @@ void __init init_pdx(void)
>       * update this function too.
>       */
>      uint64_t mask = pdx_init_mask(0x0);
> -    int bank;
>  
>      for ( bank = 0 ; bank < mem->nr_banks; bank++ )
>      {
> @@ -284,6 +285,7 @@ void __init init_pdx(void)
>      }
>  
>      pfn_pdx_hole_setup(mask >> PAGE_SHIFT);
> +#endif
>  
>      for ( bank = 0 ; bank < mem->nr_banks; bank++ )
>      {

Otherwise, LGTM! Tested (compilation) via both Arm AArch32 and AArch64 builds.

Many thanks,
Hari
Re: [PATCH 2/2] xen/arm: Skip loops in init_pdx() when no PDX compression is used
Posted by Orzel, Michal 3 months, 3 weeks ago 

On 07/07/2025 13:56, Hari Limaye wrote:
> Hi Michal,
> 
>> On Fri, Jul 04, 2025 at 09:54:28AM +0000, Michal Orzel wrote:
>> When CONFIG_PDX_COMPRESSION=n, pdx_init_mask(), pdx_region_mask() and
>> pfn_pdx_hole_setup() are just stubs doing nothing. It does not make
>> sense to keep the two loops iterating over all the memory banks.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>  xen/arch/arm/setup.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 93b730ffb5fb..12b76a0a9837 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -255,7 +255,9 @@ void __init init_pdx(void)
>>  {
>>      const struct membanks *mem = bootinfo_get_mem();
>>      paddr_t bank_start, bank_size, bank_end, ram_end = 0;
>> +    int bank;
> 
> Minor/question: Would we potentially prefer to move the declaration of
> the loop counter `bank` variables to each for loop? As the variable is
> not used outside of the loops and is always initialized to 0, this seems
> perhaps better from a variable scope perspective?
Maybe it does but it was too minor for me to be willing to incorporate it in
this patch whose goal was not to reduce the variable scope (I think adding such
change would not fit the purpose of the patch).

> 
>>  
>> +#ifdef CONFIG_PDX_COMPRESSION
>>      /*
>>       * Arm does not have any restrictions on the bits to compress. Pass 0 to
>>       * let the common code further restrict the mask.
>> @@ -264,7 +266,6 @@ void __init init_pdx(void)
>>       * update this function too.
>>       */
>>      uint64_t mask = pdx_init_mask(0x0);
>> -    int bank;
>>  
>>      for ( bank = 0 ; bank < mem->nr_banks; bank++ )
>>      {
>> @@ -284,6 +285,7 @@ void __init init_pdx(void)
>>      }
>>  
>>      pfn_pdx_hole_setup(mask >> PAGE_SHIFT);
>> +#endif
>>  
>>      for ( bank = 0 ; bank < mem->nr_banks; bank++ )
>>      {
> 
> Otherwise, LGTM! Tested (compilation) via both Arm AArch32 and AArch64 builds.
> 
> Many thanks,
> Hari

Thanks.

~Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.