Introduce a command line option to allow disabling PDX compression.  The
disabling is done by turning pfn_pdx_add_region() into a no-op, so when
attempting to initialize the selected compression algorithm the array of
ranges to compress is empty.

Signed-off-by: Roger Pau Monné <roger.pau@cloud.com>
---
Changes since v1:
 - New in this version.
---
 docs/misc/xen-command-line.pandoc |  9 +++++++++
 xen/common/pdx.c                  | 10 +++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index b0eadd2c5d58..c747a326be86 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2072,6 +2072,15 @@ for all of them (`true`), only for those subject to XPTI (`xpti`) or for
 those not subject to XPTI (`no-xpti`). The feature is used only in case
 INVPCID is supported and not disabled via `invpcid=false`.
 
+### pdx-compress
+> `= <boolean>`
+
+> Default: `true` if CONFIG_PDX_NONE is unset
+
+Only relevant when the hypervisor is build with PFN PDX compression. Controls
+whether Xen will engage in PFN compression.  The algorithm used for PFN
+compression is selected at build time from Kconfig.
+
 ### ple_gap
 > `= <integer>`
 
diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index 6f488366e5a9..8c107676da59 100644
--- 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/param.h>
 #include <xen/pfn.h>
 #include <xen/sections.h>
 
@@ -76,9 +77,13 @@ static struct pfn_range {
 } ranges[MAX_PFN_RANGES] __initdata;
 static unsigned int __initdata nr_ranges;
 
