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

Commit Message

Henry Wang Aug. 24, 2022, 7:31 a.m. UTC

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 <[e-mail]>
Signed-off-by: Penny Zheng <[e-mail]>
---
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(-)

Comments

Michal Orzel Aug. 24, 2022, 1:46 p.m. UTC | #1

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

Henry Wang Aug. 25, 2022, 1:04 a.m. UTC | #2

Hi Michal,

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

> -----Original Message-----
> From: Michal Orzel <[e-mail]>
> 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

Stefano Stabellini Aug. 30, 2022, 12:45 a.m. UTC | #3

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 <[e-mail]>
> Signed-off-by: Penny Zheng <[e-mail]>
> ---
> 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".

---

Stefano Stabellini Aug. 30, 2022, 12:47 a.m. UTC | #4

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

Henry Wang Aug. 30, 2022, 12:58 a.m. UTC | #5

Hi Stefano and Michal,

> -----Original Message-----
> From: Stefano Stabellini <[e-mail]>
> Sent: Tuesday, August 30, 2022 8:47 AM
> To: Henry Wang <[e-mail]>
> Cc: Michal Orzel <[e-mail]>; [e-mail];
> Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>;
> Bertrand Marquis <[e-mail]>; Wei Chen
> <[e-mail]>; Volodymyr Babchuk
> <[e-mail]>; Penny Zheng <[e-mail]>
> 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

Henry Wang Aug. 30, 2022, 1:02 a.m. UTC | #6

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <[e-mail]>
> 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

Michal Orzel Aug. 30, 2022, 6:29 a.m. UTC | #7

Hi Henry,

On 30/08/2022 02:58, Henry Wang wrote:
> 
> Hi Stefano and Michal,
> 
>> -----Original Message-----
>> From: Stefano Stabellini <[e-mail]>
>> Sent: Tuesday, August 30, 2022 8:47 AM
>> To: Henry Wang <[e-mail]>
>> Cc: Michal Orzel <[e-mail]>; [e-mail];
>> Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>;
>> Bertrand Marquis <[e-mail]>; Wei Chen
>> <[e-mail]>; Volodymyr Babchuk
>> <[e-mail]>; Penny Zheng <[e-mail]>
>> 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

Michal Orzel Aug. 30, 2022, 7:10 a.m. UTC | #8

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 <[e-mail]>
>>> Sent: Tuesday, August 30, 2022 8:47 AM
>>> To: Henry Wang <[e-mail]>
>>> Cc: Michal Orzel <[e-mail]>; [e-mail];
>>> Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>;
>>> Bertrand Marquis <[e-mail]>; Wei Chen
>>> <[e-mail]>; Volodymyr Babchuk
>>> <[e-mail]>; Penny Zheng <[e-mail]>
>>> 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

Henry Wang Aug. 30, 2022, 7:21 a.m. UTC | #9

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <[e-mail]>
> >>>>>> +        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

Julien Grall Sept. 1, 2022, 2:36 p.m. UTC | #10

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 <[e-mail]>
> Signed-off-by: Penny Zheng <[e-mail]>
> ---
> 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,

Henry Wang Sept. 2, 2022, 1:28 a.m. UTC | #11

Hi Julien,

> -----Original Message-----
> From: Julien Grall <[e-mail]>
> 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

Julien Grall Sept. 2, 2022, 8:01 a.m. UTC | #12

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,

Henry Wang Sept. 2, 2022, 8:21 a.m. UTC | #13

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

Patch

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 {