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

[Julien Grall <julien@xxxxxxx>]

Hi Roger,

On 11/08/2025 09:07, Roger Pau Monné wrote:
> On Fri, Aug 08, 2025 at 06:21:29PM +0100, Julien Grall wrote:
> > 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?
> I cannot introduce a `bank` local variable as it's already used as the
> index cursor for the loop.  Would you be fine with:
Ah yes. I am fine with the logic below. I am happy if you want to commit 
message without resending the series.
Cheers,

--
Julien Grall