Rename the current CONFIG_PDX_COMPRESSION to CONFIG_PDX_MASK_COMPRESSION,
and make it part of the PDX compression choice block, in preparation for
adding further PDX compression algorithms.
No functional change intended as the PDX compression defaults should still
be the same for all architectures, however the choice block cannot be
protected under EXPERT and still have a default choice being
unconditionally selected. As a result, the new "PDX (Page inDeX)
compression" item will be unconditionally visible in Kconfig.
As part of this preparation work to introduce new PDX compressions, adjust
some of the comments on pdx.h to note they apply to a specific PDX
compression. Also shuffle function prototypes and dummy implementations
around to make it easier to introduce a new PDX compression. Note all
PDX compression implementations are expected to provide a
pdx_is_region_compressible() that takes the same set of arguments.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/Kconfig | 18 +++++++++++++++---
xen/common/pdx.c | 4 ++--
xen/include/xen/pdx.h | 32 +++++++++++++++++++-------------
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 867710134ae5..de3e01d6320e 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -52,9 +52,10 @@ config EVTCHN_FIFO
If unsure, say Y.
-config PDX_COMPRESSION
- bool "PDX (Page inDeX) compression" if EXPERT && !X86 && !RISCV
- default ARM || PPC
+choice
+ prompt "PDX (Page inDeX) compression"
+ default PDX_MASK_COMPRESSION if !X86 && !RISCV
+ default PDX_NONE
help
PDX compression is a technique designed to reduce the memory
overhead of physical memory management on platforms with sparse RAM
@@ -67,6 +68,17 @@ config PDX_COMPRESSION
If your platform does not have sparse RAM banks, do not enable PDX
compression.
+config PDX_MASK_COMPRESSION
+ bool "Mask compression"
+ help
+ Compression relying on all RAM addresses sharing a zeroed bit region.
+
+config PDX_NONE
+ bool "None"
+ help
+ No compression
+endchoice
+
config ALTERNATIVE_CALL
bool
diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index b8384e6189df..00aa7e43006d 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -34,7 +34,7 @@ bool __mfn_valid(unsigned long mfn)
{
bool invalid = mfn >= max_page;
-#ifdef CONFIG_PDX_COMPRESSION
+#ifdef CONFIG_PDX_MASK_COMPRESSION
invalid |= mfn & pfn_hole_mask;
#endif
@@ -55,7 +55,7 @@ void set_pdx_range(unsigned long smfn, unsigned long emfn)
__set_bit(idx, pdx_group_valid);
}
-#ifdef CONFIG_PDX_COMPRESSION
+#ifdef CONFIG_PDX_MASK_COMPRESSION
/*
* Diagram to make sense of the following variables. The masks and shifts
diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
index c1423d64a95b..8e373cac8b87 100644
--- a/xen/include/xen/pdx.h
+++ b/xen/include/xen/pdx.h
@@ -25,7 +25,7 @@
* this by keeping a bitmap of the ranges in the frame table containing
* invalid entries and not allocating backing memory for them.
*
- * ## PDX compression
+ * ## PDX mask compression
*
* This is a technique to avoid wasting memory on machines known to have
* split their machine address space in several big discontinuous and highly
@@ -101,22 +101,13 @@ bool __mfn_valid(unsigned long mfn);
#define paddr_to_pdx(pa) pfn_to_pdx(paddr_to_pfn(pa))
#define pdx_to_paddr(px) pfn_to_paddr(pdx_to_pfn(px))
-#ifdef CONFIG_PDX_COMPRESSION
+#ifdef CONFIG_PDX_MASK_COMPRESSION
extern unsigned long pfn_pdx_bottom_mask, ma_va_bottom_mask;
extern unsigned int pfn_pdx_hole_shift;
extern unsigned long pfn_hole_mask;
extern unsigned long pfn_top_mask, ma_top_mask;
-/**
- * Validate a region's compatibility with the current compression runtime
- *
- * @param base Base address of the region
- * @param npages Number of PAGE_SIZE-sized pages in the region
- * @return True iff the region can be used with the current compression
- */
-bool pdx_is_region_compressible(paddr_t base, unsigned long npages);
-
/**
* Calculates a mask covering "moving" bits of all addresses of a region
*
@@ -209,7 +200,9 @@ static inline paddr_t directmapoff_to_maddr(unsigned long offset)
*/
void pfn_pdx_hole_setup(unsigned long mask);
-#else /* !CONFIG_PDX_COMPRESSION */
+#endif /* CONFIG_PDX_MASK_COMPRESSION */
+
+#ifdef CONFIG_PDX_NONE
/* Without PDX compression we can skip some computations */
@@ -241,7 +234,20 @@ static inline void pfn_pdx_hole_setup(unsigned long mask)
{
}
-#endif /* CONFIG_PDX_COMPRESSION */
+#else /* !CONFIG_PDX_NONE */
+
+/* Shared functions implemented by all PDX compressions. */
+
+/**
+ * Validate a region's compatibility with the current compression runtime
+ *
+ * @param base Base address of the region
+ * @param npages Number of PAGE_SIZE-sized pages in the region
+ * @return True iff the region can be used with the current compression
+ */
+bool pdx_is_region_compressible(paddr_t base, unsigned long npages);
+
+#endif /* !CONFIG_PDX_NONE */
#endif /* __XEN_PDX_H__ */
/*
--
2.49.0
On 20.06.2025 13:11, Roger Pau Monne wrote: > Rename the current CONFIG_PDX_COMPRESSION to CONFIG_PDX_MASK_COMPRESSION, > and make it part of the PDX compression choice block, in preparation for > adding further PDX compression algorithms. > > No functional change intended as the PDX compression defaults should still > be the same for all architectures, however the choice block cannot be > protected under EXPERT and still have a default choice being > unconditionally selected. As a result, the new "PDX (Page inDeX) > compression" item will be unconditionally visible in Kconfig. Just to mention it: Afaict there is a functional change, but one I actually appreciate, at least in part. So far ... > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -52,9 +52,10 @@ config EVTCHN_FIFO > > If unsure, say Y. > > -config PDX_COMPRESSION > - bool "PDX (Page inDeX) compression" if EXPERT && !X86 && !RISCV > - default ARM || PPC ... for x86 (and RISC-V) this option couldn't be selected. Whereas ... > @@ -67,6 +68,17 @@ config PDX_COMPRESSION > If your platform does not have sparse RAM banks, do not enable PDX > compression. > > +config PDX_MASK_COMPRESSION > + bool "Mask compression" > + help > + Compression relying on all RAM addresses sharing a zeroed bit region. ... this option is now available, as the prior !X86 && !RISCV doesn't re-appear here. (As the description mentions it, that dependency clearly can't appear on the enclosing choice itself.) Since x86 actually still should have mask compression implemented properly, that's fine (from my pov; iirc I even asked that it would have remained available when the earlier change was done), whereas I think for RISC-V it's not quite right to offer the option. It also did escape me why the option was made available for PPC, which I'm pretty sure also lacks the logic to determine a suitable mask. Jan
On Tue, Jun 24, 2025 at 03:13:27PM +0200, Jan Beulich wrote: > On 20.06.2025 13:11, Roger Pau Monne wrote: > > Rename the current CONFIG_PDX_COMPRESSION to CONFIG_PDX_MASK_COMPRESSION, > > and make it part of the PDX compression choice block, in preparation for > > adding further PDX compression algorithms. > > > > No functional change intended as the PDX compression defaults should still > > be the same for all architectures, however the choice block cannot be > > protected under EXPERT and still have a default choice being > > unconditionally selected. As a result, the new "PDX (Page inDeX) > > compression" item will be unconditionally visible in Kconfig. > > Just to mention it: Afaict there is a functional change, but one I actually > appreciate, at least in part. So far ... > > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -52,9 +52,10 @@ config EVTCHN_FIFO > > > > If unsure, say Y. > > > > -config PDX_COMPRESSION > > - bool "PDX (Page inDeX) compression" if EXPERT && !X86 && !RISCV > > - default ARM || PPC > > ... for x86 (and RISC-V) this option couldn't be selected. Whereas ... > > > @@ -67,6 +68,17 @@ config PDX_COMPRESSION > > If your platform does not have sparse RAM banks, do not enable PDX > > compression. > > > > +config PDX_MASK_COMPRESSION > > + bool "Mask compression" > > + help > > + Compression relying on all RAM addresses sharing a zeroed bit region. > > ... this option is now available, as the prior !X86 && !RISCV doesn't > re-appear here. (As the description mentions it, that dependency clearly > can't appear on the enclosing choice itself.) Since x86 actually still > should have mask compression implemented properly, that's fine (from my > pov; iirc I even asked that it would have remained available when the > earlier change was done), whereas I think for RISC-V it's not quite right > to offer the option. It also did escape me why the option was made > available for PPC, which I'm pretty sure also lacks the logic to determine > a suitable mask. Yes, the only architectures that have functional PDX compression are x86 and ARM, as neither RISC-V nor PowerPC call the initialization functions. AFAICT this is harmless apart from giving the wrong impression to the user that PDX compression might be implemented. Would you prefer for me to introduce a new HAS_PDX config option that's selected by x86 and ARM, and is used to enable the choice PDX config? Thanks, Roger.
On 26.06.2025 09:49, Roger Pau Monné wrote:
> On Tue, Jun 24, 2025 at 03:13:27PM +0200, Jan Beulich wrote:
>> On 20.06.2025 13:11, Roger Pau Monne wrote:
>>> Rename the current CONFIG_PDX_COMPRESSION to CONFIG_PDX_MASK_COMPRESSION,
>>> and make it part of the PDX compression choice block, in preparation for
>>> adding further PDX compression algorithms.
>>>
>>> No functional change intended as the PDX compression defaults should still
>>> be the same for all architectures, however the choice block cannot be
>>> protected under EXPERT and still have a default choice being
>>> unconditionally selected. As a result, the new "PDX (Page inDeX)
>>> compression" item will be unconditionally visible in Kconfig.
>>
>> Just to mention it: Afaict there is a functional change, but one I actually
>> appreciate, at least in part. So far ...
>>
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -52,9 +52,10 @@ config EVTCHN_FIFO
>>>
>>> If unsure, say Y.
>>>
>>> -config PDX_COMPRESSION
>>> - bool "PDX (Page inDeX) compression" if EXPERT && !X86 && !RISCV
>>> - default ARM || PPC
>>
>> ... for x86 (and RISC-V) this option couldn't be selected. Whereas ...
>>
>>> @@ -67,6 +68,17 @@ config PDX_COMPRESSION
>>> If your platform does not have sparse RAM banks, do not enable PDX
>>> compression.
>>>
>>> +config PDX_MASK_COMPRESSION
>>> + bool "Mask compression"
>>> + help
>>> + Compression relying on all RAM addresses sharing a zeroed bit region.
>>
>> ... this option is now available, as the prior !X86 && !RISCV doesn't
>> re-appear here. (As the description mentions it, that dependency clearly
>> can't appear on the enclosing choice itself.) Since x86 actually still
>> should have mask compression implemented properly, that's fine (from my
>> pov; iirc I even asked that it would have remained available when the
>> earlier change was done), whereas I think for RISC-V it's not quite right
>> to offer the option. It also did escape me why the option was made
>> available for PPC, which I'm pretty sure also lacks the logic to determine
>> a suitable mask.
>
> Yes, the only architectures that have functional PDX compression are
> x86 and ARM, as neither RISC-V nor PowerPC call the initialization
> functions. AFAICT this is harmless apart from giving the wrong
> impression to the user that PDX compression might be implemented.
>
> Would you prefer for me to introduce a new HAS_PDX config option
> that's selected by x86 and ARM, and is used to enable the choice PDX
> config?
Hmm, no, I don't think I want you to make any change to the code. I'm
actually happy with the slight relaxation for x86 (and RISC-V), and
aiui you don't alter behavior for PPC. The fact that behavior there
(and for RISC-V) doesn't look quite right isn't an effect of your
change.
A change may be wanted to the description, to avoid giving the wrong
(afaict) impression of this being "no functional change". Considering
how things ended up the way they are prior to this series, this
becoming explicit may cause _others_ to want you to make changes,
though. Hence I simply wanted to raise that aspect, to give others a
hint that they may need to chime in.
For the record, below is what I think would represent original
behavior ("help" parts omitted), albeit still leaving out the EXPERT
aspect (as it's not clear to me what a condition on a prompt means in
a choice element):
choice
prompt "PDX (Page inDeX) compression"
default PDX_MASK_COMPRESSION if !X86 && !RISCV
default PDX_NONE
config PDX_MASK_COMPRESSION
bool "Mask compression"
depends on !X86 && !RISCV
config PDX_NONE
bool "None"
endchoice
But again, specifically for x86 I'd prefer if PDX_MASK_COMPRESSION
became available (again), so the above is not a suggestion to change
the code, unless others insisted on restoring prior behavior.
Jan
© 2016 - 2025 Red Hat, Inc.