It's set everywhere and can't be turned off because it's presence is
assumed in several parts of the codebase. This is an initial patch towards
adding a more fine-grained CONFIG_HAS_PDX_COMPRESSION that can actually be
disabled on systems that don't typically benefit from it.
No functional change.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
xen/arch/arm/Kconfig | 1 -
xen/arch/x86/Kconfig | 1 -
xen/common/Kconfig | 3 ---
xen/common/Makefile | 2 +-
xen/include/xen/pdx.h | 3 ---
5 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 439cc94f33..ea1949fbaa 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -14,7 +14,6 @@ config ARM
select HAS_ALTERNATIVE
select HAS_DEVICE_TREE
select HAS_PASSTHROUGH
- select HAS_PDX
select HAS_PMAP
select HAS_UBSAN
select IOMMU_FORCE_PT_SHARE
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 92f3a627da..30df085d96 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -24,7 +24,6 @@ config X86
select HAS_PASSTHROUGH
select HAS_PCI
select HAS_PCI_MSI
- select HAS_PDX
select HAS_SCHED_GRANULARITY
select HAS_UBSAN
select HAS_VPCI if HVM
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index dd8d7c3f1c..40ec63c4b2 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -53,9 +53,6 @@ config HAS_IOPORTS
config HAS_KEXEC
bool
-config HAS_PDX
- bool
-
config HAS_PMAP
bool
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 46049eac35..0020cafb8a 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -29,7 +29,7 @@ obj-y += multicall.o
obj-y += notifier.o
obj-$(CONFIG_NUMA) += numa.o
obj-y += page_alloc.o
-obj-$(CONFIG_HAS_PDX) += pdx.o
+obj-y += pdx.o
obj-$(CONFIG_PERF_COUNTERS) += perfc.o
obj-bin-$(CONFIG_HAS_PMAP) += pmap.init.o
obj-y += preempt.o
diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
index de5439a5e5..67ae20e89c 100644
--- a/xen/include/xen/pdx.h
+++ b/xen/include/xen/pdx.h
@@ -67,8 +67,6 @@
* region involved.
*/
-#ifdef CONFIG_HAS_PDX
-
extern unsigned long max_pdx;
extern unsigned long pfn_pdx_bottom_mask, ma_va_bottom_mask;
extern unsigned int pfn_pdx_hole_shift;
@@ -171,7 +169,6 @@ static inline unsigned long pdx_to_pfn(unsigned long pdx)
*/
void pfn_pdx_hole_setup(unsigned long mask);
-#endif /* HAS_PDX */
#endif /* __XEN_PDX_H__ */
/*
--
2.34.1On 17.07.2023 18:03, Alejandro Vallejo wrote: > It's set everywhere and can't be turned off because it's presence is > assumed in several parts of the codebase. This is an initial patch towards > adding a more fine-grained CONFIG_HAS_PDX_COMPRESSION that can actually be > disabled on systems that don't typically benefit from it. > > No functional change. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> On its own I don't think this is okay, as it's unclear whether it would affect RISC-V or PPC in a negative way. If at all, it should only go in together with the later re-introduction of a CONFIG_*. Still I question whether that new CONFIG_HAS_PDX_COMPRESSION (which imo ought to be CONFIG_PDX_COMPRESSION) then wouldn't better depend on the arch-selected HAS_PDX, such that it wouldn't wrongly be offered to choose a value when compression isn't supported in the first place. Jan
On 18/07/2023 10:19 am, Jan Beulich wrote: > On 17.07.2023 18:03, Alejandro Vallejo wrote: >> It's set everywhere and can't be turned off because it's presence is >> assumed in several parts of the codebase. This is an initial patch towards >> adding a more fine-grained CONFIG_HAS_PDX_COMPRESSION that can actually be >> disabled on systems that don't typically benefit from it. >> >> No functional change. >> >> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > On its own I don't think this is okay, as it's unclear whether it would > affect RISC-V or PPC in a negative way. Neither could compile without layering violations of PDX logic in common code, and neither have got that far yet. Now is an excellent time to be doing this, because it removes work for RISC-V/PPC... > If at all, it should only go in > together with the later re-introduction of a CONFIG_*. Still I question > whether that new CONFIG_HAS_PDX_COMPRESSION (which imo ought to be > CONFIG_PDX_COMPRESSION) then wouldn't better depend on the arch-selected > HAS_PDX, such that it wouldn't wrongly be offered to choose a value when > compression isn't supported in the first place. ... although I do agree that the resulting option shouldn't be user selectable. It's a property of the architecture, not something a user should be worrying about. I also don't see why this patch needs to come here in the series. The main refactoring in the following two patches looks to be independent. ~Andrew
On 18.07.2023 11:35, Andrew Cooper wrote: > On 18/07/2023 10:19 am, Jan Beulich wrote: >> On 17.07.2023 18:03, Alejandro Vallejo wrote: >>> It's set everywhere and can't be turned off because it's presence is >>> assumed in several parts of the codebase. This is an initial patch towards >>> adding a more fine-grained CONFIG_HAS_PDX_COMPRESSION that can actually be >>> disabled on systems that don't typically benefit from it. >>> >>> No functional change. >>> >>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> >> On its own I don't think this is okay, as it's unclear whether it would >> affect RISC-V or PPC in a negative way. > > Neither could compile without layering violations of PDX logic in common > code, and neither have got that far yet. > > Now is an excellent time to be doing this, because it removes work for > RISC-V/PPC... > >> If at all, it should only go in >> together with the later re-introduction of a CONFIG_*. Still I question >> whether that new CONFIG_HAS_PDX_COMPRESSION (which imo ought to be >> CONFIG_PDX_COMPRESSION) then wouldn't better depend on the arch-selected >> HAS_PDX, such that it wouldn't wrongly be offered to choose a value when >> compression isn't supported in the first place. > > ... although I do agree that the resulting option shouldn't be user > selectable. It's a property of the architecture, not something a user > should be worrying about. I disagree. Exotic x86 hardware may or may not want supporting, which can only be determined by the build meister, as long as this is indeed meant to become a build-time decision (rather than a runtime one; see my response to the cover letter of this series). Jan
On Tue, Jul 18, 2023 at 11:38:14AM +0200, Jan Beulich wrote:
> On 18.07.2023 11:35, Andrew Cooper wrote:
> > On 18/07/2023 10:19 am, Jan Beulich wrote:
> >> On 17.07.2023 18:03, Alejandro Vallejo wrote:
> >>> It's set everywhere and can't be turned off because it's presence is
> >>> assumed in several parts of the codebase. This is an initial patch towards
> >>> adding a more fine-grained CONFIG_HAS_PDX_COMPRESSION that can actually be
> >>> disabled on systems that don't typically benefit from it.
> >>>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> >> On its own I don't think this is okay, as it's unclear whether it would
> >> affect RISC-V or PPC in a negative way.
> >
> > Neither could compile without layering violations of PDX logic in common
> > code, and neither have got that far yet.
> >
> > Now is an excellent time to be doing this, because it removes work for
> > RISC-V/PPC...
> >
> >> If at all, it should only go in
> >> together with the later re-introduction of a CONFIG_*.
I could merge this patch with patch 8. I do feel they tackle different
matters, but when HAS_PDX goes away doesn't matter.
> >> Still I question
> >> whether that new CONFIG_HAS_PDX_COMPRESSION (which imo ought to be
> >> CONFIG_PDX_COMPRESSION)
Sure, I'll change that in v2.
> >> then wouldn't better depend on the arch-selected
> >> HAS_PDX, such that it wouldn't wrongly be offered to choose a value when
> >> compression isn't supported in the first place.
There are several concepts to consider:
* Frame numbers (mfn)
* Frame table indices (pdx)
* Mapping between the 2 (pdx_to_pfn/pfn_to_pdx)
(I purposefully ignore the directmap. There's a similar argument for it)
An arch opting out of pdx and an arch opting out of pdx compression are in
the exact same situation, except the later has the ability to easily toggle
it back on. Using pdx (_not_ compression) as a first-class type has several
advantages.
1. It allows common code to deal with pdx too. There are some conversions
to/from pdx that have to do with arch-specific code calling common
code or vice-versa. This is particularly bad in the presence of
compression This is particularly bad in the presence of compression
because it implies fairly frequent reads of global state.
2. It allows a port to get pdx-compression for free if it opts into it;
and more importantly, the ability to toggle it.
3. Simplifies the compression removal logic. Otherwise #ifdefs need to be
sprinkled around any architecture that may want to toggle it and
that's several orders of magnitude more difficult to read.
TL;DR: There is not a benefit in a new port purposefuilly avoiding PDX.
It's just a name for a particular index. It's _compression_ that makes it
have a whacky relationship with mfns and might want toggling.
Incidentally, note that common/numa.c does use PDX.
> >
> > ... although I do agree that the resulting option shouldn't be user
> > selectable. It's a property of the architecture, not something a user
> > should be worrying about.
>
> I disagree. Exotic x86 hardware may or may not want supporting, which
> can only be determined by the build meister, as long as this is indeed
> meant to become a build-time decision (rather than a runtime one; see
> my response to the cover letter of this series).
>
> Jan
I won't get into x86, because I have never seen such exotic hardware, but
ARM definitely has a lot more heterogeneous system designs where it might
be needed on a system-by-system basis.
Alejandro
© 2016 - 2026 Red Hat, Inc.