+static bool __initdata pdx_compress = true;
+boolean_param("pdx-compress", pdx_compress);
+
 void __init pfn_pdx_add_region(paddr_t base, paddr_t size)
 {
-    if ( !size )
+    /* Without ranges there's no PFN compression. */
+    if ( !size || !pdx_compress )
         return;
 
     if ( nr_ranges >= ARRAY_SIZE(ranges) )
@@ -215,6 +220,9 @@ void __init pfn_pdx_compression_setup(paddr_t base)
     unsigned int i, j, bottom_shift = 0, hole_shift = 0;
     unsigned long mask = pdx_init_mask(base) >> PAGE_SHIFT;
 
+    if ( !nr_ranges )
+        return;
+
     if ( nr_ranges > ARRAY_SIZE(ranges) )
     {
         printk(XENLOG_WARNING
-- 
2.49.0

On 20.06.2025 13:11, Roger Pau Monne wrote:
> Introduce a command line option to allow disabling PDX compression.  The
> disabling is done by turning pfn_pdx_add_region() into a no-op, so when
> attempting to initialize the selected compression algorithm the array of
> ranges to compress is empty.

While neat, this also feels fragile. It's not obvious that for any
algorithm pfn_pdx_compression_setup() would leave compression disabled
when there are zero ranges. In principle, if it was written differently
for mask compression, there being no ranges could result in compression
simply squeezing out all of the address bits. Yet as long as we think
we're going to keep this in mind ...

> Signed-off-by: Roger Pau Monné <roger.pau@cloud.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

On Tue, Jun 24, 2025 at 03:40:16PM +0200, Jan Beulich wrote:
> On 20.06.2025 13:11, Roger Pau Monne wrote:
> > Introduce a command line option to allow disabling PDX compression.  The
> > disabling is done by turning pfn_pdx_add_region() into a no-op, so when
> > attempting to initialize the selected compression algorithm the array of
> > ranges to compress is empty.
> 
> While neat, this also feels fragile. It's not obvious that for any
> algorithm pfn_pdx_compression_setup() would leave compression disabled
> when there are zero ranges. In principle, if it was written differently
> for mask compression, there being no ranges could result in compression
> simply squeezing out all of the address bits. Yet as long as we think
> we're going to keep this in mind ...

It seemed to me that nr_rages == 0 (so no ranges reported) should
result in no compression, for example on x86 this means there's no
SRAT.

> > Signed-off-by: Roger Pau Monné <roger.pau@cloud.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks, Roger.

On 25.06.2025 17:46, Roger Pau Monné wrote:
> On Tue, Jun 24, 2025 at 03:40:16PM +0200, Jan Beulich wrote:
>> On 20.06.2025 13:11, Roger Pau Monne wrote:
>>> Introduce a command line option to allow disabling PDX compression.  The
>>> disabling is done by turning pfn_pdx_add_region() into a no-op, so when
>>> attempting to initialize the selected compression algorithm the array of
>>> ranges to compress is empty.
>>
>> While neat, this also feels fragile. It's not obvious that for any
>> algorithm pfn_pdx_compression_setup() would leave compression disabled
>> when there are zero ranges. In principle, if it was written differently
>> for mask compression, there being no ranges could result in compression
>> simply squeezing out all of the address bits. Yet as long as we think
>> we're going to keep this in mind ...
> 
> It seemed to me that nr_rages == 0 (so no ranges reported) should
> result in no compression, for example on x86 this means there's no
> SRAT.

Just to mention it: While in the pfn_pdx_compression_setup() flavor in
patch 3 there's no explicit check (hence the logic is assumed to be
coping with that situation), the one introduced in the last patch does
have such an explicit check. Apparently there the logic doesn't cleanly
cover that case all by itself.

Jan

On Wed, Jun 25, 2025 at 06:00:48PM +0200, Jan Beulich wrote:
> On 25.06.2025 17:46, Roger Pau Monné wrote:
> > On Tue, Jun 24, 2025 at 03:40:16PM +0200, Jan Beulich wrote:
> >> On 20.06.2025 13:11, Roger Pau Monne wrote:
> >>> Introduce a command line option to allow disabling PDX compression.  The
> >>> disabling is done by turning pfn_pdx_add_region() into a no-op, so when
> >>> attempting to initialize the selected compression algorithm the array of
> >>> ranges to compress is empty.
> >>
> >> While neat, this also feels fragile. It's not obvious that for any
> >> algorithm pfn_pdx_compression_setup() would leave compression disabled
> >> when there are zero ranges. In principle, if it was written differently
> >> for mask compression, there being no ranges could result in compression
> >> simply squeezing out all of the address bits. Yet as long as we think
> >> we're going to keep this in mind ...
> > 
> > It seemed to me that nr_rages == 0 (so no ranges reported) should
> > result in no compression, for example on x86 this means there's no
> > SRAT.
> 
> Just to mention it: While in the pfn_pdx_compression_setup() flavor in
> patch 3 there's no explicit check (hence the logic is assumed to be
> coping with that situation),

If you prefer I can leave the pfn_pdx_compression_setup() as-is in
patch 3, as AFAICT that implementation does cope with nr_ranges == 0,
that would result in a mask with just the low bits set, and hence
hole_shift will be 0.

> the one introduced in the last patch does
> have such an explicit check. Apparently there the logic doesn't cleanly
> cover that case all by itself.

No, I don't think the logic in patch 8 will cope nicely with nr_ranges
== 0, it seems to me at least the flsl() against a 0 pdx size mask
would result in an invalid pdx_index_shift given the current logic.

IMO it's best to short-circuit the nr_ranges == 0 case early in the
function, as that avoids complexity.

Thanks, Roger.

On 25.06.2025 19:45, Roger Pau Monné wrote:
> On Wed, Jun 25, 2025 at 06:00:48PM +0200, Jan Beulich wrote:
>> On 25.06.2025 17:46, Roger Pau Monné wrote:
>>> On Tue, Jun 24, 2025 at 03:40:16PM +0200, Jan Beulich wrote:
>>>> On 20.06.2025 13:11, Roger Pau Monne wrote:
>>>>> Introduce a command line option to allow disabling PDX compression.  The
>>>>> disabling is done by turning pfn_pdx_add_region() into a no-op, so when
>>>>> attempting to initialize the selected compression algorithm the array of
>>>>> ranges to compress is empty.
>>>>
>>>> While neat, this also feels fragile. It's not obvious that for any
>>>> algorithm pfn_pdx_compression_setup() would leave compression disabled
>>>> when there are zero ranges. In principle, if it was written differently
>>>> for mask compression, there being no ranges could result in compression
>>>> simply squeezing out all of the address bits. Yet as long as we think
>>>> we're going to keep this in mind ...
>>>
>>> It seemed to me that nr_rages == 0 (so no ranges reported) should
>>> result in no compression, for example on x86 this means there's no
>>> SRAT.
>>
>> Just to mention it: While in the pfn_pdx_compression_setup() flavor in
>> patch 3 there's no explicit check (hence the logic is assumed to be
>> coping with that situation),
> 
> If you prefer I can leave the pfn_pdx_compression_setup() as-is in
> patch 3, as AFAICT that implementation does cope with nr_ranges == 0,
> that would result in a mask with just the low bits set, and hence
> hole_shift will be 0.
> 
>> the one introduced in the last patch does
>> have such an explicit check. Apparently there the logic doesn't cleanly
>> cover that case all by itself.
> 
> No, I don't think the logic in patch 8 will cope nicely with nr_ranges
> == 0, it seems to me at least the flsl() against a 0 pdx size mask
> would result in an invalid pdx_index_shift given the current logic.
> 
> IMO it's best to short-circuit the nr_ranges == 0 case early in the
> function, as that avoids complexity.

FTAOD - I didn't mean to ask for any change. I merely meant to point out
that already within this series the special use of setting nr_ranges to
zero requires (a tiny bit of) extra care. But yes, since nr_ranges can
also end up being zero for other reasons, that bit of care is necessary
anyway.

Jan