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

[Michal Orzel <michal.orzel@xxxxxxx>]

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