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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Michal,

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

> -----Original Message-----
> From: Michal Orzel <michal.orzel@xxxxxxx>
> 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