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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 ...

> +        {
> +            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.

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

> --- 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.)

> +        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.

Jan