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@citrix.com>
---
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
+ * the maximum PFN shift to use as the lookup table index.
*/
for ( i = 0; i < nr_ranges; i++ )
+ {
+ mask |= pdx_region_mask(ranges[i].base_pfn, ranges[i].pages);
for ( unsigned int j = i + 1; j < nr_ranges; j++ )
idx_mask |= ranges[i].base_pfn ^ ranges[j].base_pfn;
+ }
/* Mask out the bits that change inside of ranges. */
idx_mask &= ~mask;
--
2.53.0
On 6/9/26 10:53 AM, 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@citrix.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Thanks.
~ Oleksii
On 09/06/2026 9:53 am, 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@citrix.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 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.
What's the 2s here? If it is what I think it is, I'd suggest "Dell
R740, dual socket,"
~Andrew
On Tue, Jun 09, 2026 at 09:59:37AM +0100, Andrew Cooper wrote:
> On 09/06/2026 9:53 am, 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@citrix.com>
>
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> > 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.
>
> What's the 2s here? If it is what I think it is, I'd suggest "Dell
> R740, dual socket,"
Yes, it's what you think it is. I've used "2s" in existing comments,
but I'm perfectly fine with spelling it out.
Thanks, Roger.
On 09/06/2026 10:14 am, Roger Pau Monné wrote:
> On Tue, Jun 09, 2026 at 09:59:37AM +0100, Andrew Cooper wrote:
>> On 09/06/2026 9:53 am, 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@citrix.com>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>>> 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.
>> What's the 2s here? If it is what I think it is, I'd suggest "Dell
>> R740, dual socket,"
> Yes, it's what you think it is. I've used "2s" in existing comments,
Oh, fine.
~Andrew
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@citrix.com>
> ---
> 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.
© 2016 - 2026 Red Hat, Inc.