[PATCH v2 0/4] Prerequisite patches for R82 upstreaming

[PATCH v2 0/4] Prerequisite patches for R82 upstreaming

[Luca Fancellu]

In this serie I've taken out patches from the R82 branch already in the ML[1]
and some new patches I've done based on the current status of staging that will
not impact the current Armv8-R earlyboot work.

[1] https://patchwork.kernel.org/project/xen-devel/cover/20230626033443.2943270-1-Penny.Zheng@arm.com/

Luca Fancellu (3):
  common/vmap: Fall back to simple allocator when !HAS_VMAP
  arm/setup: Move MMU specific extern declarations to mmu/setup.h
  xen/arm: Use vmap_contig instead of __vmap where it's possible

Penny Zheng (1):
  xen/arm: do not give memory back to static heap

 xen/arch/arm/alternative.c           |  3 +-
 xen/arch/arm/arm32/mmu/mm.c          |  4 +-
 xen/arch/arm/cpuerrata.c             |  5 +--
 xen/arch/arm/include/asm/mmu/setup.h | 31 ++++++++++++++
 xen/arch/arm/include/asm/setup.h     | 20 +++------
 xen/arch/arm/kernel.c                |  9 ++--
 xen/arch/arm/livepatch.c             |  3 +-
 xen/arch/arm/mmu/setup.c             |  8 +++-
 xen/arch/arm/setup.c                 | 27 ++++++------
 xen/common/device-tree/bootfdt.c     |  4 +-
 xen/common/device-tree/bootinfo.c    |  2 +-
 xen/common/page_alloc.c              |  5 +++
 xen/include/xen/bootfdt.h            | 14 ++++++-
 xen/include/xen/vmap.h               | 61 ++++++++++++++++------------
 xen/include/xen/xvmalloc.h           | 36 +++++++++++++---
 15 files changed, 156 insertions(+), 76 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/mmu/setup.h

-- 
2.34.1

[PATCH v2 1/4] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

When HAS_VMAP is disabled, the xv{malloc,zalloc,...} functions
should fall back to the simple x{malloc,zalloc,...} variant,
implement that because MPU systems won't have virtual memory.

Additionally remove VMAP_VIRT_START from vmap.h guards since
MPU systems won't have it defined and protect with #ifdef
CONFIG_HAS_VMAP all the declaration that won't be used in a
MPU system built without HAS_VMAP.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v1:
 - put back static inline iounmap
 - changed commit message
 - hide not used declaration for system with !HAS_VMAP
 - correct function declared in xvmalloc.h to be static inline
 - prefer '#ifdef' instead of '#if defined' where possible
---
---
 xen/include/xen/vmap.h     | 61 ++++++++++++++++++++++----------------
 xen/include/xen/xvmalloc.h | 36 ++++++++++++++++++----
 2 files changed, 66 insertions(+), 31 deletions(-)

diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index c1dd7ac22f30..a9f4a07bbb65 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -5,12 +5,19 @@
  * purpose area (VMAP_DEFAULT) and a livepatch-specific area (VMAP_XEN). The
  * latter is used when loading livepatches and the former for everything else.
  */
-#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
+#ifndef __XEN_VMAP_H__
 #define __XEN_VMAP_H__
 
 #include <xen/mm-frame.h>
 #include <xen/page-size.h>
 
+/*
+ * MPU systems won't have HAS_VMAP enabled, but will provide implementation
+ * only for some of the functions of this module. So hide the definition for
+ * some of these function to systems where !HAS_VMAP
+ */
+#ifdef CONFIG_HAS_VMAP
+
 /* Identifiers for the linear ranges tracked by vmap */
 enum vmap_region {
     /*
@@ -68,25 +75,6 @@ void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
  */
 void *vmap(const mfn_t *mfn, unsigned int nr);
 
-/*
- * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
- *
- * @param mfn Base mfn of the physical region
- * @param nr  Number of mfns in the physical region
- * @return Pointer to the mapped area on success; NULL otherwise.
- */
-void *vmap_contig(mfn_t mfn, unsigned int nr);
-
-/*
- * Unmaps a range of virtually contiguous memory from one of the vmap regions
- *
- * The system remembers internally how wide the mapping is and unmaps it all.
- * It also can determine the vmap region type from the `va`.
- *
- * @param va Virtual base address of the range to unmap
- */
-void vunmap(const void *va);
-
 /*
  * Allocate `size` octets of possibly non-contiguous physical memory and map
  * them contiguously in the VMAP_DEFAULT vmap region
@@ -112,6 +100,33 @@ void *vzalloc(size_t size);
  */
 void vfree(void *va);
 
+/* Return the number of pages in the mapping starting at address 'va' */
+unsigned int vmap_size(const void *va);
+
+/* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
+void *arch_vmap_virt_end(void);
+
+#endif /* CONFIG_HAS_VMAP */
+
+/*
+ * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
+ *
+ * @param mfn Base mfn of the physical region
+ * @param nr  Number of mfns in the physical region
+ * @return Pointer to the mapped area on success; NULL otherwise.
+ */
+void *vmap_contig(mfn_t mfn, unsigned int nr);
+
+/*
+ * Unmaps a range of virtually contiguous memory from one of the vmap regions
+ *
+ * The system remembers internally how wide the mapping is and unmaps it all.
+ * It also can determine the vmap region type from the `va`.
+ *
+ * @param va Virtual base address of the range to unmap
+ */
+void vunmap(const void *va);
+
 /*
  * Analogous to vmap_contig(), but for IO memory
  *
@@ -124,9 +139,6 @@ void vfree(void *va);
  */
 void __iomem *ioremap(paddr_t pa, size_t len);
 
-/* Return the number of pages in the mapping starting at address 'va' */
-unsigned int vmap_size(const void *va);
-
 /* Analogous to vunmap(), but for IO memory mapped via ioremap() */
 static inline void iounmap(void __iomem *va)
 {
@@ -135,9 +147,6 @@ static inline void iounmap(void __iomem *va)
     vunmap((void *)(addr & PAGE_MASK));
 }
 
-/* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
-void *arch_vmap_virt_end(void);
-
 /* Initialises the VMAP_DEFAULT virtual range */
 static inline void vm_init(void)
 {
diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
index 440d85a284bb..e97a30f61e96 100644
--- a/xen/include/xen/xvmalloc.h
+++ b/xen/include/xen/xvmalloc.h
@@ -40,20 +40,46 @@
     ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \
                              __alignof__(typeof(*(ptr)))))
 
