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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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:

    for ( bank = 0 ; bank < mem->nr_banks; bank++ )
    {
        const struct membank *m = &mem->bank[bank];

        if ( !pdx_is_region_compressible(m->start,
                                         PFN_UP(m->start + m->size) -
                                         PFN_DOWN(m->start)) )
        {
            pfn_pdx_compression_reset();
            printk(XENLOG_WARNING
                   "PFN compression disabled, RAM region [%#" PRIpaddr ", %#"
                   PRIpaddr "] not covered\n",
                   m->start, m->start + m->size - 1);
            break;
        }
    }

> The rest of the logic looks fine. So:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx> # ARM

Thanks, Roger.