Re: [PATCH v4 2/8] pdx: provide a unified set of unit functions

[Julien Grall <julien@xxxxxxx>]

Hi Roger,

On 05/08/2025 10:52, Roger Pau Monne wrote:
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index a77b31071ed8..ba35bf1fe3bb 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -256,9 +256,11 @@ 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;
> +    unsigned int bank;
>   
>   #ifndef CONFIG_PDX_NONE
> +    for ( bank = 0 ; bank < mem->nr_banks; bank++ )
> +        pfn_pdx_add_region(mem->bank[bank].start, mem->bank[bank].size);
>       /*
>        * Arm does not have any restrictions on the bits to compress. Pass 0 to
>        * let the common code further restrict the mask.
> @@ -266,26 +268,24 @@ void __init init_pdx(void)
>        * If the logic changes in pfn_pdx_hole_setup we might have to
>        * update this function too.
>        */
> -    uint64_t mask = pdx_init_mask(0x0);
> -
> -    for ( bank = 0 ; bank < mem->nr_banks; bank++ )
> -    {
> -        bank_start = mem->bank[bank].start;
> -        bank_size = mem->bank[bank].size;
> -
> -        mask |= bank_start | pdx_region_mask(bank_start, bank_size);
> -    }
> +    pfn_pdx_compression_setup(0);
>   
>       for ( bank = 0 ; bank < mem->nr_banks; bank++ )
>       {
> -        bank_start = mem->bank[bank].start;
> -        bank_size = mem->bank[bank].size;
> -
> -        if (~mask & pdx_region_mask(bank_start, bank_size))
> -            mask = 0;
> +        if ( !pdx_is_region_compressible(
> +                  mem->bank[bank].start,
> +                  PFN_UP(mem->bank[bank].start + mem->bank[bank].size) -
> +                  PFN_DOWN(mem->bank[bank].start)) )
This code is a bit too verbose. Can we at least introduce "bank = 
&mem->bank[bank]" to reduce a bit the verbosity?
The rest of the logic looks fine. So:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx> # ARM

Cheers,

--
Julien Grall