[PATCH v3 0/4] Make PDX compression optional

[PATCH v3 0/4] Make PDX compression optional

[Alejandro Vallejo]

Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
disable it because the whole codebase performs unconditional
compression/decompression operations on addresses. This has the
unfortunate side effect that systems without a need for compression still
have to pay the performance impact of juggling bits on every pfn<->pdx
conversion (this requires reading several global variables). This series
attempts to:

  * Leave the state of pdx and pdx compression documented
  * Factor out compression so it _can_ be removed through Kconfig
  * Make it so compression is disabled on x86 and enabled on both Aarch32
    and Aarch64 by default.

Series summary:

Patch 1 Moves hard-coded compression-related logic to helper functions
Patch 2 Refactors all instances of regions being validated for pdx
        compression conformance so it's done through a helper
Patch 3 Non-functional reorder in order to simplify the patch 8 diff
Patch 4 Adds new Kconfig option to compile out PDX compression and removes
        the old CONFIG_HAS_PDX, as it was non removable

Already committed:

v1/patch 1 documents the current general understanding of the pdx concept and
           pdx compression in particular
v1/patch 3 Marks the pdx compression globals as ro_after_init
v2/patch 1 Documents the differences between arm32 and arm64 directmaps

Alejandro Vallejo (4):
  mm: Factor out the pdx compression logic in ma/va converters
  mm/pdx: Standardize region validation wrt pdx compression
  pdx: Reorder pdx.[ch]
  pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

 xen/arch/arm/Kconfig                   |   1 -
 xen/arch/arm/include/asm/mm.h          |   3 +-
 xen/arch/x86/Kconfig                   |   1 -
 xen/arch/x86/domain.c                  |  19 ++--
 xen/arch/x86/include/asm/x86_64/page.h |  28 +++---
 xen/arch/x86/x86_64/mm.c               |   6 +-
 xen/common/Kconfig                     |  13 ++-
 xen/common/Makefile                    |   2 +-
 xen/common/efi/boot.c                  |  13 ++-
 xen/common/pdx.c                       |  76 +++++++++------
 xen/include/xen/pdx.h                  | 127 +++++++++++++++++++------
 11 files changed, 193 insertions(+), 96 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] mm: Factor out the pdx compression logic in ma/va converters

[Alejandro Vallejo]

This patch factors out the pdx compression logic hardcoded in both ports
for the maddr<->vaddr conversion functions.

Touches both x86 and arm ports.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v3:
  * size_t -> unsigned long (Jan)
  * uint64_t -> paddr_t (Jan)
---
 xen/arch/arm/include/asm/mm.h          |  3 +--
 xen/arch/x86/include/asm/x86_64/page.h | 28 +++++++++++---------------
 xen/include/xen/pdx.h                  | 25 +++++++++++++++++++++++
 3 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 5b530f0f40..c0d7f0f181 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -319,8 +319,7 @@ static inline void *maddr_to_virt(paddr_t ma)
            (DIRECTMAP_SIZE >> PAGE_SHIFT));
     return (void *)(XENHEAP_VIRT_START -
                     (directmap_base_pdx << PAGE_SHIFT) +
-                    ((ma & ma_va_bottom_mask) |
-                     ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
+                    maddr_to_directmapoff(ma));
 }
 #endif
 
