[PATCH v5 0/4] Introduce static heap

[PATCH v5 0/4] Introduce static heap

[Henry Wang]

The static heap, or statically configured heap, refers to parts
of RAM reserved in the beginning for heap. Like the static memory
allocation, such static heap regions are reserved by configuration
in the device tree using physical address ranges.

This feature is useful to run Xen on Arm MPU systems, where only a
finite number of memory protection regions are available. The limited
number of protection regions places requirement on planning the use
of MPU protection regions and one or more MPU protection regions needs
to be reserved only for heap.

The first patch enhances the error handling in processing the dts
chosen node. The second patch introduces the static heap and the
device tree parsing code. The third patch renames xenheap_* to
directmap_* for better readablity. The fourth patch adds the
implementation of the static heap pages handling in boot and heap
allocator for Arm.

Changes from v4 to v5:
- Use #address-cells and #size-cells for static heap, do not introduce
  new address/size cells. Update the dt-binding accordingly.
- Correct a typo in code comments.
- Drop Stefano's acked-by in patch #2 as it is not valid.
- Mention the function and code comment rename in commit mesg in patch #3.

Henry Wang (4):
  xen/arm: bootfdt: Make process_chosen_node() return int
  docs, xen/arm: Introduce static heap memory
  xen/arm: mm: Rename xenheap_* variable to directmap_*
  xen/arm: Handle static heap pages in boot and heap allocator

 SUPPORT.md                            |   7 ++
 docs/misc/arm/device-tree/booting.txt |  52 +++++++++
 xen/arch/arm/bootfdt.c                |  53 ++++++---
 xen/arch/arm/domain_build.c           |   8 +-
 xen/arch/arm/include/asm/config.h     |   2 +-
 xen/arch/arm/include/asm/mm.h         |  31 ++---
 xen/arch/arm/include/asm/setup.h      |  23 +++-
 xen/arch/arm/mm.c                     |  50 +++++----
 xen/arch/arm/setup.c                  | 156 ++++++++++++++++++++------
 9 files changed, 293 insertions(+), 89 deletions(-)

-- 
2.17.1

[PATCH v5 1/4] xen/arm: bootfdt: Make process_chosen_node() return int

[Henry Wang]

At the boot time, it is saner to stop booting early if an error occurs
when parsing the device tree chosen node, rather than seeing random
behavior afterwards. Therefore, this commit changes the return type of
the process_chosen_node() from void to int, and return correct errno
based on the error type.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes from v4 to v5:
- No changes.
Changes from v3 to v4:
- Add Reviewed-by from Michal and Acked-by from Julien.
Changes from v2 to v3:
- Adjust the order of this patch, make it the #1.
Changes from v1 to v2:
- New commit.
---
 xen/arch/arm/bootfdt.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index ec81a45de9..1a79b969af 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -293,9 +293,9 @@ static void __init process_multiboot_node(const void *fdt, int node,
                      kind, start, domU);
 }
 