+#ifdef CONFIG_HAS_VMAP
+
 /* Free any of the above. */
 void xvfree(void *va);
 
+/* Underlying functions */
+void *_xvmalloc(size_t size, unsigned int align);
+void *_xvzalloc(size_t size, unsigned int align);
+void *_xvrealloc(void *va, size_t size, unsigned int align);
+
+#else /* !CONFIG_HAS_VMAP */
+
+static inline void xvfree(void *va)
+{
+    xfree(va);
+}
+
+static inline void *_xvmalloc(size_t size, unsigned int align)
+{
+    return _xmalloc(size, align);
+}
+
+static inline void *_xvzalloc(size_t size, unsigned int align)
+{
+    return _xzalloc(size, align);
+}
+
+static inline void *_xvrealloc(void *va, size_t size, unsigned int align)
+{
+    return _xrealloc(va, size, align);
+}
+
+#endif /* CONFIG_HAS_VMAP */
+
 /* Free an allocation, and zero the pointer to it. */
 #define XVFREE(p) do { \
     xvfree(p);         \
     (p) = NULL;        \
 } while ( false )
 
-/* Underlying functions */
-void *_xvmalloc(size_t size, unsigned int align);
-void *_xvzalloc(size_t size, unsigned int align);
-void *_xvrealloc(void *va, size_t size, unsigned int align);
-
 static inline void *_xvmalloc_array(
     size_t size, unsigned int align, unsigned long num)
 {
-- 
2.34.1

Re: [PATCH v2 1/4] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Jan Beulich]

On 19.11.2024 09:58, Luca Fancellu wrote:
> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -5,12 +5,19 @@
>   * purpose area (VMAP_DEFAULT) and a livepatch-specific area (VMAP_XEN). The
>   * latter is used when loading livepatches and the former for everything else.
>   */
> -#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
> +#ifndef __XEN_VMAP_H__
>  #define __XEN_VMAP_H__
>  
>  #include <xen/mm-frame.h>
>  #include <xen/page-size.h>
>  
> +/*
> + * MPU systems won't have HAS_VMAP enabled, but will provide implementation
> + * only for some of the functions of this module. So hide the definition for
> + * some of these function to systems where !HAS_VMAP
> + */
> +#ifdef CONFIG_HAS_VMAP

What you're hiding are declarations, not definitions. While this may feel like
splitting hair, the question really is: Do the declarations actually need
hiding? IOW won't it suffice to have the definitions unavailable? While this
would mean that wrong uses are flagged only when linking, we do such all the
time when we expect e.g. DCE to remove actual uses of respective identifiers.

Jan

Re: [PATCH v2 1/4] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

Hi Jan,

> On 25 Nov 2024, at 16:21, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.11.2024 09:58, Luca Fancellu wrote:
>> --- a/xen/include/xen/vmap.h
>> +++ b/xen/include/xen/vmap.h
>> @@ -5,12 +5,19 @@
>>  * purpose area (VMAP_DEFAULT) and a livepatch-specific area (VMAP_XEN). The
>>  * latter is used when loading livepatches and the former for everything else.
>>  */
>> -#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
>> +#ifndef __XEN_VMAP_H__
>> #define __XEN_VMAP_H__
>> 
>> #include <xen/mm-frame.h>
>> #include <xen/page-size.h>
>> 
>> +/*
>> + * MPU systems won't have HAS_VMAP enabled, but will provide implementation
>> + * only for some of the functions of this module. So hide the definition for
>> + * some of these function to systems where !HAS_VMAP
>> + */
>> +#ifdef CONFIG_HAS_VMAP
> 
> What you're hiding are declarations, not definitions.

yes, I realised the mistake after sending

> While this may feel like
> splitting hair, the question really is: Do the declarations actually need
> hiding? IOW won't it suffice to have the definitions unavailable? While this
> would mean that wrong uses are flagged only when linking, we do such all the
> time when we expect e.g. DCE to remove actual uses of respective identifiers.

I misunderstood your comment on the previous version and I thought your preference
was to hide the declarations. I’ll try without hiding them and I’ll send the change soon.

Cheers,
Luca

[PATCH v2 2/4] arm/setup: Move MMU specific extern declarations to mmu/setup.h

