Re: [PATCH v2 3/8] pdx: provide a unified set of unit functions

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

On Tue, Jun 24, 2025 at 03:32:23PM +0200, Jan Beulich wrote:
> On 20.06.2025 13:11, Roger Pau Monne wrote:
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -255,6 +255,10 @@ void __init init_pdx(void)
> >  {
> >      const struct membanks *mem = bootinfo_get_mem();
> >      paddr_t bank_start, bank_size, bank_end;
> > +    unsigned int bank;
> > +
> > +    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
> > @@ -263,28 +267,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);
> > -    int bank;
> > +    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;
> > -
> > -        mask |= bank_start | pdx_region_mask(bank_start, bank_size);
> > -    }
> > -
> > -    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)) )
> 
> Nit: This, according to my understanding, is an "impossible" style. It wants
> to either be
> 
>         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)) )
> 
> or ...

I will switch to the example above, thanks.

> > +        {
> > +            pfn_pdx_compression_reset();
> > +            printk(XENLOG_WARNING
> > +                   "PFN compression disabled, RAM region [%#" PRIpaddr ", 
> > %#"
> > +                   PRIpaddr "] not covered\n",
> > +                   mem->bank[bank].start,
> > +                   mem->bank[bank].start + mem->bank[bank].size - 1);
> 
> ... like this. But it's not written down anywhere, so I guess I shouldn't
> insist.
> 
> And then - isn't the use of PFN_UP() and PFN_DOWN() the wrong way round?
> Partial pages aren't usable anyway, so the smaller range is what matters
> for every individual bank. However, for two contiguous banks (no idea
> whether Arm would fold such into a single one, like we do with same-type
> E820 regions on x86) this gets more complicated then.

I think it's safer to always attempt to cover the wider range, even if
the first and last pages are not fully covered, and shouldn't be used
as RAM.  Like you said it will get more complicated if ranges are
contiguous but the start and end are not page aligned.

> > @@ -299,19 +295,29 @@ void __init srat_parse_regions(paddr_t addr)
> >  
> >     /* Set "PXM" as early as feasible. */
> >     numa_fw_nid_name = "PXM";
> > -   srat_region_mask = pdx_init_mask(addr);
> >     acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
> >                           srat_parse_region, 0);
> >  
> > -   for (mask = srat_region_mask, i = 0; mask && i < e820.nr_map; i++) {
> > +   pfn_pdx_compression_setup(addr);
> > +
> > +   /* Ensure all RAM ranges in the e820 are covered. */
> > +   for (i = 0; i < e820.nr_map; i++) {
> >             if (e820.map[i].type != E820_RAM)
> >                     continue;
> >  
> > -           if (~mask & pdx_region_mask(e820.map[i].addr, e820.map[i].size))
> > -                   mask = 0;
> > +           if (!pdx_is_region_compressible(e820.map[i].addr,
> > +               PFN_UP(e820.map[i].addr + e820.map[i].size) -
> > +               PFN_DOWN(e820.map[i].addr)))
> 
> Indentation is off here in any event, i.e. irrespective of my earlier
> remark.

Hm, yes, I've made a mess with indentation here.

> 
> > --- a/xen/common/pdx.c
> > +++ b/xen/common/pdx.c
> > @@ -19,6 +19,7 @@
> >  #include <xen/mm.h>
> >  #include <xen/bitops.h>
> >  #include <xen/nospec.h>
> > +#include <xen/pfn.h>
> >  #include <xen/sections.h>
> >  
> >  /**
> > @@ -55,6 +56,44 @@ void set_pdx_range(unsigned long smfn, unsigned long 
> > emfn)
> >          __set_bit(idx, pdx_group_valid);
> >  }
> >  
> > +#ifndef CONFIG_PDX_NONE
> > +
> > +#ifdef CONFIG_X86
> > +# include <asm/e820.h>
> > +# define MAX_PFN_RANGES E820MAX
> > +#elif defined(CONFIG_HAS_DEVICE_TREE)
> > +# include <xen/bootfdt.h>
> > +# define MAX_PFN_RANGES NR_MEM_BANKS
> > +#endif
> > +
> > +#ifndef MAX_PFN_RANGES
> > +# error "Missing architecture maximum number of RAM ranges"
> > +#endif
> > +
> > +/* Generic PFN compression helpers. */
> > +static struct pfn_range {
> > +    unsigned long base, size;
> > +} ranges[MAX_PFN_RANGES] __initdata;
> > +static unsigned int __initdata nr_ranges;
> > +
> > +void __init pfn_pdx_add_region(paddr_t base, paddr_t size)
> > +{
> > +    if ( !size )
> > +        return;
> > +
> > +    if ( nr_ranges >= ARRAY_SIZE(ranges) )
> > +    {
> > +        ASSERT((nr_ranges + 1) > nr_ranges);
> 
> This looks overly pessimistic to me. (I won't outright insist on its removal,
> though.)

TBH I've added this later, I don't have a strong opinion either.  I
don't think we usually check for overflows, so I understand this might
look odd.

> > +        nr_ranges++;
> 
> This requires pretty careful use of the variable as an upper bound of loops.
> It's fine in pfn_pdx_compression_setup(), but it feels a little risky.

It does require careful handling in pfn_pdx_compression_setup(), but
also has the benefit of providing the possibly new required upper
bound for PDX to be usable in the error message.

Thanks, Roger.