diff --git a/xen/arch/x86/include/asm/x86_64/page.h b/xen/arch/x86/include/asm/x86_64/page.h
index 53faa7875b..e40b451221 100644
--- a/xen/arch/x86/include/asm/x86_64/page.h
+++ b/xen/arch/x86/include/asm/x86_64/page.h
@@ -36,26 +36,22 @@ static inline unsigned long __virt_to_maddr(unsigned long va)
 {
     ASSERT(va < DIRECTMAP_VIRT_END);
     if ( va >= DIRECTMAP_VIRT_START )
-        va -= DIRECTMAP_VIRT_START;
-    else
-    {
-        BUILD_BUG_ON(XEN_VIRT_END - XEN_VIRT_START != GB(1));
-        /* Signed, so ((long)XEN_VIRT_START >> 30) fits in an imm32. */
-        ASSERT(((long)va >> (PAGE_ORDER_1G + PAGE_SHIFT)) ==
-               ((long)XEN_VIRT_START >> (PAGE_ORDER_1G + PAGE_SHIFT)));
-
-        va += xen_phys_start - XEN_VIRT_START;
-    }
-    return (va & ma_va_bottom_mask) |
-           ((va << pfn_pdx_hole_shift) & ma_top_mask);
+        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
+
+    BUILD_BUG_ON(XEN_VIRT_END - XEN_VIRT_START != GB(1));
+    /* Signed, so ((long)XEN_VIRT_START >> 30) fits in an imm32. */
+    ASSERT(((long)va >> (PAGE_ORDER_1G + PAGE_SHIFT)) ==
+           ((long)XEN_VIRT_START >> (PAGE_ORDER_1G + PAGE_SHIFT)));
+
+    return xen_phys_start + va - XEN_VIRT_START;
 }
 
 static inline void *__maddr_to_virt(unsigned long ma)
 {
-    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
-    return (void *)(DIRECTMAP_VIRT_START +
-                    ((ma & ma_va_bottom_mask) |
-                     ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
+    /* Offset in the direct map, accounting for pdx compression */
+    unsigned long va_offset = maddr_to_directmapoff(ma);
+    ASSERT(va_offset < DIRECTMAP_SIZE);
+    return (void *)(DIRECTMAP_VIRT_START + va_offset);
 }
 
 /* read access (should only be used for debug printk's) */
diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
index de5439a5e5..8f29598230 100644
--- a/xen/include/xen/pdx.h
+++ b/xen/include/xen/pdx.h
@@ -160,6 +160,31 @@ static inline unsigned long pdx_to_pfn(unsigned long pdx)
 #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn))
 #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx))
 
+/**
+ * Computes the offset into the direct map of an maddr
+ *
+ * @param ma Machine address
+ * @return Offset on the direct map where that
+ *         machine address can be accessed
+ */
+static inline unsigned long maddr_to_directmapoff(paddr_t ma)
+{
+    return ((ma & ma_top_mask) >> pfn_pdx_hole_shift) |
+           (ma & ma_va_bottom_mask);
+}
+
+/**
+ * Computes a machine address given a direct map offset
+ *
+ * @param offset Offset into the direct map
+ * @return Corresponding machine address of that virtual location
+ */
+static inline paddr_t directmapoff_to_maddr(unsigned long offset)
+{
+    return (((paddr_t)offset << pfn_pdx_hole_shift) & ma_top_mask) |
+           (offset & ma_va_bottom_mask);
+}
+
 /**
  * Initializes global variables with information about the compressible
  * range of the current memory regions.
-- 
2.34.1

[PATCH v3 2/4] mm/pdx: Standardize region validation wrt pdx compression

[Alejandro Vallejo]

Regions must be occasionally validated for pdx compression validity. That
is, whether any of the machine addresses spanning the region have a bit set
in the pdx "hole" (which is expected to always contain zeroes). There are
a few such tests through the code, and they all check for different things.

This patch replaces all such occurrences with a call to a centralized
function that checks a region for validity.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v3:
  * Pack variable declarations on top of the function (Jan)
  * Restrict variable scope by moving their declaration to the top of the
    loop in which they are used (Jan)
---
 xen/arch/x86/x86_64/mm.c |  6 ++++--
 xen/common/efi/boot.c    | 13 ++++++++++---
 xen/common/pdx.c         | 10 ++++++++--
 xen/include/xen/pdx.h    |  9 +++++++++
 4 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 15b36e332d..d3f7c59638 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1158,6 +1158,8 @@ static int transfer_pages_to_heap(struct mem_hotadd_info *info)
 static int mem_hotadd_check(unsigned long spfn, unsigned long epfn)
 {
     unsigned long s, e, length, sidx, eidx;
+    paddr_t mem_base = pfn_to_paddr(spfn);
+    unsigned long mem_npages = epfn - spfn;
 
     if ( (spfn >= epfn) )
         return 0;
@@ -1168,7 +1170,7 @@ static int mem_hotadd_check(unsigned long spfn, unsigned long epfn)
     if ( (spfn | epfn) & ((1UL << PAGETABLE_ORDER) - 1) )
         return 0;
 
-    if ( (spfn | epfn) & pfn_hole_mask )
+    if ( !pdx_is_region_compressible(mem_base, mem_npages) )
         return 0;
 
     /* Make sure the new range is not present now */
@@ -1207,7 +1209,7 @@ static int mem_hotadd_check(unsigned long spfn, unsigned long epfn)
 
     length += (e - s) * sizeof(struct page_info);
 
-    if ((length >> PAGE_SHIFT) > (epfn - spfn))
+    if ((length >> PAGE_SHIFT) > mem_npages)
         return 0;
 
     return 1;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 79a654af69..be4e3a0259 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -14,6 +14,7 @@
 #include <xen/multiboot.h>
 #include <xen/param.h>
 #include <xen/pci_regs.h>
+#include <xen/pdx.h>
 #include <xen/pfn.h>
 #if EFI_PAGE_SIZE != PAGE_SIZE
 # error Cannot use xen/pfn.h here!
@@ -1645,9 +1646,11 @@ static __init void copy_mapping(unsigned long mfn, unsigned long end,
 
 static bool __init cf_check ram_range_valid(unsigned long smfn, unsigned long emfn)
 {
+    paddr_t ram_base = pfn_to_paddr(smfn);
+    unsigned long ram_npages = emfn - smfn;
     unsigned long sz = pfn_to_pdx(emfn - 1) / PDX_GROUP_COUNT + 1;
 
-    return !(smfn & pfn_hole_mask) &&
+    return pdx_is_region_compressible(ram_base, ram_npages) &&
            find_next_bit(pdx_group_valid, sz,
                          pfn_to_pdx(smfn) / PDX_GROUP_COUNT) < sz;
 }
@@ -1681,6 +1684,8 @@ void __init efi_init_memory(void)
         u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
         unsigned long smfn, emfn;
         unsigned int prot = PAGE_HYPERVISOR_RWX;
+        paddr_t mem_base;
+        unsigned long mem_npages;
 
         printk(XENLOG_INFO " %013" PRIx64 "-%013" PRIx64
                            " type=%u attr=%016" PRIx64 "\n",
@@ -1732,6 +1737,9 @@ void __init efi_init_memory(void)
         smfn = PFN_DOWN(desc->PhysicalStart);
         emfn = PFN_UP(desc->PhysicalStart + len);
 
+        mem_base = pfn_to_paddr(smfn);
+        mem_npages = emfn - smfn;
+
         if ( desc->Attribute & EFI_MEMORY_WB )
             prot |= _PAGE_WB;
         else if ( desc->Attribute & EFI_MEMORY_WT )
@@ -1759,8 +1767,7 @@ void __init efi_init_memory(void)
             prot |= _PAGE_NX;
 
         if ( pfn_to_pdx(emfn - 1) < (DIRECTMAP_SIZE >> PAGE_SHIFT) &&
-             !(smfn & pfn_hole_mask) &&
-             !((smfn ^ (emfn - 1)) & ~pfn_pdx_bottom_mask) )
+             pdx_is_region_compressible(mem_base, mem_npages))
         {
             if ( (unsigned long)mfn_to_virt(emfn - 1) >= HYPERVISOR_VIRT_END )
                 prot &= ~_PAGE_GLOBAL;
diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index 99d4a90a50..3c88ceeb9c 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -88,7 +88,7 @@ bool __mfn_valid(unsigned long mfn)
 }
 
 /* Sets all bits from the most-significant 1-bit down to the LSB */
-static uint64_t __init fill_mask(uint64_t mask)
+static uint64_t fill_mask(uint64_t mask)
 {
     while (mask & (mask + 1))
         mask |= mask + 1;
@@ -96,6 +96,12 @@ static uint64_t __init fill_mask(uint64_t mask)
     return mask;
 }
 
+bool pdx_is_region_compressible(paddr_t base, unsigned long npages)
+{
+    return !(paddr_to_pfn(base) & pfn_hole_mask) &&
+           !(pdx_region_mask(base, npages * PAGE_SIZE) & ~ma_va_bottom_mask);
+}
+
 /* We don't want to compress the low MAX_ORDER bits of the addresses. */
 uint64_t __init pdx_init_mask(uint64_t base_addr)
 {
@@ -103,7 +109,7 @@ uint64_t __init pdx_init_mask(uint64_t base_addr)
                          (uint64_t)1 << (MAX_ORDER + PAGE_SHIFT)) - 1);
 }
 
-uint64_t __init pdx_region_mask(uint64_t base, uint64_t len)
+uint64_t pdx_region_mask(uint64_t base, uint64_t len)
 {
     /*
      * We say a bit "moves" in a range if there exist 2 addresses in that
diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
index 8f29598230..5674506f3a 100644
--- a/xen/include/xen/pdx.h
+++ b/xen/include/xen/pdx.h
@@ -79,6 +79,15 @@ extern unsigned long pfn_top_mask, ma_top_mask;
                          (sizeof(*frame_table) & -sizeof(*frame_table)))
 extern unsigned long pdx_group_valid[];
 
+/**
+ * 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
  *
-- 
2.34.1

[PATCH v3 3/4] pdx: Reorder pdx.[ch]

[Alejandro Vallejo]

The next patch compiles out compression-related chunks, and it's helpful to
have them grouped together beforehand.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v3:
  * No changes
---
 xen/common/pdx.c      | 58 +++++++++++++++++++++---------------------
 xen/include/xen/pdx.h | 59 ++++++++++++++++++++++---------------------
 2 files changed, 59 insertions(+), 58 deletions(-)

diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index 3c88ceeb9c..d3d38965bd 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -20,6 +20,35 @@
 #include <xen/bitops.h>
 #include <xen/nospec.h>
 
+/**
+ * Maximum (non-inclusive) usable pdx. Must be
+ * modifiable after init due to memory hotplug
+ */
+unsigned long __read_mostly max_pdx;
+
+unsigned long __read_mostly pdx_group_valid[BITS_TO_LONGS(
+    (FRAMETABLE_NR + PDX_GROUP_COUNT - 1) / PDX_GROUP_COUNT)] = { [0] = 1 };
+
+bool __mfn_valid(unsigned long mfn)
+{
+    if ( unlikely(evaluate_nospec(mfn >= max_page)) )
+        return false;
+    return likely(!(mfn & pfn_hole_mask)) &&
+           likely(test_bit(pfn_to_pdx(mfn) / PDX_GROUP_COUNT,
+                           pdx_group_valid));
+}
+
+void set_pdx_range(unsigned long smfn, unsigned long emfn)
+{
+    unsigned long idx, eidx;
+
+    idx = pfn_to_pdx(smfn) / PDX_GROUP_COUNT;
+    eidx = (pfn_to_pdx(emfn - 1) + PDX_GROUP_COUNT) / PDX_GROUP_COUNT;
+
+    for ( ; idx < eidx; ++idx )
+        __set_bit(idx, pdx_group_valid);
+}
+
 /*
  * Diagram to make sense of the following variables. The masks and shifts
  * are done on mfn values in order to convert to/from pdx:
@@ -47,12 +76,6 @@
  * ones.
  */
 
-/**
- * Maximum (non-inclusive) usable pdx. Must be
- * modifiable after init due to memory hotplug
- */
-unsigned long __read_mostly max_pdx;
-
 /** Mask for the lower non-compressible bits of an mfn */
 unsigned long __ro_after_init pfn_pdx_bottom_mask = ~0UL;
 
@@ -75,18 +98,6 @@ unsigned long __ro_after_init pfn_hole_mask = 0;
 /** Number of bits of the "compressible" bit slice of an mfn */
 unsigned int __ro_after_init pfn_pdx_hole_shift = 0;
 
-unsigned long __read_mostly pdx_group_valid[BITS_TO_LONGS(
-    (FRAMETABLE_NR + PDX_GROUP_COUNT - 1) / PDX_GROUP_COUNT)] = { [0] = 1 };
-
-bool __mfn_valid(unsigned long mfn)
-{
-    if ( unlikely(evaluate_nospec(mfn >= max_page)) )
-        return false;
-    return likely(!(mfn & pfn_hole_mask)) &&
-           likely(test_bit(pfn_to_pdx(mfn) / PDX_GROUP_COUNT,
-                           pdx_group_valid));
-}
-
 /* Sets all bits from the most-significant 1-bit down to the LSB */
 static uint64_t fill_mask(uint64_t mask)
 {
@@ -124,17 +135,6 @@ uint64_t pdx_region_mask(uint64_t base, uint64_t len)
     return fill_mask(base ^ (base + len - 1));
 }
 
-void set_pdx_range(unsigned long smfn, unsigned long emfn)
-{
-    unsigned long idx, eidx;
-
-    idx = pfn_to_pdx(smfn) / PDX_GROUP_COUNT;
-    eidx = (pfn_to_pdx(emfn - 1) + PDX_GROUP_COUNT) / PDX_GROUP_COUNT;
-
-    for ( ; idx < eidx; ++idx )
-        __set_bit(idx, pdx_group_valid);
-}
-
 void __init pfn_pdx_hole_setup(unsigned long mask)
 {
     unsigned int i, j, bottom_shift = 0, hole_shift = 0;
diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
index 5674506f3a..b5367a33ca 100644
--- a/xen/include/xen/pdx.h
+++ b/xen/include/xen/pdx.h
@@ -70,15 +70,41 @@
 #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;
-extern unsigned long pfn_hole_mask;
-extern unsigned long pfn_top_mask, ma_top_mask;
 
 #define PDX_GROUP_COUNT ((1 << PDX_GROUP_SHIFT) / \
                          (sizeof(*frame_table) & -sizeof(*frame_table)))
 extern unsigned long pdx_group_valid[];
 
+/**
+ * Mark [smfn, emfn) as allocatable in the frame table
+ *
+ * @param smfn Start mfn
+ * @param emfn End mfn
+ */
+void set_pdx_range(unsigned long smfn, unsigned long emfn);
+
+/**
+ * Invoked to determine if an mfn has an associated valid frame table entry
+ *
+ * In order for it to be legal it must pass bounds, grouping and
+ * compression sanity checks.
+ *
+ * @param mfn To-be-checked mfn
+ * @return True iff all checks pass
+ */
+bool __mfn_valid(unsigned long mfn);
+
+#define page_to_pdx(pg)  ((pg) - frame_table)
+#define pdx_to_page(pdx) gcc11_wrap(frame_table + (pdx))
+
+#define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn))
+#define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx))
+
+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
  *
@@ -120,28 +146,6 @@ uint64_t pdx_region_mask(uint64_t base, uint64_t len);
  */
 uint64_t pdx_init_mask(uint64_t base_addr);
 
-/**
- * Mark [smfn, emfn) as accesible in the frame table
- *
- * @param smfn Start mfn
- * @param emfn End mfn
- */
-void set_pdx_range(unsigned long smfn, unsigned long emfn);
-
-#define page_to_pdx(pg)  ((pg) - frame_table)
-#define pdx_to_page(pdx) gcc11_wrap(frame_table + (pdx))
-
-/**
- * Invoked to determine if an mfn has an associated valid frame table entry
- *
- * In order for it to be legal it must pass bounds, grouping and
- * compression sanity checks.
- *
- * @param mfn To-be-checked mfn
- * @return True iff all checks pass
- */
-bool __mfn_valid(unsigned long mfn);
-
 /**
  * Map pfn to its corresponding pdx
  *
@@ -166,9 +170,6 @@ static inline unsigned long pdx_to_pfn(unsigned long pdx)
            ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
 }
 
-#define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn))
-#define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx))
-
 /**
  * Computes the offset into the direct map of an maddr
  *
-- 
2.34.1

[PATCH v3 4/4] pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

[Alejandro Vallejo]

Adds a new compile-time flag to allow disabling pdx compression and
compiles out compression-related code/data. It also shorts the pdx<->pfn
conversion macros and creates stubs for masking fucntions.

While at it, removes the old arch-defined CONFIG_HAS_PDX flag, as it was
not removable in practice.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v3:
  * Fixed a typo in Kconfig's help
  * Gated CONFIG_PDX_COMPRESSION under EXPERT=y and left default=ARM
  * Fixed commit message after renaming CONFIG_PDX_COMPRESSION (Julien)
  * Changed #else comment to the reason it executes (Julien)
  * Various blank-line related style changes (Jan)
---
 xen/arch/arm/Kconfig  |  1 -
 xen/arch/x86/Kconfig  |  1 -
 xen/arch/x86/domain.c | 19 +++++++++++++------
 xen/common/Kconfig    | 13 ++++++++++---
 xen/common/Makefile   |  2 +-
 xen/common/pdx.c      | 16 ++++++++++++----
 xen/include/xen/pdx.h | 38 +++++++++++++++++++++++++++++++++++---
 7 files changed, 71 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 57bd1d01d7..224db89c05 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/arch/x86/domain.c b/xen/arch/x86/domain.c
index fe86a7f853..f476df17e4 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -458,7 +458,7 @@ void domain_cpu_policy_changed(struct domain *d)
     }
 }
 
-#ifndef CONFIG_BIGMEM
+#if !defined(CONFIG_BIGMEM) && defined(CONFIG_PDX_COMPRESSION)
 /*
  * The hole may be at or above the 44-bit boundary, so we need to determine
  * the total bit count until reaching 32 significant (not squashed out) bits
@@ -485,13 +485,20 @@ static unsigned int __init noinline _domain_struct_bits(void)
 struct domain *alloc_domain_struct(void)
 {
     struct domain *d;
-#ifdef CONFIG_BIGMEM
-    const unsigned int bits = 0;
-#else
+
     /*
-     * We pack the PDX of the domain structure into a 32-bit field within
-     * the page_info structure. Hence the MEMF_bits() restriction.
+     * Without CONFIG_BIGMEM, we pack the PDX of the domain structure into
+     * a 32-bit field within the page_info structure. Hence the MEMF_bits()
+     * restriction. With PDX compression in place the number of bits must
+     * be calculated at runtime, but it's fixed otherwise.
+     *
+     * On systems with CONFIG_BIGMEM there's no packing, and so there's no
+     * such restriction.
      */
+#if defined(CONFIG_BIGMEM) || !defined(CONFIG_PDX_COMPRESSION)
+    const unsigned int bits = IS_ENABLED(CONFIG_BIGMEM) ? 0 :
+                                                          32 + PAGE_SHIFT;
+#else
     static unsigned int __read_mostly bits;
 
     if ( unlikely(!bits) )
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 0d248ab941..2c1d1fc3a2 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -23,6 +23,16 @@ config GRANT_TABLE
 
 	  If unsure, say Y.
 
+config PDX_COMPRESSION
+	bool "PDX (Page inDeX) compression support" if EXPERT
+	default ARM
+	help
+	  PDX compression is a technique that allows the hypervisor to
+	  represent physical addresses in a very space-efficient manner.
+	  This is very helpful reducing memory wastage in systems with
+	  memory banks with base addresses far from each other, but carries
+	  a performance cost.
+
 config ALTERNATIVE_CALL
 	bool
 
@@ -53,9 +63,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/common/pdx.c b/xen/common/pdx.c
index d3d38965bd..d3d63b0750 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -31,11 +31,16 @@ unsigned long __read_mostly pdx_group_valid[BITS_TO_LONGS(
 
 bool __mfn_valid(unsigned long mfn)
 {
-    if ( unlikely(evaluate_nospec(mfn >= max_page)) )
+    bool invalid = mfn >= max_page;
+
+#ifdef CONFIG_PDX_COMPRESSION
+    invalid |= mfn & pfn_hole_mask;
+#endif
+
+    if ( unlikely(evaluate_nospec(invalid)) )
         return false;
-    return likely(!(mfn & pfn_hole_mask)) &&
-           likely(test_bit(pfn_to_pdx(mfn) / PDX_GROUP_COUNT,
-                           pdx_group_valid));
+
+    return test_bit(pfn_to_pdx(mfn) / PDX_GROUP_COUNT, pdx_group_valid);
 }
 
 void set_pdx_range(unsigned long smfn, unsigned long emfn)
@@ -49,6 +54,8 @@ void set_pdx_range(unsigned long smfn, unsigned long emfn)
         __set_bit(idx, pdx_group_valid);
 }
 
+#ifdef CONFIG_PDX_COMPRESSION
+
 /*
  * Diagram to make sense of the following variables. The masks and shifts
  * are done on mfn values in order to convert to/from pdx:
@@ -176,6 +183,7 @@ void __init pfn_pdx_hole_setup(unsigned long mask)
     ma_top_mask         = pfn_top_mask << PAGE_SHIFT;
 }
 
+#endif /* CONFIG_PDX_COMPRESSION */
 
 /*
  * Local variables:
diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
index b5367a33ca..6511aba6c7 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;
 
 #define PDX_GROUP_COUNT ((1 << PDX_GROUP_SHIFT) / \
@@ -100,6 +98,8 @@ bool __mfn_valid(unsigned long mfn);
 #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn))
 #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx))
 
+#ifdef CONFIG_PDX_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;
@@ -206,7 +206,39 @@ static inline paddr_t directmapoff_to_maddr(unsigned long offset)
  */
 void pfn_pdx_hole_setup(unsigned long mask);
 
-#endif /* HAS_PDX */
+#else /* !CONFIG_PDX_COMPRESSION */
+
+/* Without PDX compression we can skip some computations */
+
+/* pdx<->pfn == identity */
+#define pdx_to_pfn(x) (x)
+#define pfn_to_pdx(x) (x)
+
+/* directmap is indexed by by maddr */
+#define maddr_to_directmapoff(x) (x)
+#define directmapoff_to_maddr(x) (x)
+
+static inline bool pdx_is_region_compressible(unsigned long smfn,
+                                              unsigned long emfn)
+{
+    return true;
+}
+
+static inline uint64_t pdx_init_mask(uint64_t base_addr)
+{
+    return 0;
+}
+
+static inline uint64_t pdx_region_mask(uint64_t base, uint64_t len)
+{
+    return 0;
+}
+
+static inline void pfn_pdx_hole_setup(unsigned long mask)
+{
+}
+
+#endif /* CONFIG_PDX_COMPRESSION */
 #endif /* __XEN_PDX_H__ */
 
 /*
-- 
2.34.1

Re: [PATCH v3 0/4] Make PDX compression optional

[Alejandro Vallejo]

On Tue, Aug 08, 2023 at 02:02:16PM +0100, Alejandro Vallejo wrote:
> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
> disable it because the whole codebase performs unconditional
> compression/decompression operations on addresses. This has the
> unfortunate side effect that systems without a need for compression still
> have to pay the performance impact of juggling bits on every pfn<->pdx
> conversion (this requires reading several global variables). This series
> attempts to:
> 
>   * Leave the state of pdx and pdx compression documented
>   * Factor out compression so it _can_ be removed through Kconfig
>   * Make it so compression is disabled on x86 and enabled on both Aarch32
>     and Aarch64 by default.
> 
> Series summary:
> 
> Patch 1 Moves hard-coded compression-related logic to helper functions
> Patch 2 Refactors all instances of regions being validated for pdx
>         compression conformance so it's done through a helper
> Patch 3 Non-functional reorder in order to simplify the patch 8 diff
> Patch 4 Adds new Kconfig option to compile out PDX compression and removes
>         the old CONFIG_HAS_PDX, as it was non removable
> 
> Already committed:
> 
> v1/patch 1 documents the current general understanding of the pdx concept and
>            pdx compression in particular
> v1/patch 3 Marks the pdx compression globals as ro_after_init
> v2/patch 1 Documents the differences between arm32 and arm64 directmaps
> 
> Alejandro Vallejo (4):
>   mm: Factor out the pdx compression logic in ma/va converters
>   mm/pdx: Standardize region validation wrt pdx compression
>   pdx: Reorder pdx.[ch]
>   pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

@Jan: Just making sure, are you generally ok with this series as-is?

Thanks,
Alejandro

Re: [PATCH v3 0/4] Make PDX compression optional

[Jan Beulich]

On 16.08.2023 11:36, Alejandro Vallejo wrote:
> On Tue, Aug 08, 2023 at 02:02:16PM +0100, Alejandro Vallejo wrote:
>> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
>> disable it because the whole codebase performs unconditional
>> compression/decompression operations on addresses. This has the
>> unfortunate side effect that systems without a need for compression still
>> have to pay the performance impact of juggling bits on every pfn<->pdx
>> conversion (this requires reading several global variables). This series
>> attempts to:
>>
>>   * Leave the state of pdx and pdx compression documented
>>   * Factor out compression so it _can_ be removed through Kconfig
>>   * Make it so compression is disabled on x86 and enabled on both Aarch32
>>     and Aarch64 by default.
>>
>> Series summary:
>>
>> Patch 1 Moves hard-coded compression-related logic to helper functions
>> Patch 2 Refactors all instances of regions being validated for pdx
>>         compression conformance so it's done through a helper
>> Patch 3 Non-functional reorder in order to simplify the patch 8 diff
>> Patch 4 Adds new Kconfig option to compile out PDX compression and removes
>>         the old CONFIG_HAS_PDX, as it was non removable
>>
>> Already committed:
>>
>> v1/patch 1 documents the current general understanding of the pdx concept and
>>            pdx compression in particular
>> v1/patch 3 Marks the pdx compression globals as ro_after_init
>> v2/patch 1 Documents the differences between arm32 and arm64 directmaps
>>
>> Alejandro Vallejo (4):
>>   mm: Factor out the pdx compression logic in ma/va converters
>>   mm/pdx: Standardize region validation wrt pdx compression
>>   pdx: Reorder pdx.[ch]
>>   pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option
> 
> @Jan: Just making sure, are you generally ok with this series as-is?

Well, okay would be too strong; I still don't see why my runtime patching
series isn't re-considered. But I don't mind it going in the way it is now.
I won't ack any part of it, though (in case that wasn't obvious), so it'll
be up to Andrew or Roger to supply the necessary x86 acks.

Jan

Re: [PATCH v3 0/4] Make PDX compression optional

[Julien Grall]

Hi Jan,

On 16/08/2023 10:43, Jan Beulich wrote:
> On 16.08.2023 11:36, Alejandro Vallejo wrote:
>> On Tue, Aug 08, 2023 at 02:02:16PM +0100, Alejandro Vallejo wrote:
>>> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
>>> disable it because the whole codebase performs unconditional
>>> compression/decompression operations on addresses. This has the
>>> unfortunate side effect that systems without a need for compression still
>>> have to pay the performance impact of juggling bits on every pfn<->pdx
>>> conversion (this requires reading several global variables). This series
>>> attempts to:
>>>
>>>    * Leave the state of pdx and pdx compression documented
>>>    * Factor out compression so it _can_ be removed through Kconfig
>>>    * Make it so compression is disabled on x86 and enabled on both Aarch32
>>>      and Aarch64 by default.
>>>
>>> Series summary:
>>>
>>> Patch 1 Moves hard-coded compression-related logic to helper functions
>>> Patch 2 Refactors all instances of regions being validated for pdx
>>>          compression conformance so it's done through a helper
>>> Patch 3 Non-functional reorder in order to simplify the patch 8 diff
>>> Patch 4 Adds new Kconfig option to compile out PDX compression and removes
>>>          the old CONFIG_HAS_PDX, as it was non removable
>>>
>>> Already committed:
>>>
>>> v1/patch 1 documents the current general understanding of the pdx concept and
>>>             pdx compression in particular
>>> v1/patch 3 Marks the pdx compression globals as ro_after_init
>>> v2/patch 1 Documents the differences between arm32 and arm64 directmaps
>>>
>>> Alejandro Vallejo (4):
>>>    mm: Factor out the pdx compression logic in ma/va converters
>>>    mm/pdx: Standardize region validation wrt pdx compression
>>>    pdx: Reorder pdx.[ch]
>>>    pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option
>>
>> @Jan: Just making sure, are you generally ok with this series as-is?
> 
> Well, okay would be too strong; I still don't see why my runtime patching
> series isn't re-considered.

Do you have a pointer to the series? I would be interested to have a look.

That said... the problem with alt-patching is this is architectural 
specific. Right now, this seems to be a bit unnecessary given that we 
believe that virtually no-one will have a platform (I know we talked 
about a potential one...) where PDX is compressing.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 0/4] Make PDX compression optional

[Jan Beulich]

On 16.08.2023 13:12, Julien Grall wrote:
> Hi Jan,
> 
> On 16/08/2023 10:43, Jan Beulich wrote:
>> On 16.08.2023 11:36, Alejandro Vallejo wrote:
>>> On Tue, Aug 08, 2023 at 02:02:16PM +0100, Alejandro Vallejo wrote:
>>>> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
>>>> disable it because the whole codebase performs unconditional
>>>> compression/decompression operations on addresses. This has the
>>>> unfortunate side effect that systems without a need for compression still
>>>> have to pay the performance impact of juggling bits on every pfn<->pdx
>>>> conversion (this requires reading several global variables). This series
>>>> attempts to:
>>>>
>>>>    * Leave the state of pdx and pdx compression documented
>>>>    * Factor out compression so it _can_ be removed through Kconfig
>>>>    * Make it so compression is disabled on x86 and enabled on both Aarch32
>>>>      and Aarch64 by default.
>>>>
>>>> Series summary:
>>>>
>>>> Patch 1 Moves hard-coded compression-related logic to helper functions
>>>> Patch 2 Refactors all instances of regions being validated for pdx
>>>>          compression conformance so it's done through a helper
>>>> Patch 3 Non-functional reorder in order to simplify the patch 8 diff
>>>> Patch 4 Adds new Kconfig option to compile out PDX compression and removes
>>>>          the old CONFIG_HAS_PDX, as it was non removable
>>>>
>>>> Already committed:
>>>>
>>>> v1/patch 1 documents the current general understanding of the pdx concept and
>>>>             pdx compression in particular
>>>> v1/patch 3 Marks the pdx compression globals as ro_after_init
>>>> v2/patch 1 Documents the differences between arm32 and arm64 directmaps
>>>>
>>>> Alejandro Vallejo (4):
>>>>    mm: Factor out the pdx compression logic in ma/va converters
>>>>    mm/pdx: Standardize region validation wrt pdx compression
>>>>    pdx: Reorder pdx.[ch]
>>>>    pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option
>>>
>>> @Jan: Just making sure, are you generally ok with this series as-is?
>>
>> Well, okay would be too strong; I still don't see why my runtime patching
>> series isn't re-considered.
> 
> Do you have a pointer to the series? I would be interested to have a look.

Sure, I can dig it out a 2nd time:
https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01616.html

> That said... the problem with alt-patching is this is architectural 
> specific. Right now, this seems to be a bit unnecessary given that we 
> believe that virtually no-one will have a platform (I know we talked 
> about a potential one...) where PDX is compressing.

But it defaults to enabled on other than x86 anyway. So it seems like
it's generally wanted everywhere except on x86, and on x86 it can
(could) be patched out.

Jan

Re: [PATCH v3 0/4] Make PDX compression optional

[Julien Grall]

Hi,

On 16/08/2023 12:27, Jan Beulich wrote:
> On 16.08.2023 13:12, Julien Grall wrote:
>> Hi Jan,
>>
>> On 16/08/2023 10:43, Jan Beulich wrote:
>>> On 16.08.2023 11:36, Alejandro Vallejo wrote:
>>>> On Tue, Aug 08, 2023 at 02:02:16PM +0100, Alejandro Vallejo wrote:
>>>>> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
>>>>> disable it because the whole codebase performs unconditional
>>>>> compression/decompression operations on addresses. This has the
>>>>> unfortunate side effect that systems without a need for compression still
>>>>> have to pay the performance impact of juggling bits on every pfn<->pdx
>>>>> conversion (this requires reading several global variables). This series
>>>>> attempts to:
>>>>>
>>>>>     * Leave the state of pdx and pdx compression documented
>>>>>     * Factor out compression so it _can_ be removed through Kconfig
>>>>>     * Make it so compression is disabled on x86 and enabled on both Aarch32
>>>>>       and Aarch64 by default.
>>>>>
>>>>> Series summary:
>>>>>
>>>>> Patch 1 Moves hard-coded compression-related logic to helper functions
>>>>> Patch 2 Refactors all instances of regions being validated for pdx
>>>>>           compression conformance so it's done through a helper
>>>>> Patch 3 Non-functional reorder in order to simplify the patch 8 diff
>>>>> Patch 4 Adds new Kconfig option to compile out PDX compression and removes
>>>>>           the old CONFIG_HAS_PDX, as it was non removable
>>>>>
>>>>> Already committed:
>>>>>
>>>>> v1/patch 1 documents the current general understanding of the pdx concept and
>>>>>              pdx compression in particular
>>>>> v1/patch 3 Marks the pdx compression globals as ro_after_init
>>>>> v2/patch 1 Documents the differences between arm32 and arm64 directmaps
>>>>>
>>>>> Alejandro Vallejo (4):
>>>>>     mm: Factor out the pdx compression logic in ma/va converters
>>>>>     mm/pdx: Standardize region validation wrt pdx compression
>>>>>     pdx: Reorder pdx.[ch]
>>>>>     pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option
>>>>
>>>> @Jan: Just making sure, are you generally ok with this series as-is?
>>>
>>> Well, okay would be too strong; I still don't see why my runtime patching
>>> series isn't re-considered.
>>
>> Do you have a pointer to the series? I would be interested to have a look.
> 
> Sure, I can dig it out a 2nd time:
> https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01616.html

Thanks! AFAIU, the optimization would only happen after the alternative 
has been patched. This is happening after initializing the heap. With 
Alejandro series, you will have some performance gain for the boot which 
I care about.

> 
>> That said... the problem with alt-patching is this is architectural
>> specific. Right now, this seems to be a bit unnecessary given that we
>> believe that virtually no-one will have a platform (I know we talked
>> about a potential one...) where PDX is compressing.
> 
> But it defaults to enabled on other than x86 anyway. So it seems like
> it's generally wanted everywhere except on x86, and on x86 it can
> (could) be patched out.

IIUC, you are saying that we would want both Alejandro series (to allow 
an admin to disable PDX at boot) and your series (to fully optimize the 
PDX at runtime). Is that correct?

If so, it is unclear to me why you want your series to be re-considered 
now. Can't this be done as a follow-up if there is a desire to further 
optimize?

Cheers,

-- 
Julien Grall

Re: [PATCH v3 0/4] Make PDX compression optional

[Jan Beulich]

On 16.08.2023 15:06, Julien Grall wrote:
> Hi,
> 
> On 16/08/2023 12:27, Jan Beulich wrote:
>> On 16.08.2023 13:12, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 16/08/2023 10:43, Jan Beulich wrote:
>>>> On 16.08.2023 11:36, Alejandro Vallejo wrote:
>>>>> On Tue, Aug 08, 2023 at 02:02:16PM +0100, Alejandro Vallejo wrote:
>>>>>> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
>>>>>> disable it because the whole codebase performs unconditional
>>>>>> compression/decompression operations on addresses. This has the
>>>>>> unfortunate side effect that systems without a need for compression still
>>>>>> have to pay the performance impact of juggling bits on every pfn<->pdx
>>>>>> conversion (this requires reading several global variables). This series
>>>>>> attempts to:
>>>>>>
>>>>>>     * Leave the state of pdx and pdx compression documented
>>>>>>     * Factor out compression so it _can_ be removed through Kconfig
>>>>>>     * Make it so compression is disabled on x86 and enabled on both Aarch32
>>>>>>       and Aarch64 by default.
>>>>>>
>>>>>> Series summary:
>>>>>>
>>>>>> Patch 1 Moves hard-coded compression-related logic to helper functions
>>>>>> Patch 2 Refactors all instances of regions being validated for pdx
>>>>>>           compression conformance so it's done through a helper
>>>>>> Patch 3 Non-functional reorder in order to simplify the patch 8 diff
>>>>>> Patch 4 Adds new Kconfig option to compile out PDX compression and removes
>>>>>>           the old CONFIG_HAS_PDX, as it was non removable
>>>>>>
>>>>>> Already committed:
>>>>>>
>>>>>> v1/patch 1 documents the current general understanding of the pdx concept and
>>>>>>              pdx compression in particular
>>>>>> v1/patch 3 Marks the pdx compression globals as ro_after_init
>>>>>> v2/patch 1 Documents the differences between arm32 and arm64 directmaps
>>>>>>
>>>>>> Alejandro Vallejo (4):
>>>>>>     mm: Factor out the pdx compression logic in ma/va converters
>>>>>>     mm/pdx: Standardize region validation wrt pdx compression
>>>>>>     pdx: Reorder pdx.[ch]
>>>>>>     pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option
>>>>>
>>>>> @Jan: Just making sure, are you generally ok with this series as-is?
>>>>
>>>> Well, okay would be too strong; I still don't see why my runtime patching
>>>> series isn't re-considered.
>>>
>>> Do you have a pointer to the series? I would be interested to have a look.
>>
>> Sure, I can dig it out a 2nd time:
>> https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01616.html
> 
> Thanks! AFAIU, the optimization would only happen after the alternative 
> has been patched. This is happening after initializing the heap. With 
> Alejandro series, you will have some performance gain for the boot which 
> I care about.

Fair aspect.

>>> That said... the problem with alt-patching is this is architectural
>>> specific. Right now, this seems to be a bit unnecessary given that we
>>> believe that virtually no-one will have a platform (I know we talked
>>> about a potential one...) where PDX is compressing.
>>
>> But it defaults to enabled on other than x86 anyway. So it seems like
>> it's generally wanted everywhere except on x86, and on x86 it can
>> (could) be patched out.
> 
> IIUC, you are saying that we would want both Alejandro series (to allow 
> an admin to disable PDX at boot) and your series (to fully optimize the 
> PDX at runtime). Is that correct?

Not really, no. I don't view a build-time decision as necessary; I favor
runtime decisions whenever possible. But as said I also don't mind this
series going in.

> If so, it is unclear to me why you want your series to be re-considered 
> now. Can't this be done as a follow-up if there is a desire to further 
> optimize?

In principle it could be, yes, but I'm afraid I know that no follow-up
is going to happen (and me trying to revive the earlier work would be
yet another case of pointlessly invested time).

Jan

Re: [PATCH v3 0/4] Make PDX compression optional

[Julien Grall]

Hi,

On 16/08/2023 14:14, Jan Beulich wrote:
> On 16.08.2023 15:06, Julien Grall wrote:
>> Hi,
>>
>> On 16/08/2023 12:27, Jan Beulich wrote:
>>> On 16.08.2023 13:12, Julien Grall wrote:
>>>> Hi Jan,
>>>>
>>>> On 16/08/2023 10:43, Jan Beulich wrote:
>>>>> On 16.08.2023 11:36, Alejandro Vallejo wrote:
>>>>>> On Tue, Aug 08, 2023 at 02:02:16PM +0100, Alejandro Vallejo wrote:
>>>>>>> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
>>>>>>> disable it because the whole codebase performs unconditional
>>>>>>> compression/decompression operations on addresses. This has the
>>>>>>> unfortunate side effect that systems without a need for compression still
>>>>>>> have to pay the performance impact of juggling bits on every pfn<->pdx
>>>>>>> conversion (this requires reading several global variables). This series
>>>>>>> attempts to:
>>>>>>>
>>>>>>>      * Leave the state of pdx and pdx compression documented
>>>>>>>      * Factor out compression so it _can_ be removed through Kconfig
>>>>>>>      * Make it so compression is disabled on x86 and enabled on both Aarch32
>>>>>>>        and Aarch64 by default.
>>>>>>>
>>>>>>> Series summary:
>>>>>>>
>>>>>>> Patch 1 Moves hard-coded compression-related logic to helper functions
>>>>>>> Patch 2 Refactors all instances of regions being validated for pdx
>>>>>>>            compression conformance so it's done through a helper
>>>>>>> Patch 3 Non-functional reorder in order to simplify the patch 8 diff
>>>>>>> Patch 4 Adds new Kconfig option to compile out PDX compression and removes
>>>>>>>            the old CONFIG_HAS_PDX, as it was non removable
>>>>>>>
>>>>>>> Already committed:
>>>>>>>
>>>>>>> v1/patch 1 documents the current general understanding of the pdx concept and
>>>>>>>               pdx compression in particular
>>>>>>> v1/patch 3 Marks the pdx compression globals as ro_after_init
>>>>>>> v2/patch 1 Documents the differences between arm32 and arm64 directmaps
>>>>>>>
>>>>>>> Alejandro Vallejo (4):
>>>>>>>      mm: Factor out the pdx compression logic in ma/va converters
>>>>>>>      mm/pdx: Standardize region validation wrt pdx compression
>>>>>>>      pdx: Reorder pdx.[ch]
>>>>>>>      pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option
>>>>>>
>>>>>> @Jan: Just making sure, are you generally ok with this series as-is?
>>>>>
>>>>> Well, okay would be too strong; I still don't see why my runtime patching
>>>>> series isn't re-considered.
>>>>
>>>> Do you have a pointer to the series? I would be interested to have a look.
>>>
>>> Sure, I can dig it out a 2nd time:
>>> https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01616.html
>>
>> Thanks! AFAIU, the optimization would only happen after the alternative
>> has been patched. This is happening after initializing the heap. With
>> Alejandro series, you will have some performance gain for the boot which
>> I care about.
> 
> Fair aspect.
> 
>>>> That said... the problem with alt-patching is this is architectural
>>>> specific. Right now, this seems to be a bit unnecessary given that we
>>>> believe that virtually no-one will have a platform (I know we talked
>>>> about a potential one...) where PDX is compressing.
>>>
>>> But it defaults to enabled on other than x86 anyway. So it seems like
>>> it's generally wanted everywhere except on x86, and on x86 it can
>>> (could) be patched out.
>>
>> IIUC, you are saying that we would want both Alejandro series (to allow
>> an admin to disable PDX at boot) and your series (to fully optimize the
>> PDX at runtime). Is that correct?
> 
> Not really, no. I don't view a build-time decision as necessary; I favor
> runtime decisions whenever possible.

At least on Arm, we want to cater two different set of users. One set 
will want to tailor Xen to there platform and runtime decision may not 
be desirable. The other set (e.g. distros) will want to run Xen 
everywhere so runtime is preferable.

So I think we should be able to offer both build and runtime option when 
it makes sense. The PDX is one example where both could be interesting, 
yet at the moment there seem to be an appetite for build time only 
configuration.

> 
>> If so, it is unclear to me why you want your series to be re-considered
>> now. Can't this be done as a follow-up if there is a desire to further
>> optimize?
> 
> In principle it could be, yes, but I'm afraid I know that no follow-up
> is going to happen (and me trying to revive the earlier work would be
> yet another case of pointlessly invested time).

Right which is why I wrote "desire". But my point was that Alejandro 
work is not a one-way door. It would still possible to have runtime 
patching on top of it.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 4/4] pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

[Andrew Cooper]

Several things.

First, Shawn: PPC has gained a HAS_PDX, the deletion of which needs
merging into this patch.

It was added as part of 4a2f68f909304 which was "minimal to build". 
This series address the issue you presumably encountered where pdx.c
appears to be optional but wasn't.


Do PPC platforms have (or potentially have) sparse RAM banks?

If like x86 the answer is definitely no, then you want to have
PDX_COMPRESSION=n

If the answer is definitely yes always, then you want PDX_COMPRESSION=y

If the answer is system specific, then you want to offer users a choice.


On 08/08/2023 2:02 pm, Alejandro Vallejo wrote:
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 0d248ab941..2c1d1fc3a2 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -23,6 +23,16 @@ config GRANT_TABLE
>  
>  	  If unsure, say Y.
>  
> +config PDX_COMPRESSION
> +	bool "PDX (Page inDeX) compression support" if EXPERT

This still needs hiding on x86.  The minimal hack for 4.18 is "if EXPERT
&& !X86".

Other adjustments needed depending on the PPC answer above.

> +	default ARM
> +	help
> +	  PDX compression is a technique that allows the hypervisor to
> +	  represent physical addresses in a very space-efficient manner.
> +	  This is very helpful reducing memory wastage in systems with
> +	  memory banks with base addresses far from each other, but carries
> +	  a performance cost.

This is still intractable for a non-Xen-maintainer, and inaccurate. 
Whether you get any benefit at all depends on how the sparseness happens
to line up.

---
PDX compression is a technique designed to reduce the memory overhead of
physical memory management on platforms with sparse RAM banks.

If your platform does have sparse RAM banks, enabling PDX compression
may reduce the memory overhead of Xen, but does carry a runtime
performance cost.

If your platform does not have sparse RAM banks, do not enable PDX
compression.
---

~Andrew

Re: [PATCH v3 4/4] pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

[Jan Beulich]

On 22.09.2023 22:03, Andrew Cooper wrote:
> On 08/08/2023 2:02 pm, Alejandro Vallejo wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -23,6 +23,16 @@ config GRANT_TABLE
>>  
>>  	  If unsure, say Y.
>>  
>> +config PDX_COMPRESSION
>> +	bool "PDX (Page inDeX) compression support" if EXPERT
> 
> This still needs hiding on x86.  The minimal hack for 4.18 is "if EXPERT
> && !X86".

If you insist on complete hiding and I insist on allowing it to be used by
people who want to play with exotic hardware, then this won't go anywhere.
I think I've come far enough with accepting a compromise, and so I think
it's your turn now to also take a step from your original position.

Jan

Re: [PATCH v3 4/4] pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

[Roger Pau Monné]

On Mon, Sep 25, 2023 at 08:36:03AM +0200, Jan Beulich wrote:
> On 22.09.2023 22:03, Andrew Cooper wrote:
> > On 08/08/2023 2:02 pm, Alejandro Vallejo wrote:
> >> --- a/xen/common/Kconfig
> >> +++ b/xen/common/Kconfig
> >> @@ -23,6 +23,16 @@ config GRANT_TABLE
> >>  
> >>  	  If unsure, say Y.
> >>  
> >> +config PDX_COMPRESSION
> >> +	bool "PDX (Page inDeX) compression support" if EXPERT
> > 
> > This still needs hiding on x86.  The minimal hack for 4.18 is "if EXPERT
> > && !X86".
> 
> If you insist on complete hiding and I insist on allowing it to be used by
> people who want to play with exotic hardware, then this won't go anywhere.
> I think I've come far enough with accepting a compromise, and so I think
> it's your turn now to also take a step from your original position.

Just because I'm not familiar with this, is there any x86 hardware
that has such sparse memory map that would benefit from PDX?

Wouldn't anyone doing bring up on such exotic hardware also likely need to
perform other adjustments to Xen, and hence commenting out the !X86 in
Kconfig be acceptable? (we would likely make it selectable at that
point if such platforms exist).

Thanks, Roger.

Re: [PATCH v3 4/4] pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

[Jan Beulich]

On 25.09.2023 11:46, Roger Pau Monné wrote:
> On Mon, Sep 25, 2023 at 08:36:03AM +0200, Jan Beulich wrote:
>> On 22.09.2023 22:03, Andrew Cooper wrote:
>>> On 08/08/2023 2:02 pm, Alejandro Vallejo wrote:
>>>> --- a/xen/common/Kconfig
>>>> +++ b/xen/common/Kconfig
>>>> @@ -23,6 +23,16 @@ config GRANT_TABLE
>>>>  
>>>>  	  If unsure, say Y.
>>>>  
>>>> +config PDX_COMPRESSION
>>>> +	bool "PDX (Page inDeX) compression support" if EXPERT
>>>
>>> This still needs hiding on x86.  The minimal hack for 4.18 is "if EXPERT
>>> && !X86".
>>
>> If you insist on complete hiding and I insist on allowing it to be used by
>> people who want to play with exotic hardware, then this won't go anywhere.
>> I think I've come far enough with accepting a compromise, and so I think
>> it's your turn now to also take a step from your original position.
> 
> Just because I'm not familiar with this, is there any x86 hardware
> that has such sparse memory map that would benefit from PDX?
> 
> Wouldn't anyone doing bring up on such exotic hardware also likely need to
> perform other adjustments to Xen, and hence commenting out the !X86 in
> Kconfig be acceptable? (we would likely make it selectable at that
> point if such platforms exist).

As mentioned before, the reason PDX was introduced was to actually make Xen
work on such exotic x86 hardware. While I can't tell for sure, that hardware
probably has never made it into production. Yet still things were known to
work there after the original adjustments, so no, I would not expect other
adjustments to be necessary (provided there was no bitrot).

Jan

Re: [PATCH v3 4/4] pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

[Andrew Cooper]

On 25/09/2023 10:46 am, Roger Pau Monné wrote:
> On Mon, Sep 25, 2023 at 08:36:03AM +0200, Jan Beulich wrote:
>> On 22.09.2023 22:03, Andrew Cooper wrote:
>>> On 08/08/2023 2:02 pm, Alejandro Vallejo wrote:
>>>> --- a/xen/common/Kconfig
>>>> +++ b/xen/common/Kconfig
>>>> @@ -23,6 +23,16 @@ config GRANT_TABLE
>>>>  
>>>>  	  If unsure, say Y.
>>>>  
>>>> +config PDX_COMPRESSION
>>>> +	bool "PDX (Page inDeX) compression support" if EXPERT
>>> This still needs hiding on x86.  The minimal hack for 4.18 is "if EXPERT
>>> && !X86".
>> If you insist on complete hiding and I insist on allowing it to be used by
>> people who want to play with exotic hardware, then this won't go anywhere.
>> I think I've come far enough with accepting a compromise, and so I think
>> it's your turn now to also take a step from your original position.
> Just because I'm not familiar with this, is there any x86 hardware
> that has such sparse memory map that would benefit from PDX?

There is one known system which never shipped.  Xen's implementation was
from the anticipation of that system shipping.  Nothing else known.

None of the other major kernels have facilities such as this, which is
very likely a contributory factor to the system not shipping.

> Wouldn't anyone doing bring up on such exotic hardware also likely need to
> perform other adjustments to Xen, and hence commenting out the !X86 in
> Kconfig be acceptable? (we would likely make it selectable at that
> point if such platforms exist).

People with bizarre hardware can cover the cost of bringing it up.  And
that includes tweaking Kconfig so they can select this option.

~Andrew

Re: [PATCH v3 4/4] pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

[Jan Beulich]

On 25.09.2023 12:01, Andrew Cooper wrote:
> On 25/09/2023 10:46 am, Roger Pau Monné wrote:
>> On Mon, Sep 25, 2023 at 08:36:03AM +0200, Jan Beulich wrote:
>>> On 22.09.2023 22:03, Andrew Cooper wrote:
>>>> On 08/08/2023 2:02 pm, Alejandro Vallejo wrote:
>>>>> --- a/xen/common/Kconfig
>>>>> +++ b/xen/common/Kconfig
>>>>> @@ -23,6 +23,16 @@ config GRANT_TABLE
>>>>>  
>>>>>  	  If unsure, say Y.
>>>>>  
>>>>> +config PDX_COMPRESSION
>>>>> +	bool "PDX (Page inDeX) compression support" if EXPERT
>>>> This still needs hiding on x86.  The minimal hack for 4.18 is "if EXPERT
>>>> && !X86".
>>> If you insist on complete hiding and I insist on allowing it to be used by
>>> people who want to play with exotic hardware, then this won't go anywhere.
>>> I think I've come far enough with accepting a compromise, and so I think
>>> it's your turn now to also take a step from your original position.
>> Just because I'm not familiar with this, is there any x86 hardware
>> that has such sparse memory map that would benefit from PDX?
> 
> There is one known system which never shipped.  Xen's implementation was
> from the anticipation of that system shipping.  Nothing else known.
> 
> None of the other major kernels have facilities such as this, which is
> very likely a contributory factor to the system not shipping.

Possibly, yet there's one important difference (mentioned before): VA space
that Xen can use (for directmap and frame_table[]) is far more constrained
than for baremetal systems (Linux, Windows). Hence the guys who got in touch
back at the time were not in need of Linux side changes at all (as far as I
was told back then).

Jan

Re: [PATCH v3 4/4] pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

[Shawn Anastasio]

On 9/22/23 3:03 PM, Andrew Cooper wrote:
> Several things.
> 
> First, Shawn: PPC has gained a HAS_PDX, the deletion of which needs
> merging into this patch.
> 
> It was added as part of 4a2f68f909304 which was "minimal to build". 
> This series address the issue you presumably encountered where pdx.c
> appears to be optional but wasn't.
>

That's correct, build was broken without HAS_PDX.

> 
> Do PPC platforms have (or potentially have) sparse RAM banks?
>

Yes, they do. Especially on POWER9, it is very common for there to be
large gaps in the physical address space between RAM banks.

> If like x86 the answer is definitely no, then you want to have
> PDX_COMPRESSION=n
> 
> If the answer is definitely yes always, then you want PDX_COMPRESSION=y
>
> If the answer is system specific, then you want to offer users a choice.

It looks like PDX_COMPRESSION=y would probably make the most sense for
ppc, but I'm not against making it a choice.

Thanks,
Shawn

Re: [PATCH v3 4/4] pdx: Add CONFIG_PDX_COMPRESSION as a common Kconfig option

[Andrew Cooper]

On 25/09/2023 6:37 pm, Shawn Anastasio wrote:
> On 9/22/23 3:03 PM, Andrew Cooper wrote:
>> Several things.
>>
>> First, Shawn: PPC has gained a HAS_PDX, the deletion of which needs
>> merging into this patch.
>>
>> It was added as part of 4a2f68f909304 which was "minimal to build". 
>> This series address the issue you presumably encountered where pdx.c
>> appears to be optional but wasn't.
>>
> That's correct, build was broken without HAS_PDX.
>
>> Do PPC platforms have (or potentially have) sparse RAM banks?
>>
> Yes, they do. Especially on POWER9, it is very common for there to be
> large gaps in the physical address space between RAM banks.
>
>> If like x86 the answer is definitely no, then you want to have
>> PDX_COMPRESSION=n
>>
>> If the answer is definitely yes always, then you want PDX_COMPRESSION=y
>>
>> If the answer is system specific, then you want to offer users a choice.
> It looks like PDX_COMPRESSION=y would probably make the most sense for
> ppc, but I'm not against making it a choice.

Thanks.  I'll make suitable adjustments.

~Andrew