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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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@xxxxxxx>
>> ---
>>  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