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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Henry,

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

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

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

~Michal

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