[Luca Fancellu]

Move some extern declarations related to MMU structures and define
from asm/setup.h to asm/mmu/setup.h, in order to increase encapsulation
and allow the MPU part to build, since it has no clue about them.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v1:
 - Moved extern to mmu/setup.h instead of mmu/mm.h
 - moved also pte_of_xenaddr()
---
---
 xen/arch/arm/include/asm/mmu/setup.h | 31 ++++++++++++++++++++++++++++
 xen/arch/arm/include/asm/setup.h     | 20 ++++++------------
 2 files changed, 37 insertions(+), 14 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/mmu/setup.h

diff --git a/xen/arch/arm/include/asm/mmu/setup.h b/xen/arch/arm/include/asm/mmu/setup.h
new file mode 100644
index 000000000000..3fe752b04c63
--- /dev/null
+++ b/xen/arch/arm/include/asm/mmu/setup.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef __ARM_MMU_SETUP_H__
+#define __ARM_MMU_SETUP_H__
+
+#include <asm/lpae.h>
+#include <asm/mmu/layout.h>
+
+extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
+
+#ifdef CONFIG_ARM_64
+extern lpae_t boot_first[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_first_id[XEN_PT_LPAE_ENTRIES];
+#endif
+extern lpae_t boot_second[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_second_id[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_third[XEN_PT_LPAE_ENTRIES * XEN_NR_ENTRIES(2)];
+extern lpae_t boot_third_id[XEN_PT_LPAE_ENTRIES];
+
+/* Find where Xen will be residing at runtime and return a PT entry */
+lpae_t pte_of_xenaddr(vaddr_t va);
+
+#endif /* __ARM_MMU_SETUP_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 64c227d171fc..a5a80d9b477f 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -6,6 +6,12 @@
 #include <xen/bootfdt.h>
 #include <xen/device_tree.h>
 
+#if defined(CONFIG_MMU)
+# include <asm/mmu/setup.h>
+#elif !defined(CONFIG_MPU)
+# error "Unknown memory management layout"
+#endif
+
 #define MAX_FDT_SIZE SZ_2M
 
 struct map_range_data
@@ -65,20 +71,6 @@ int map_irq_to_domain(struct domain *d, unsigned int irq,
 int map_range_to_domain(const struct dt_device_node *dev,
                         uint64_t addr, uint64_t len, void *data);
 
-extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
-
-#ifdef CONFIG_ARM_64
-extern lpae_t boot_first[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_first_id[XEN_PT_LPAE_ENTRIES];
-#endif
-extern lpae_t boot_second[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_second_id[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_third[XEN_PT_LPAE_ENTRIES * XEN_NR_ENTRIES(2)];
-extern lpae_t boot_third_id[XEN_PT_LPAE_ENTRIES];
-
-/* Find where Xen will be residing at runtime and return a PT entry */
-lpae_t pte_of_xenaddr(vaddr_t va);
-
 extern const char __ro_after_init_start[], __ro_after_init_end[];
 
 struct init_info
-- 
2.34.1

[PATCH v2 3/4] xen/arm: Use vmap_contig instead of __vmap where it's possible

[Luca Fancellu]

Currently the arm code uses __vmap function in few parts to map
physically contiguous pages, vmap_contig was introduced recently
and does the same because it's a wrapper for __vmap, so use the
latter instead of the direct __vmap function.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes v1:
 - Add ack-by Julien
---
---
 xen/arch/arm/alternative.c | 3 +--
 xen/arch/arm/cpuerrata.c   | 5 ++---
 xen/arch/arm/kernel.c      | 2 +-
 xen/arch/arm/livepatch.c   | 3 +--
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index d99b5070937d..fec7dbd2cde9 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -209,8 +209,7 @@ void __init apply_alternatives_all(void)
      * The text and inittext section are read-only. So re-map Xen to
      * be able to patch the code.
      */
-    xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
-                    VMAP_DEFAULT);
+    xenmap = vmap_contig(xen_mfn, 1U << xen_order);
     /* Re-mapping Xen is not expected to fail during boot. */
     BUG_ON(!xenmap);
 
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index eef9c0ea0e21..17cf134f1b0d 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -61,9 +61,8 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char *hyp_vec_start,
      * Vectors are part of the text that are mapped read-only. So re-map
      * the vector table to be able to update vectors.
      */
-    dst_remapped = __vmap(&dst_mfn,
-                          1UL << get_order_from_bytes(VECTOR_TABLE_SIZE),
-                          1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
+    dst_remapped = vmap_contig(dst_mfn,
+                               1UL << get_order_from_bytes(VECTOR_TABLE_SIZE));
     if ( !dst_remapped )
         return false;
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 669d143cee1b..293d7efaed9c 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -211,7 +211,7 @@ static __init int kernel_decompress(struct bootmodule *mod, uint32_t offset)
         return -ENOMEM;
     }
     mfn = page_to_mfn(pages);
-    output = __vmap(&mfn, 1 << kernel_order_out, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
+    output = vmap_contig(mfn, 1 << kernel_order_out);
 
     rc = perform_gunzip(output, input, size);
     clean_dcache_va_range(output, output_size);
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 037746d9528d..3805b2974663 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -39,8 +39,7 @@ int arch_livepatch_quiesce(void)
      * The text section is read-only. So re-map Xen to be able to patch
      * the code.
      */
-    vmap_of_xen_text = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR,
-                              VMAP_DEFAULT);
+    vmap_of_xen_text = vmap_contig(text_mfn, 1U << text_order);
 
     if ( !vmap_of_xen_text )
     {
-- 
2.34.1

Re: [PATCH v2 3/4] xen/arm: Use vmap_contig instead of __vmap where it's possible

[Roger Pau Monné]

On Tue, Nov 19, 2024 at 08:58:05AM +0000, Luca Fancellu wrote:
> Currently the arm code uses __vmap function in few parts to map
> physically contiguous pages, vmap_contig was introduced recently
> and does the same because it's a wrapper for __vmap, so use the
> latter instead of the direct __vmap function.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Acked-by: Julien Grall <jgrall@amazon.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

[PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Luca Fancellu]

From: Penny Zheng <Penny.Zheng@arm.com>

If Xenheap is statically configured in Device Tree, its size is
definite. So, the memory shall not be given back into static heap, like
it's normally done in free_init_memory, etc, once the initialization
is finished.

Extract static_heap flag from init data bootinfo, as it will be needed
after destroying the init data section.
Introduce a new helper xen_is_using_staticheap() to tell whether Xenheap
is statically configured in the Device Tree.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v1:
 - moved static_heap to common/page_alloc.c
 - protect static_heap access with CONFIG_STATIC_MEMORY
 - update comment in arm/kernel.c kernel_decompress()
---
---
 xen/arch/arm/arm32/mmu/mm.c       |  4 ++--
 xen/arch/arm/kernel.c             |  7 ++++---
 xen/arch/arm/mmu/setup.c          |  8 ++++++--
 xen/arch/arm/setup.c              | 27 ++++++++++++++-------------
 xen/common/device-tree/bootfdt.c  |  4 +++-
 xen/common/device-tree/bootinfo.c |  2 +-
 xen/common/page_alloc.c           |  5 +++++
 xen/include/xen/bootfdt.h         | 14 +++++++++++++-
 8 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index 063611412be0..b7ca7c94c9ca 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -199,7 +199,7 @@ void __init setup_mm(void)
 
     total_pages = ram_size >> PAGE_SHIFT;
 
-    if ( bootinfo.static_heap )
+    if ( xen_is_using_staticheap() )
     {
         const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
 
@@ -246,7 +246,7 @@ void __init setup_mm(void)
 
     do
     {
-        e = bootinfo.static_heap ?
+        e = xen_is_using_staticheap() ?
             fit_xenheap_in_static_heap(pfn_to_paddr(xenheap_pages), MB(32)) :
             consider_modules(ram_start, ram_end,
                              pfn_to_paddr(xenheap_pages),
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 293d7efaed9c..d2245ec9d2ef 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -244,10 +244,11 @@ static __init int kernel_decompress(struct bootmodule *mod, uint32_t offset)
     size += offset;
 
     /*
-     * Free the original kernel, update the pointers to the
-     * decompressed kernel
+     * In case Xen is not using the static heap feature, free the original
+     * kernel, update the pointers to the decompressed kernel
      */
-    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
+    if ( !xen_is_using_staticheap() )
+        fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
 
     return 0;
 }
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index 9664e85ee6c0..83c0a1480447 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -341,8 +341,12 @@ void free_init_memory(void)
     if ( rc )
         panic("Unable to remove the init section (rc = %d)\n", rc);
 
-    init_domheap_pages(pa, pa + len);
-    printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
+    if ( !xen_is_using_staticheap() )
+    {
+        init_domheap_pages(pa, pa + len);
+        printk("Freed %ldkB init memory.\n",
+               (long)(__init_end-__init_begin) >> 10);
+    }
 }
 
 /**
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 71ebaa77ca94..91340d5dc201 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -206,24 +206,25 @@ void __init discard_initial_modules(void)
     struct bootmodules *mi = &bootinfo.modules;
     int i;
 
-    for ( i = 0; i < mi->nr_mods; i++ )
+    if ( !xen_is_using_staticheap() )
     {
-        paddr_t s = mi->module[i].start;
-        paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
-
-        if ( mi->module[i].kind == BOOTMOD_XEN )
-            continue;
+        for ( i = 0; i < mi->nr_mods; i++ )
+        {
+            paddr_t s = mi->module[i].start;
+            paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
 
-        if ( !mfn_valid(maddr_to_mfn(s)) ||
-             !mfn_valid(maddr_to_mfn(e)) )
-            continue;
+            if ( mi->module[i].kind == BOOTMOD_XEN )
+                continue;
 
-        fw_unreserved_regions(s, e, init_domheap_pages, 0);
-    }
+            if ( !mfn_valid(maddr_to_mfn(s)) ||
+                 !mfn_valid(maddr_to_mfn(e)) )
+                continue;
 
-    mi->nr_mods = 0;
+            fw_unreserved_regions(s, e, init_domheap_pages, 0);
+        }
 
-    remove_early_mappings();
+        mi->nr_mods = 0;
+    }
 }
 
 /* Relocate the FDT in Xen heap */
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 927f59c64b0d..6cc9ae146a97 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -403,7 +403,9 @@ static int __init process_chosen_node(const void *fdt, int node,
         if ( rc )
             return rc;
 
-        bootinfo.static_heap = true;
+#ifdef CONFIG_STATIC_MEMORY
+        static_heap = true;
+#endif
     }
 
     printk("Checking for initrd in /chosen\n");
diff --git a/xen/common/device-tree/bootinfo.c b/xen/common/device-tree/bootinfo.c
index f2e6a1145b7c..1e83d5172938 100644
--- a/xen/common/device-tree/bootinfo.c
+++ b/xen/common/device-tree/bootinfo.c
@@ -386,7 +386,7 @@ void __init populate_boot_allocator(void)
     const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
     paddr_t s, e;
 
-    if ( bootinfo.static_heap )
+    if ( xen_is_using_staticheap() )
     {
         for ( i = 0 ; i < reserved_mem->nr_banks; i++ )
         {
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 33c8c917d984..b1fdb4efcff0 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -164,6 +164,11 @@
 #define PGT_TYPE_INFO_INITIALIZER 0
 #endif
 
+#ifdef CONFIG_STATIC_MEMORY
+/* Flag saved when Xen is using the static heap feature (xen,static-heap) */
+bool __ro_after_init static_heap;
+#endif
+
 unsigned long __read_mostly max_page;
 unsigned long __read_mostly total_pages;
 paddr_t __ro_after_init mem_hotplug;
diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
index 16fa05f38f38..c861590e38c8 100644
--- a/xen/include/xen/bootfdt.h
+++ b/xen/include/xen/bootfdt.h
@@ -132,7 +132,6 @@ struct bootinfo {
 #ifdef CONFIG_STATIC_SHM
     struct shared_meminfo shmem;
 #endif
-    bool static_heap;
 };
 
 #ifdef CONFIG_ACPI
@@ -157,6 +156,10 @@ struct bootinfo {
 
 extern struct bootinfo bootinfo;
 
+#ifdef CONFIG_STATIC_MEMORY
+extern bool static_heap;
+#endif
+
 bool check_reserved_regions_overlap(paddr_t region_start,
                                     paddr_t region_size);
 
@@ -206,4 +209,13 @@ static inline struct shmem_membank_extra *bootinfo_get_shmem_extra(void)
 }
 #endif
 
+static inline bool xen_is_using_staticheap(void)
+{
+#ifdef CONFIG_STATIC_MEMORY
+    return static_heap;
+#else
+    return false;
+#endif
+}
+
 #endif /* XEN_BOOTFDT_H */
-- 
2.34.1

Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Jan Beulich]

On 19.11.2024 09:58, Luca Fancellu wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> If Xenheap is statically configured in Device Tree, its size is
> definite. So, the memory shall not be given back into static heap, like
> it's normally done in free_init_memory, etc, once the initialization
> is finished.

I'm somewhat confused by the use of "back" here? Isn't the change all
about init-time behavior, i.e. memory that's handed to the allocator for
the very first time? Else I may be misunderstanding something here, in
which case I'd like to ask for the description to be refined.

> --- a/xen/include/xen/bootfdt.h
> +++ b/xen/include/xen/bootfdt.h
> @@ -132,7 +132,6 @@ struct bootinfo {
>  #ifdef CONFIG_STATIC_SHM
>      struct shared_meminfo shmem;
>  #endif
> -    bool static_heap;
>  };
>  
>  #ifdef CONFIG_ACPI
> @@ -157,6 +156,10 @@ struct bootinfo {
>  
>  extern struct bootinfo bootinfo;
>  
> +#ifdef CONFIG_STATIC_MEMORY
> +extern bool static_heap;
> +#endif

Just to double check Misra-wise: Is there a guarantee that this header will
always be included by page-alloc.c, so that the definition of the symbol
has an earlier declaration already visible? I ask because this header
doesn't look like one where symbols defined in page-alloc.c would normally
be declared. And I sincerely hope that this header isn't one of those that
end up being included virtually everywhere.

> @@ -206,4 +209,13 @@ static inline struct shmem_membank_extra *bootinfo_get_shmem_extra(void)
>  }
>  #endif
>  
> +static inline bool xen_is_using_staticheap(void)
> +{
> +#ifdef CONFIG_STATIC_MEMORY
> +    return static_heap;
> +#else
> +    return false;
> +#endif
> +}

As to naming: How about using_static_heap()? The xen_ prefix doesn't look to
be carrying any useful information, and the then remaining is_ prefix would
be largely redundant with "using". Plus there surely wants to be a separator
between "static" and "heap".

Jan

Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Luca Fancellu]

Hi Jan,

thanks for your review

> On 25 Nov 2024, at 16:32, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.11.2024 09:58, Luca Fancellu wrote:
>> From: Penny Zheng <Penny.Zheng@arm.com>
>> 
>> If Xenheap is statically configured in Device Tree, its size is
>> definite. So, the memory shall not be given back into static heap, like
>> it's normally done in free_init_memory, etc, once the initialization
>> is finished.
> 
> I'm somewhat confused by the use of "back" here? Isn't the change all
> about init-time behavior, i.e. memory that's handed to the allocator for
> the very first time? Else I may be misunderstanding something here, in
> which case I'd like to ask for the description to be refined.

Yes, I’ve tried to rephrase it, do you think this is more clear?

If the Xen heap is statically configured in Device Tree, its size is
definite, so only the defined memory shall be given to the boot
allocator. Have a check where init_domheap_pages() is called
which takes into account if static heap feature is used.

> 
>> --- a/xen/include/xen/bootfdt.h
>> +++ b/xen/include/xen/bootfdt.h
>> @@ -132,7 +132,6 @@ struct bootinfo {
>> #ifdef CONFIG_STATIC_SHM
>>     struct shared_meminfo shmem;
>> #endif
>> -    bool static_heap;
>> };
>> 
>> #ifdef CONFIG_ACPI
>> @@ -157,6 +156,10 @@ struct bootinfo {
>> 
>> extern struct bootinfo bootinfo;
>> 
>> +#ifdef CONFIG_STATIC_MEMORY
>> +extern bool static_heap;
>> +#endif
> 
> Just to double check Misra-wise: Is there a guarantee that this header will
> always be included by page-alloc.c, so that the definition of the symbol
> has an earlier declaration already visible? I ask because this header
> doesn't look like one where symbols defined in page-alloc.c would normally
> be declared. And I sincerely hope that this header isn't one of those that
> end up being included virtually everywhere.

I’ve read again MISRA rule 8.4 and you are right, I should have included bootfdt.h in
page-alloc.c in order to have the declaration visible before defining static_heap.

I will include the header in page-alloc.c

> 
>> @@ -206,4 +209,13 @@ static inline struct shmem_membank_extra *bootinfo_get_shmem_extra(void)
>> }
>> #endif
>> 
>> +static inline bool xen_is_using_staticheap(void)
>> +{
>> +#ifdef CONFIG_STATIC_MEMORY
>> +    return static_heap;
>> +#else
>> +    return false;
>> +#endif
>> +}
> 
> As to naming: How about using_static_heap()? The xen_ prefix doesn't look to
> be carrying any useful information, and the then remaining is_ prefix would
> be largely redundant with "using". Plus there surely wants to be a separator
> between "static" and "heap".

yes, sounds a better name, I’ll use it

Cheers,
Luca

Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Jan Beulich]

On 26.11.2024 11:56, Luca Fancellu wrote:
> Hi Jan,
> 
> thanks for your review
> 
>> On 25 Nov 2024, at 16:32, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 19.11.2024 09:58, Luca Fancellu wrote:
>>> From: Penny Zheng <Penny.Zheng@arm.com>
>>>
>>> If Xenheap is statically configured in Device Tree, its size is
>>> definite. So, the memory shall not be given back into static heap, like
>>> it's normally done in free_init_memory, etc, once the initialization
>>> is finished.
>>
>> I'm somewhat confused by the use of "back" here? Isn't the change all
>> about init-time behavior, i.e. memory that's handed to the allocator for
>> the very first time? Else I may be misunderstanding something here, in
>> which case I'd like to ask for the description to be refined.
> 
> Yes, I’ve tried to rephrase it, do you think this is more clear?
> 
> If the Xen heap is statically configured in Device Tree, its size is
> definite, so only the defined memory shall be given to the boot
> allocator. Have a check where init_domheap_pages() is called
> which takes into account if static heap feature is used.

This reads better, thanks. Follow-on question: Is what is statically
configured for the heap guaranteed to never overlap with anything passed
to init_domheap_pages() in those places that you touch?

>>> --- a/xen/include/xen/bootfdt.h
>>> +++ b/xen/include/xen/bootfdt.h
>>> @@ -132,7 +132,6 @@ struct bootinfo {
>>> #ifdef CONFIG_STATIC_SHM
>>>     struct shared_meminfo shmem;
>>> #endif
>>> -    bool static_heap;
>>> };
>>>
>>> #ifdef CONFIG_ACPI
>>> @@ -157,6 +156,10 @@ struct bootinfo {
>>>
>>> extern struct bootinfo bootinfo;
>>>
>>> +#ifdef CONFIG_STATIC_MEMORY
>>> +extern bool static_heap;
>>> +#endif
>>
>> Just to double check Misra-wise: Is there a guarantee that this header will
>> always be included by page-alloc.c, so that the definition of the symbol
>> has an earlier declaration already visible? I ask because this header
>> doesn't look like one where symbols defined in page-alloc.c would normally
>> be declared. And I sincerely hope that this header isn't one of those that
>> end up being included virtually everywhere.
> 
> I’ve read again MISRA rule 8.4 and you are right, I should have included bootfdt.h in
> page-alloc.c in order to have the declaration visible before defining static_heap.
> 
> I will include the header in page-alloc.c

Except that, as said, I don't think that header should be included in this file.
Instead I think the declaration wants to move elsewhere.

Jan

Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Luca Fancellu]

Hi Jan,

> 
> This reads better, thanks. Follow-on question: Is what is statically
> configured for the heap guaranteed to never overlap with anything passed
> to init_domheap_pages() in those places that you touch?

I think so, the places of the check are mainly memory regions related to boot modules,
when we add a boot module we also do a check in order to see if it clashes with any
reserved regions already defined, which the static heap is part of.

Could you explain me why the question?

> 
>>>> --- a/xen/include/xen/bootfdt.h
>>>> +++ b/xen/include/xen/bootfdt.h
>>>> @@ -132,7 +132,6 @@ struct bootinfo {
>>>> #ifdef CONFIG_STATIC_SHM
>>>>    struct shared_meminfo shmem;
>>>> #endif
>>>> -    bool static_heap;
>>>> };
>>>> 
>>>> #ifdef CONFIG_ACPI
>>>> @@ -157,6 +156,10 @@ struct bootinfo {
>>>> 
>>>> extern struct bootinfo bootinfo;
>>>> 
>>>> +#ifdef CONFIG_STATIC_MEMORY
>>>> +extern bool static_heap;
>>>> +#endif
>>> 
>>> Just to double check Misra-wise: Is there a guarantee that this header will
>>> always be included by page-alloc.c, so that the definition of the symbol
>>> has an earlier declaration already visible? I ask because this header
>>> doesn't look like one where symbols defined in page-alloc.c would normally
>>> be declared. And I sincerely hope that this header isn't one of those that
>>> end up being included virtually everywhere.
>> 
>> I’ve read again MISRA rule 8.4 and you are right, I should have included bootfdt.h in
>> page-alloc.c in order to have the declaration visible before defining static_heap.
>> 
>> I will include the header in page-alloc.c
> 
> Except that, as said, I don't think that header should be included in this file.
> Instead I think the declaration wants to move elsewhere.

Ok sorry, I misunderstood your comment, I thought you were suggesting to have the
declaration visible in that file since we are defining there the variable.

So Julien suggested that file, it was hosted before in common/device-tree/device-tree.c,
see the comment here:
https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-6-luca.fancellu@arm.com/#26125054

Since it seems you disagree with Julien, could you suggest a more suitable place?

Cheers,
Luca

Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Jan Beulich]

On 26.11.2024 14:25, Luca Fancellu wrote:
>> This reads better, thanks. Follow-on question: Is what is statically
>> configured for the heap guaranteed to never overlap with anything passed
>> to init_domheap_pages() in those places that you touch?
> 
> I think so, the places of the check are mainly memory regions related to boot modules,
> when we add a boot module we also do a check in order to see if it clashes with any
> reserved regions already defined, which the static heap is part of.
> 
> Could you explain me why the question?

Well, if there was a chance of overlap, then parts of the free region would
need to go one way, and the rest the other way.

>>>>> --- a/xen/include/xen/bootfdt.h
>>>>> +++ b/xen/include/xen/bootfdt.h
>>>>> @@ -132,7 +132,6 @@ struct bootinfo {
>>>>> #ifdef CONFIG_STATIC_SHM
>>>>>    struct shared_meminfo shmem;
>>>>> #endif
>>>>> -    bool static_heap;
>>>>> };
>>>>>
>>>>> #ifdef CONFIG_ACPI
>>>>> @@ -157,6 +156,10 @@ struct bootinfo {
>>>>>
>>>>> extern struct bootinfo bootinfo;
>>>>>
>>>>> +#ifdef CONFIG_STATIC_MEMORY
>>>>> +extern bool static_heap;
>>>>> +#endif
>>>>
>>>> Just to double check Misra-wise: Is there a guarantee that this header will
>>>> always be included by page-alloc.c, so that the definition of the symbol
>>>> has an earlier declaration already visible? I ask because this header
>>>> doesn't look like one where symbols defined in page-alloc.c would normally
>>>> be declared. And I sincerely hope that this header isn't one of those that
>>>> end up being included virtually everywhere.
>>>
>>> I’ve read again MISRA rule 8.4 and you are right, I should have included bootfdt.h in
>>> page-alloc.c in order to have the declaration visible before defining static_heap.
>>>
>>> I will include the header in page-alloc.c
>>
>> Except that, as said, I don't think that header should be included in this file.
>> Instead I think the declaration wants to move elsewhere.
> 
> Ok sorry, I misunderstood your comment, I thought you were suggesting to have the
> declaration visible in that file since we are defining there the variable.
> 
> So Julien suggested that file, it was hosted before in common/device-tree/device-tree.c,
> see the comment here:
> https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-6-luca.fancellu@arm.com/#26125054
> 
> Since it seems you disagree with Julien, could you suggest a more suitable place?

Anything defined in page-alloc.c should by default have its declaration in
xen/mm.h, imo. Exceptions would need justification.

Obviously a possible alternative is to move the definition, not the declaration.

Jan

Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Luca Fancellu]

> On 26 Nov 2024, at 13:29, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 26.11.2024 14:25, Luca Fancellu wrote:
>>> This reads better, thanks. Follow-on question: Is what is statically
>>> configured for the heap guaranteed to never overlap with anything passed
>>> to init_domheap_pages() in those places that you touch?
>> 
>> I think so, the places of the check are mainly memory regions related to boot modules,
>> when we add a boot module we also do a check in order to see if it clashes with any
>> reserved regions already defined, which the static heap is part of.
>> 
>> Could you explain me why the question?
> 
> Well, if there was a chance of overlap, then parts of the free region would
> need to go one way, and the rest the other way.

oh ok, sure of course, thanks for answering.

> 
>>>>>> --- a/xen/include/xen/bootfdt.h
>>>>>> +++ b/xen/include/xen/bootfdt.h
>>>>>> @@ -132,7 +132,6 @@ struct bootinfo {
>>>>>> #ifdef CONFIG_STATIC_SHM
>>>>>>   struct shared_meminfo shmem;
>>>>>> #endif
>>>>>> -    bool static_heap;
>>>>>> };
>>>>>> 
>>>>>> #ifdef CONFIG_ACPI
>>>>>> @@ -157,6 +156,10 @@ struct bootinfo {
>>>>>> 
>>>>>> extern struct bootinfo bootinfo;
>>>>>> 
>>>>>> +#ifdef CONFIG_STATIC_MEMORY
>>>>>> +extern bool static_heap;
>>>>>> +#endif
>>>>> 
>>>>> Just to double check Misra-wise: Is there a guarantee that this header will
>>>>> always be included by page-alloc.c, so that the definition of the symbol
>>>>> has an earlier declaration already visible? I ask because this header
>>>>> doesn't look like one where symbols defined in page-alloc.c would normally
>>>>> be declared. And I sincerely hope that this header isn't one of those that
>>>>> end up being included virtually everywhere.
>>>> 
>>>> I’ve read again MISRA rule 8.4 and you are right, I should have included bootfdt.h in
>>>> page-alloc.c in order to have the declaration visible before defining static_heap.
>>>> 
>>>> I will include the header in page-alloc.c
>>> 
>>> Except that, as said, I don't think that header should be included in this file.
>>> Instead I think the declaration wants to move elsewhere.
>> 
>> Ok sorry, I misunderstood your comment, I thought you were suggesting to have the
>> declaration visible in that file since we are defining there the variable.
>> 
>> So Julien suggested that file, it was hosted before in common/device-tree/device-tree.c,
>> see the comment here:
>> https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-6-luca.fancellu@arm.com/#26125054
>> 
>> Since it seems you disagree with Julien, could you suggest a more suitable place?
> 
> Anything defined in page-alloc.c should by default have its declaration in
> xen/mm.h, imo. Exceptions would need justification.

I would be fine to have the declaration in xen/mm.h, I just need to import xen/mm.h in bootfdt.h so that it is visible to
“using_static_heap”, @Julien would you be ok with that?

> 
> Obviously a possible alternative is to move the definition, not the declaration.
> 
> Jan

Cheers,
Luca

Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Julien Grall]

Hi Luca,

Sorry for the late answer.

On 26/11/2024 13:52, Luca Fancellu wrote:
> 
> 
>> On 26 Nov 2024, at 13:29, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 26.11.2024 14:25, Luca Fancellu wrote:
>>>> This reads better, thanks. Follow-on question: Is what is statically
>>>> configured for the heap guaranteed to never overlap with anything passed
>>>> to init_domheap_pages() in those places that you touch?
>>>
>>> I think so, the places of the check are mainly memory regions related to boot modules,
>>> when we add a boot module we also do a check in order to see if it clashes with any
>>> reserved regions already defined, which the static heap is part of.
>>>
>>> Could you explain me why the question?
>>
>> Well, if there was a chance of overlap, then parts of the free region would
>> need to go one way, and the rest the other way.
> 
> oh ok, sure of course, thanks for answering.
> 
>>
>>>>>>> --- a/xen/include/xen/bootfdt.h
>>>>>>> +++ b/xen/include/xen/bootfdt.h
>>>>>>> @@ -132,7 +132,6 @@ struct bootinfo {
>>>>>>> #ifdef CONFIG_STATIC_SHM
>>>>>>>    struct shared_meminfo shmem;
>>>>>>> #endif
>>>>>>> -    bool static_heap;
>>>>>>> };
>>>>>>>
>>>>>>> #ifdef CONFIG_ACPI
>>>>>>> @@ -157,6 +156,10 @@ struct bootinfo {
>>>>>>>
>>>>>>> extern struct bootinfo bootinfo;
>>>>>>>
>>>>>>> +#ifdef CONFIG_STATIC_MEMORY
>>>>>>> +extern bool static_heap;
>>>>>>> +#endif
>>>>>>
>>>>>> Just to double check Misra-wise: Is there a guarantee that this header will
>>>>>> always be included by page-alloc.c, so that the definition of the symbol
>>>>>> has an earlier declaration already visible? I ask because this header
>>>>>> doesn't look like one where symbols defined in page-alloc.c would normally
>>>>>> be declared. And I sincerely hope that this header isn't one of those that
>>>>>> end up being included virtually everywhere.
>>>>>
>>>>> I’ve read again MISRA rule 8.4 and you are right, I should have included bootfdt.h in
>>>>> page-alloc.c in order to have the declaration visible before defining static_heap.
>>>>>
>>>>> I will include the header in page-alloc.c
>>>>
>>>> Except that, as said, I don't think that header should be included in this file.
>>>> Instead I think the declaration wants to move elsewhere.
>>>
>>> Ok sorry, I misunderstood your comment, I thought you were suggesting to have the
>>> declaration visible in that file since we are defining there the variable.
>>>
>>> So Julien suggested that file, it was hosted before in common/device-tree/device-tree.c,
>>> see the comment here:
>>> https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-6-luca.fancellu@arm.com/#26125054
>>>
>>> Since it seems you disagree with Julien, could you suggest a more suitable place?
>>
>> Anything defined in page-alloc.c should by default have its declaration in
>> xen/mm.h, imo. Exceptions would need justification.
> 
> I would be fine to have the declaration in xen/mm.h, I just need to import xen/mm.h in bootfdt.h so that it is visible to
> “using_static_heap”, @Julien would you be ok with that?

I think using_static_heap() should be defined in xen/mm.h as well 
because we expect everyone to use using_static_heap() rather than 
static_heap.

This is to avoid adding #ifdef for every user.

Cheers,

-- 
Julien Grall