Re: [PATCH for-4.22] xen/pdx: fix off-by-one index in offset mask calculation

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

On Tue, Jun 09, 2026 at 10:53:22AM +0200, Roger Pau Monne wrote:
> Adjust the mask calculation in case the last range is merged with the
> previous one, as then the mask must be calculated from the previous range,
> which the current one has been merged into.
> 
> Instead of fixing the off-by-one in place, move the calculation of the bit
> change mask to the next loop, after the ranges have been merged.  This
> simplifies the logic by consolidating mask calculation in a single place,
> possibly making it less error prone in the future.
> 
> Also add a test case that triggers the bug being fixed by this commit.
> 
> Fixes: c5c45bcbd6a1 ("pdx: introduce a new compression algorithm based on 
> region offsets")
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  tools/tests/pdx/test-pdx.c | 14 ++++++++++++++
>  xen/common/pdx.c           | 13 ++++++-------
>  2 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/tests/pdx/test-pdx.c b/tools/tests/pdx/test-pdx.c
> index d783186577ef..ba57f1793011 100644
> --- a/tools/tests/pdx/test-pdx.c
> +++ b/tools/tests/pdx/test-pdx.c
> @@ -191,6 +191,20 @@ int main(int argc, char **argv)
>              },
>              .compress = false,
>          },
> +        /*
> +         * 2s Dell R740, merging of ranges causes mask differences in PDX
> +         * offset mode.  Useful for checking mask calculations.
> +         */
> +        {
> +            .ranges = {
> +                { .start = 0x0000000UL, .end = 0x0080000UL },
> +                { .start = 0x0100000UL, .end = 0x3070000UL },
> +                { .start = 0x3070000UL, .end = 0x3870000UL },
> +                { .start = 0x3870000UL, .end = 0x6870000UL },
> +                { .start = 0x6870000UL, .end = 0x7070000UL },
> +            },
> +            .compress = false,
> +        },
>      };
>      int ret_code = EXIT_SUCCESS;
>  
> diff --git a/xen/common/pdx.c b/xen/common/pdx.c
> index 7e070ff962e8..a84c7d19ade4 100644
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -391,10 +391,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
>          if ( !i ||
>               ranges[i].base_pfn >=
>               (ranges[i - 1].base_pfn + ranges[i - 1].pages) )
> -        {
> -            mask |= pdx_region_mask(ranges[i].base_pfn, ranges[i].pages);
>              continue;
> -        }
>  
>          ranges[i - 1].pages = ranges[i].base_pfn + ranges[i].pages -
>                                ranges[i - 1].base_pfn;
> @@ -402,19 +399,21 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
>          if ( i + 1 < nr_ranges )
>              memmove(&ranges[i], &ranges[i + 1],
>                      (nr_ranges - (i + 1)) * sizeof(ranges[0]));
> -        else /* last range */
> -            mask |= pdx_region_mask(ranges[i].base_pfn, ranges[i].pages);
>          nr_ranges--;
>          i--;
>      }
>  
>      /*
> -     * Populate a mask with the non-equal bits of the different ranges, do 
> this
> -     * to calculate the maximum PFN shift to use as the lookup table index.
> +     * Populate two masks: one with the non-equal bits of the different 
> ranges,
> +     * another with the bits that change inside regions. Do this to calculate

Forgot to refresh the patch before sending, this line should be:

"another with the bits that change inside ranges.  Do this to calculate"

IOW: s/regions/ranges/ so that we don't mix "ranges" and "regions" in
the same paragraph which is confusing.

Thanks, Roger.