-static void __init process_chosen_node(const void *fdt, int node,
-                                       const char *name,
-                                       u32 address_cells, u32 size_cells)
+static int __init process_chosen_node(const void *fdt, int node,
+                                      const char *name,
+                                      u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
     paddr_t start, end;
@@ -306,11 +306,11 @@ static void __init process_chosen_node(const void *fdt, int node,
     prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
     if ( !prop )
         /* No initrd present. */
-        return;
+        return 0;
     if ( len != sizeof(u32) && len != sizeof(u64) )
     {
         printk("linux,initrd-start property has invalid length %d\n", len);
-        return;
+        return -EINVAL;
     }
     start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
 
@@ -318,12 +318,12 @@ static void __init process_chosen_node(const void *fdt, int node,
     if ( !prop )
     {
         printk("linux,initrd-end not present but -start was\n");
-        return;
+        return -EINVAL;
     }
     if ( len != sizeof(u32) && len != sizeof(u64) )
     {
         printk("linux,initrd-end property has invalid length %d\n", len);
-        return;
+        return -EINVAL;
     }
     end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
 
@@ -331,12 +331,14 @@ static void __init process_chosen_node(const void *fdt, int node,
     {
         printk("linux,initrd limits invalid: %"PRIpaddr" >= %"PRIpaddr"\n",
                   start, end);
-        return;
+        return -EINVAL;
     }
 
     printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
 
     add_boot_module(BOOTMOD_RAMDISK, start, end-start, false);
+
+    return 0;
 }
 
 static int __init process_domain_node(const void *fdt, int node,
@@ -383,7 +385,7 @@ static int __init early_scan_node(const void *fdt,
               device_tree_node_compatible(fdt, node, "multiboot,module" )))
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
-        process_chosen_node(fdt, node, name, address_cells, size_cells);
+        rc = process_chosen_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 2 && device_tree_node_compatible(fdt, node, "xen,domain") )
         rc = process_domain_node(fdt, node, name, address_cells, size_cells);
 
-- 
2.17.1

[PATCH v5 2/4] docs, xen/arm: Introduce static heap memory

[Henry Wang]

This commit introduces the static heap memory, which is parts of RAM
reserved in the beginning of the boot time for heap.

Firstly, since a new type of memory bank is needed for marking the
memory bank solely as the heap, this commit defines `enum membank_type`
and use this enum in function device_tree_get_meminfo(). Changes of
code are done accordingly following the introduction of this enum.

Also, this commit introduces the logic to parse the static heap
configuration in device tree. If the memory bank is reserved as heap
through `xen,static-heap` property in device tree `chosen` node, the
memory will be marked as static heap type.

A documentation section is added, describing the definition of static
heap memory and the method of enabling the static heap memory through
device tree at boot time.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
Changes from v4 to v5:
- Use #address-cells and #size-cells for static heap, do not introduce
  new address/size cells. Update the dt-binding accordingly.
- Correct a typo in code comments.
- Drop Stefano's acked-by as it is not valid.
Changes from v3 to v4:
- Change of wording in comments.
- Add the static heap feature in SUPPORT.md as tech preview.
Changes from v2 to v3:
- Define `enum membank_type` properly, drop the typedef.
- Rename the feature terminology to static heap.
- Rename MEMBANK_MEMORY to MEMBANK_DEFAULT and MEMBANK_XEN_DOMAIN to
  MEMBANK_STATIC_DOMAIN. Add comments to `enum membank_type`.
- Correct typo, add the clarification of the static heap region
  should contain enough memory below 4GB to cater 32-bit DMA for Arm32,
  and add the 64KB alignment requirement in doc.
- Add Stefano's Acked-by for device tree interface.
Changes from v1 to v2:
- Rename the device tree property to xen,static-heap to avoid confusion.
- Change of commit msg and doc wording, correct typo in commit msg.
- Do not change the process_chosen_node() return type.
- Add an empty line in make_memory_node() memory type check to improve
  readability.
- Use enum membank_type to make the memory type cleaner.
Changes from RFC to v1:
- Rename the terminology to reserved heap.
---
 SUPPORT.md                            |  7 ++++
 docs/misc/arm/device-tree/booting.txt | 52 +++++++++++++++++++++++++++
 xen/arch/arm/bootfdt.c                | 29 ++++++++++++---
 xen/arch/arm/domain_build.c           |  8 +++--
 xen/arch/arm/include/asm/setup.h      | 22 +++++++++++-
 xen/arch/arm/setup.c                  |  2 +-
 6 files changed, 111 insertions(+), 9 deletions(-)

diff --git a/SUPPORT.md b/SUPPORT.md
index 8e040d1c1e..b02a5d25ca 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -293,6 +293,13 @@ pre-defined by configuration using physical address ranges.
 
     Status, ARM: Tech Preview
 
+### Static Heap
+
+Allow reserving parts of RAM through the device tree using physical
+address ranges as heap.
+
+    Status, ARM: Tech Preview
+
 ### Memory Sharing
 
 Allow sharing of identical pages between guests
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..a5062db217 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -378,3 +378,55 @@ device-tree:
 
 This will reserve a 512MB region starting at the host physical address
 0x30000000 to be exclusively used by DomU1.
+
+
+Static Heap Memory
+==================
+
+The static heap memory refers to parts of RAM reserved in the beginning of
+boot time for heap. The memory is reserved by configuration in the device
+tree using physical address ranges.
+
+The static heap memory declared in the device tree defines the memory areas
+that will be reserved to be used exclusively as heap.
+
+- For Arm32, since there are separated heaps, the static heap will be used
+for both domheap and xenheap. The admin should make sure that the static
+heap region should contain enough memory below 4GB to cater 32-bit DMA.
+
+- For Arm64, since there is a single heap, the defined static heap areas
+shall always go to the heap allocator.
+
+The static heap memory is an optional feature and can be enabled by adding
+below device tree properties.
+
+The dtb should have the following properties:
+
+- xen,static-heap
+
+    Property under the top-level "chosen" node. It specifies the address
+    and size of Xen static heap memory. Note that at least a 64KB
+    alignment is required.
+
+- #address-cells and #size-cells
+
+    Specify the number of cells used for the address and size of the
+    "xen,static-heap" property. Note that according to the device tree
+    specification, the number of address cells and size cells of
+    "xen,static-heap" is determined by the parent #address-cells and
+    #size-cells of the top-level "chosen" node.
+
+Below is an example on how to specify the static heap in device tree:
+
+    / {
+        #address-cells = <0x2>;
+        #size-cells = <0x2>;
+        ...
+        chosen {
+            xen,static-heap = <0x0 0x30000000 0x0 0x40000000>;
+            ...
+        };
+    };
+
+RAM starting from the host physical address 0x30000000 of 1GB size will
+be reserved as static heap.
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 1a79b969af..3c98c00981 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -64,7 +64,7 @@ void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
 static int __init device_tree_get_meminfo(const void *fdt, int node,
                                           const char *prop_name,
                                           u32 address_cells, u32 size_cells,
-                                          void *data, bool xen_domain)
+                                          void *data, enum membank_type type)
 {
     const struct fdt_property *prop;
     unsigned int i, banks;
@@ -95,7 +95,7 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
             continue;
         mem->bank[mem->nr_banks].start = start;
         mem->bank[mem->nr_banks].size = size;
-        mem->bank[mem->nr_banks].xen_domain = xen_domain;
+        mem->bank[mem->nr_banks].type = type;
         mem->nr_banks++;
     }
 
@@ -185,7 +185,7 @@ static int __init process_memory_node(const void *fdt, int node,
                                       void *data)
 {
     return device_tree_get_meminfo(fdt, node, "reg", address_cells, size_cells,
-                                   data, false);
+                                   data, MEMBANK_DEFAULT);
 }
 
 static int __init process_reserved_memory_node(const void *fdt, int node,
@@ -301,6 +301,26 @@ static int __init process_chosen_node(const void *fdt, int node,
     paddr_t start, end;
     int len;
 
+    if ( fdt_get_property(fdt, node, "xen,static-heap", NULL) )
+    {
+        int rc;
+
+        printk("Checking for static heap in /chosen\n");
+        if ( address_cells < 1 || size_cells < 1 )
+        {
+            printk("fdt: node `%s': invalid #address-cells or #size-cells\n",
+                   name);
+            return -EINVAL;
+        }
+
+        rc = device_tree_get_meminfo(fdt, node, "xen,static-heap",
+                                     address_cells, size_cells,
+                                     &bootinfo.reserved_mem,
+                                     MEMBANK_STATIC_HEAP);
+        if ( rc )
+            return rc;
+    }
+
     printk("Checking for initrd in /chosen\n");
 
     prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
@@ -360,7 +380,8 @@ static int __init process_domain_node(const void *fdt, int node,
                                      "#xen,static-mem-size-cells", 0);
 
     return device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
-                                   size_cells, &bootinfo.reserved_mem, true);
+                                   size_cells, &bootinfo.reserved_mem,
+                                   MEMBANK_STATIC_DOMAIN);
 }
 
 static int __init early_scan_node(const void *fdt,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b76a84e8f5..cea82374f7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1038,9 +1038,11 @@ static int __init make_memory_node(const struct domain *d,
     if ( mem->nr_banks == 0 )
         return -ENOENT;
 
-    /* find first memory range not bound to a Xen domain */
-    for ( i = 0; i < mem->nr_banks && mem->bank[i].xen_domain; i++ )
+    /* find the first memory range that is reserved for device (or firmware) */
+    for ( i = 0; i < mem->nr_banks &&
+                 (mem->bank[i].type != MEMBANK_DEFAULT); i++ )
         ;
+
     if ( i == mem->nr_banks )
         return 0;
 
@@ -1062,7 +1064,7 @@ static int __init make_memory_node(const struct domain *d,
         u64 start = mem->bank[i].start;
         u64 size = mem->bank[i].size;
 
-        if ( mem->bank[i].xen_domain )
+        if ( mem->bank[i].type == MEMBANK_STATIC_DOMAIN )
             continue;
 
         dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 5815ccf8c5..09188acae8 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -22,11 +22,31 @@ typedef enum {
     BOOTMOD_UNKNOWN
 }  bootmodule_kind;
 
+enum membank_type {
+    /*
+     * The MEMBANK_DEFAULT type refers to either reserved memory for the
+     * device/firmware (when the bank is in 'reserved_mem') or any RAM (when
+     * the bank is in 'mem').
+     */
+    MEMBANK_DEFAULT,
+    /*
+     * The MEMBANK_STATIC_DOMAIN type is used to indicate whether the memory
+     * bank is bound to a static Xen domain. It is only valid when the bank
+     * is in reserved_mem.
+     */
+    MEMBANK_STATIC_DOMAIN,
+    /*
+     * The MEMBANK_STATIC_HEAP type is used to indicate whether the memory
+     * bank is reserved as static heap. It is only valid when the bank is
+     * in reserved_mem.
+     */
+    MEMBANK_STATIC_HEAP,
+};
 
 struct membank {
     paddr_t start;
     paddr_t size;
-    bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
+    enum membank_type type;
 };
 
 struct meminfo {
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 7814fe323d..3c36c050bf 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -644,7 +644,7 @@ static void __init init_staticmem_pages(void)
 
     for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
     {
-        if ( bootinfo.reserved_mem.bank[bank].xen_domain )
+        if ( bootinfo.reserved_mem.bank[bank].type == MEMBANK_STATIC_DOMAIN )
         {
             mfn_t bank_start = _mfn(PFN_UP(bootinfo.reserved_mem.bank[bank].start));
             unsigned long bank_pages = PFN_DOWN(bootinfo.reserved_mem.bank[bank].size);
-- 
2.17.1

[PATCH v5 3/4] xen/arm: mm: Rename xenheap_* variable to directmap_*

[Henry Wang]

With the static heap setup, keep using xenheap_* in the function
setup_xenheap_mappings() will make the code confusing to read,
because we always need to map the full RAM on Arm64. Therefore,
renaming all "xenheap_*" variables to "directmap_*" to make clear
the area is used to access the RAM easily.

On Arm32, only the xenheap is direct mapped today. So the renaming
to "directmap_*" would be still valid for Arm32.

As the xenheap_* is renamed to directmap_*, rename the function
setup_xenheap_mappings() to setup_directmap_mappings() to reflect
the variable renaming, also change the code comment and printed
error message in the function accordingly.

No functional change is intended.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes from v4 to v5:
- Mention the function and code comment rename in commit mesg.
- Add Julien's Acked-by.
Changes from v3 to v4:
- Also rename the setup_xenheap_mappings() function name and
  printed messages inside the function.
- Update more comments.
Changes from v2 to v3:
- Adjust the order of this patch, make it #3.
Changes from v1 to v2:
- New commit.
---
 xen/arch/arm/bootfdt.c            |  2 +-
 xen/arch/arm/include/asm/config.h |  2 +-
 xen/arch/arm/include/asm/mm.h     | 31 ++++++++++---------
 xen/arch/arm/mm.c                 | 50 +++++++++++++++++--------------
 xen/arch/arm/setup.c              | 36 +++++++++++-----------
 5 files changed, 64 insertions(+), 57 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 3c98c00981..b4536aed68 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -500,7 +500,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
     device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
 
     /*
-     * On Arm64 setup_xenheap_mappings() expects to be called with the lowest
+     * On Arm64 setup_directmap_mappings() expects to be called with the lowest
      * bank in memory first. There is no requirement that the DT will provide
      * the banks sorted in ascending order. So sort them through.
      */
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 2fafb9f228..0fefed1b8a 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -160,7 +160,7 @@
 #define DIRECTMAP_SIZE         (SLOT0_ENTRY_SIZE * (265-256))
 #define DIRECTMAP_VIRT_END     (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1)
 
-#define XENHEAP_VIRT_START     xenheap_virt_start
+#define XENHEAP_VIRT_START     directmap_virt_start
 
 #define HYPERVISOR_VIRT_END    DIRECTMAP_VIRT_END
 
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 749fbefa0c..7d21120f98 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -154,19 +154,19 @@ struct page_info
 #define _PGC_need_scrub   _PGC_allocated
 #define PGC_need_scrub    PGC_allocated
 
-extern mfn_t xenheap_mfn_start, xenheap_mfn_end;
-extern vaddr_t xenheap_virt_end;
+extern mfn_t directmap_mfn_start, directmap_mfn_end;
+extern vaddr_t directmap_virt_end;
 #ifdef CONFIG_ARM_64
-extern vaddr_t xenheap_virt_start;
-extern unsigned long xenheap_base_pdx;
+extern vaddr_t directmap_virt_start;
+extern unsigned long directmap_base_pdx;
 #endif
 
 #ifdef CONFIG_ARM_32
 #define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
 #define is_xen_heap_mfn(mfn) ({                                 \
     unsigned long mfn_ = mfn_x(mfn);                            \
-    (mfn_ >= mfn_x(xenheap_mfn_start) &&                        \
-     mfn_ < mfn_x(xenheap_mfn_end));                            \
+    (mfn_ >= mfn_x(directmap_mfn_start) &&                      \
+     mfn_ < mfn_x(directmap_mfn_end));                          \
 })
 #else
 #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
@@ -203,9 +203,12 @@ extern void remove_early_mappings(void);
 extern int init_secondary_pagetables(int cpu);
 /* Switch secondary CPUS to its own pagetables and finalise MMU setup */
 extern void mmu_init_secondary_cpu(void);
-/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory.
- * Base must be 32MB aligned and size a multiple of 32MB. */
-extern void setup_xenheap_mappings(unsigned long base_mfn, unsigned long nr_mfns);
+/*
+ * For Arm32, set up the direct-mapped xenheap: up to 1GB of contiguous,
+ * always-mapped memory. Base must be 32MB aligned and size a multiple of 32MB.
+ * For Arm64, map the region in the directmap area.
+ */
+extern void setup_directmap_mappings(unsigned long base_mfn, unsigned long nr_mfns);
 /* Map a frame table to cover physical addresses ps through pe */
 extern void setup_frametable_mappings(paddr_t ps, paddr_t pe);
 /* map a physical range in virtual memory */
@@ -267,16 +270,16 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
 static inline void *maddr_to_virt(paddr_t ma)
 {
     ASSERT(is_xen_heap_mfn(maddr_to_mfn(ma)));
-    ma -= mfn_to_maddr(xenheap_mfn_start);
+    ma -= mfn_to_maddr(directmap_mfn_start);
     return (void *)(unsigned long) ma + XENHEAP_VIRT_START;
 }
 #else
 static inline void *maddr_to_virt(paddr_t ma)
 {
-    ASSERT((mfn_to_pdx(maddr_to_mfn(ma)) - xenheap_base_pdx) <
+    ASSERT((mfn_to_pdx(maddr_to_mfn(ma)) - directmap_base_pdx) <
            (DIRECTMAP_SIZE >> PAGE_SHIFT));
     return (void *)(XENHEAP_VIRT_START -
-                    (xenheap_base_pdx << PAGE_SHIFT) +
+                    (directmap_base_pdx << PAGE_SHIFT) +
                     ((ma & ma_va_bottom_mask) |
                      ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
 }
@@ -319,10 +322,10 @@ static inline struct page_info *virt_to_page(const void *v)
     unsigned long pdx;
 
     ASSERT(va >= XENHEAP_VIRT_START);
-    ASSERT(va < xenheap_virt_end);
+    ASSERT(va < directmap_virt_end);
 
     pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT;
-    pdx += mfn_to_pdx(xenheap_mfn_start);
+    pdx += mfn_to_pdx(directmap_mfn_start);
     return frame_table + pdx - frametable_base_pdx;
 }
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7f5b317d3e..11ee49598b 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -132,12 +132,12 @@ uint64_t init_ttbr;
 static paddr_t phys_offset;
 
 /* Limits of the Xen heap */
-mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
-mfn_t xenheap_mfn_end __read_mostly;
-vaddr_t xenheap_virt_end __read_mostly;
+mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
+mfn_t directmap_mfn_end __read_mostly;
+vaddr_t directmap_virt_end __read_mostly;
 #ifdef CONFIG_ARM_64
-vaddr_t xenheap_virt_start __read_mostly;
-unsigned long xenheap_base_pdx __read_mostly;
+vaddr_t directmap_virt_start __read_mostly;
+unsigned long directmap_base_pdx __read_mostly;
 #endif
 
 unsigned long frametable_base_pdx __read_mostly;
@@ -597,55 +597,59 @@ void mmu_init_secondary_cpu(void)
 }
 
 #ifdef CONFIG_ARM_32
-/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory. */
-void __init setup_xenheap_mappings(unsigned long base_mfn,
-                                   unsigned long nr_mfns)
+/*
+ * Set up the direct-mapped xenheap:
+ * up to 1GB of contiguous, always-mapped memory.
+ */
+void __init setup_directmap_mappings(unsigned long base_mfn,
+                                     unsigned long nr_mfns)
 {
     int rc;
 
     rc = map_pages_to_xen(XENHEAP_VIRT_START, _mfn(base_mfn), nr_mfns,
                           PAGE_HYPERVISOR_RW | _PAGE_BLOCK);
     if ( rc )
-        panic("Unable to setup the xenheap mappings.\n");
+        panic("Unable to setup the directmap mappings.\n");
 
-    /* Record where the xenheap is, for translation routines. */
-    xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
+    /* Record where the directmap is, for translation routines. */
+    directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
 }
 #else /* CONFIG_ARM_64 */
-void __init setup_xenheap_mappings(unsigned long base_mfn,
-                                   unsigned long nr_mfns)
+/* Map the region in the directmap area. */
+void __init setup_directmap_mappings(unsigned long base_mfn,
+                                     unsigned long nr_mfns)
 {
     int rc;
 
-    /* First call sets the xenheap physical and virtual offset. */
-    if ( mfn_eq(xenheap_mfn_start, INVALID_MFN) )
+    /* First call sets the directmap physical and virtual offset. */
+    if ( mfn_eq(directmap_mfn_start, INVALID_MFN) )
     {
         unsigned long mfn_gb = base_mfn & ~((FIRST_SIZE >> PAGE_SHIFT) - 1);
 
-        xenheap_mfn_start = _mfn(base_mfn);
-        xenheap_base_pdx = mfn_to_pdx(_mfn(base_mfn));
+        directmap_mfn_start = _mfn(base_mfn);
+        directmap_base_pdx = mfn_to_pdx(_mfn(base_mfn));
         /*
          * The base address may not be aligned to the first level
          * size (e.g. 1GB when using 4KB pages). This would prevent
          * superpage mappings for all the regions because the virtual
          * address and machine address should both be suitably aligned.
          *
-         * Prevent that by offsetting the start of the xenheap virtual
+         * Prevent that by offsetting the start of the directmap virtual
          * address.
          */
-        xenheap_virt_start = DIRECTMAP_VIRT_START +
+        directmap_virt_start = DIRECTMAP_VIRT_START +
             (base_mfn - mfn_gb) * PAGE_SIZE;
     }
 
-    if ( base_mfn < mfn_x(xenheap_mfn_start) )
-        panic("cannot add xenheap mapping at %lx below heap start %lx\n",
-              base_mfn, mfn_x(xenheap_mfn_start));
+    if ( base_mfn < mfn_x(directmap_mfn_start) )
+        panic("cannot add directmap mapping at %lx below heap start %lx\n",
+              base_mfn, mfn_x(directmap_mfn_start));
 
     rc = map_pages_to_xen((vaddr_t)__mfn_to_virt(base_mfn),
                           _mfn(base_mfn), nr_mfns,
                           PAGE_HYPERVISOR_RW | _PAGE_BLOCK);
     if ( rc )
-        panic("Unable to setup the xenheap mappings.\n");
+        panic("Unable to setup the directmap mappings.\n");
 }
 #endif
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 3c36c050bf..9f3838d004 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -697,11 +697,11 @@ static void __init populate_boot_allocator(void)
 
 #ifdef CONFIG_ARM_32
             /* Avoid the xenheap */
-            if ( s < mfn_to_maddr(xenheap_mfn_end) &&
-                 mfn_to_maddr(xenheap_mfn_start) < e )
+            if ( s < mfn_to_maddr(directmap_mfn_end) &&
+                 mfn_to_maddr(directmap_mfn_start) < e )
             {
-                e = mfn_to_maddr(xenheap_mfn_start);
-                n = mfn_to_maddr(xenheap_mfn_end);
+                e = mfn_to_maddr(directmap_mfn_start);
+                n = mfn_to_maddr(directmap_mfn_end);
             }
 #endif
 
@@ -791,17 +791,17 @@ static void __init setup_mm(void)
 
     /*
      * We need some memory to allocate the page-tables used for the
-     * xenheap mappings. So populate the boot allocator first.
+     * directmap mappings. So populate the boot allocator first.
      *
-     * This requires us to set xenheap_mfn_{start, end} first so the Xenheap
-     * region can be avoided.
+     * This requires us to set directmap_mfn_{start, end} first so the
+     * direct-mapped Xenheap region can be avoided.
      */
-    xenheap_mfn_start = _mfn((e >> PAGE_SHIFT) - xenheap_pages);
-    xenheap_mfn_end = mfn_add(xenheap_mfn_start, xenheap_pages);
+    directmap_mfn_start = _mfn((e >> PAGE_SHIFT) - xenheap_pages);
+    directmap_mfn_end = mfn_add(directmap_mfn_start, xenheap_pages);
 
     populate_boot_allocator();
 
-    setup_xenheap_mappings(mfn_x(xenheap_mfn_start), xenheap_pages);
+    setup_directmap_mappings(mfn_x(directmap_mfn_start), xenheap_pages);
 
     /* Frame table covers all of RAM region, including holes */
     setup_frametable_mappings(ram_start, ram_end);
@@ -816,8 +816,8 @@ static void __init setup_mm(void)
               smp_processor_id());
 
     /* Add xenheap memory that was not already added to the boot allocator. */
-    init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start),
-                       mfn_to_maddr(xenheap_mfn_end));
+    init_xenheap_pages(mfn_to_maddr(directmap_mfn_start),
+                       mfn_to_maddr(directmap_mfn_end));
 
     init_staticmem_pages();
 }
@@ -833,7 +833,7 @@ static void __init setup_mm(void)
     init_pdx();
 
     /*
-     * We need some memory to allocate the page-tables used for the xenheap
+     * We need some memory to allocate the page-tables used for the directmap
      * mappings. But some regions may contain memory already allocated
      * for other uses (e.g. modules, reserved-memory...).
      *
@@ -852,15 +852,15 @@ static void __init setup_mm(void)
         ram_start = min(ram_start, bank->start);
         ram_end = max(ram_end, bank_end);
 
-        setup_xenheap_mappings(PFN_DOWN(bank->start),
-                               PFN_DOWN(bank->size));
+        setup_directmap_mappings(PFN_DOWN(bank->start),
+                                 PFN_DOWN(bank->size));
     }
 
     total_pages += ram_size >> PAGE_SHIFT;
 
-    xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
-    xenheap_mfn_start = maddr_to_mfn(ram_start);
-    xenheap_mfn_end = maddr_to_mfn(ram_end);
+    directmap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
+    directmap_mfn_start = maddr_to_mfn(ram_start);
+    directmap_mfn_end = maddr_to_mfn(ram_end);
 
     setup_frametable_mappings(ram_start, ram_end);
     max_page = PFN_DOWN(ram_end);
-- 
2.17.1

[PATCH v5 4/4] xen/arm: Handle static heap pages in boot and heap allocator

[Henry Wang]

This commit firstly adds a bool field `static_heap` to bootinfo.
This newly introduced field is set at the device tree parsing time
if the static heap ranges are defined in the device tree chosen
node.

For Arm32, In `setup_mm`, if the static heap is enabled, we use the
static heap region for both domheap and xenheap allocation. Note
that the xenheap on Arm32 should be always contiguous, so also add
a helper fit_xenheap_in_static_heap() for Arm32 to find the required
xenheap in the static heap regions.

For Arm64, In `setup_mm`, if the static heap is enabled and used,
we make sure that only these static heap pages are added to the boot
allocator. These static heap pages in the boot allocator are
added to the heap allocator at `end_boot_allocator()`.

If the static heap is disabled, we stick to current page allocation
strategy at boot time.

Also, take the chance to correct a "double not" print in Arm32
`setup_mm()` and replace the open-coding address ~0 by INVALID_PADDR.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
Changes from v4 to v5:
- No changes.
Changes from v3 to v4:
- Add Julien's Reviewed-by.
Changes from v2 to v3:
- Adjustment of the terminology change to "static heap".
- Change of wording in comments.
- int i -> unsigned int i.
- Avoid extra indentation by reverting the check of MEMBANK_RSVD_HEAP.
- Use MB(32).
- Drop unnecessary panic and unused variables.
- Avoid the ternary operation in assigning the heap_pages.
- Rework populate_boot_allocator() for static heap.
Changes from v1 to v2:
- Move the global bool `reserved_heap` to bootinfo.
- Replace the open open-coding address ~0 by INVALID_PADDR.
- Do not use reverted logic in heap_pages calculation.
- Remove unused Arm32 reserved_heap_start variable.
- Decouple the arm32 reserved heap too small size check with region
  end check.
- Reuse the arm32 original xenheap finding logic with the new helper
  to make sure xenheap on arm32 is contiguous.
Changes from RFC to v1:
- Rebase on top of latest `setup_mm()` changes.
- Added Arm32 logic in `setup_mm()`.
---
 xen/arch/arm/bootfdt.c           |   2 +
 xen/arch/arm/include/asm/setup.h |   1 +
 xen/arch/arm/setup.c             | 118 +++++++++++++++++++++++++++----
 3 files changed, 107 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index b4536aed68..b092dbc21b 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -319,6 +319,8 @@ static int __init process_chosen_node(const void *fdt, int node,
                                      MEMBANK_STATIC_HEAP);
         if ( rc )
             return rc;
+
+        bootinfo.static_heap = true;
     }
 
     printk("Checking for initrd in /chosen\n");
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 09188acae8..5b86cf0245 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -97,6 +97,7 @@ struct bootinfo {
 #ifdef CONFIG_ACPI
     struct meminfo acpi;
 #endif
+    bool static_heap;
 };
 
 struct map_range_data
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 9f3838d004..e0f9809d7e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -556,6 +556,44 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
     }
     return e;
 }
+
+/*
+ * Find a contiguous region that fits in the static heap region with
+ * required size and alignment, and return the end address of the region
+ * if found otherwise 0.
+ */
+static paddr_t __init fit_xenheap_in_static_heap(uint32_t size, paddr_t align)
+{
+    unsigned int i;
+    paddr_t end = 0, aligned_start, aligned_end;
+    paddr_t bank_start, bank_size, bank_end;
+
+    for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+    {
+        if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+            continue;
+
+        bank_start = bootinfo.reserved_mem.bank[i].start;
+        bank_size = bootinfo.reserved_mem.bank[i].size;
+        bank_end = bank_start + bank_size;
+
+        if ( bank_size < size )
+            continue;
+
+        aligned_end = bank_end & ~(align - 1);
+        aligned_start = (aligned_end - size) & ~(align - 1);
+
+        if ( aligned_start > bank_start )
+            /*
+             * Allocate the xenheap as high as possible to keep low-memory
+             * available (assuming the admin supplied region below 4GB)
+             * for other use (e.g. domain memory allocation).
+             */
+            end = max(end, aligned_end);
+    }
+
+    return end;
+}
 #endif
 
 /*
@@ -661,22 +699,51 @@ static void __init init_staticmem_pages(void)
 }
 
 /*
- * Populate the boot allocator. All the RAM but the following regions
- * will be added:
+ * Populate the boot allocator.
+ * If a static heap was not provided by the admin, all the RAM but the
+ * following regions will be added:
  *  - Modules (e.g., Xen, Kernel)
  *  - Reserved regions
  *  - Xenheap (arm32 only)
+ * If a static heap was provided by the admin, populate the boot
+ * allocator with the corresponding regions only, but with Xenheap excluded
+ * on arm32.
  */
 static void __init populate_boot_allocator(void)
 {
     unsigned int i;
     const struct meminfo *banks = &bootinfo.mem;
+    paddr_t s, e;
+
+    if ( bootinfo.static_heap )
+    {
+        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+        {
+            if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+                continue;
+
+            s = bootinfo.reserved_mem.bank[i].start;
+            e = s + bootinfo.reserved_mem.bank[i].size;
+#ifdef CONFIG_ARM_32
+            /* Avoid the xenheap, note that the xenheap cannot across a bank */
+            if ( s <= mfn_to_maddr(directmap_mfn_start) &&
+                 e >= mfn_to_maddr(directmap_mfn_end) )
+            {
+                init_boot_pages(s, mfn_to_maddr(directmap_mfn_start));
+                init_boot_pages(mfn_to_maddr(directmap_mfn_end), e);
+            }
+            else
+#endif
+                init_boot_pages(s, e);
+        }
+
+        return;
+    }
 
     for ( i = 0; i < banks->nr_banks; i++ )
     {
         const struct membank *bank = &banks->bank[i];
         paddr_t bank_end = bank->start + bank->size;
-        paddr_t s, e;
 
         s = bank->start;
         while ( s < bank_end )
@@ -714,8 +781,8 @@ static void __init populate_boot_allocator(void)
 #ifdef CONFIG_ARM_32
 static void __init setup_mm(void)
 {
-    paddr_t ram_start, ram_end, ram_size, e;
-    unsigned long ram_pages;
+    paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size;
+    paddr_t static_heap_end = 0, static_heap_size = 0;
     unsigned long heap_pages, xenheap_pages, domheap_pages;
     unsigned int i;
     const uint32_t ctr = READ_CP32(CTR);
@@ -735,30 +802,51 @@ static void __init setup_mm(void)
 
     for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
     {
-        paddr_t bank_start = bootinfo.mem.bank[i].start;
-        paddr_t bank_size = bootinfo.mem.bank[i].size;
-        paddr_t bank_end = bank_start + bank_size;
+        bank_start = bootinfo.mem.bank[i].start;
+        bank_size = bootinfo.mem.bank[i].size;
+        bank_end = bank_start + bank_size;
 
         ram_size  = ram_size + bank_size;
         ram_start = min(ram_start,bank_start);
         ram_end   = max(ram_end,bank_end);
     }
 
-    total_pages = ram_pages = ram_size >> PAGE_SHIFT;
+    total_pages = ram_size >> PAGE_SHIFT;
+
+    if ( bootinfo.static_heap )
+    {
+        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+        {
+            if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+                continue;
+
+            bank_start = bootinfo.reserved_mem.bank[i].start;
+            bank_size = bootinfo.reserved_mem.bank[i].size;
+            bank_end = bank_start + bank_size;
+
+            static_heap_size += bank_size;
+            static_heap_end = max(static_heap_end, bank_end);
+        }
+
+        heap_pages = static_heap_size >> PAGE_SHIFT;
+    }
+    else
+        heap_pages = total_pages;
 
     /*
      * If the user has not requested otherwise via the command line
      * then locate the xenheap using these constraints:
      *
+     *  - must be contiguous
      *  - must be 32 MiB aligned
      *  - must not include Xen itself or the boot modules
-     *  - must be at most 1GB or 1/32 the total RAM in the system if less
+     *  - must be at most 1GB or 1/32 the total RAM in the system (or static
+          heap if enabled) if less
      *  - must be at least 32M
      *
      * We try to allocate the largest xenheap possible within these
      * constraints.
      */
-    heap_pages = ram_pages;
     if ( opt_xenheap_megabytes )
         xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
     else
@@ -770,7 +858,9 @@ static void __init setup_mm(void)
 
     do
     {
-        e = consider_modules(ram_start, ram_end,
+        e = bootinfo.static_heap ?
+            fit_xenheap_in_static_heap(pfn_to_paddr(xenheap_pages), MB(32)) :
+            consider_modules(ram_start, ram_end,
                              pfn_to_paddr(xenheap_pages),
                              32<<20, 0);
         if ( e )
@@ -780,7 +870,7 @@ static void __init setup_mm(void)
     } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
 
     if ( ! e )
-        panic("Not not enough space for xenheap\n");
+        panic("Not enough space for xenheap\n");
 
     domheap_pages = heap_pages - xenheap_pages;
 
@@ -825,7 +915,7 @@ static void __init setup_mm(void)
 static void __init setup_mm(void)
 {
     const struct meminfo *banks = &bootinfo.mem;
-    paddr_t ram_start = ~0;
+    paddr_t ram_start = INVALID_PADDR;
     paddr_t ram_end = 0;
     paddr_t ram_size = 0;
     unsigned int i;
-- 
2.17.1

Re: [PATCH v5 2/4] docs, xen/arm: Introduce static heap memory

[Michal Orzel]

Hi Henry,

On 08/09/2022 06:25, Henry Wang wrote:
> 
> This commit introduces the static heap memory, which is parts of RAM
> reserved in the beginning of the boot time for heap.
> 
> Firstly, since a new type of memory bank is needed for marking the
> memory bank solely as the heap, this commit defines `enum membank_type`
> and use this enum in function device_tree_get_meminfo(). Changes of
> code are done accordingly following the introduction of this enum.
> 
> Also, this commit introduces the logic to parse the static heap
> configuration in device tree. If the memory bank is reserved as heap
> through `xen,static-heap` property in device tree `chosen` node, the
> memory will be marked as static heap type.
> 
> A documentation section is added, describing the definition of static
> heap memory and the method of enabling the static heap memory through
> device tree at boot time.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> Changes from v4 to v5:
> - Use #address-cells and #size-cells for static heap, do not introduce
>   new address/size cells. Update the dt-binding accordingly.
> - Correct a typo in code comments.
> - Drop Stefano's acked-by as it is not valid.
> Changes from v3 to v4:
> - Change of wording in comments.
> - Add the static heap feature in SUPPORT.md as tech preview.
> Changes from v2 to v3:
> - Define `enum membank_type` properly, drop the typedef.
> - Rename the feature terminology to static heap.
> - Rename MEMBANK_MEMORY to MEMBANK_DEFAULT and MEMBANK_XEN_DOMAIN to
>   MEMBANK_STATIC_DOMAIN. Add comments to `enum membank_type`.
> - Correct typo, add the clarification of the static heap region
>   should contain enough memory below 4GB to cater 32-bit DMA for Arm32,
>   and add the 64KB alignment requirement in doc.
> - Add Stefano's Acked-by for device tree interface.
> Changes from v1 to v2:
> - Rename the device tree property to xen,static-heap to avoid confusion.
> - Change of commit msg and doc wording, correct typo in commit msg.
> - Do not change the process_chosen_node() return type.
> - Add an empty line in make_memory_node() memory type check to improve
>   readability.
> - Use enum membank_type to make the memory type cleaner.
> Changes from RFC to v1:
> - Rename the terminology to reserved heap.
> ---
>  SUPPORT.md                            |  7 ++++
>  docs/misc/arm/device-tree/booting.txt | 52 +++++++++++++++++++++++++++
>  xen/arch/arm/bootfdt.c                | 29 ++++++++++++---
>  xen/arch/arm/domain_build.c           |  8 +++--
>  xen/arch/arm/include/asm/setup.h      | 22 +++++++++++-
>  xen/arch/arm/setup.c                  |  2 +-
>  6 files changed, 111 insertions(+), 9 deletions(-)
> 
> diff --git a/SUPPORT.md b/SUPPORT.md
> index 8e040d1c1e..b02a5d25ca 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -293,6 +293,13 @@ pre-defined by configuration using physical address ranges.
> 
>      Status, ARM: Tech Preview
> 
> +### Static Heap
> +
> +Allow reserving parts of RAM through the device tree using physical
> +address ranges as heap.
> +
> +    Status, ARM: Tech Preview
> +
>  ### Memory Sharing
> 
>  Allow sharing of identical pages between guests
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..a5062db217 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -378,3 +378,55 @@ device-tree:
> 
>  This will reserve a 512MB region starting at the host physical address
>  0x30000000 to be exclusively used by DomU1.
> +
> +
> +Static Heap Memory
> +==================
> +
> +The static heap memory refers to parts of RAM reserved in the beginning of
> +boot time for heap. The memory is reserved by configuration in the device
> +tree using physical address ranges.
> +
> +The static heap memory declared in the device tree defines the memory areas
> +that will be reserved to be used exclusively as heap.
> +
> +- For Arm32, since there are separated heaps, the static heap will be used
> +for both domheap and xenheap. The admin should make sure that the static
> +heap region should contain enough memory below 4GB to cater 32-bit DMA.
> +
> +- For Arm64, since there is a single heap, the defined static heap areas
> +shall always go to the heap allocator.
> +
> +The static heap memory is an optional feature and can be enabled by adding
> +below device tree properties.
> +
> +The dtb should have the following properties:
> +
> +- xen,static-heap
> +
> +    Property under the top-level "chosen" node. It specifies the address
> +    and size of Xen static heap memory. Note that at least a 64KB
> +    alignment is required.
> +
> +- #address-cells and #size-cells
> +
> +    Specify the number of cells used for the address and size of the
> +    "xen,static-heap" property. Note that according to the device tree
> +    specification, the number of address cells and size cells of
> +    "xen,static-heap" is determined by the parent #address-cells and
> +    #size-cells of the top-level "chosen" node.
I am not sure we should put the information about #address-cells and #size-cells in that form.
Firstly because /chosen node is always a child of / node and according to specs [1]
the #address-cells and #size-cells are required properties for the root node.

If we want to still mention it I would just write under xen,static-heap:
"Number of address and size cells for the xen,static-heap property is determined
by the root node #address-cells/#size-cells".

> +
> +Below is an example on how to specify the static heap in device tree:
> +
> +    / {
> +        #address-cells = <0x2>;
> +        #size-cells = <0x2>;
> +        ...
> +        chosen {
> +            xen,static-heap = <0x0 0x30000000 0x0 0x40000000>;
> +            ...
> +        };
> +    };
> +
> +RAM starting from the host physical address 0x30000000 of 1GB size will
> +be reserved as static heap.
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 1a79b969af..3c98c00981 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -64,7 +64,7 @@ void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
>  static int __init device_tree_get_meminfo(const void *fdt, int node,
>                                            const char *prop_name,
>                                            u32 address_cells, u32 size_cells,
> -                                          void *data, bool xen_domain)
> +                                          void *data, enum membank_type type)
>  {
>      const struct fdt_property *prop;
>      unsigned int i, banks;
> @@ -95,7 +95,7 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
>              continue;
>          mem->bank[mem->nr_banks].start = start;
>          mem->bank[mem->nr_banks].size = size;
> -        mem->bank[mem->nr_banks].xen_domain = xen_domain;
> +        mem->bank[mem->nr_banks].type = type;
>          mem->nr_banks++;
>      }
> 
> @@ -185,7 +185,7 @@ static int __init process_memory_node(const void *fdt, int node,
>                                        void *data)
>  {
>      return device_tree_get_meminfo(fdt, node, "reg", address_cells, size_cells,
> -                                   data, false);
> +                                   data, MEMBANK_DEFAULT);
>  }
> 
>  static int __init process_reserved_memory_node(const void *fdt, int node,
> @@ -301,6 +301,26 @@ static int __init process_chosen_node(const void *fdt, int node,
>      paddr_t start, end;
>      int len;
> 
> +    if ( fdt_get_property(fdt, node, "xen,static-heap", NULL) )
> +    {
> +        int rc;
> +
> +        printk("Checking for static heap in /chosen\n");
> +        if ( address_cells < 1 || size_cells < 1 )
> +        {
> +            printk("fdt: node `%s': invalid #address-cells or #size-cells\n",
> +                   name);
> +            return -EINVAL;
> +        }
This check is now the direct copy of the one in device_tree_get_meminfo so please remove it
to avoid code duplication.

~Michal

[1] https://devicetree-specification.readthedocs.io/en/v0.3/devicenodes.html

RE: [PATCH v5 2/4] docs, xen/arm: Introduce static heap memory

[Henry Wang]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> > +- #address-cells and #size-cells
> > +
> > +    Specify the number of cells used for the address and size of the
> > +    "xen,static-heap" property. Note that according to the device tree
> > +    specification, the number of address cells and size cells of
> > +    "xen,static-heap" is determined by the parent #address-cells and
> > +    #size-cells of the top-level "chosen" node.
> I am not sure we should put the information about #address-cells and #size-
> cells in that form.
> Firstly because /chosen node is always a child of / node and according to
> specs [1]
> the #address-cells and #size-cells are required properties for the root node.
> 
> If we want to still mention it I would just write under xen,static-heap:
> "Number of address and size cells for the xen,static-heap property is
> determined
> by the root node #address-cells/#size-cells".

Thanks, I will address this comments and ...

> 
> > +        printk("Checking for static heap in /chosen\n");
> > +        if ( address_cells < 1 || size_cells < 1 )
> > +        {
> > +            printk("fdt: node `%s': invalid #address-cells or #size-cells\n",
> > +                   name);
> > +            return -EINVAL;
> > +        }
> This check is now the direct copy of the one in device_tree_get_meminfo so
> please remove it
> to avoid code duplication.

...this in v6, but I want to see if there would be more comments before sending
it.

Kind regards,
Henry

> 
> ~Michal
> 
> [1] https://devicetree-
> specification.readthedocs.io/en/v0.3/devicenodes.html

Re: [PATCH v5 2/4] docs, xen/arm: Introduce static heap memory

[Julien Grall]

Hi Henry,

On 08/09/2022 05:25, Henry Wang wrote:
> This commit introduces the static heap memory, which is parts of RAM
> reserved in the beginning of the boot time for heap.
> 
> Firstly, since a new type of memory bank is needed for marking the
> memory bank solely as the heap, this commit defines `enum membank_type`
> and use this enum in function device_tree_get_meminfo(). Changes of
> code are done accordingly following the introduction of this enum.
> 
> Also, this commit introduces the logic to parse the static heap
> configuration in device tree. If the memory bank is reserved as heap
> through `xen,static-heap` property in device tree `chosen` node, the
> memory will be marked as static heap type.
> 
> A documentation section is added, describing the definition of static
> heap memory and the method of enabling the static heap memory through
> device tree at boot time.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

With Michal's comments addressed:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall