[PATCH 0/2] Introduce reserved heap

[PATCH 0/2] Introduce reserved heap

[Henry Wang]

The reserved heap, or statically configured heap, refers to parts
of RAM reserved in the beginning for heap. Like the static memory
allocation, such reserved 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 introduces the reserved heap and the device tree parsing
code. The second patch adds the implementation of the reserved heap
pages handling in boot and heap allocator for Arm.

Changes from RFC to v1:
- Rename the terminology to reserved heap.
- Rebase on top of latest `setup_mm()` changes.
- Added Arm32 logic in `setup_mm()`.

Henry Wang (2):
  docs, xen/arm: Introduce reserved heap memory
  xen/arm: Handle reserved heap pages in boot and heap allocator

 docs/misc/arm/device-tree/booting.txt | 46 ++++++++++++++++
 xen/arch/arm/bootfdt.c                | 51 +++++++++++++----
 xen/arch/arm/domain_build.c           |  5 +-
 xen/arch/arm/include/asm/setup.h      |  3 +
 xen/arch/arm/setup.c                  | 79 +++++++++++++++++++++------
 5 files changed, 156 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Henry Wang]

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

A new boolean field `xen_heap` in `struct membank` is added to store the
configuration telling if the memory bank is reserved as heap through
`xen,static-mem` property in device tree `chosen` node.

Also, this commit introduces the logic to parse the reserved heap
configuation in device tree by reusing the device tree entry definition
of the static memory allocation feature:

- Add a boolean parameter `xen_heap` to `device_tree_get_meminfo` to
reflect whether the memory bank is reserved as heap.

- Use `device_tree_get_meminfo` to parse the reserved heap configuation
in `chosen` node of the device tree.

- In order to reuse the function `device_tree_get_meminfo`, the
return type of `process_chosen_node` is changed from void to int.

A documentation section is added, describing the definition of reserved
heap memory and the method of enabling the reserved 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>
---
The name of the device tree property was chosen because we want to
reuse as much as the device tree parsing helpers from the static
memory allocation feature, but we would like to hear the upstream
reviewers' opinion about if using "xen,static-heap" is better.
---
Changes from RFC to v1:
- Rename the terminology to reserved heap.
---
 docs/misc/arm/device-tree/booting.txt | 46 +++++++++++++++++++++++++
 xen/arch/arm/bootfdt.c                | 49 +++++++++++++++++++++------
 xen/arch/arm/domain_build.c           |  5 +--
 xen/arch/arm/include/asm/setup.h      |  1 +
 4 files changed, 89 insertions(+), 12 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..e064f64d9a 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -378,3 +378,49 @@ device-tree:
 
 This will reserve a 512MB region starting at the host physical address
 0x30000000 to be exclusively used by DomU1.
+
+
+Reserved Heap Memory
+====================
+
+The reserved heap memory (also known as the statically-configured heap) refers
+to parts of RAM reserved in the beginning for heap. The memory is reserved by
+configuration in the device tree using physical address ranges.
+
+The reserved 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 can be seperated heaps, the reserved heap will be used
+for both domheap and xenheap.
+- For Arm64, since domheap and xenheap are the same, the defined reserved heap
+areas shall always go to the heap allocator.
+
+The reserved heap memory is an optional feature and can be enabled by adding a
+device tree property in the `chosen` node. Currently, this feature reuses the
+static memory allocation device tree configuration.
+
+The dtb property should look like as follows:
+
+- property name
+
+    "xen,static-mem" (Should be used in the `chosen` node)
+
+- cells
+
+    Specify the start address and the length of the reserved heap memory.
+    The number of cells for the address and the size should be defined
+    using the properties `#xen,static-mem-address-cells` and
+    `#xen,static-mem-size-cells` respectively.
+
+Below is an example on how to specify the reserved heap in device tree:
+
+    / {
+        chosen {
+            #xen,static-mem-address-cells = <0x2>;
+            #xen,static-mem-size-cells = <0x2>;
+            xen,static-mem = <0x0 0x30000000 0x0 0x40000000>;
+        };
+    };
+
+RAM at 0x30000000 of 1G size will be reserved as heap.
+
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index ec81a45de9..33704ca487 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -64,7 +64,8 @@ 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, bool xen_domain,
+                                          bool xen_heap)
 {
     const struct fdt_property *prop;
     unsigned int i, banks;
@@ -96,6 +97,7 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
         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].xen_heap = xen_heap;
         mem->nr_banks++;
     }
 
@@ -185,7 +187,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, false, false);
 }
 
 static int __init process_reserved_memory_node(const void *fdt, int node,
@@ -293,7 +295,7 @@ 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,
+static int __init process_chosen_node(const void *fdt, int node,
                                        const char *name,
                                        u32 address_cells, u32 size_cells)
 {
@@ -301,16 +303,40 @@ static void __init process_chosen_node(const void *fdt, int node,
     paddr_t start, end;
     int len;
 
+    if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
+    {
+        u32 address_cells = device_tree_get_u32(fdt, node,
+                                                "#xen,static-mem-address-cells",
+                                                0);
+        u32 size_cells = device_tree_get_u32(fdt, node,
+                                             "#xen,static-mem-size-cells", 0);
+        int rc;
+
+        printk("Checking for reserved heap in /chosen\n");
+        if ( address_cells < 1 || size_cells < 1 )
+        {
+            printk("fdt: node `%s': invalid #xen,static-mem-address-cells or #xen,static-mem-size-cells\n",
+                   name);
+            return -EINVAL;
+        }
+
+        rc = device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
+                                     size_cells, &bootinfo.reserved_mem, false,
+                                     true);
+        if ( rc )
+            return rc;
+    }
+
     printk("Checking for initrd in /chosen\n");
 
     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 +344,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 +357,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,
@@ -358,7 +386,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, true,
+                                   false);
 }
 
 static int __init early_scan_node(const void *fdt,
@@ -383,7 +412,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);
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3fd1186b53..6f97f5f06a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1038,8 +1038,9 @@ 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 first memory range not bound to neither a Xen domain nor heap */
+    for ( i = 0; i < mem->nr_banks &&
+                 (mem->bank[i].xen_domain || mem->bank[i].xen_heap); i++ )
         ;
     if ( i == mem->nr_banks )
         return 0;
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 2bb01ecfa8..e80f3d6201 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -27,6 +27,7 @@ struct membank {
     paddr_t start;
     paddr_t size;
     bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
+    bool xen_heap;   /* whether the memory bank is reserved as heap. */
 };
 
 struct meminfo {
-- 
2.17.1

[PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

This commit firstly adds a global variable `reserved_heap`.
This newly introduced global variable is set at the device tree
parsing time if the reserved heap ranges are defined in the device
tree chosen node.

For Arm32, In `setup_mm`, if the reserved heap is enabled, we use
the reserved heap region for both domheap and xenheap allocation.

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

If the reserved 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()`.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
With reserved heap enabled, for Arm64, naming of global variables such
as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous,
wondering if we should rename these variables.
---
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 |  2 +
 xen/arch/arm/setup.c             | 79 +++++++++++++++++++++++++-------
 3 files changed, 67 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 33704ca487..ab73b6e212 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt, int node,
                                      true);
         if ( rc )
             return rc;
+
+        reserved_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 e80f3d6201..00536a6d55 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -92,6 +92,8 @@ extern struct bootinfo bootinfo;
 
 extern domid_t max_init_domid;
 
+extern bool reserved_heap;
+
 void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
 
 size_t estimate_efi_size(unsigned int mem_nr_banks);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 500307edc0..fe76cf6325 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
 
 domid_t __read_mostly max_init_domid;
 
+bool __read_mostly reserved_heap;
+
 static __used void init_done(void)
 {
     /* Must be done past setting system_state. */
@@ -699,8 +701,10 @@ 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 reserved_heap_start = ~0, reserved_heap_end = 0,
+            reserved_heap_size = 0;
+    unsigned long ram_pages, reserved_heap_pages = 0;
     unsigned long heap_pages, xenheap_pages, domheap_pages;
     unsigned int i;
     const uint32_t ctr = READ_CP32(CTR);
@@ -720,9 +724,9 @@ 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);
@@ -731,6 +735,25 @@ static void __init setup_mm(void)
 
     total_pages = ram_pages = ram_size >> PAGE_SHIFT;
 
+    if ( reserved_heap )
+    {
+        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+        {
+            if ( bootinfo.reserved_mem.bank[i].xen_heap )
+            {
+                bank_start = bootinfo.reserved_mem.bank[i].start;
+                bank_size = bootinfo.reserved_mem.bank[i].size;
+                bank_end = bank_start + bank_size;
+
+                reserved_heap_size += bank_size;
+                reserved_heap_start = min(reserved_heap_start, bank_start);
+                reserved_heap_end = max(reserved_heap_end, bank_end);
+            }
+        }
+
+        reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT;
+    }
+
     /*
      * If the user has not requested otherwise via the command line
      * then locate the xenheap using these constraints:
@@ -743,7 +766,8 @@ static void __init setup_mm(void)
      * We try to allocate the largest xenheap possible within these
      * constraints.
      */
-    heap_pages = ram_pages;
+    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
+
     if ( opt_xenheap_megabytes )
         xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
     else
@@ -755,17 +779,21 @@ static void __init setup_mm(void)
 
     do
     {
-        e = consider_modules(ram_start, ram_end,
+        e = !reserved_heap ?
+            consider_modules(ram_start, ram_end,
                              pfn_to_paddr(xenheap_pages),
-                             32<<20, 0);
+                             32<<20, 0) :
+            reserved_heap_end;
+
         if ( e )
             break;
 
         xenheap_pages >>= 1;
     } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
 
-    if ( ! e )
-        panic("Not not enough space for xenheap\n");
+    if ( ! e ||
+         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
+        panic("Not enough space for xenheap\n");
 
     domheap_pages = heap_pages - xenheap_pages;
 
@@ -810,9 +838,9 @@ 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_end = 0;
-    paddr_t ram_size = 0;
+    paddr_t ram_start = ~0, bank_start = ~0;
+    paddr_t ram_end = 0, bank_end = 0;
+    paddr_t ram_size = 0, bank_size = 0;
     unsigned int i;
 
     init_pdx();
@@ -821,17 +849,36 @@ static void __init setup_mm(void)
      * We need some memory to allocate the page-tables used for the xenheap
      * mappings. But some regions may contain memory already allocated
      * for other uses (e.g. modules, reserved-memory...).
-     *
+     * If reserved heap regions are properly defined, (only) add these regions
+     * in the boot allocator.
+     */
+    if ( reserved_heap )
+    {
+        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+        {
+            if ( bootinfo.reserved_mem.bank[i].xen_heap )
+            {
+                bank_start = bootinfo.reserved_mem.bank[i].start;
+                bank_size = bootinfo.reserved_mem.bank[i].size;
+                bank_end = bank_start + bank_size;
+
+                init_boot_pages(bank_start, bank_end);
+            }
+        }
+    }
+    /*
+     * No reserved heap regions:
      * For simplicity, add all the free regions in the boot allocator.
      */
-    populate_boot_allocator();
+    else
+        populate_boot_allocator();
 
     total_pages = 0;
 
     for ( i = 0; i < banks->nr_banks; i++ )
     {
         const struct membank *bank = &banks->bank[i];
-        paddr_t bank_end = bank->start + bank->size;
+        bank_end = bank->start + bank->size;
 
         ram_size = ram_size + bank->size;
         ram_start = min(ram_start, bank->start);
-- 
2.17.1

Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Michal Orzel]

Hi Henry,

On 24/08/2022 09:31, Henry Wang wrote:
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..e064f64d9a 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -378,3 +378,49 @@ device-tree:
> 
>  This will reserve a 512MB region starting at the host physical address
>  0x30000000 to be exclusively used by DomU1.
> +
> +
> +Reserved Heap Memory
> +====================
> +
> +The reserved heap memory (also known as the statically-configured heap) refers
> +to parts of RAM reserved in the beginning for heap. The memory is reserved by
I think we are missing "... in the beginning" of what.

> +configuration in the device tree using physical address ranges.
> +
> +The reserved 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 can be seperated heaps, the reserved heap will be used
Maybe "there are" instead of "there can be" as we do define for Arm32:
#define CONFIG_SEPARATE_XENHEAP 1
and I do not think we have some flexibility to change this.

> +for both domheap and xenheap.
> +- For Arm64, since domheap and xenheap are the same, the defined reserved heap
Instead of writing "since domheap and xenheap are the same" maybe it'd be better to write:
"For Arm64, as there is a single heap..."

> +areas shall always go to the heap allocator.
> +
> +The reserved heap memory is an optional feature and can be enabled by adding a
> +device tree property in the `chosen` node. Currently, this feature reuses the
> +static memory allocation device tree configuration.
> +
> +The dtb property should look like as follows:
> +
> +- property name
> +
> +    "xen,static-mem" (Should be used in the `chosen` node)
> +
> +- cells
> +
> +    Specify the start address and the length of the reserved heap memory.
> +    The number of cells for the address and the size should be defined
> +    using the properties `#xen,static-mem-address-cells` and
> +    `#xen,static-mem-size-cells` respectively.
> +
> +Below is an example on how to specify the reserved heap in device tree:
> +
> +    / {
> +        chosen {
> +            #xen,static-mem-address-cells = <0x2>;
> +            #xen,static-mem-size-cells = <0x2>;
> +            xen,static-mem = <0x0 0x30000000 0x0 0x40000000>;
Please add "..." here as this does not represent the complete working chosen node.

> +        };
> +    };
> +
> +RAM at 0x30000000 of 1G size will be reserved as heap.
> +
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index ec81a45de9..33704ca487 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -64,7 +64,8 @@ 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, bool xen_domain,
> +                                          bool xen_heap)
>  {
>      const struct fdt_property *prop;
>      unsigned int i, banks;
> @@ -96,6 +97,7 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
>          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].xen_heap = xen_heap;
>          mem->nr_banks++;
>      }
> 
> @@ -185,7 +187,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, false, false);
>  }
> 
>  static int __init process_reserved_memory_node(const void *fdt, int node,
> @@ -293,7 +295,7 @@ 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,
> +static int __init process_chosen_node(const void *fdt, int node,
You do not really need to change the return type of this function.
Currently process_chosen_node just returns on an error condition so you could do the same.

>                                         const char *name,
>                                         u32 address_cells, u32 size_cells)
>  {
> @@ -301,16 +303,40 @@ static void __init process_chosen_node(const void *fdt, int node,
>      paddr_t start, end;
>      int len;
> 
> +    if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
> +    {
> +        u32 address_cells = device_tree_get_u32(fdt, node,
> +                                                "#xen,static-mem-address-cells",
> +                                                0);
> +        u32 size_cells = device_tree_get_u32(fdt, node,
> +                                             "#xen,static-mem-size-cells", 0);
> +        int rc;
> +
> +        printk("Checking for reserved heap in /chosen\n");
> +        if ( address_cells < 1 || size_cells < 1 )
address_cells and size_cells cannot be negative so you could just check if there are 0.

> +        {
> +            printk("fdt: node `%s': invalid #xen,static-mem-address-cells or #xen,static-mem-size-cells\n",
> +                   name);
> +            return -EINVAL;
> +        }
> +
> +        rc = device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
> +                                     size_cells, &bootinfo.reserved_mem, false,
> +                                     true);
> +        if ( rc )
> +            return rc;
> +    }
> +
>      printk("Checking for initrd in /chosen\n");
> 
>      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;
This change breaks the current behavior and will result in triggering the printk in early_scan_node for parsing failure.
Is this intended? If so, you could mention this in the commit msg.

>      }
>      start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> 
> @@ -318,12 +344,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 +357,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,
> @@ -358,7 +386,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, true,
> +                                   false);
>  }
> 
>  static int __init early_scan_node(const void *fdt,
> @@ -383,7 +412,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);
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3fd1186b53..6f97f5f06a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1038,8 +1038,9 @@ 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 first memory range not bound to neither a Xen domain nor heap */
> +    for ( i = 0; i < mem->nr_banks &&
> +                 (mem->bank[i].xen_domain || mem->bank[i].xen_heap); i++ )
>          ;
Could you please add an empty line here to improve readability?

>      if ( i == mem->nr_banks )
>          return 0;
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 2bb01ecfa8..e80f3d6201 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -27,6 +27,7 @@ struct membank {
>      paddr_t start;
>      paddr_t size;
>      bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
> +    bool xen_heap;   /* whether the memory bank is reserved as heap. */
>  };
> 
>  struct meminfo {
> --
> 2.17.1
> 
> 

~Michal

RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Henry Wang]

Hi Michal,

It is great to hear from you! Hope you are doing well.

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> Subject: Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory
> Hi Henry,
> > +to parts of RAM reserved in the beginning for heap. The memory is
> reserved by
> I think we are missing "... in the beginning" of what.

Correct, I will change it to "... in the beginning of boot time".

> 
> > +configuration in the device tree using physical address ranges.
> > +
> > +The reserved 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 can be seperated heaps, the reserved heap will
> be used
> Maybe "there are" instead of "there can be" as we do define for Arm32:
> #define CONFIG_SEPARATE_XENHEAP 1
> and I do not think we have some flexibility to change this.

Ack.

> 
> > +for both domheap and xenheap.
> > +- For Arm64, since domheap and xenheap are the same, the defined
> reserved heap
> Instead of writing "since domheap and xenheap are the same" maybe it'd be
> better to write:
> "For Arm64, as there is a single heap..."

Yep, will change in v2.

> 
> > +areas shall always go to the heap allocator.
> > +
> > +The reserved heap memory is an optional feature and can be enabled by
> adding a
> > +device tree property in the `chosen` node. Currently, this feature reuses
> the
> > +static memory allocation device tree configuration.
> > +
> > +The dtb property should look like as follows:
> > +
> > +- property name
> > +
> > +    "xen,static-mem" (Should be used in the `chosen` node)
> > +
> > +- cells
> > +
> > +    Specify the start address and the length of the reserved heap memory.
> > +    The number of cells for the address and the size should be defined
> > +    using the properties `#xen,static-mem-address-cells` and
> > +    `#xen,static-mem-size-cells` respectively.
> > +
> > +Below is an example on how to specify the reserved heap in device tree:
> > +
> > +    / {
> > +        chosen {
> > +            #xen,static-mem-address-cells = <0x2>;
> > +            #xen,static-mem-size-cells = <0x2>;
> > +            xen,static-mem = <0x0 0x30000000 0x0 0x40000000>;
> Please add "..." here as this does not represent the complete working chosen
> node.

Sure, will add in v2.

> 
> > +        };
> > +    };
> > +
> > +RAM at 0x30000000 of 1G size will be reserved as heap.
> > +
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index ec81a45de9..33704ca487 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -64,7 +64,8 @@ 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, bool xen_domain,
> > +                                          bool xen_heap)
> >  {
> >      const struct fdt_property *prop;
> >      unsigned int i, banks;
> > @@ -96,6 +97,7 @@ static int __init device_tree_get_meminfo(const void
> *fdt, int node,
> >          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].xen_heap = xen_heap;
> >          mem->nr_banks++;
> >      }
> >
> > @@ -185,7 +187,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, false, false);
> >  }
> >
> >  static int __init process_reserved_memory_node(const void *fdt, int node,
> > @@ -293,7 +295,7 @@ 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,
> > +static int __init process_chosen_node(const void *fdt, int node,
> You do not really need to change the return type of this function.
> Currently process_chosen_node just returns on an error condition so you
> could do the same.

Thanks for pointing this out, will do in v2.

> 
> >                                         const char *name,
> >                                         u32 address_cells, u32 size_cells)
> >  {
> > @@ -301,16 +303,40 @@ static void __init process_chosen_node(const
> void *fdt, int node,
> >      paddr_t start, end;
> >      int len;
> >
> > +    if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
> > +    {
> > +        u32 address_cells = device_tree_get_u32(fdt, node,
> > +                                                "#xen,static-mem-address-cells",
> > +                                                0);
> > +        u32 size_cells = device_tree_get_u32(fdt, node,
> > +                                             "#xen,static-mem-size-cells", 0);
> > +        int rc;
> > +
> > +        printk("Checking for reserved heap in /chosen\n");
> > +        if ( address_cells < 1 || size_cells < 1 )
> address_cells and size_cells cannot be negative so you could just check if
> there are 0.

In bootfdt.c function device_tree_get_meminfo(), the address and size cells
are checked using <1 instead of =0. I agree they cannot be negative, but I am
not very sure if there were other reasons to do the "<1" check in
device_tree_get_meminfo(). Are you fine with we don't keep the consistency
here?

> 
> > +        {
> > +            printk("fdt: node `%s': invalid #xen,static-mem-address-cells or
> #xen,static-mem-size-cells\n",
> > +                   name);
> > +            return -EINVAL;
> > +        }
> > +
> > +        rc = device_tree_get_meminfo(fdt, node, "xen,static-mem",
> address_cells,
> > +                                     size_cells, &bootinfo.reserved_mem, false,
> > +                                     true);
> > +        if ( rc )
> > +            return rc;
> > +    }
> > +
> >      printk("Checking for initrd in /chosen\n");
> >
> >      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;
> This change breaks the current behavior and will result in triggering the
> printk in early_scan_node for parsing failure.
> Is this intended? If so, you could mention this in the commit msg.

I think I will follow your advice above for the return type so here we won't
have any changes in v2.

> 
> >      }
> >      start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> >
> > @@ -318,12 +344,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 +357,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,
> > @@ -358,7 +386,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, true,
> > +                                   false);
> >  }
> >
> >  static int __init early_scan_node(const void *fdt,
> > @@ -383,7 +412,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);
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 3fd1186b53..6f97f5f06a 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1038,8 +1038,9 @@ 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 first memory range not bound to neither a Xen domain nor heap
> */
> > +    for ( i = 0; i < mem->nr_banks &&
> > +                 (mem->bank[i].xen_domain || mem->bank[i].xen_heap); i++ )
> >          ;
> Could you please add an empty line here to improve readability?

Sure.

Kind regards,
Henry

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Michal Orzel]

Hi Henry,

On 24/08/2022 09:31, Henry Wang wrote:
> 
> This commit firstly adds a global variable `reserved_heap`.
> This newly introduced global variable is set at the device tree
> parsing time if the reserved heap ranges are defined in the device
> tree chosen node.
> 
Did you consider putting reserved_heap into bootinfo structure?
It would help to avoid introducing new global variables that are only used
in places making use of the bootinfo anyway.

> For Arm32, In `setup_mm`, if the reserved heap is enabled, we use
> the reserved heap region for both domheap and xenheap allocation.
> 
> For Arm64, In `setup_mm`, if the reserved heap is enabled and used,
> we make sure that only these reserved heap pages are added to the
> boot allocator. These reserved heap pages in the boot allocator are
> added to the heap allocator at `end_boot_allocator()`.
> 
> If the reserved 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()`.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> ---
> With reserved heap enabled, for Arm64, naming of global variables such
> as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous,
> wondering if we should rename these variables.
> ---
> 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 |  2 +
>  xen/arch/arm/setup.c             | 79 +++++++++++++++++++++++++-------
>  3 files changed, 67 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 33704ca487..ab73b6e212 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt, int node,
>                                       true);
>          if ( rc )
>              return rc;
> +
> +        reserved_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 e80f3d6201..00536a6d55 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo;
> 
>  extern domid_t max_init_domid;
> 
> +extern bool reserved_heap;
> +
>  void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> 
>  size_t estimate_efi_size(unsigned int mem_nr_banks);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 500307edc0..fe76cf6325 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
> 
>  domid_t __read_mostly max_init_domid;
> 
> +bool __read_mostly reserved_heap;
> +
>  static __used void init_done(void)
>  {
>      /* Must be done past setting system_state. */
> @@ -699,8 +701,10 @@ 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 reserved_heap_start = ~0, reserved_heap_end = 0,
> +            reserved_heap_size = 0;
> +    unsigned long ram_pages, reserved_heap_pages = 0;
>      unsigned long heap_pages, xenheap_pages, domheap_pages;
>      unsigned int i;
>      const uint32_t ctr = READ_CP32(CTR);
> @@ -720,9 +724,9 @@ 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);
> @@ -731,6 +735,25 @@ static void __init setup_mm(void)
> 
>      total_pages = ram_pages = ram_size >> PAGE_SHIFT;
> 
> +    if ( reserved_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> +            {
> +                bank_start = bootinfo.reserved_mem.bank[i].start;
> +                bank_size = bootinfo.reserved_mem.bank[i].size;
> +                bank_end = bank_start + bank_size;
> +
> +                reserved_heap_size += bank_size;
> +                reserved_heap_start = min(reserved_heap_start, bank_start);
You do not need reserved_heap_start as you do not use it at any place later on.
In your current implementation you just need reserved_heap_size and reserved_heap_end.

> +                reserved_heap_end = max(reserved_heap_end, bank_end);
> +            }
> +        }
> +
> +        reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT;
> +    }
> +
>      /*
>       * If the user has not requested otherwise via the command line
>       * then locate the xenheap using these constraints:
> @@ -743,7 +766,8 @@ static void __init setup_mm(void)
>       * We try to allocate the largest xenheap possible within these
>       * constraints.
>       */
> -    heap_pages = ram_pages;
> +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
I must say that the reverted logic is harder to read. This is a matter of taste but
please consider the following:
heap_pages = reserved_heap ? reserved_heap_pages : ram_pages;
The same applies to ...

> +
>      if ( opt_xenheap_megabytes )
>          xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
>      else
> @@ -755,17 +779,21 @@ static void __init setup_mm(void)
> 
>      do
>      {
> -        e = consider_modules(ram_start, ram_end,
> +        e = !reserved_heap ?
... here.

> +            consider_modules(ram_start, ram_end,
>                               pfn_to_paddr(xenheap_pages),
> -                             32<<20, 0);
> +                             32<<20, 0) :
> +            reserved_heap_end;
> +
>          if ( e )
>              break;
> 
>          xenheap_pages >>= 1;
>      } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
> 
> -    if ( ! e )
> -        panic("Not not enough space for xenheap\n");
> +    if ( ! e ||
> +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
I'm not sure about this. You are checking if the size of the reserved heap is less than 32MB
and this has nothing to do with the following panic message.

> +        panic("Not enough space for xenheap\n");
> 
>      domheap_pages = heap_pages - xenheap_pages;
> 
> @@ -810,9 +838,9 @@ 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_end = 0;
> -    paddr_t ram_size = 0;
> +    paddr_t ram_start = ~0, bank_start = ~0;
> +    paddr_t ram_end = 0, bank_end = 0;
> +    paddr_t ram_size = 0, bank_size = 0;
>      unsigned int i;
> 
>      init_pdx();
> @@ -821,17 +849,36 @@ static void __init setup_mm(void)
>       * We need some memory to allocate the page-tables used for the xenheap
>       * mappings. But some regions may contain memory already allocated
>       * for other uses (e.g. modules, reserved-memory...).
> -     *
> +     * If reserved heap regions are properly defined, (only) add these regions
How can you say at this stage whether the reserved heap regions are defined properly?

> +     * in the boot allocator.
> +     */
> +    if ( reserved_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> +            {
> +                bank_start = bootinfo.reserved_mem.bank[i].start;
> +                bank_size = bootinfo.reserved_mem.bank[i].size;
> +                bank_end = bank_start + bank_size;
> +
> +                init_boot_pages(bank_start, bank_end);
> +            }
> +        }
> +    }
> +    /*
> +     * No reserved heap regions:
>       * For simplicity, add all the free regions in the boot allocator.
>       */
> -    populate_boot_allocator();
> +    else
> +        populate_boot_allocator();
> 
>      total_pages = 0;
> 
>      for ( i = 0; i < banks->nr_banks; i++ )
>      {
>          const struct membank *bank = &banks->bank[i];
> -        paddr_t bank_end = bank->start + bank->size;
> +        bank_end = bank->start + bank->size;
> 
>          ram_size = ram_size + bank->size;
>          ram_start = min(ram_start, bank->start);
> --
> 2.17.1
> 
> 

~Michal

Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Stefano Stabellini]

On Wed, 24 Aug 2022, Henry Wang wrote:
> This commit introduces the reserved heap memory, which is parts of RAM
> reserved in the beginning of the boot time for heap.
> 
> A new boolean field `xen_heap` in `struct membank` is added to store the
> configuration telling if the memory bank is reserved as heap through
> `xen,static-mem` property in device tree `chosen` node.
> 
> Also, this commit introduces the logic to parse the reserved heap
> configuation in device tree by reusing the device tree entry definition
> of the static memory allocation feature:
> 
> - Add a boolean parameter `xen_heap` to `device_tree_get_meminfo` to
> reflect whether the memory bank is reserved as heap.
> 
> - Use `device_tree_get_meminfo` to parse the reserved heap configuation
> in `chosen` node of the device tree.
> 
> - In order to reuse the function `device_tree_get_meminfo`, the
> return type of `process_chosen_node` is changed from void to int.
> 
> A documentation section is added, describing the definition of reserved
> heap memory and the method of enabling the reserved 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>
> ---
> The name of the device tree property was chosen because we want to
> reuse as much as the device tree parsing helpers from the static
> memory allocation feature, but we would like to hear the upstream
> reviewers' opinion about if using "xen,static-heap" is better.
> ---
> Changes from RFC to v1:
> - Rename the terminology to reserved heap.
> ---
>  docs/misc/arm/device-tree/booting.txt | 46 +++++++++++++++++++++++++
>  xen/arch/arm/bootfdt.c                | 49 +++++++++++++++++++++------
>  xen/arch/arm/domain_build.c           |  5 +--
>  xen/arch/arm/include/asm/setup.h      |  1 +
>  4 files changed, 89 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..e064f64d9a 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -378,3 +378,49 @@ device-tree:
>  
>  This will reserve a 512MB region starting at the host physical address
>  0x30000000 to be exclusively used by DomU1.
> +
> +
> +Reserved Heap Memory
> +====================
> +
> +The reserved heap memory (also known as the statically-configured heap) refers
> +to parts of RAM reserved in the beginning for heap. The memory is reserved by
> +configuration in the device tree using physical address ranges.
> +
> +The reserved 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 can be seperated heaps, the reserved heap will be used
> +for both domheap and xenheap.
> +- For Arm64, since domheap and xenheap are the same, the defined reserved heap
> +areas shall always go to the heap allocator.
> +
> +The reserved heap memory is an optional feature and can be enabled by adding a
> +device tree property in the `chosen` node. Currently, this feature reuses the
> +static memory allocation device tree configuration.
> +
> +The dtb property should look like as follows:
> +
> +- property name
> +
> +    "xen,static-mem" (Should be used in the `chosen` node)
> +
> +- cells
> +
> +    Specify the start address and the length of the reserved heap memory.
> +    The number of cells for the address and the size should be defined
> +    using the properties `#xen,static-mem-address-cells` and
> +    `#xen,static-mem-size-cells` respectively.

I would choose a different name for the property not to be confused with
a domain's xen,static-mem property which is for a different purpose: the
memory of the domain.

---

- xen,static-heap

    Property under the top-level "chosen" node. It specifies the address
    and size of Xen reserved heap memory.


- #xen,static-heap-address-cells and #xen,static-heap-size-cells

    Specify the number of cells used for the address and size of the
    xen,static-heap property under "chosen".

---

RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Stefano Stabellini]

On Thu, 25 Aug 2022, Henry Wang wrote:
> > >                                         const char *name,
> > >                                         u32 address_cells, u32 size_cells)
> > >  {
> > > @@ -301,16 +303,40 @@ static void __init process_chosen_node(const
> > void *fdt, int node,
> > >      paddr_t start, end;
> > >      int len;
> > >
> > > +    if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
> > > +    {
> > > +        u32 address_cells = device_tree_get_u32(fdt, node,
> > > +                                                "#xen,static-mem-address-cells",
> > > +                                                0);
> > > +        u32 size_cells = device_tree_get_u32(fdt, node,
> > > +                                             "#xen,static-mem-size-cells", 0);
> > > +        int rc;
> > > +
> > > +        printk("Checking for reserved heap in /chosen\n");
> > > +        if ( address_cells < 1 || size_cells < 1 )
> > address_cells and size_cells cannot be negative so you could just check if
> > there are 0.
> 
> In bootfdt.c function device_tree_get_meminfo(), the address and size cells
> are checked using <1 instead of =0. I agree they cannot be negative, but I am
> not very sure if there were other reasons to do the "<1" check in
> device_tree_get_meminfo(). Are you fine with we don't keep the consistency
> here?

I would keep the < 1 check but it doesn't make much difference either
way

RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Henry Wang]

Hi Stefano and Michal,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: Tuesday, August 30, 2022 8:47 AM
> To: Henry Wang <Henry.Wang@arm.com>
> Cc: Michal Orzel <michal.orzel@amd.com>; xen-devel@lists.xenproject.org;
> Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>;
> Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Penny Zheng <Penny.Zheng@arm.com>
> Subject: RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory
> 
> On Thu, 25 Aug 2022, Henry Wang wrote:
> > > >                                         const char *name,
> > > >                                         u32 address_cells, u32 size_cells)
> > > >  {
> > > > @@ -301,16 +303,40 @@ static void __init process_chosen_node(const
> > > void *fdt, int node,
> > > >      paddr_t start, end;
> > > >      int len;
> > > >
> > > > +    if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
> > > > +    {
> > > > +        u32 address_cells = device_tree_get_u32(fdt, node,
> > > > +                                                "#xen,static-mem-address-cells",
> > > > +                                                0);
> > > > +        u32 size_cells = device_tree_get_u32(fdt, node,
> > > > +                                             "#xen,static-mem-size-cells", 0);
> > > > +        int rc;
> > > > +
> > > > +        printk("Checking for reserved heap in /chosen\n");
> > > > +        if ( address_cells < 1 || size_cells < 1 )
> > > address_cells and size_cells cannot be negative so you could just check if
> > > there are 0.
> >
> > In bootfdt.c function device_tree_get_meminfo(), the address and size cells
> > are checked using <1 instead of =0. I agree they cannot be negative, but I
> am
> > not very sure if there were other reasons to do the "<1" check in
> > device_tree_get_meminfo(). Are you fine with we don't keep the
> consistency
> > here?
> 
> I would keep the < 1 check but it doesn't make much difference either
> way

I also would prefer to keep these two places consistent and I agree Michal is
making a good point.

Kind regards,
Henry

RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Henry Wang]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory
> 
> On Wed, 24 Aug 2022, Henry Wang wrote:
> > ---
> > The name of the device tree property was chosen because we want to
> > reuse as much as the device tree parsing helpers from the static
> > memory allocation feature, but we would like to hear the upstream
> > reviewers' opinion about if using "xen,static-heap" is better.
> > ---
> > +The dtb property should look like as follows:
> > +
> > +- property name
> > +
> > +    "xen,static-mem" (Should be used in the `chosen` node)
> > +
> > +- cells
> > +
> > +    Specify the start address and the length of the reserved heap memory.
> > +    The number of cells for the address and the size should be defined
> > +    using the properties `#xen,static-mem-address-cells` and
> > +    `#xen,static-mem-size-cells` respectively.
> 
> I would choose a different name for the property not to be confused with
> a domain's xen,static-mem property which is for a different purpose: the
> memory of the domain.

Sure, thank you for the input. I will correct these in v2.

> 
> ---
> 
> - xen,static-heap
> 
>     Property under the top-level "chosen" node. It specifies the address
>     and size of Xen reserved heap memory.
> 
> 
> - #xen,static-heap-address-cells and #xen,static-heap-size-cells
> 
>     Specify the number of cells used for the address and size of the
>     xen,static-heap property under "chosen".

Thanks for these!

Kind regards,
Henry

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Stefano Stabellini]

On Wed, 24 Aug 2022, Henry Wang wrote:
> This commit firstly adds a global variable `reserved_heap`.
> This newly introduced global variable is set at the device tree
> parsing time if the reserved heap ranges are defined in the device
> tree chosen node.
> 
> For Arm32, In `setup_mm`, if the reserved heap is enabled, we use
> the reserved heap region for both domheap and xenheap allocation.
> 
> For Arm64, In `setup_mm`, if the reserved heap is enabled and used,
> we make sure that only these reserved heap pages are added to the
> boot allocator. These reserved heap pages in the boot allocator are
> added to the heap allocator at `end_boot_allocator()`.
> 
> If the reserved 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()`.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> ---
> With reserved heap enabled, for Arm64, naming of global variables such
> as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous,
> wondering if we should rename these variables.
> ---
> 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 |  2 +
>  xen/arch/arm/setup.c             | 79 +++++++++++++++++++++++++-------
>  3 files changed, 67 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 33704ca487..ab73b6e212 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt, int node,
>                                       true);
>          if ( rc )
>              return rc;
> +
> +        reserved_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 e80f3d6201..00536a6d55 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo;
>  
>  extern domid_t max_init_domid;
>  
> +extern bool reserved_heap;
> +
>  void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
>  
>  size_t estimate_efi_size(unsigned int mem_nr_banks);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 500307edc0..fe76cf6325 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>  
>  domid_t __read_mostly max_init_domid;
>  
> +bool __read_mostly reserved_heap;
> +
>  static __used void init_done(void)
>  {
>      /* Must be done past setting system_state. */
> @@ -699,8 +701,10 @@ 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 reserved_heap_start = ~0, reserved_heap_end = 0,

INVALID_PADDR or ~0ULL


> +            reserved_heap_size = 0;
> +    unsigned long ram_pages, reserved_heap_pages = 0;
>      unsigned long heap_pages, xenheap_pages, domheap_pages;
>      unsigned int i;
>      const uint32_t ctr = READ_CP32(CTR);
> @@ -720,9 +724,9 @@ 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);
> @@ -731,6 +735,25 @@ static void __init setup_mm(void)
>  
>      total_pages = ram_pages = ram_size >> PAGE_SHIFT;
>  
> +    if ( reserved_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> +            {
> +                bank_start = bootinfo.reserved_mem.bank[i].start;
> +                bank_size = bootinfo.reserved_mem.bank[i].size;
> +                bank_end = bank_start + bank_size;
> +
> +                reserved_heap_size += bank_size;
> +                reserved_heap_start = min(reserved_heap_start, bank_start);
> +                reserved_heap_end = max(reserved_heap_end, bank_end);
> +            }
> +        }
> +
> +        reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT;
> +    }
> +
>      /*
>       * If the user has not requested otherwise via the command line
>       * then locate the xenheap using these constraints:
> @@ -743,7 +766,8 @@ static void __init setup_mm(void)
>       * We try to allocate the largest xenheap possible within these
>       * constraints.
>       */
> -    heap_pages = ram_pages;
> +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> +
>      if ( opt_xenheap_megabytes )
>          xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
>      else
> @@ -755,17 +779,21 @@ static void __init setup_mm(void)
>  
>      do
>      {
> -        e = consider_modules(ram_start, ram_end,
> +        e = !reserved_heap ?
> +            consider_modules(ram_start, ram_end,
>                               pfn_to_paddr(xenheap_pages),
> -                             32<<20, 0);
> +                             32<<20, 0) :
> +            reserved_heap_end;
> +
>          if ( e )
>              break;
>  
>          xenheap_pages >>= 1;
>      } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
>  
> -    if ( ! e )
> -        panic("Not not enough space for xenheap\n");
> +    if ( ! e ||
> +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
> +        panic("Not enough space for xenheap\n");


I would skip the do/while loop completely if reserved_heap. We don't
need it anyway and we can automatically calculate xenheap_pages in a
single line.



>      domheap_pages = heap_pages - xenheap_pages;
>  
> @@ -810,9 +838,9 @@ 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_end = 0;
> -    paddr_t ram_size = 0;
> +    paddr_t ram_start = ~0, bank_start = ~0;
> +    paddr_t ram_end = 0, bank_end = 0;
> +    paddr_t ram_size = 0, bank_size = 0;
>      unsigned int i;

Please use INVALID_PADDR or ~0ULL


>  
>      init_pdx();
> @@ -821,17 +849,36 @@ static void __init setup_mm(void)
>       * We need some memory to allocate the page-tables used for the xenheap
>       * mappings. But some regions may contain memory already allocated
>       * for other uses (e.g. modules, reserved-memory...).
> -     *
> +     * If reserved heap regions are properly defined, (only) add these regions
> +     * in the boot allocator.
> +     */
> +    if ( reserved_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> +            {
> +                bank_start = bootinfo.reserved_mem.bank[i].start;
> +                bank_size = bootinfo.reserved_mem.bank[i].size;
> +                bank_end = bank_start + bank_size;
> +
> +                init_boot_pages(bank_start, bank_end);
> +            }
> +        }
> +    }
> +    /*
> +     * No reserved heap regions:
>       * For simplicity, add all the free regions in the boot allocator.
>       */
> -    populate_boot_allocator();
> +    else
> +        populate_boot_allocator();
>  
>      total_pages = 0;
>  
>      for ( i = 0; i < banks->nr_banks; i++ )
>      {
>          const struct membank *bank = &banks->bank[i];
> -        paddr_t bank_end = bank->start + bank->size;
> +        bank_end = bank->start + bank->size;
>  
>          ram_size = ram_size + bank->size;
>          ram_start = min(ram_start, bank->start);
> -- 
> 2.17.1
> 

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Michal,

Sorry about the late reply - I had a couple of days off. Thank you very
much for the review! I will add my reply and answer some of your
questions below.

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> heap allocator
> 
> > This commit firstly adds a global variable `reserved_heap`.
> > This newly introduced global variable is set at the device tree
> > parsing time if the reserved heap ranges are defined in the device
> > tree chosen node.
> >
> Did you consider putting reserved_heap into bootinfo structure?

Actually I did, but I saw current bootinfo only contains some structs so
I was not sure if this is the preferred way, but since you are raising this
question, I will follow this method in v2.

> It would help to avoid introducing new global variables that are only used
> in places making use of the bootinfo anyway.

Ack.

> 
> > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > +            {
> > +                bank_start = bootinfo.reserved_mem.bank[i].start;
> > +                bank_size = bootinfo.reserved_mem.bank[i].size;
> > +                bank_end = bank_start + bank_size;
> > +
> > +                reserved_heap_size += bank_size;
> > +                reserved_heap_start = min(reserved_heap_start, bank_start);
> You do not need reserved_heap_start as you do not use it at any place later
> on.
> In your current implementation you just need reserved_heap_size and
> reserved_heap_end.

Good point, thank you and I will remove in v2.

> 
> >      /*
> >       * If the user has not requested otherwise via the command line
> >       * then locate the xenheap using these constraints:
> > @@ -743,7 +766,8 @@ static void __init setup_mm(void)
> >       * We try to allocate the largest xenheap possible within these
> >       * constraints.
> >       */
> > -    heap_pages = ram_pages;
> > +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> I must say that the reverted logic is harder to read. This is a matter of taste
> but
> please consider the following:
> heap_pages = reserved_heap ? reserved_heap_pages : ram_pages;
> The same applies to ...

Sure, I will use the way you suggested.

> 
> > +
> >      if ( opt_xenheap_megabytes )
> >          xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> >      else
> > @@ -755,17 +779,21 @@ static void __init setup_mm(void)
> >
> >      do
> >      {
> > -        e = consider_modules(ram_start, ram_end,
> > +        e = !reserved_heap ?
> ... here.

And here :))

> 
> > +            consider_modules(ram_start, ram_end,
> >                               pfn_to_paddr(xenheap_pages),
> > -                             32<<20, 0);
> > +                             32<<20, 0) :
> > +            reserved_heap_end;
> > +
> >          if ( e )
> >              break;
> >
> >          xenheap_pages >>= 1;
> >      } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> PAGE_SHIFT) );
> >
> > -    if ( ! e )
> > -        panic("Not not enough space for xenheap\n");
> > +    if ( ! e ||
> > +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
> I'm not sure about this. You are checking if the size of the reserved heap is
> less than 32MB
> and this has nothing to do with the following panic message.

Hmmm, I am not sure if I understand your question correctly, so here there
are actually 2 issues:
(1) The double not in the panic message.
(2) The size of xenheap.

If you check the comment of the xenheap constraints above, one rule of the
xenheap size is it "must be at least 32M". If I am not mistaken, we need to
follow the same rule with the reserved heap setup, so here we need to check
the size and if <32M then panic.

> 
> > +        panic("Not enough space for xenheap\n");
> >
> >      domheap_pages = heap_pages - xenheap_pages;
> >
> > @@ -810,9 +838,9 @@ 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_end = 0;
> > -    paddr_t ram_size = 0;
> > +    paddr_t ram_start = ~0, bank_start = ~0;
> > +    paddr_t ram_end = 0, bank_end = 0;
> > +    paddr_t ram_size = 0, bank_size = 0;
> >      unsigned int i;
> >
> >      init_pdx();
> > @@ -821,17 +849,36 @@ static void __init setup_mm(void)
> >       * We need some memory to allocate the page-tables used for the
> xenheap
> >       * mappings. But some regions may contain memory already allocated
> >       * for other uses (e.g. modules, reserved-memory...).
> > -     *
> > +     * If reserved heap regions are properly defined, (only) add these
> regions
> How can you say at this stage whether the reserved heap regions are defined
> properly?

Because if the reserved heap regions are not properly defined, in the device
tree parsing phase the global variable "reserved_heap" can never be true.

Did I understand your question correctly? Or maybe we need to change the
wording here in the comment?

Kind regards,
Henry

> 
> > +     * in the boot allocator.
> > +     */
> > +    if ( reserved_heap )
> > +    {
> > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > +            {
> > +                bank_start = bootinfo.reserved_mem.bank[i].start;
> > +                bank_size = bootinfo.reserved_mem.bank[i].size;
> > +                bank_end = bank_start + bank_size;
> > +
> > +                init_boot_pages(bank_start, bank_end);
> > +            }
> > +        }
> > +    }
> > +    /*
> > +     * No reserved heap regions:
> >       * For simplicity, add all the free regions in the boot allocator.
> >       */
> > -    populate_boot_allocator();
> > +    else
> > +        populate_boot_allocator();
> >
> >      total_pages = 0;
> >
> >      for ( i = 0; i < banks->nr_banks; i++ )
> >      {
> >          const struct membank *bank = &banks->bank[i];
> > -        paddr_t bank_end = bank->start + bank->size;
> > +        bank_end = bank->start + bank->size;
> >
> >          ram_size = ram_size + bank->size;
> >          ram_start = min(ram_start, bank->start);
> > --
> > 2.17.1
> >
> >
> 
> ~Michal

Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Michal Orzel]

Hi Henry,

On 30/08/2022 02:58, Henry Wang wrote:
> 
> Hi Stefano and Michal,
> 
>> -----Original Message-----
>> From: Stefano Stabellini <sstabellini@kernel.org>
>> Sent: Tuesday, August 30, 2022 8:47 AM
>> To: Henry Wang <Henry.Wang@arm.com>
>> Cc: Michal Orzel <michal.orzel@amd.com>; xen-devel@lists.xenproject.org;
>> Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>;
>> Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>; Volodymyr Babchuk
>> <Volodymyr_Babchuk@epam.com>; Penny Zheng <Penny.Zheng@arm.com>
>> Subject: RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory
>>
>> On Thu, 25 Aug 2022, Henry Wang wrote:
>>>>>                                         const char *name,
>>>>>                                         u32 address_cells, u32 size_cells)
>>>>>  {
>>>>> @@ -301,16 +303,40 @@ static void __init process_chosen_node(const
>>>> void *fdt, int node,
>>>>>      paddr_t start, end;
>>>>>      int len;
>>>>>
>>>>> +    if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
>>>>> +    {
>>>>> +        u32 address_cells = device_tree_get_u32(fdt, node,
>>>>> +                                                "#xen,static-mem-address-cells",
>>>>> +                                                0);
>>>>> +        u32 size_cells = device_tree_get_u32(fdt, node,
>>>>> +                                             "#xen,static-mem-size-cells", 0);
>>>>> +        int rc;
>>>>> +
>>>>> +        printk("Checking for reserved heap in /chosen\n");
>>>>> +        if ( address_cells < 1 || size_cells < 1 )
>>>> address_cells and size_cells cannot be negative so you could just check if
>>>> there are 0.
>>>
>>> In bootfdt.c function device_tree_get_meminfo(), the address and size cells
>>> are checked using <1 instead of =0. I agree they cannot be negative, but I
>> am
>>> not very sure if there were other reasons to do the "<1" check in
>>> device_tree_get_meminfo(). Are you fine with we don't keep the
>> consistency
>>> here?
>>
>> I would keep the < 1 check but it doesn't make much difference either
>> way
> 
> I also would prefer to keep these two places consistent and I agree Michal is
> making a good point.
I'm ok with that so let's keep the consistency.

> 
> Kind regards,
> Henry
> 

~Michal

Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Michal Orzel]

On 30/08/2022 08:29, Michal Orzel wrote:
> Hi Henry,
> 
> On 30/08/2022 02:58, Henry Wang wrote:
>>
>> Hi Stefano and Michal,
>>
>>> -----Original Message-----
>>> From: Stefano Stabellini <sstabellini@kernel.org>
>>> Sent: Tuesday, August 30, 2022 8:47 AM
>>> To: Henry Wang <Henry.Wang@arm.com>
>>> Cc: Michal Orzel <michal.orzel@amd.com>; xen-devel@lists.xenproject.org;
>>> Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>;
>>> Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>>> <Wei.Chen@arm.com>; Volodymyr Babchuk
>>> <Volodymyr_Babchuk@epam.com>; Penny Zheng <Penny.Zheng@arm.com>
>>> Subject: RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory
>>>
>>> On Thu, 25 Aug 2022, Henry Wang wrote:
>>>>>>                                         const char *name,
>>>>>>                                         u32 address_cells, u32 size_cells)
>>>>>>  {
>>>>>> @@ -301,16 +303,40 @@ static void __init process_chosen_node(const
>>>>> void *fdt, int node,
>>>>>>      paddr_t start, end;
>>>>>>      int len;
>>>>>>
>>>>>> +    if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
>>>>>> +    {
>>>>>> +        u32 address_cells = device_tree_get_u32(fdt, node,
>>>>>> +                                                "#xen,static-mem-address-cells",
>>>>>> +                                                0);
>>>>>> +        u32 size_cells = device_tree_get_u32(fdt, node,
>>>>>> +                                             "#xen,static-mem-size-cells", 0);
>>>>>> +        int rc;
>>>>>> +
>>>>>> +        printk("Checking for reserved heap in /chosen\n");
>>>>>> +        if ( address_cells < 1 || size_cells < 1 )
>>>>> address_cells and size_cells cannot be negative so you could just check if
>>>>> there are 0.
>>>>
>>>> In bootfdt.c function device_tree_get_meminfo(), the address and size cells
>>>> are checked using <1 instead of =0. I agree they cannot be negative, but I
>>> am
>>>> not very sure if there were other reasons to do the "<1" check in
>>>> device_tree_get_meminfo(). Are you fine with we don't keep the
>>> consistency
>>>> here?
>>>
>>> I would keep the < 1 check but it doesn't make much difference either
>>> way
>>
>> I also would prefer to keep these two places consistent and I agree Michal is
>> making a good point.
> I'm ok with that so let's keep the consistency.
Actually, why do we want to duplicate exactly the same check in process_chosen_node that is already
present in device_tree_get_meminfo? There is no need for that so just remove it from process_chosen_node.

> 
>>
>> Kind regards,
>> Henry
>>
> 
> ~Michal

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Michal Orzel]

Hi Henry,

On 30/08/2022 08:11, Henry Wang wrote:
> 
> Hi Michal,
> 
> Sorry about the late reply - I had a couple of days off. Thank you very
> much for the review! I will add my reply and answer some of your
> questions below.
> 
>> -----Original Message-----
>> From: Michal Orzel <michal.orzel@amd.com>
>> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
>> heap allocator
>>
>>> This commit firstly adds a global variable `reserved_heap`.
>>> This newly introduced global variable is set at the device tree
>>> parsing time if the reserved heap ranges are defined in the device
>>> tree chosen node.
>>>
>> Did you consider putting reserved_heap into bootinfo structure?
> 
> Actually I did, but I saw current bootinfo only contains some structs so
> I was not sure if this is the preferred way, but since you are raising this
> question, I will follow this method in v2.
This is what I think would be better but maintainers will have a decisive vote.

> 
>> It would help to avoid introducing new global variables that are only used
>> in places making use of the bootinfo anyway.
> 
> Ack.
> 
>>
>>> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
>>> +        {
>>> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
>>> +            {
>>> +                bank_start = bootinfo.reserved_mem.bank[i].start;
>>> +                bank_size = bootinfo.reserved_mem.bank[i].size;
>>> +                bank_end = bank_start + bank_size;
>>> +
>>> +                reserved_heap_size += bank_size;
>>> +                reserved_heap_start = min(reserved_heap_start, bank_start);
>> You do not need reserved_heap_start as you do not use it at any place later
>> on.
>> In your current implementation you just need reserved_heap_size and
>> reserved_heap_end.
> 
> Good point, thank you and I will remove in v2.
> 
>>
>>>      /*
>>>       * If the user has not requested otherwise via the command line
>>>       * then locate the xenheap using these constraints:
>>> @@ -743,7 +766,8 @@ static void __init setup_mm(void)
>>>       * We try to allocate the largest xenheap possible within these
>>>       * constraints.
>>>       */
>>> -    heap_pages = ram_pages;
>>> +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
>> I must say that the reverted logic is harder to read. This is a matter of taste
>> but
>> please consider the following:
>> heap_pages = reserved_heap ? reserved_heap_pages : ram_pages;
>> The same applies to ...
> 
> Sure, I will use the way you suggested.
> 
>>
>>> +
>>>      if ( opt_xenheap_megabytes )
>>>          xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
>>>      else
>>> @@ -755,17 +779,21 @@ static void __init setup_mm(void)
>>>
>>>      do
>>>      {
>>> -        e = consider_modules(ram_start, ram_end,
>>> +        e = !reserved_heap ?
>> ... here.
> 
> And here :))
> 
>>
>>> +            consider_modules(ram_start, ram_end,
>>>                               pfn_to_paddr(xenheap_pages),
>>> -                             32<<20, 0);
>>> +                             32<<20, 0) :
>>> +            reserved_heap_end;
>>> +
>>>          if ( e )
>>>              break;
>>>
>>>          xenheap_pages >>= 1;
>>>      } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
>> PAGE_SHIFT) );
>>>
>>> -    if ( ! e )
>>> -        panic("Not not enough space for xenheap\n");
>>> +    if ( ! e ||
>>> +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
>> I'm not sure about this. You are checking if the size of the reserved heap is
>> less than 32MB
>> and this has nothing to do with the following panic message.
> 
> Hmmm, I am not sure if I understand your question correctly, so here there
> are actually 2 issues:
> (1) The double not in the panic message.
> (2) The size of xenheap.
> 
> If you check the comment of the xenheap constraints above, one rule of the
> xenheap size is it "must be at least 32M". If I am not mistaken, we need to
> follow the same rule with the reserved heap setup, so here we need to check
> the size and if <32M then panic.
This is totally fine. What I mean is that the check you introduced does not correspond
to the panic message below. In case of reserved heap, its size is selected by the user.
"Not enough space for xenheap" means that there is not enough space to be reserved for heap,
meaning its size is too large. But your check is about size being too small.

> 
>>
>>> +        panic("Not enough space for xenheap\n");
>>>
>>>      domheap_pages = heap_pages - xenheap_pages;
>>>
>>> @@ -810,9 +838,9 @@ 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_end = 0;
>>> -    paddr_t ram_size = 0;
>>> +    paddr_t ram_start = ~0, bank_start = ~0;
>>> +    paddr_t ram_end = 0, bank_end = 0;
>>> +    paddr_t ram_size = 0, bank_size = 0;
>>>      unsigned int i;
>>>
>>>      init_pdx();
>>> @@ -821,17 +849,36 @@ static void __init setup_mm(void)
>>>       * We need some memory to allocate the page-tables used for the
>> xenheap
>>>       * mappings. But some regions may contain memory already allocated
>>>       * for other uses (e.g. modules, reserved-memory...).
>>> -     *
>>> +     * If reserved heap regions are properly defined, (only) add these
>> regions
>> How can you say at this stage whether the reserved heap regions are defined
>> properly?
> 
> Because if the reserved heap regions are not properly defined, in the device
> tree parsing phase the global variable "reserved_heap" can never be true.
> 
> Did I understand your question correctly? Or maybe we need to change the
> wording here in the comment?

FWICS, reserved_heap will be set to true even if a user describes an empty region
for reserved heap. This cannot be consider a properly defined region for a heap.

~Michal

RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Henry Wang]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> >>>>>> +        printk("Checking for reserved heap in /chosen\n");
> >>>>>> +        if ( address_cells < 1 || size_cells < 1 )
> >>>>> address_cells and size_cells cannot be negative so you could just check
> if
> >>>>> there are 0.
> >>>>
> >>>> In bootfdt.c function device_tree_get_meminfo(), the address and size
> cells
> >>>> are checked using <1 instead of =0. I agree they cannot be negative, but
> I
> >>> am
> >>>> not very sure if there were other reasons to do the "<1" check in
> >>>> device_tree_get_meminfo(). Are you fine with we don't keep the
> >>> consistency
> >>>> here?
> >>>
> >>> I would keep the < 1 check but it doesn't make much difference either
> >>> way
> >>
> >> I also would prefer to keep these two places consistent and I agree Michal
> is
> >> making a good point.
> > I'm ok with that so let's keep the consistency.
> Actually, why do we want to duplicate exactly the same check in
> process_chosen_node that is already
> present in device_tree_get_meminfo? There is no need for that so just
> remove it from process_chosen_node.

Well, yes and no IMHO, because we are using "#xen,static-heap-address-cells"
and "#xen,static-heap-size-cells" instead of normal "#address-cells" and
"#size-cells". These properties are dependent on user's input so I would say
adding a check and proper printk to inform user with the related information
would be a good idea. Also I think catching the incorrect
"#xen,static-heap-address-cells" and "#xen,static-heap-size-cells" and return
early would also be a good idea.

Kind regards,
Henry

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> >>>
> >> Did you consider putting reserved_heap into bootinfo structure?
> >
> > Actually I did, but I saw current bootinfo only contains some structs so
> > I was not sure if this is the preferred way, but since you are raising this
> > question, I will follow this method in v2.
> This is what I think would be better but maintainers will have a decisive vote.

Then let's wait for more input from maintainers.

> 
> >>>
> >>> -    if ( ! e )
> >>> -        panic("Not not enough space for xenheap\n");
> >>> +    if ( ! e ||
> >>> +         ( reserved_heap && reserved_heap_pages < 32<<(20-
> PAGE_SHIFT) ) )
> >> I'm not sure about this. You are checking if the size of the reserved heap is
> >> less than 32MB
> >> and this has nothing to do with the following panic message.
> >
> > Hmmm, I am not sure if I understand your question correctly, so here there
> > are actually 2 issues:
> > (1) The double not in the panic message.
> > (2) The size of xenheap.
> >
> > If you check the comment of the xenheap constraints above, one rule of
> the
> > xenheap size is it "must be at least 32M". If I am not mistaken, we need to
> > follow the same rule with the reserved heap setup, so here we need to
> check
> > the size and if <32M then panic.
> This is totally fine. What I mean is that the check you introduced does not
> correspond
> to the panic message below. In case of reserved heap, its size is selected by
> the user.
> "Not enough space for xenheap" means that there is not enough space to be
> reserved for heap,
> meaning its size is too large. But your check is about size being too small.

Actually my understanding of "Not enough space for xenheap" is xenheap
is too large so we need to reserve more space, which is slightly different than
your opinion. But I am not the native speaker so it is highly likely that I am
making mistakes...

How about changing the panic message to "Not enough memory for xenheap"?
This would remove the ambiguity here IMHO.

> 
> >>> +     * If reserved heap regions are properly defined, (only) add these
> >> regions
> >> How can you say at this stage whether the reserved heap regions are
> defined
> >> properly?
> >
> > Because if the reserved heap regions are not properly defined, in the
> device
> > tree parsing phase the global variable "reserved_heap" can never be true.
> >
> > Did I understand your question correctly? Or maybe we need to change the
> > wording here in the comment?
> 
> FWICS, reserved_heap will be set to true even if a user describes an empty
> region
> for reserved heap. This cannot be consider a properly defined region for a
> heap.

Oh good point, thank you for pointing this out. I will change the comments
here to "If there are non-empty reserved heap regions". I am not sure if adding
an empty region check before setting the "reserved_heap" would be a good
idea, because adding such check would add another for loop to find a non-empty
reserved heap bank. What do you think?

Kind regards,
Henry

> 
> ~Michal

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> > +    paddr_t reserved_heap_start = ~0, reserved_heap_end = 0,
> 
> INVALID_PADDR or ~0ULL

Ack.

> 
> >      /*
> >       * If the user has not requested otherwise via the command line
> >       * then locate the xenheap using these constraints:
> > @@ -743,7 +766,8 @@ static void __init setup_mm(void)
> >       * We try to allocate the largest xenheap possible within these
> >       * constraints.
> >       */
> > -    heap_pages = ram_pages;
> > +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> > +
> >      if ( opt_xenheap_megabytes )
> >          xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> >      else
> > @@ -755,17 +779,21 @@ static void __init setup_mm(void)
> >
> >      do
> >      {
> > -        e = consider_modules(ram_start, ram_end,
> > +        e = !reserved_heap ?
> > +            consider_modules(ram_start, ram_end,
> >                               pfn_to_paddr(xenheap_pages),
> > -                             32<<20, 0);
> > +                             32<<20, 0) :
> > +            reserved_heap_end;
> > +
> >          if ( e )
> >              break;
> >
> >          xenheap_pages >>= 1;
> >      } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> PAGE_SHIFT) );
> >
> > -    if ( ! e )
> > -        panic("Not not enough space for xenheap\n");
> > +    if ( ! e ||
> > +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
> > +        panic("Not enough space for xenheap\n");
> 
> 
> I would skip the do/while loop completely if reserved_heap. We don't
> need it anyway

I agree with this.

> and we can automatically calculate xenheap_pages in a single line.

Here I am a little bit confused. Sorry to ask but could you please explain
a little bit more about why we can calculate the xenheap_pages in a single
line? Below is the code snippet in my mind, is this correct?

if (reserved_heap)
    e = reserved_heap_end;
else
{
    do
    {
        e = consider_modules(ram_start, ram_end,
                             pfn_to_paddr(xenheap_pages),
                             32<<20, 0);
        if ( e )
            break;

        xenheap_pages >>= 1;
    } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
}

> 
> >      domheap_pages = heap_pages - xenheap_pages;
> >
> > @@ -810,9 +838,9 @@ 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_end = 0;
> > -    paddr_t ram_size = 0;
> > +    paddr_t ram_start = ~0, bank_start = ~0;
> > +    paddr_t ram_end = 0, bank_end = 0;
> > +    paddr_t ram_size = 0, bank_size = 0;
> >      unsigned int i;
> 
> Please use INVALID_PADDR or ~0ULL

Ack.

Kind regards,
Henry

> 
> 
> >
> >      init_pdx();
> > @@ -821,17 +849,36 @@ static void __init setup_mm(void)
> >       * We need some memory to allocate the page-tables used for the
> xenheap
> >       * mappings. But some regions may contain memory already allocated
> >       * for other uses (e.g. modules, reserved-memory...).
> > -     *
> > +     * If reserved heap regions are properly defined, (only) add these
> regions
> > +     * in the boot allocator.
> > +     */
> > +    if ( reserved_heap )
> > +    {
> > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > +            {
> > +                bank_start = bootinfo.reserved_mem.bank[i].start;
> > +                bank_size = bootinfo.reserved_mem.bank[i].size;
> > +                bank_end = bank_start + bank_size;
> > +
> > +                init_boot_pages(bank_start, bank_end);
> > +            }
> > +        }
> > +    }
> > +    /*
> > +     * No reserved heap regions:
> >       * For simplicity, add all the free regions in the boot allocator.
> >       */
> > -    populate_boot_allocator();
> > +    else
> > +        populate_boot_allocator();
> >
> >      total_pages = 0;
> >
> >      for ( i = 0; i < banks->nr_banks; i++ )
> >      {
> >          const struct membank *bank = &banks->bank[i];
> > -        paddr_t bank_end = bank->start + bank->size;
> > +        bank_end = bank->start + bank->size;
> >
> >          ram_size = ram_size + bank->size;
> >          ram_start = min(ram_start, bank->start);
> > --
> > 2.17.1
> >

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Michal Orzel]

Hi Henry,

On 30/08/2022 10:00, Henry Wang wrote:
> 
> Hi Michal,
> 
>> -----Original Message-----
>> From: Michal Orzel <michal.orzel@amd.com>
>>>>>
>>>> Did you consider putting reserved_heap into bootinfo structure?
>>>
>>> Actually I did, but I saw current bootinfo only contains some structs so
>>> I was not sure if this is the preferred way, but since you are raising this
>>> question, I will follow this method in v2.
>> This is what I think would be better but maintainers will have a decisive vote.
> 
> Then let's wait for more input from maintainers.
> 
>>
>>>>>
>>>>> -    if ( ! e )
>>>>> -        panic("Not not enough space for xenheap\n");
>>>>> +    if ( ! e ||
>>>>> +         ( reserved_heap && reserved_heap_pages < 32<<(20-
>> PAGE_SHIFT) ) )
>>>> I'm not sure about this. You are checking if the size of the reserved heap is
>>>> less than 32MB
>>>> and this has nothing to do with the following panic message.
>>>
>>> Hmmm, I am not sure if I understand your question correctly, so here there
>>> are actually 2 issues:
>>> (1) The double not in the panic message.
>>> (2) The size of xenheap.
>>>
>>> If you check the comment of the xenheap constraints above, one rule of
>> the
>>> xenheap size is it "must be at least 32M". If I am not mistaken, we need to
>>> follow the same rule with the reserved heap setup, so here we need to
>> check
>>> the size and if <32M then panic.
>> This is totally fine. What I mean is that the check you introduced does not
>> correspond
>> to the panic message below. In case of reserved heap, its size is selected by
>> the user.
>> "Not enough space for xenheap" means that there is not enough space to be
>> reserved for heap,
>> meaning its size is too large. But your check is about size being too small.
> 
> Actually my understanding of "Not enough space for xenheap" is xenheap
> is too large so we need to reserve more space, which is slightly different than
> your opinion. But I am not the native speaker so it is highly likely that I am
> making mistakes...My understanding is exactly the same as yours :), meaning heap is too large.
But your check is against heap being to small (less than 32M).
So basically if the following check fails:
"( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )"
it means that the heap region defined by a user is too small (not too large),
because according to requirements it should be at least 32M.

> 
> How about changing the panic message to "Not enough memory for xenheap"?
> This would remove the ambiguity here IMHO.
> 
>>
>>>>> +     * If reserved heap regions are properly defined, (only) add these
>>>> regions
>>>> How can you say at this stage whether the reserved heap regions are
>> defined
>>>> properly?
>>>
>>> Because if the reserved heap regions are not properly defined, in the
>> device
>>> tree parsing phase the global variable "reserved_heap" can never be true.
>>>
>>> Did I understand your question correctly? Or maybe we need to change the
>>> wording here in the comment?
>>
>> FWICS, reserved_heap will be set to true even if a user describes an empty
>> region
>> for reserved heap. This cannot be consider a properly defined region for a
>> heap.
> 
> Oh good point, thank you for pointing this out. I will change the comments
> here to "If there are non-empty reserved heap regions". I am not sure if adding
> an empty region check before setting the "reserved_heap" would be a good
> idea, because adding such check would add another for loop to find a non-empty
> reserved heap bank. What do you think?
> 
> Kind regards,
> Henry
> 
>>
>> ~Michal

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> >> This is totally fine. What I mean is that the check you introduced does not
> >> correspond
> >> to the panic message below. In case of reserved heap, its size is selected
> by
> >> the user.
> >> "Not enough space for xenheap" means that there is not enough space to
> be
> >> reserved for heap,
> >> meaning its size is too large. But your check is about size being too small.
> >
> > Actually my understanding of "Not enough space for xenheap" is xenheap
> > is too large so we need to reserve more space, which is slightly different
> than
> > your opinion. But I am not the native speaker so it is highly likely that I am
> > making mistakes...
> My understanding is exactly the same as yours :),
> meaning heap is too large.

Oh I think get your point. Let me try to explain myself and thanks for your
patience :))

The reserved heap region defined in the device tree should be used for both
Xenheap and domain heap, so if we reserved a too small region (<32M),
an error should pop because the reserved region is not enough for xenheap,
and user should reserve more.
[...]

> But your check is against heap being to small (less than 32M).
> So basically if the following check fails:
> "( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )"
> it means that the heap region defined by a user is too small (not too large),
> because according to requirements it should be at least 32M.

[...]
So in that case, printing "Not enough space for xenheap" means the reserved
region cannot satisfy the minimal requirement of the space of xenheap (at least
32M), and this is in consistent with the check.

Kind regards,
Henry

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Michal Orzel]

On 30/08/2022 11:17, Henry Wang wrote:
> 
> Hi Michal,
> 
>> -----Original Message-----
>> From: Michal Orzel <michal.orzel@amd.com>
>>>> This is totally fine. What I mean is that the check you introduced does not
>>>> correspond
>>>> to the panic message below. In case of reserved heap, its size is selected
>> by
>>>> the user.
>>>> "Not enough space for xenheap" means that there is not enough space to
>> be
>>>> reserved for heap,
>>>> meaning its size is too large. But your check is about size being too small.
>>>
>>> Actually my understanding of "Not enough space for xenheap" is xenheap
>>> is too large so we need to reserve more space, which is slightly different
>> than
>>> your opinion. But I am not the native speaker so it is highly likely that I am
>>> making mistakes...
>> My understanding is exactly the same as yours :),
>> meaning heap is too large.
> 
> Oh I think get your point. Let me try to explain myself and thanks for your
> patience :))
> 
> The reserved heap region defined in the device tree should be used for both
> Xenheap and domain heap, so if we reserved a too small region (<32M),
> an error should pop because the reserved region is not enough for xenheap,
> and user should reserve more.
> [...]
> 
>> But your check is against heap being to small (less than 32M).
>> So basically if the following check fails:
>> "( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )"
>> it means that the heap region defined by a user is too small (not too large),
>> because according to requirements it should be at least 32M.
> 
> [...]
> So in that case, printing "Not enough space for xenheap" means the reserved
> region cannot satisfy the minimal requirement of the space of xenheap (at least
> 32M), and this is in consistent with the check.

Ok, it clearly depends on the way someone understands this sentence.
Currently this panic can be triggered if the heap size is too large and
should be read as "heap is too large to fit in because there is not enough space
within RAM considering modules (e - s < size)". Usually (and also in this case)
space refers to a region to contain another one.

You are reusing the same message for different meaning, that is "user defined too
small heap and this space (read as size) is not enough".

Let's leave it to someone else to decide.

~Michal

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> >
> > Oh I think get your point. Let me try to explain myself and thanks for your
> > patience :))
> >
> > The reserved heap region defined in the device tree should be used for
> both
> > Xenheap and domain heap, so if we reserved a too small region (<32M),
> > an error should pop because the reserved region is not enough for
> xenheap,
> > and user should reserve more.
> > [...]
> >
> >> But your check is against heap being to small (less than 32M).
> >> So basically if the following check fails:
> >> "( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )"
> >> it means that the heap region defined by a user is too small (not too large),
> >> because according to requirements it should be at least 32M.
> >
> > [...]
> > So in that case, printing "Not enough space for xenheap" means the
> reserved
> > region cannot satisfy the minimal requirement of the space of xenheap (at
> least
> > 32M), and this is in consistent with the check.
> 
> Ok, it clearly depends on the way someone understands this sentence.
> Currently this panic can be triggered if the heap size is too large and
> should be read as "heap is too large to fit in because there is not enough
> space
> within RAM considering modules (e - s < size)". Usually (and also in this case)
> space refers to a region to contain another one.
> 
> You are reusing the same message for different meaning, that is "user
> defined too
> small heap and this space (read as size) is not enough".

Yes, thanks for the explanation. I think maybe rewording the message
to "Not enough memory for allocating xenheap" would remove the ambiguity
to some extent? Because the user-defined heap region should cover both
xenheap and domain heap at the same time, the small user-defined heap
means "xenheap is too large to fit in the user-defined heap region", which is
in consistent with your interpretation of the current "xenheap is too large to fit
in because there is not enough space within RAM considering modules"

> 
> Let's leave it to someone else to decide.

I agree.

Kind regards,
Henry

> 
> ~Michal

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Stefano Stabellini]

On Tue, 30 Aug 2022, Henry Wang wrote:
> > >      /*
> > >       * If the user has not requested otherwise via the command line
> > >       * then locate the xenheap using these constraints:
> > > @@ -743,7 +766,8 @@ static void __init setup_mm(void)
> > >       * We try to allocate the largest xenheap possible within these
> > >       * constraints.
> > >       */
> > > -    heap_pages = ram_pages;
> > > +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> > > +
> > >      if ( opt_xenheap_megabytes )
> > >          xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> > >      else
> > > @@ -755,17 +779,21 @@ static void __init setup_mm(void)
> > >
> > >      do
> > >      {
> > > -        e = consider_modules(ram_start, ram_end,
> > > +        e = !reserved_heap ?
> > > +            consider_modules(ram_start, ram_end,
> > >                               pfn_to_paddr(xenheap_pages),
> > > -                             32<<20, 0);
> > > +                             32<<20, 0) :
> > > +            reserved_heap_end;
> > > +
> > >          if ( e )
> > >              break;
> > >
> > >          xenheap_pages >>= 1;
> > >      } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> > PAGE_SHIFT) );
> > >
> > > -    if ( ! e )
> > > -        panic("Not not enough space for xenheap\n");
> > > +    if ( ! e ||
> > > +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
> > > +        panic("Not enough space for xenheap\n");
> > 
> > 
> > I would skip the do/while loop completely if reserved_heap. We don't
> > need it anyway
> 
> I agree with this.
> 
> > and we can automatically calculate xenheap_pages in a single line.
> 
> Here I am a little bit confused. Sorry to ask but could you please explain
> a little bit more about why we can calculate the xenheap_pages in a single
> line? Below is the code snippet in my mind, is this correct?
> 
> if (reserved_heap)

coding style

>     e = reserved_heap_end;
> else
> {
>     do
>     {
>         e = consider_modules(ram_start, ram_end,
>                              pfn_to_paddr(xenheap_pages),
>                              32<<20, 0);
>         if ( e )
>             break;
> 
>         xenheap_pages >>= 1;
>     } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
> }

Yes, this is what I meant.

But also, here the loop is also for adjusting xenheap_pages, and
xenheap_pages is initialized as follows:


    if ( opt_xenheap_megabytes )
        xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
    else
    {
        xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
        xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
        xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
    }


In the reserved_heap case, it doesn't make sense to initialize
xenheap_pages like that, right? It should be something like:

    if ( opt_xenheap_megabytes )
        xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
    else if ( reserved_heap )
        xenheap_pages = heap_pages;
    else
    {
        xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
        xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
        xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
    }

But also it looks like that on arm32 we have specific requirements for
Xen heap:

     *  - 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 least 32M

I think we should check at least the 32MB alignment and 32MB minimum
size before using the xen_heap bank.


In short I think this patch should:

- add a check for 32MB alignment and size of the xen_heap memory bank
- if reserved_heap, set xenheap_pages = heap_pages
- if reserved_heap, skip the consider_modules do/while

Does it make sense?

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Stefano Stabellini]

On Tue, 30 Aug 2022, Henry Wang wrote:
> > -----Original Message-----
> > From: Michal Orzel <michal.orzel@amd.com>
> > >
> > > Oh I think get your point. Let me try to explain myself and thanks for your
> > > patience :))
> > >
> > > The reserved heap region defined in the device tree should be used for
> > both
> > > Xenheap and domain heap, so if we reserved a too small region (<32M),
> > > an error should pop because the reserved region is not enough for
> > xenheap,
> > > and user should reserve more.
> > > [...]
> > >
> > >> But your check is against heap being to small (less than 32M).
> > >> So basically if the following check fails:
> > >> "( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )"
> > >> it means that the heap region defined by a user is too small (not too large),
> > >> because according to requirements it should be at least 32M.
> > >
> > > [...]
> > > So in that case, printing "Not enough space for xenheap" means the
> > reserved
> > > region cannot satisfy the minimal requirement of the space of xenheap (at
> > least
> > > 32M), and this is in consistent with the check.
> > 
> > Ok, it clearly depends on the way someone understands this sentence.
> > Currently this panic can be triggered if the heap size is too large and
> > should be read as "heap is too large to fit in because there is not enough
> > space
> > within RAM considering modules (e - s < size)". Usually (and also in this case)
> > space refers to a region to contain another one.
> > 
> > You are reusing the same message for different meaning, that is "user
> > defined too
> > small heap and this space (read as size) is not enough".
> 
> Yes, thanks for the explanation. I think maybe rewording the message
> to "Not enough memory for allocating xenheap" would remove the ambiguity
> to some extent? Because the user-defined heap region should cover both
> xenheap and domain heap at the same time, the small user-defined heap
> means "xenheap is too large to fit in the user-defined heap region", which is
> in consistent with your interpretation of the current "xenheap is too large to fit
> in because there is not enough space within RAM considering modules"

I think we should have a separate check specific for the device tree
input parameters to make sure the region is correct, that way we can
have a specific error message, such as:

"xen,static-heap address needs to be 32MB aligned and the size a
multiple of 32MB."

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Stefano Stabellini]

On Tue, 30 Aug 2022, Henry Wang wrote:
> Hi Michal,
> 
> > -----Original Message-----
> > From: Michal Orzel <michal.orzel@amd.com>
> > >>>
> > >> Did you consider putting reserved_heap into bootinfo structure?
> > >
> > > Actually I did, but I saw current bootinfo only contains some structs so
> > > I was not sure if this is the preferred way, but since you are raising this
> > > question, I will follow this method in v2.
> > This is what I think would be better but maintainers will have a decisive vote.
> 
> Then let's wait for more input from maintainers.

I don't have a strong preference and the way the current code is
written, it would actually take less memory as is (the extra bool
xen_heap comes for free.)

I would keep the patch as is for now and for 4.17.

If Julien prefers a refactoring of bootinfo/meminfo I think it could be
done after the release if you are up to it.

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> > > and we can automatically calculate xenheap_pages in a single line.
> >
> > Here I am a little bit confused. Sorry to ask but could you please explain
> > a little bit more about why we can calculate the xenheap_pages in a single
> > line? Below is the code snippet in my mind, is this correct?
> >
> > if (reserved_heap)
> 
> coding style
> 
> >     e = reserved_heap_end;
> > else
> > {
> >     do
> >     {
> >         e = consider_modules(ram_start, ram_end,
> >                              pfn_to_paddr(xenheap_pages),
> >                              32<<20, 0);
> >         if ( e )
> >             break;
> >
> >         xenheap_pages >>= 1;
> >     } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> PAGE_SHIFT) );
> > }
> 
> Yes, this is what I meant.

Thank you very much for your detailed explanation below!
[...]

> 
> But also, here the loop is also for adjusting xenheap_pages, and
> xenheap_pages is initialized as follows:
> 
> 
>     if ( opt_xenheap_megabytes )
>         xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
>     else
>     {
>         xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
>         xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
>         xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
>     }
> 
> 
> In the reserved_heap case, it doesn't make sense to initialize
> xenheap_pages like that, right? It should be something like:

I am not sure about that, since we already have
heap_pages = reserved_heap ? reserved_heap_pages : ram_pages;

the heap_pages is supposed to contain domheap_pages + xenheap_pages
based on the reserved heap definition discussed in the RFC.

from the code in...

> 
>     if ( opt_xenheap_megabytes )
>         xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
>     else if ( reserved_heap )
>         xenheap_pages = heap_pages;

...here, setting xenheap_pages to heap_pages makes me a little bit
confused.

>     else
>     {
>         xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
>         xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
>         xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
>     }

If we keep this logic as this patch does, we can have the requirements...

> 
> But also it looks like that on arm32 we have specific requirements for
> Xen heap:
> 
>      *  - 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 least 32M

...here, with the "1/32 the total RAM" now being "1/32 of the total reserved
heap region", since heap_pages is now reserved_heap_pages.

> 
> I think we should check at least the 32MB alignment and 32MB minimum
> size before using the xen_heap bank.
> 
> 
> In short I think this patch should:
> 
> - add a check for 32MB alignment and size of the xen_heap memory bank
> - if reserved_heap, set xenheap_pages = heap_pages
> - if reserved_heap, skip the consider_modules do/while
> 
> Does it make sense?

I left some of my thoughts above to explain my understanding, but I might
be wrong, thank you for your patience!

Kind regards,
Henry

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Subject: RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> heap allocator
> 
> On Tue, 30 Aug 2022, Henry Wang wrote:
> > > -----Original Message-----
> > > From: Michal Orzel <michal.orzel@amd.com>
> > > >
> > > > Oh I think get your point. Let me try to explain myself and thanks for
> your
> > > > patience :))
> > > >
> > > > The reserved heap region defined in the device tree should be used for
> > > both
> > > > Xenheap and domain heap, so if we reserved a too small region (<32M),
> > > > an error should pop because the reserved region is not enough for
> > > xenheap,
> > > > and user should reserve more.
> > > > [...]
> > > >
> > > >> But your check is against heap being to small (less than 32M).
> > > >> So basically if the following check fails:
> > > >> "( reserved_heap && reserved_heap_pages < 32<<(20-
> PAGE_SHIFT) ) )"
> > > >> it means that the heap region defined by a user is too small (not too
> large),
> > > >> because according to requirements it should be at least 32M.
> > > >
> > > > [...]
> > > > So in that case, printing "Not enough space for xenheap" means the
> > > reserved
> > > > region cannot satisfy the minimal requirement of the space of xenheap
> (at
> > > least
> > > > 32M), and this is in consistent with the check.
> > >
> > > Ok, it clearly depends on the way someone understands this sentence.
> > > Currently this panic can be triggered if the heap size is too large and
> > > should be read as "heap is too large to fit in because there is not enough
> > > space
> > > within RAM considering modules (e - s < size)". Usually (and also in this
> case)
> > > space refers to a region to contain another one.
> > >
> > > You are reusing the same message for different meaning, that is "user
> > > defined too
> > > small heap and this space (read as size) is not enough".
> >
> > Yes, thanks for the explanation. I think maybe rewording the message
> > to "Not enough memory for allocating xenheap" would remove the
> ambiguity
> > to some extent? Because the user-defined heap region should cover both
> > xenheap and domain heap at the same time, the small user-defined heap
> > means "xenheap is too large to fit in the user-defined heap region", which
> is
> > in consistent with your interpretation of the current "xenheap is too large
> to fit
> > in because there is not enough space within RAM considering modules"
> 
> I think we should have a separate check specific for the device tree
> input parameters to make sure the region is correct, that way we can
> have a specific error message, such as:
> 
> "xen,static-heap address needs to be 32MB aligned and the size a
> multiple of 32MB."

Sure, will follow this.

Kind regards,
Henry

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Stefano,

> -----Original Message-----
> From: Henry Wang
> I am not sure about that, since we already have
> heap_pages = reserved_heap ? reserved_heap_pages : ram_pages;
> 
> the heap_pages is supposed to contain domheap_pages + xenheap_pages
> based on the reserved heap definition discussed in the RFC.

To add a little bit more about the background, here is the RFC discussion [1].
I should have attached this in my previous reply, sorry.

[1] https://lore.kernel.org/xen-devel/316007B7-51BA-4820-8F6F-018BC6D3A077@arm.com/

Kind regards,
Henry

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Stefano Stabellini]

On Wed, 31 Aug 2022, Henry Wang wrote:
> > -----Original Message-----
> > From: Stefano Stabellini <sstabellini@kernel.org>
> > > > and we can automatically calculate xenheap_pages in a single line.
> > >
> > > Here I am a little bit confused. Sorry to ask but could you please explain
> > > a little bit more about why we can calculate the xenheap_pages in a single
> > > line? Below is the code snippet in my mind, is this correct?
> > >
> > > if (reserved_heap)
> > 
> > coding style
> > 
> > >     e = reserved_heap_end;
> > > else
> > > {
> > >     do
> > >     {
> > >         e = consider_modules(ram_start, ram_end,
> > >                              pfn_to_paddr(xenheap_pages),
> > >                              32<<20, 0);
> > >         if ( e )
> > >             break;
> > >
> > >         xenheap_pages >>= 1;
> > >     } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> > PAGE_SHIFT) );
> > > }
> > 
> > Yes, this is what I meant.
> 
> Thank you very much for your detailed explanation below!
> [...]
> 
> > 
> > But also, here the loop is also for adjusting xenheap_pages, and
> > xenheap_pages is initialized as follows:
> > 
> > 
> >     if ( opt_xenheap_megabytes )
> >         xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> >     else
> >     {
> >         xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
> >         xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
> >         xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
> >     }
> > 
> > 
> > In the reserved_heap case, it doesn't make sense to initialize
> > xenheap_pages like that, right? It should be something like:
> 
> I am not sure about that, since we already have
> heap_pages = reserved_heap ? reserved_heap_pages : ram_pages;
> 
> the heap_pages is supposed to contain domheap_pages + xenheap_pages
> based on the reserved heap definition discussed in the RFC.
> 
> from the code in...
> 
> > 
> >     if ( opt_xenheap_megabytes )
> >         xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> >     else if ( reserved_heap )
> >         xenheap_pages = heap_pages;
> 
> ...here, setting xenheap_pages to heap_pages makes me a little bit
> confused.
> 
> >     else
> >     {
> >         xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
> >         xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
> >         xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
> >     }
> 
> If we keep this logic as this patch does, we can have the requirements...
> 
> > 
> > But also it looks like that on arm32 we have specific requirements for
> > Xen heap:
> > 
> >      *  - 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 least 32M
> 
> ...here, with the "1/32 the total RAM" now being "1/32 of the total reserved
> heap region", since heap_pages is now reserved_heap_pages.
 
I see. I didn't realize the full implications of the memory being used
for both xenheap and domheap on arm32. In that case, I would simply do
the following:


    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;

    if ( opt_xenheap_megabytes )
        xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
    else
    {
        xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
        xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
        xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
    }

    if ( reserved_heap )
        e = reserved_heap_end;
    else
    {
        do
        {
            e = consider_modules(ram_start, ram_end,
                                 pfn_to_paddr(xenheap_pages),
                                 32<<20, 0);

            if ( e )
                break;

            xenheap_pages >>= 1;
        } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
    }

    if ( ! e ||
         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
        panic("Not enough space for xenheap\n");

    domheap_pages = heap_pages - xenheap_pages;

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> > > But also it looks like that on arm32 we have specific requirements for
> > > Xen heap:
> > >
> > >      *  - 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 least 32M
> >
> > ...here, with the "1/32 the total RAM" now being "1/32 of the total reserved
> > heap region", since heap_pages is now reserved_heap_pages.
> 
> I see. I didn't realize the full implications of the memory being used
> for both xenheap and domheap on arm32. In that case, I would simply do
> the following:
> 
> 
>     heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> 
>     if ( opt_xenheap_megabytes )
>         xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
>     else
>     {
>         xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
>         xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
>         xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
>     }
> 
>     if ( reserved_heap )
>         e = reserved_heap_end;
>     else
>     {
>         do
>         {
>             e = consider_modules(ram_start, ram_end,
>                                  pfn_to_paddr(xenheap_pages),
>                                  32<<20, 0);
> 
>             if ( e )
>                 break;
> 
>             xenheap_pages >>= 1;
>         } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> PAGE_SHIFT) );
>     }
> 
>     if ( ! e ||
>          ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
>         panic("Not enough space for xenheap\n");
> 
>     domheap_pages = heap_pages - xenheap_pages;

Thanks very much for your time and patience. I will follow this way - with
the comment also updated of course (I didn't realize the comment needs to
be changed until yesterday when I sent the reply to your last comment.)

Kind regards,
Henry

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Arm maintainers,

> -----Original Message-----
> Hi Henry,
> 
> On 30/08/2022 08:11, Henry Wang wrote:
> >
> > Hi Michal,
> >
> > Sorry about the late reply - I had a couple of days off. Thank you very
> > much for the review! I will add my reply and answer some of your
> > questions below.
> >
> >> -----Original Message-----
> >> From: Michal Orzel <michal.orzel@amd.com>
> >> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot
> and
> >> heap allocator
> >>
> >>> This commit firstly adds a global variable `reserved_heap`.
> >>> This newly introduced global variable is set at the device tree
> >>> parsing time if the reserved heap ranges are defined in the device
> >>> tree chosen node.
> >>>
> >> Did you consider putting reserved_heap into bootinfo structure?
> >
> > Actually I did, but I saw current bootinfo only contains some structs so
> > I was not sure if this is the preferred way, but since you are raising this
> > question, I will follow this method in v2.
>
> This is what I think would be better but maintainers will have a decisive vote.

I think this is the only uncertain comment that I received during the latest
review of this series. I agree that Michal is making a good point (Thanks!) but we
are curious about what maintainers think. Could you please kindly share your
opinion on the more preferred approach? I will do that in v2. Thanks very much!

Kind regards,
Henry

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Bertrand Marquis]

Hi,

> On 1 Sep 2022, at 02:03, Henry Wang <Henry.Wang@arm.com> wrote:
> 
> Hi Arm maintainers,
> 
>> -----Original Message-----
>> Hi Henry,
>> 
>> On 30/08/2022 08:11, Henry Wang wrote:
>>> 
>>> Hi Michal,
>>> 
>>> Sorry about the late reply - I had a couple of days off. Thank you very
>>> much for the review! I will add my reply and answer some of your
>>> questions below.
>>> 
>>>> -----Original Message-----
>>>> From: Michal Orzel <michal.orzel@amd.com>
>>>> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot
>> and
>>>> heap allocator
>>>> 
>>>>> This commit firstly adds a global variable `reserved_heap`.
>>>>> This newly introduced global variable is set at the device tree
>>>>> parsing time if the reserved heap ranges are defined in the device
>>>>> tree chosen node.
>>>>> 
>>>> Did you consider putting reserved_heap into bootinfo structure?
>>> 
>>> Actually I did, but I saw current bootinfo only contains some structs so
>>> I was not sure if this is the preferred way, but since you are raising this
>>> question, I will follow this method in v2.
>> 
>> This is what I think would be better but maintainers will have a decisive vote.
> 
> I think this is the only uncertain comment that I received during the latest
> review of this series. I agree that Michal is making a good point (Thanks!) but we
> are curious about what maintainers think. Could you please kindly share your
> opinion on the more preferred approach? I will do that in v2. Thanks very much!

I think it does make sense to put this in bootinfo. 

Cheers
Bertrand

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Bertrand,

> -----Original Message-----
> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
> > On 1 Sep 2022, at 02:03, Henry Wang <Henry.Wang@arm.com> wrote:
> >
> > Hi Arm maintainers,
> >
> >> -----Original Message-----
> >> Hi Henry,
> >>
> >> On 30/08/2022 08:11, Henry Wang wrote:
> >>>
> >>> Hi Michal,
> >>>
> >>> Sorry about the late reply - I had a couple of days off. Thank you very
> >>> much for the review! I will add my reply and answer some of your
> >>> questions below.
> >>>
> >>>> -----Original Message-----
> >>>> From: Michal Orzel <michal.orzel@amd.com>
> >>>> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot
> >> and
> >>>> heap allocator
> >>>>
> >>>>> This commit firstly adds a global variable `reserved_heap`.
> >>>>> This newly introduced global variable is set at the device tree
> >>>>> parsing time if the reserved heap ranges are defined in the device
> >>>>> tree chosen node.
> >>>>>
> >>>> Did you consider putting reserved_heap into bootinfo structure?
> >>>
> >>> Actually I did, but I saw current bootinfo only contains some structs so
> >>> I was not sure if this is the preferred way, but since you are raising this
> >>> question, I will follow this method in v2.
> >>
> >> This is what I think would be better but maintainers will have a decisive
> vote.
> >
> > I think this is the only uncertain comment that I received during the latest
> > review of this series. I agree that Michal is making a good point (Thanks!)
> but we
> > are curious about what maintainers think. Could you please kindly share
> your
> > opinion on the more preferred approach? I will do that in v2. Thanks very
> much!
> 
> I think it does make sense to put this in bootinfo.

I am good with that, then I think I will move this to bootinfo in v2 unless other
objections. Thank you for the input.

Kind regards,
Henry

> 
> Cheers
> Bertrand
> 

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Julien Grall]

On 30/08/2022 02:04, Stefano Stabellini wrote:
>>   size_t estimate_efi_size(unsigned int mem_nr_banks);
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 500307edc0..fe76cf6325 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>>   
>>   domid_t __read_mostly max_init_domid;
>>   
>> +bool __read_mostly reserved_heap;
>> +
>>   static __used void init_done(void)
>>   {
>>       /* Must be done past setting system_state. */
>> @@ -699,8 +701,10 @@ 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 reserved_heap_start = ~0, reserved_heap_end = 0,
> 
> INVALID_PADDR or ~0ULL

I would strongly prefer the former. It is more difficult to understand 
what's the value means in the latter.

Cheers,

-- 
Julien Grall

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> >> +    paddr_t reserved_heap_start = ~0, reserved_heap_end = 0,
> >
> > INVALID_PADDR or ~0ULL
> 
> I would strongly prefer the former. It is more difficult to understand
> what's the value means in the latter.

Thanks for the input. You mean the INVALID_PADDR correct? Yeah
this is the one that I used in my local v2, will send it tomorrow after
doing the bootinfo change.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Julien Grall]

On 01/09/2022 15:01, Henry Wang wrote:
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>>>> +    paddr_t reserved_heap_start = ~0, reserved_heap_end = 0,
>>>
>>> INVALID_PADDR or ~0ULL
>>
>> I would strongly prefer the former. It is more difficult to understand
>> what's the value means in the latter.
> 
> Thanks for the input. You mean the INVALID_PADDR correct? 

That's correct.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Julien Grall]

Hi Henry,

On 24/08/2022 08:31, Henry Wang wrote:
> This commit introduces the reserved heap memory, which is parts of RAM
> reserved in the beginning of the boot time for heap.
> 
> A new boolean field `xen_heap` in `struct membank` is added to store the
> configuration telling if the memory bank is reserved as heap through
> `xen,static-mem` property in device tree `chosen` node.
> 
> Also, this commit introduces the logic to parse the reserved heap
> configuation in device tree by reusing the device tree entry definition

typo: s/configuation/configuration/

> of the static memory allocation feature:
> 
> - Add a boolean parameter `xen_heap` to `device_tree_get_meminfo` to
> reflect whether the memory bank is reserved as heap.
> 
> - Use `device_tree_get_meminfo` to parse the reserved heap configuation
> in `chosen` node of the device tree.
> 
> - In order to reuse the function `device_tree_get_meminfo`, the
> return type of `process_chosen_node` is changed from void to int.
> 
> A documentation section is added, describing the definition of reserved
> heap memory and the method of enabling the reserved 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>
> ---
> The name of the device tree property was chosen because we want to
> reuse as much as the device tree parsing helpers from the static
> memory allocation feature, but we would like to hear the upstream
> reviewers' opinion about if using "xen,static-heap" is better.
> ---
> Changes from RFC to v1:
> - Rename the terminology to reserved heap.
> ---
>   docs/misc/arm/device-tree/booting.txt | 46 +++++++++++++++++++++++++

I have skipped the documentation and looked at the code instead.

>   xen/arch/arm/bootfdt.c                | 49 +++++++++++++++++++++------
>   xen/arch/arm/domain_build.c           |  5 +--
>   xen/arch/arm/include/asm/setup.h      |  1 +
>   4 files changed, 89 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..e064f64d9a 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -378,3 +378,49 @@ device-tree:
>   
>   This will reserve a 512MB region starting at the host physical address
>   0x30000000 to be exclusively used by DomU1.
> +
> +
> +Reserved Heap Memory
> +====================
> +
> +The reserved heap memory (also known as the statically-configured heap) refers
> +to parts of RAM reserved in the beginning for heap. The memory is reserved by
> +configuration in the device tree using physical address ranges.
> +
> +The reserved 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 can be seperated heaps, the reserved heap will be used
> +for both domheap and xenheap.
> +- For Arm64, since domheap and xenheap are the same, the defined reserved heap
> +areas shall always go to the heap allocator.
> +
> +The reserved heap memory is an optional feature and can be enabled by adding a
> +device tree property in the `chosen` node. Currently, this feature reuses the
> +static memory allocation device tree configuration.
> +
> +The dtb property should look like as follows:
> +
> +- property name
> +
> +    "xen,static-mem" (Should be used in the `chosen` node)
> +
> +- cells
> +
> +    Specify the start address and the length of the reserved heap memory.
> +    The number of cells for the address and the size should be defined
> +    using the properties `#xen,static-mem-address-cells` and
> +    `#xen,static-mem-size-cells` respectively.
> +
> +Below is an example on how to specify the reserved heap in device tree:
> +
> +    / {
> +        chosen {
> +            #xen,static-mem-address-cells = <0x2>;
> +            #xen,static-mem-size-cells = <0x2>;
> +            xen,static-mem = <0x0 0x30000000 0x0 0x40000000>;
> +        };
> +    };
> +
> +RAM at 0x30000000 of 1G size will be reserved as heap.
> +
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index ec81a45de9..33704ca487 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -64,7 +64,8 @@ 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, bool xen_domain,
> +                                          bool xen_heap)

It sounds like to me, we want to have an enum rather than multiple 
boolean. This would also make easier...

>   {
>       const struct fdt_property *prop;
>       unsigned int i, banks;
> @@ -96,6 +97,7 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
>           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].xen_heap = xen_heap;
>           mem->nr_banks++;
>       }
>   
> @@ -185,7 +187,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, false, false);

... to understand the two "false" here.

>   }
>   
>   static int __init process_reserved_memory_node(const void *fdt, int node,
> @@ -293,7 +295,7 @@ 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,
> +static int __init process_chosen_node(const void *fdt, int node,
>                                          const char *name,
>                                          u32 address_cells, u32 size_cells) >   {
> @@ -301,16 +303,40 @@ static void __init process_chosen_node(const void *fdt, int node,
>       paddr_t start, end;
>       int len;
>   
> +    if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
> +    {
> +        u32 address_cells = device_tree_get_u32(fdt, node,
> +                                                "#xen,static-mem-address-cells",
> +                                                0);
> +        u32 size_cells = device_tree_get_u32(fdt, node,
> +                                             "#xen,static-mem-size-cells", 0);
> +        int rc;
> +
> +        printk("Checking for reserved heap in /chosen\n");
> +        if ( address_cells < 1 || size_cells < 1 )
> +        {
> +            printk("fdt: node `%s': invalid #xen,static-mem-address-cells or #xen,static-mem-size-cells\n",
> +                   name);
> +            return -EINVAL;
> +        }
> +
> +        rc = device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
> +                                     size_cells, &bootinfo.reserved_mem, false,
> +                                     true);
> +        if ( rc )
> +            return rc;
> +    }
> +
>       printk("Checking for initrd in /chosen\n");
>   
>       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;

This is technically a change in behavior for Xen (we would panic rather 
than continue). I am happy with the proposal. However, this doesn't seem 
to be explained in the commit message.

That said, I think this should be split in a separate patch along with 
the ones below (including the prototype changes).

>       }
>       start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
>   
> @@ -318,12 +344,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 +357,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,
> @@ -358,7 +386,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, true,
> +                                   false);
>   }
>   
>   static int __init early_scan_node(const void *fdt,
> @@ -383,7 +412,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);
>   
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3fd1186b53..6f97f5f06a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1038,8 +1038,9 @@ 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 first memory range not bound to neither a Xen domain nor heap */
> +    for ( i = 0; i < mem->nr_banks &&
> +                 (mem->bank[i].xen_domain || mem->bank[i].xen_heap); i++ )
>           ;
>       if ( i == mem->nr_banks )
>           return 0;
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 2bb01ecfa8..e80f3d6201 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -27,6 +27,7 @@ struct membank {
>       paddr_t start;
>       paddr_t size;
>       bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
> +    bool xen_heap;   /* whether the memory bank is reserved as heap. */

We have multiple heap: static, domain, xen. AFAIU, 'xen_heap' refers to 
both domain and xen whereas 'xen_domain' refers to 'static'.

In line with what I wrote above, I think it would be better if we have a 
single field 'heap' which is an enum listing the type of heap.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Julien Grall]

Hi Henry,

On 24/08/2022 08:31, Henry Wang wrote:
> This commit firstly adds a global variable `reserved_heap`.
> This newly introduced global variable is set at the device tree
> parsing time if the reserved heap ranges are defined in the device
> tree chosen node.
> 
> For Arm32, In `setup_mm`, if the reserved heap is enabled, we use
> the reserved heap region for both domheap and xenheap allocation.
> 
> For Arm64, In `setup_mm`, if the reserved heap is enabled and used,
> we make sure that only these reserved heap pages are added to the
> boot allocator. These reserved heap pages in the boot allocator are
> added to the heap allocator at `end_boot_allocator()`.
> 
> If the reserved 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()`.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> ---
> With reserved heap enabled, for Arm64, naming of global variables such
> as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous,
> wondering if we should rename these variables.
> ---
> 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 |  2 +
>   xen/arch/arm/setup.c             | 79 +++++++++++++++++++++++++-------
>   3 files changed, 67 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 33704ca487..ab73b6e212 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt, int node,
>                                        true);
>           if ( rc )
>               return rc;
> +
> +        reserved_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 e80f3d6201..00536a6d55 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo;
>   
>   extern domid_t max_init_domid;
>   
> +extern bool reserved_heap;
> +
>   void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
>   
>   size_t estimate_efi_size(unsigned int mem_nr_banks);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 500307edc0..fe76cf6325 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>   
>   domid_t __read_mostly max_init_domid;
>   
> +bool __read_mostly reserved_heap;
> +
>   static __used void init_done(void)
>   {
>       /* Must be done past setting system_state. */
> @@ -699,8 +701,10 @@ 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 reserved_heap_start = ~0, reserved_heap_end = 0,
> +            reserved_heap_size = 0;
> +    unsigned long ram_pages, reserved_heap_pages = 0;
>       unsigned long heap_pages, xenheap_pages, domheap_pages;
>       unsigned int i;
>       const uint32_t ctr = READ_CP32(CTR);
> @@ -720,9 +724,9 @@ 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);
> @@ -731,6 +735,25 @@ static void __init setup_mm(void)
>   
>       total_pages = ram_pages = ram_size >> PAGE_SHIFT;
>   
> +    if ( reserved_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> +            {
> +                bank_start = bootinfo.reserved_mem.bank[i].start;
> +                bank_size = bootinfo.reserved_mem.bank[i].size;
> +                bank_end = bank_start + bank_size;
> +
> +                reserved_heap_size += bank_size;
> +                reserved_heap_start = min(reserved_heap_start, bank_start);
> +                reserved_heap_end = max(reserved_heap_end, bank_end);
> +            }
> +        }
> +
> +        reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT;
> +    }
> +
>       /*
>        * If the user has not requested otherwise via the command line
>        * then locate the xenheap using these constraints:
> @@ -743,7 +766,8 @@ static void __init setup_mm(void)
>        * We try to allocate the largest xenheap possible within these
>        * constraints.
>        */
> -    heap_pages = ram_pages;
> +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> +
>       if ( opt_xenheap_megabytes )
>           xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
>       else
> @@ -755,17 +779,21 @@ static void __init setup_mm(void)
>   
>       do
>       {
> -        e = consider_modules(ram_start, ram_end,
> +        e = !reserved_heap ?
> +            consider_modules(ram_start, ram_end,
>                                pfn_to_paddr(xenheap_pages),
> -                             32<<20, 0);
> +                             32<<20, 0) :
> +            reserved_heap_end;

Not entirely related to this series. Now the assumption is the admin 
will make sure that none of the reserved regions will overlap.

Do we have any tool to help the admin to verify it? If yes, can we have 
a pointer in the documentation? If not, should this be done in Xen?

Also, what happen with UEFI? Is it easy to guarantee the region will not 
be used?

> +
>           if ( e )
>               break;
>   
>           xenheap_pages >>= 1;
>       } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
>   
> -    if ( ! e )
> -        panic("Not not enough space for xenheap\n");
> +    if ( ! e ||
> +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
> +        panic("Not enough space for xenheap\n");

So on arm32, the xenheap *must* be contiguous. AFAICT, 
reserved_heap_pages is the total number of pages in the heap. They may 
not be contiguous. So I think this wants to be reworked so we look for 
one of the region that match the definition written above the loop.

>   
>       domheap_pages = heap_pages - xenheap_pages;
>   
> @@ -810,9 +838,9 @@ 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_end = 0;
> -    paddr_t ram_size = 0;
> +    paddr_t ram_start = ~0, bank_start = ~0;
> +    paddr_t ram_end = 0, bank_end = 0;
> +    paddr_t ram_size = 0, bank_size = 0;
>       unsigned int i;
>   
>       init_pdx();
> @@ -821,17 +849,36 @@ static void __init setup_mm(void)
>        * We need some memory to allocate the page-tables used for the xenheap
>        * mappings. But some regions may contain memory already allocated
>        * for other uses (e.g. modules, reserved-memory...).
> -     *
> +     * If reserved heap regions are properly defined, (only) add these regions
> +     * in the boot allocator. > +     */
> +    if ( reserved_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> +            {
> +                bank_start = bootinfo.reserved_mem.bank[i].start;
> +                bank_size = bootinfo.reserved_mem.bank[i].size;
> +                bank_end = bank_start + bank_size;
> +
> +                init_boot_pages(bank_start, bank_end);
> +            }
> +        }
> +    }
> +    /*
> +     * No reserved heap regions:
>        * For simplicity, add all the free regions in the boot allocator.
>        */
> -    populate_boot_allocator();
> +    else
> +        populate_boot_allocator();
>   
>       total_pages = 0;
>   
>       for ( i = 0; i < banks->nr_banks; i++ )
>       {

This code is now becoming quite confusing to understanding. This loop is 
meant to map the xenheap. If I follow your documentation, it would mean 
that only the reserved region should be mapped.

More confusingly, xenheap_* variables will cover the full RAM. 
Effectively, this is now more obvious that this is use for 
direct-mapping. So I think it would be better to rename the variable to 
directmap_* or similar.

Ideally this should be in a separate patch.

Cheers,

-- 
Julien Grall

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Julien,

Thanks for your review.

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> heap allocator
> 
> Hi Henry,
> 
> On 24/08/2022 08:31, Henry Wang wrote:
> > This commit firstly adds a global variable `reserved_heap`.
> > This newly introduced global variable is set at the device tree
> > parsing time if the reserved heap ranges are defined in the device
> > tree chosen node.
> >
> > For Arm32, In `setup_mm`, if the reserved heap is enabled, we use
> > the reserved heap region for both domheap and xenheap allocation.
> >
> > For Arm64, In `setup_mm`, if the reserved heap is enabled and used,
> > we make sure that only these reserved heap pages are added to the
> > boot allocator. These reserved heap pages in the boot allocator are
> > added to the heap allocator at `end_boot_allocator()`.
> >
> > If the reserved 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()`.
> >
> > Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> > ---
> > With reserved heap enabled, for Arm64, naming of global variables such
> > as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous,
> > wondering if we should rename these variables.
> > ---
> > 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 |  2 +
> >   xen/arch/arm/setup.c             | 79 +++++++++++++++++++++++++-------
> >   3 files changed, 67 insertions(+), 16 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 33704ca487..ab73b6e212 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void
> *fdt, int node,
> >                                        true);
> >           if ( rc )
> >               return rc;
> > +
> > +        reserved_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 e80f3d6201..00536a6d55 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo;
> >
> >   extern domid_t max_init_domid;
> >
> > +extern bool reserved_heap;
> > +
> >   void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> >
> >   size_t estimate_efi_size(unsigned int mem_nr_banks);
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 500307edc0..fe76cf6325 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes",
> opt_xenheap_megabytes);
> >
> >   domid_t __read_mostly max_init_domid;
> >
> > +bool __read_mostly reserved_heap;
> > +
> >   static __used void init_done(void)
> >   {
> >       /* Must be done past setting system_state. */
> > @@ -699,8 +701,10 @@ 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 reserved_heap_start = ~0, reserved_heap_end = 0,
> > +            reserved_heap_size = 0;
> > +    unsigned long ram_pages, reserved_heap_pages = 0;
> >       unsigned long heap_pages, xenheap_pages, domheap_pages;
> >       unsigned int i;
> >       const uint32_t ctr = READ_CP32(CTR);
> > @@ -720,9 +724,9 @@ 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);
> > @@ -731,6 +735,25 @@ static void __init setup_mm(void)
> >
> >       total_pages = ram_pages = ram_size >> PAGE_SHIFT;
> >
> > +    if ( reserved_heap )
> > +    {
> > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > +            {
> > +                bank_start = bootinfo.reserved_mem.bank[i].start;
> > +                bank_size = bootinfo.reserved_mem.bank[i].size;
> > +                bank_end = bank_start + bank_size;
> > +
> > +                reserved_heap_size += bank_size;
> > +                reserved_heap_start = min(reserved_heap_start, bank_start);
> > +                reserved_heap_end = max(reserved_heap_end, bank_end);
> > +            }
> > +        }
> > +
> > +        reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT;
> > +    }
> > +
> >       /*
> >        * If the user has not requested otherwise via the command line
> >        * then locate the xenheap using these constraints:
> > @@ -743,7 +766,8 @@ static void __init setup_mm(void)
> >        * We try to allocate the largest xenheap possible within these
> >        * constraints.
> >        */
> > -    heap_pages = ram_pages;
> > +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> > +
> >       if ( opt_xenheap_megabytes )
> >           xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> >       else
> > @@ -755,17 +779,21 @@ static void __init setup_mm(void)
> >
> >       do
> >       {
> > -        e = consider_modules(ram_start, ram_end,
> > +        e = !reserved_heap ?
> > +            consider_modules(ram_start, ram_end,
> >                                pfn_to_paddr(xenheap_pages),
> > -                             32<<20, 0);
> > +                             32<<20, 0) :
> > +            reserved_heap_end;
> 
> Not entirely related to this series. Now the assumption is the admin
> will make sure that none of the reserved regions will overlap.
> 
> Do we have any tool to help the admin to verify it? If yes, can we have
> a pointer in the documentation? If not, should this be done in Xen?

In the RFC we had the same discussion of this issue [1] and I think a
follow-up series might needed to do the overlap check if we want to
do that in Xen. For the existing tool, I am thinking of ImageBuilder, but
I am curious about Stefano's opinion.

> 
> Also, what happen with UEFI? Is it easy to guarantee the region will not
> be used?

For now I think it is not easy to guarantee that, do you have some ideas
in mind? I think I can follow this in above follow-up series to improve things. 

> 
> > +
> >           if ( e )
> >               break;
> >
> >           xenheap_pages >>= 1;
> >       } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> PAGE_SHIFT) );
> >
> > -    if ( ! e )
> > -        panic("Not not enough space for xenheap\n");
> > +    if ( ! e ||
> > +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
> > +        panic("Not enough space for xenheap\n");
> 
> So on arm32, the xenheap *must* be contiguous. AFAICT,
> reserved_heap_pages is the total number of pages in the heap. They may
> not be contiguous. So I think this wants to be reworked so we look for
> one of the region that match the definition written above the loop.

Thanks for raising this concern, I will do this in V2.

> 
> >
> >       domheap_pages = heap_pages - xenheap_pages;
> >
> > @@ -810,9 +838,9 @@ 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_end = 0;
> > -    paddr_t ram_size = 0;
> > +    paddr_t ram_start = ~0, bank_start = ~0;
> > +    paddr_t ram_end = 0, bank_end = 0;
> > +    paddr_t ram_size = 0, bank_size = 0;
> >       unsigned int i;
> >
> >       init_pdx();
> > @@ -821,17 +849,36 @@ static void __init setup_mm(void)
> >        * We need some memory to allocate the page-tables used for the
> xenheap
> >        * mappings. But some regions may contain memory already allocated
> >        * for other uses (e.g. modules, reserved-memory...).
> > -     *
> > +     * If reserved heap regions are properly defined, (only) add these
> regions
> > +     * in the boot allocator. > +     */
> > +    if ( reserved_heap )
> > +    {
> > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > +            {
> > +                bank_start = bootinfo.reserved_mem.bank[i].start;
> > +                bank_size = bootinfo.reserved_mem.bank[i].size;
> > +                bank_end = bank_start + bank_size;
> > +
> > +                init_boot_pages(bank_start, bank_end);
> > +            }
> > +        }
> > +    }
> > +    /*
> > +     * No reserved heap regions:
> >        * For simplicity, add all the free regions in the boot allocator.
> >        */
> > -    populate_boot_allocator();
> > +    else
> > +        populate_boot_allocator();
> >
> >       total_pages = 0;
> >
> >       for ( i = 0; i < banks->nr_banks; i++ )
> >       {
> 
> This code is now becoming quite confusing to understanding. This loop is
> meant to map the xenheap. If I follow your documentation, it would mean
> that only the reserved region should be mapped.

Yes I think this is the same question that I raised in the scissors line of the
commit message of this patch. What I intend to do is still mapping the whole
RAM because of the xenheap_* variables that you mentioned in...

> 
> More confusingly, xenheap_* variables will cover the full RAM.

...here. But only adding the reserved region to the boot allocator so the
reserved region can become the heap later on. I am wondering if we
have a more clear way to do that, any suggestions?

> Effectively, this is now more obvious that this is use for
> direct-mapping. So I think it would be better to rename the variable to
> directmap_* or similar.
> 
> Ideally this should be in a separate patch.

[1] https://lore.kernel.org/xen-devel/48a0712c-eff8-dfc1-2136-59317f22321f@xen.org/

Kind regards,
Henry


> 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Stefano Stabellini]

On Thu, 1 Sep 2022, Henry Wang wrote:
> > -----Original Message-----
> > From: Julien Grall <julien@xen.org>
> > Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> > heap allocator
> > 
> > Hi Henry,
> > 
> > On 24/08/2022 08:31, Henry Wang wrote:
> > > This commit firstly adds a global variable `reserved_heap`.
> > > This newly introduced global variable is set at the device tree
> > > parsing time if the reserved heap ranges are defined in the device
> > > tree chosen node.
> > >
> > > For Arm32, In `setup_mm`, if the reserved heap is enabled, we use
> > > the reserved heap region for both domheap and xenheap allocation.
> > >
> > > For Arm64, In `setup_mm`, if the reserved heap is enabled and used,
> > > we make sure that only these reserved heap pages are added to the
> > > boot allocator. These reserved heap pages in the boot allocator are
> > > added to the heap allocator at `end_boot_allocator()`.
> > >
> > > If the reserved 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()`.
> > >
> > > Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> > > ---
> > > With reserved heap enabled, for Arm64, naming of global variables such
> > > as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous,
> > > wondering if we should rename these variables.
> > > ---
> > > 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 |  2 +
> > >   xen/arch/arm/setup.c             | 79 +++++++++++++++++++++++++-------
> > >   3 files changed, 67 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > index 33704ca487..ab73b6e212 100644
> > > --- a/xen/arch/arm/bootfdt.c
> > > +++ b/xen/arch/arm/bootfdt.c
> > > @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void
> > *fdt, int node,
> > >                                        true);
> > >           if ( rc )
> > >               return rc;
> > > +
> > > +        reserved_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 e80f3d6201..00536a6d55 100644
> > > --- a/xen/arch/arm/include/asm/setup.h
> > > +++ b/xen/arch/arm/include/asm/setup.h
> > > @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo;
> > >
> > >   extern domid_t max_init_domid;
> > >
> > > +extern bool reserved_heap;
> > > +
> > >   void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> > >
> > >   size_t estimate_efi_size(unsigned int mem_nr_banks);
> > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > > index 500307edc0..fe76cf6325 100644
> > > --- a/xen/arch/arm/setup.c
> > > +++ b/xen/arch/arm/setup.c
> > > @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes",
> > opt_xenheap_megabytes);
> > >
> > >   domid_t __read_mostly max_init_domid;
> > >
> > > +bool __read_mostly reserved_heap;
> > > +
> > >   static __used void init_done(void)
> > >   {
> > >       /* Must be done past setting system_state. */
> > > @@ -699,8 +701,10 @@ 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 reserved_heap_start = ~0, reserved_heap_end = 0,
> > > +            reserved_heap_size = 0;
> > > +    unsigned long ram_pages, reserved_heap_pages = 0;
> > >       unsigned long heap_pages, xenheap_pages, domheap_pages;
> > >       unsigned int i;
> > >       const uint32_t ctr = READ_CP32(CTR);
> > > @@ -720,9 +724,9 @@ 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);
> > > @@ -731,6 +735,25 @@ static void __init setup_mm(void)
> > >
> > >       total_pages = ram_pages = ram_size >> PAGE_SHIFT;
> > >
> > > +    if ( reserved_heap )
> > > +    {
> > > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > > +        {
> > > +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > > +            {
> > > +                bank_start = bootinfo.reserved_mem.bank[i].start;
> > > +                bank_size = bootinfo.reserved_mem.bank[i].size;
> > > +                bank_end = bank_start + bank_size;
> > > +
> > > +                reserved_heap_size += bank_size;
> > > +                reserved_heap_start = min(reserved_heap_start, bank_start);
> > > +                reserved_heap_end = max(reserved_heap_end, bank_end);
> > > +            }
> > > +        }
> > > +
> > > +        reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT;
> > > +    }
> > > +
> > >       /*
> > >        * If the user has not requested otherwise via the command line
> > >        * then locate the xenheap using these constraints:
> > > @@ -743,7 +766,8 @@ static void __init setup_mm(void)
> > >        * We try to allocate the largest xenheap possible within these
> > >        * constraints.
> > >        */
> > > -    heap_pages = ram_pages;
> > > +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> > > +
> > >       if ( opt_xenheap_megabytes )
> > >           xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> > >       else
> > > @@ -755,17 +779,21 @@ static void __init setup_mm(void)
> > >
> > >       do
> > >       {
> > > -        e = consider_modules(ram_start, ram_end,
> > > +        e = !reserved_heap ?
> > > +            consider_modules(ram_start, ram_end,
> > >                                pfn_to_paddr(xenheap_pages),
> > > -                             32<<20, 0);
> > > +                             32<<20, 0) :
> > > +            reserved_heap_end;
> > 
> > Not entirely related to this series. Now the assumption is the admin
> > will make sure that none of the reserved regions will overlap.
> > 
> > Do we have any tool to help the admin to verify it? If yes, can we have
> > a pointer in the documentation? If not, should this be done in Xen?
> 
> In the RFC we had the same discussion of this issue [1] and I think a
> follow-up series might needed to do the overlap check if we want to
> do that in Xen. For the existing tool, I am thinking of ImageBuilder, but
> I am curious about Stefano's opinion.

Yes, ImageBuilder is a good option and we moved ImageBuilder under Xen
Project to make it easier for people to contribute to it:

https://gitlab.com/xen-project/imagebuilder


> > Also, what happen with UEFI? Is it easy to guarantee the region will not
> > be used?
> 
> For now I think it is not easy to guarantee that, do you have some ideas
> in mind? I think I can follow this in above follow-up series to improve things. 

For clarity, are we worried that the region is used by the bootloader
for other things? For instance U-Boot or Tianocore placing some
firmware tables inside the range specified for xenheap?

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Julien Grall]

Hi Henry,

On 01/09/2022 17:05, Henry Wang wrote:
>>> @@ -755,17 +779,21 @@ static void __init setup_mm(void)
>>>
>>>        do
>>>        {
>>> -        e = consider_modules(ram_start, ram_end,
>>> +        e = !reserved_heap ?
>>> +            consider_modules(ram_start, ram_end,
>>>                                 pfn_to_paddr(xenheap_pages),
>>> -                             32<<20, 0);
>>> +                             32<<20, 0) :
>>> +            reserved_heap_end;
>>
>> Not entirely related to this series. Now the assumption is the admin
>> will make sure that none of the reserved regions will overlap.
>>
>> Do we have any tool to help the admin to verify it? If yes, can we have
>> a pointer in the documentation? If not, should this be done in Xen?
> 
> In the RFC we had the same discussion of this issue [1] and I think a
> follow-up series might needed to do the overlap check if we want to
> do that in Xen. For the existing tool, I am thinking of ImageBuilder, but
> I am curious about Stefano's opinion.
> 
>>
>> Also, what happen with UEFI? Is it easy to guarantee the region will not
>> be used?
> 
> For now I think it is not easy to guarantee that, do you have some ideas
> in mind? I think I can follow this in above follow-up series to improve things.

I don't have any ideas how we can guarantee it (even when using image 
builder). I think we may have to end up to check the overlaps in Xen.

> 
>>
>>> +
>>>            if ( e )
>>>                break;
>>>
>>>            xenheap_pages >>= 1;
>>>        } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
>> PAGE_SHIFT) );
>>>
>>> -    if ( ! e )
>>> -        panic("Not not enough space for xenheap\n");
>>> +    if ( ! e ||
>>> +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
>>> +        panic("Not enough space for xenheap\n");
>>
>> So on arm32, the xenheap *must* be contiguous. AFAICT,
>> reserved_heap_pages is the total number of pages in the heap. They may
>> not be contiguous. So I think this wants to be reworked so we look for
>> one of the region that match the definition written above the loop.
> 
> Thanks for raising this concern, I will do this in V2.
> 
>>
>>>
>>>        domheap_pages = heap_pages - xenheap_pages;
>>>
>>> @@ -810,9 +838,9 @@ 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_end = 0;
>>> -    paddr_t ram_size = 0;
>>> +    paddr_t ram_start = ~0, bank_start = ~0;
>>> +    paddr_t ram_end = 0, bank_end = 0;
>>> +    paddr_t ram_size = 0, bank_size = 0;
>>>        unsigned int i;
>>>
>>>        init_pdx();
>>> @@ -821,17 +849,36 @@ static void __init setup_mm(void)
>>>         * We need some memory to allocate the page-tables used for the
>> xenheap
>>>         * mappings. But some regions may contain memory already allocated
>>>         * for other uses (e.g. modules, reserved-memory...).
>>> -     *
>>> +     * If reserved heap regions are properly defined, (only) add these
>> regions
>>> +     * in the boot allocator. > +     */
>>> +    if ( reserved_heap )
>>> +    {
>>> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
>>> +        {
>>> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
>>> +            {
>>> +                bank_start = bootinfo.reserved_mem.bank[i].start;
>>> +                bank_size = bootinfo.reserved_mem.bank[i].size;
>>> +                bank_end = bank_start + bank_size;
>>> +
>>> +                init_boot_pages(bank_start, bank_end);
>>> +            }
>>> +        }
>>> +    }
>>> +    /*
>>> +     * No reserved heap regions:
>>>         * For simplicity, add all the free regions in the boot allocator.
>>>         */
>>> -    populate_boot_allocator();
>>> +    else
>>> +        populate_boot_allocator();
>>>
>>>        total_pages = 0;
>>>
>>>        for ( i = 0; i < banks->nr_banks; i++ )
>>>        {
>>
>> This code is now becoming quite confusing to understanding. This loop is
>> meant to map the xenheap. If I follow your documentation, it would mean
>> that only the reserved region should be mapped.
> 
> Yes I think this is the same question that I raised in the scissors line of the
> commit message of this patch.

Sorry I didn't notice the comment after the scissors line. This is the 
same question :)

> What I intend to do is still mapping the whole
> RAM because of the xenheap_* variables that you mentioned in...
> 
>>
>> More confusingly, xenheap_* variables will cover the full RAM.
> 
> ...here. But only adding the reserved region to the boot allocator so the
> reserved region can become the heap later on. I am wondering if we
> have a more clear way to do that, any suggestions?

I think your code is correct. It only needs some renaming of the 
existing variable (maybe to directmap_*?) to make clear the area is used 
to access the RAM easily.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Julien Grall]

Hi Stefano,

On 01/09/2022 18:08, Stefano Stabellini wrote:
>>> Also, what happen with UEFI? Is it easy to guarantee the region will not
>>> be used?
>>
>> For now I think it is not easy to guarantee that, do you have some ideas
>> in mind? I think I can follow this in above follow-up series to improve things.
> 
> For clarity, are we worried that the region is used by the bootloader
> for other things? For instance U-Boot or Tianocore placing some
> firmware tables inside the range specified for xenheap?

Yes. I think it would be difficult for an admin to figure out which 
regions are used. Although they are likely (?) going to be static for a 
given UEFI/U-boot build.

My major concern is such bug can be very difficult to root cause because 
we have no safety in Xen. The most likely symptom would be random 
corruption.

Cheers,

-- 
Julien Grall

RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory
> 
> Hi Henry,
> 
> > Also, this commit introduces the logic to parse the reserved heap
> > configuation in device tree by reusing the device tree entry definition
> 
> typo: s/configuation/configuration/

Oops, sorry for that...

> 
> >   docs/misc/arm/device-tree/booting.txt | 46
> +++++++++++++++++++++++++
> 
> I have skipped the documentation and looked at the code instead.

No problem, Stefano and Michal have already provided some comments
in the doc.

> 
> It sounds like to me, we want to have an enum rather than multiple
> boolean. This would also make easier...
> 
> >   {
> >       const struct fdt_property *prop;
> >       unsigned int i, banks;
> > @@ -96,6 +97,7 @@ static int __init device_tree_get_meminfo(const void
> *fdt, int node,
> >           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].xen_heap = xen_heap;
> >           mem->nr_banks++;
> >       }
> >
> > @@ -185,7 +187,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, false, false);
> 
> ... to understand the two "false" here.

I agree, will change in v2.

> 
> >   }
> >
> >   static int __init process_reserved_memory_node(const void *fdt, int node,
> > @@ -293,7 +295,7 @@ 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,
> > +static int __init process_chosen_node(const void *fdt, int node,
> >                                          const char *name,
> >                                          u32 address_cells, u32 size_cells) >   {
> > @@ -301,16 +303,40 @@ static void __init process_chosen_node(const
> void *fdt, int node,
> >       paddr_t start, end;
> >       int len;
> >
> > +    if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
> > +    {
> > +        u32 address_cells = device_tree_get_u32(fdt, node,
> > +                                                "#xen,static-mem-address-cells",
> > +                                                0);
> > +        u32 size_cells = device_tree_get_u32(fdt, node,
> > +                                             "#xen,static-mem-size-cells", 0);
> > +        int rc;
> > +
> > +        printk("Checking for reserved heap in /chosen\n");
> > +        if ( address_cells < 1 || size_cells < 1 )
> > +        {
> > +            printk("fdt: node `%s': invalid #xen,static-mem-address-cells or
> #xen,static-mem-size-cells\n",
> > +                   name);
> > +            return -EINVAL;
> > +        }
> > +
> > +        rc = device_tree_get_meminfo(fdt, node, "xen,static-mem",
> address_cells,
> > +                                     size_cells, &bootinfo.reserved_mem, false,
> > +                                     true);
> > +        if ( rc )
> > +            return rc;
> > +    }
> > +
> >       printk("Checking for initrd in /chosen\n");
> >
> >       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;
> 
> This is technically a change in behavior for Xen (we would panic rather
> than continue). I am happy with the proposal. However, this doesn't seem
> to be explained in the commit message.
> 
> That said, I think this should be split in a separate patch along with
> the ones below (including the prototype changes).

According to Michal's comment, I've removed the return type and function
prototype change in my local v2. So this patch itself is fine. My question now
would be, do maintainers think this change of behavior with processing the
chosen node be helpful? Do we prefer an instant panic or current behavior?

I am more than happy to add a patch for changing the return/panic behavior
if everyone is happy with it.

> 
> >       }
> >       start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> >
> > @@ -318,12 +344,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 +357,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,
> > @@ -358,7 +386,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, true,
> > +                                   false);
> >   }
> >
> >   static int __init early_scan_node(const void *fdt,
> > @@ -383,7 +412,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);
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 3fd1186b53..6f97f5f06a 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1038,8 +1038,9 @@ 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 first memory range not bound to neither a Xen domain nor heap
> */
> > +    for ( i = 0; i < mem->nr_banks &&
> > +                 (mem->bank[i].xen_domain || mem->bank[i].xen_heap); i++ )
> >           ;
> >       if ( i == mem->nr_banks )
> >           return 0;
> > diff --git a/xen/arch/arm/include/asm/setup.h
> b/xen/arch/arm/include/asm/setup.h
> > index 2bb01ecfa8..e80f3d6201 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -27,6 +27,7 @@ struct membank {
> >       paddr_t start;
> >       paddr_t size;
> >       bool xen_domain; /* whether the memory bank is bound to a Xen
> domain. */
> > +    bool xen_heap;   /* whether the memory bank is reserved as heap. */
> 
> We have multiple heap: static, domain, xen. AFAIU, 'xen_heap' refers to
> both domain and xen whereas 'xen_domain' refers to 'static'.
> 
> In line with what I wrote above, I think it would be better if we have a
> single field 'heap' which is an enum listing the type of heap.

Sure, will follow this way.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Stefano Stabellini]

On Thu, 1 Sep 2022, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/09/2022 18:08, Stefano Stabellini wrote:
> > > > Also, what happen with UEFI? Is it easy to guarantee the region will not
> > > > be used?
> > > 
> > > For now I think it is not easy to guarantee that, do you have some ideas
> > > in mind? I think I can follow this in above follow-up series to improve
> > > things.
> > 
> > For clarity, are we worried that the region is used by the bootloader
> > for other things? For instance U-Boot or Tianocore placing some
> > firmware tables inside the range specified for xenheap?
> 
> Yes. I think it would be difficult for an admin to figure out which regions
> are used. Although they are likely (?) going to be static for a given
> UEFI/U-boot build.
> 
> My major concern is such bug can be very difficult to root cause because we
> have no safety in Xen. The most likely symptom would be random corruption.

Thanks for the clarification. Yeah, I think we'll have to do some
"creative thinking" to figure out a solution to this issue. I wonder if
U-boot or Tianocore have some sort of API (or build-time data) to know
the unavailable ranges.

In any case, I think we can postpone to after the release.

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Wei Chen]

Hi Julien and Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: 2022年9月2日 9:51
> To: Julien Grall <julien@xen.org>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Henry Wang
> <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Wei Chen <Wei.Chen@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> heap allocator
> 
> On Thu, 1 Sep 2022, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 01/09/2022 18:08, Stefano Stabellini wrote:
> > > > > Also, what happen with UEFI? Is it easy to guarantee the region
> will not
> > > > > be used?
> > > >
> > > > For now I think it is not easy to guarantee that, do you have some
> ideas
> > > > in mind? I think I can follow this in above follow-up series to
> improve
> > > > things.
> > >
> > > For clarity, are we worried that the region is used by the bootloader
> > > for other things? For instance U-Boot or Tianocore placing some
> > > firmware tables inside the range specified for xenheap?
> >
> > Yes. I think it would be difficult for an admin to figure out which
> regions
> > are used. Although they are likely (?) going to be static for a given
> > UEFI/U-boot build.
> >
> > My major concern is such bug can be very difficult to root cause because
> we
> > have no safety in Xen. The most likely symptom would be random
> corruption.
> 
> Thanks for the clarification. Yeah, I think we'll have to do some
> "creative thinking" to figure out a solution to this issue. I wonder if
> U-boot or Tianocore have some sort of API (or build-time data) to know
> the unavailable ranges.
> 

When Xen is booted through EFI, all the memory statically defined in the
Device tree has a certain probability of conflicting with the memory occupied
by EFI. This is difficult to avoid without the EFI bootloader intervening,
but it is possible to detect such a conflict.

For EFI reserved memory regions (like runtime service), they will not be
reported as usable memory to Xen. The usable memory regions will be added
to bootinfo.memblk as device tree boot. So I think all static defined memory
regions would be check with bootinfo.memblk to find the conflict.
For EFI relocate Xen and DTB, I think Xen itself can know these addresses
easily and easy to check.

But if we don't add code to uboot or EDK2, what can we do are just check,
print error and panic. But starting from the actual usage scenario, because
the scenarios using static heap are normally highly customized, their EFI
firmware will bypass the static memory we defined in device tree when
customizing. So maybe check conflict is enough?

Cheers,
Wei Chen

> In any case, I think we can postpone to after the release.

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Wei Chen]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei
> Chen
> Sent: 2022年9月2日 11:03
> To: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
> <julien@xen.org>
> Cc: Henry Wang <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org;
> Bertrand Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> heap allocator
> 
> Hi Julien and Stefano,
> 
> > -----Original Message-----
> > From: Stefano Stabellini <sstabellini@kernel.org>
> > Sent: 2022年9月2日 9:51
> > To: Julien Grall <julien@xen.org>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Henry Wang
> > <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; Bertrand Marquis
> > <Bertrand.Marquis@arm.com>; Wei Chen <Wei.Chen@arm.com>; Volodymyr
> Babchuk
> > <Volodymyr_Babchuk@epam.com>
> > Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> > heap allocator
> >
> > On Thu, 1 Sep 2022, Julien Grall wrote:
> > > Hi Stefano,
> >
> 
> > In any case, I think we can postpone to after the release.

Maybe we can add some notes to say that this feature is still
experimental in EFI + DTS boot?

Cheers,
Wei Chen

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> >> This code is now becoming quite confusing to understanding. This loop is
> >> meant to map the xenheap. If I follow your documentation, it would
> mean
> >> that only the reserved region should be mapped.
> >
> > Yes I think this is the same question that I raised in the scissors line of the
> > commit message of this patch.
> 
> Sorry I didn't notice the comment after the scissors line. This is the
> same question :)
> 
> > What I intend to do is still mapping the whole
> > RAM because of the xenheap_* variables that you mentioned in...
> >
> >>
> >> More confusingly, xenheap_* variables will cover the full RAM.
> >
> > ...here. But only adding the reserved region to the boot allocator so the
> > reserved region can become the heap later on. I am wondering if we
> > have a more clear way to do that, any suggestions?
> 
> I think your code is correct. It only needs some renaming of the
> existing variable (maybe to directmap_*?) to make clear the area is used
> to access the RAM easily.

Thanks for the clarification. I checked the code and found the xenheap_*
variables are:
xenheap_virt_start, xenheap_virt_end, xenheap_mfn_start,
xenheap_mfn_end, xenheap_base_pdx.

For clarification, do we need to change all of them to directmap_*?

A pure renaming should be easy (and I guess also safe), but maybe I am
overthinking because arm32 also uses xenheap_virt_end, xenheap_mfn_start
and xenheap_mfn_end. These variables refer to the real xenheap, I am not
sure renaming these would reduce the readability for arm32.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Julien Grall]

Hi Henry,

On 02/09/2022 02:28, Henry Wang wrote:
>> This is technically a change in behavior for Xen (we would panic rather
>> than continue). I am happy with the proposal. However, this doesn't seem
>> to be explained in the commit message.
>>
>> That said, I think this should be split in a separate patch along with
>> the ones below (including the prototype changes).
> 
> According to Michal's comment, I've removed the return type and function
> prototype change in my local v2. So this patch itself is fine. My question now
> would be, do maintainers think this change of behavior with processing the
> chosen node be helpful? 

Yes. I think it is saner to stop booting early rather than seen random 
behavior afterwards.

> Do we prefer an instant panic or current behavior?

I think we should leave that up to the caller. Today, this is a panic() 
but we may decide differently in the future.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Julien Grall]

Hi Wei,

On 02/09/2022 04:07, Wei Chen wrote:
> 
> 
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei
>> Chen
>> Sent: 2022年9月2日 11:03
>> To: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
>> <julien@xen.org>
>> Cc: Henry Wang <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org;
>> Bertrand Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
>> <Volodymyr_Babchuk@epam.com>
>> Subject: RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
>> heap allocator
>>
>> Hi Julien and Stefano,
>>
>>> -----Original Message-----
>>> From: Stefano Stabellini <sstabellini@kernel.org>
>>> Sent: 2022年9月2日 9:51
>>> To: Julien Grall <julien@xen.org>
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Henry Wang
>>> <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; Bertrand Marquis
>>> <Bertrand.Marquis@arm.com>; Wei Chen <Wei.Chen@arm.com>; Volodymyr
>> Babchuk
>>> <Volodymyr_Babchuk@epam.com>
>>> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
>>> heap allocator
>>>
>>> On Thu, 1 Sep 2022, Julien Grall wrote:
>>>> Hi Stefano,
>>>
>>
>>> In any case, I think we can postpone to after the release.
> 
> Maybe we can add some notes to say that this feature is still
> experimental in EFI + DTS boot?

Why EFI + DTS only? Regardless the discussion about how to properly 
checking the region, I think this wants to be a tech preview.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Julien Grall]

On 02/09/2022 04:30, Henry Wang wrote:
> Hi Julien,

Hi Henry,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>>>> This code is now becoming quite confusing to understanding. This loop is
>>>> meant to map the xenheap. If I follow your documentation, it would
>> mean
>>>> that only the reserved region should be mapped.
>>>
>>> Yes I think this is the same question that I raised in the scissors line of the
>>> commit message of this patch.
>>
>> Sorry I didn't notice the comment after the scissors line. This is the
>> same question :)
>>
>>> What I intend to do is still mapping the whole
>>> RAM because of the xenheap_* variables that you mentioned in...
>>>
>>>>
>>>> More confusingly, xenheap_* variables will cover the full RAM.
>>>
>>> ...here. But only adding the reserved region to the boot allocator so the
>>> reserved region can become the heap later on. I am wondering if we
>>> have a more clear way to do that, any suggestions?
>>
>> I think your code is correct. It only needs some renaming of the
>> existing variable (maybe to directmap_*?) to make clear the area is used
>> to access the RAM easily.
> 
> Thanks for the clarification. I checked the code and found the xenheap_*
> variables are:
> xenheap_virt_start, xenheap_virt_end, xenheap_mfn_start,
> xenheap_mfn_end, xenheap_base_pdx.
> 
> For clarification, do we need to change all of them to directmap_*?

Good question.

> 
> A pure renaming should be easy (and I guess also safe), but maybe I am
> overthinking because arm32 also uses xenheap_virt_end, xenheap_mfn_start
> and xenheap_mfn_end. These variables refer to the real xenheap, I am not
> sure renaming these would reduce the readability for arm32.

So on arm32, only the xenheap is direct mapped today. So I think it 
would be fine to switch the name to "directmap_*". For extra clarify we 
could add an alias for arm32 between "xenheap_*" and "directmap_*".

Cheers,

-- 
Julien Grall

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> > Thanks for the clarification. I checked the code and found the xenheap_*
> > variables are:
> > xenheap_virt_start, xenheap_virt_end, xenheap_mfn_start,
> > xenheap_mfn_end, xenheap_base_pdx.
> >
> > For clarification, do we need to change all of them to directmap_*?
> 
> Good question.

:))) Thanks for your patience!

> 
> >
> > A pure renaming should be easy (and I guess also safe), but maybe I am
> > overthinking because arm32 also uses xenheap_virt_end,
> xenheap_mfn_start
> > and xenheap_mfn_end. These variables refer to the real xenheap, I am not
> > sure renaming these would reduce the readability for arm32.
> 
> So on arm32, only the xenheap is direct mapped today. So I think it
> would be fine to switch the name to "directmap_*". For extra clarify we
> could add an alias for arm32 between "xenheap_*" and "directmap_*".

Sounds good to me, I think I will try to add a separate patch for purely
renaming all above-mentioned variables, then another patch for creating the
alias for arm32. Hope you would fine with this plan.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory
> 
> Hi Henry,
> 
> On 02/09/2022 02:28, Henry Wang wrote:
> >> This is technically a change in behavior for Xen (we would panic rather
> >> than continue). I am happy with the proposal. However, this doesn't seem
> >> to be explained in the commit message.
> >>
> >> That said, I think this should be split in a separate patch along with
> >> the ones below (including the prototype changes).
> >
> > According to Michal's comment, I've removed the return type and function
> > prototype change in my local v2. So this patch itself is fine. My question
> now
> > would be, do maintainers think this change of behavior with processing the
> > chosen node be helpful?
> 
> Yes. I think it is saner to stop booting early rather than seen random
> behavior afterwards.

Cool, I will then add the patch to this series.

> 
> > Do we prefer an instant panic or current behavior?
> 
> I think we should leave that up to the caller. Today, this is a panic()
> but we may decide differently in the future.

Agreed.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Wei Chen]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2022年9月2日 16:05
> To: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Cc: Henry Wang <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org;
> Bertrand Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> heap allocator
> 
> Hi Wei,
> 
> On 02/09/2022 04:07, Wei Chen wrote:
> >
> >
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Wei
> >> Chen
> >> Sent: 2022年9月2日 11:03
> >> To: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
> >> <julien@xen.org>
> >> Cc: Henry Wang <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org;
> >> Bertrand Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@epam.com>
> >> Subject: RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot
> and
> >> heap allocator
> >>
> >> Hi Julien and Stefano,
> >>
> >>> -----Original Message-----
> >>> From: Stefano Stabellini <sstabellini@kernel.org>
> >>> Sent: 2022年9月2日 9:51
> >>> To: Julien Grall <julien@xen.org>
> >>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Henry Wang
> >>> <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; Bertrand Marquis
> >>> <Bertrand.Marquis@arm.com>; Wei Chen <Wei.Chen@arm.com>; Volodymyr
> >> Babchuk
> >>> <Volodymyr_Babchuk@epam.com>
> >>> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot
> and
> >>> heap allocator
> >>>
> >>> On Thu, 1 Sep 2022, Julien Grall wrote:
> >>>> Hi Stefano,
> >>>
> >>
> >>> In any case, I think we can postpone to after the release.
> >
> > Maybe we can add some notes to say that this feature is still
> > experimental in EFI + DTS boot?
> 
> Why EFI + DTS only? Regardless the discussion about how to properly
> checking the region, I think this wants to be a tech preview.
> 

I had thought something like uboot + dts will not have reserved memory
regions like EFI runtime services. But I forgot uboot also will have
some address to load Xen and DTB. In this case, Xen still need to check
relocation addresses with static heap. So you're right, I agree with you.

Cheers,
Wei Chen.

> Cheers,
> 
> --
> Julien Grall