Hi Michal,
It is great to hear from you! Hope you are doing well.
> -----Original Message-----
> From: Michal Orzel <michal.or...@amd.com>
> Subject: Re: [PATCH 1/2] docs, xen/arm: Introduce reserved heap memory
> Hi Henry,
> > +to parts of RAM reserved in the beginning for heap. The memory is
> reserved by
> I think we are missing "... in the beginning" of what.
Correct, I will change it to "... in the beginning of boot time".
>
> > +configuration in the device tree using physical address ranges.
> > +
> > +The reserved heap memory declared in the device tree defines the
> memory areas
> > +that will be reserved to be used exclusively as heap.
> > +
> > +- For Arm32, since there can be seperated heaps, the reserved heap will
> be used
> Maybe "there are" instead of "there can be" as we do define for Arm32:
> #define CONFIG_SEPARATE_XENHEAP 1
> and I do not think we have some flexibility to change this.
Ack.
>
> > +for both domheap and xenheap.
> > +- For Arm64, since domheap and xenheap are the same, the defined
> reserved heap
> Instead of writing "since domheap and xenheap are the same" maybe it'd be
> better to write:
> "For Arm64, as there is a single heap..."
Yep, will change in v2.
>
> > +areas shall always go to the heap allocator.
> > +
> > +The reserved heap memory is an optional feature and can be enabled by
> adding a
> > +device tree property in the `chosen` node. Currently, this feature reuses
> the
> > +static memory allocation device tree configuration.
> > +
> > +The dtb property should look like as follows:
> > +
> > +- property name
> > +
> > + "xen,static-mem" (Should be used in the `chosen` node)
> > +
> > +- cells
> > +
> > + Specify the start address and the length of the reserved heap memory.
> > + The number of cells for the address and the size should be defined
> > + using the properties `#xen,static-mem-address-cells` and
> > + `#xen,static-mem-size-cells` respectively.
> > +
> > +Below is an example on how to specify the reserved heap in device tree:
> > +
> > + / {
> > + chosen {
> > + #xen,static-mem-address-cells = <0x2>;
> > + #xen,static-mem-size-cells = <0x2>;
> > + xen,static-mem = <0x0 0x30000000 0x0 0x40000000>;
> Please add "..." here as this does not represent the complete working chosen
> node.
Sure, will add in v2.
>
> > + };
> > + };
> > +
> > +RAM at 0x30000000 of 1G size will be reserved as heap.
> > +
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index ec81a45de9..33704ca487 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -64,7 +64,8 @@ void __init device_tree_get_reg(const __be32 **cell,
> u32 address_cells,
> > static int __init device_tree_get_meminfo(const void *fdt, int node,
> > const char *prop_name,
> > u32 address_cells, u32
> > size_cells,
> > - void *data, bool xen_domain)
> > + void *data, bool xen_domain,
> > + bool xen_heap)
> > {
> > const struct fdt_property *prop;
> > unsigned int i, banks;
> > @@ -96,6 +97,7 @@ static int __init device_tree_get_meminfo(const void
> *fdt, int node,
> > mem->bank[mem->nr_banks].start = start;
> > mem->bank[mem->nr_banks].size = size;
> > mem->bank[mem->nr_banks].xen_domain = xen_domain;
> > + mem->bank[mem->nr_banks].xen_heap = xen_heap;
> > mem->nr_banks++;
> > }
> >
> > @@ -185,7 +187,7 @@ static int __init process_memory_node(const void
> *fdt, int node,
> > void *data)
> > {
> > return device_tree_get_meminfo(fdt, node, "reg", address_cells,
> size_cells,
> > - data, false);
> > + data, false, false);
> > }
> >
> > static int __init process_reserved_memory_node(const void *fdt, int node,
> > @@ -293,7 +295,7 @@ static void __init process_multiboot_node(const
> void *fdt, int node,
> > kind, start, domU);
> > }
> >
> > -static void __init process_chosen_node(const void *fdt, int node,
> > +static int __init process_chosen_node(const void *fdt, int node,
> You do not really need to change the return type of this function.
> Currently process_chosen_node just returns on an error condition so you
> could do the same.
Thanks for pointing this out, will do in v2.
>
> > const char *name,
> > u32 address_cells, u32 size_cells)
> > {
> > @@ -301,16 +303,40 @@ static void __init process_chosen_node(const
> void *fdt, int node,
> > paddr_t start, end;
> > int len;
> >
> > + if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
> > + {
> > + u32 address_cells = device_tree_get_u32(fdt, node,
> > +
> > "#xen,static-mem-address-cells",
> > + 0);
> > + u32 size_cells = device_tree_get_u32(fdt, node,
> > + "#xen,static-mem-size-cells",
> > 0);
> > + int rc;
> > +
> > + printk("Checking for reserved heap in /chosen\n");
> > + if ( address_cells < 1 || size_cells < 1 )
> address_cells and size_cells cannot be negative so you could just check if
> there are 0.
In bootfdt.c function device_tree_get_meminfo(), the address and size cells
are checked using <1 instead of =0. I agree they cannot be negative, but I am
not very sure if there were other reasons to do the "<1" check in
device_tree_get_meminfo(). Are you fine with we don't keep the consistency
here?
>
> > + {
> > + printk("fdt: node `%s': invalid #xen,static-mem-address-cells
> > or
> #xen,static-mem-size-cells\n",
> > + name);
> > + return -EINVAL;
> > + }
> > +
> > + rc = device_tree_get_meminfo(fdt, node, "xen,static-mem",
> address_cells,
> > + size_cells, &bootinfo.reserved_mem,
> > false,
> > + true);
> > + if ( rc )
> > + return rc;
> > + }
> > +
> > printk("Checking for initrd in /chosen\n");
> >
> > prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
> > if ( !prop )
> > /* No initrd present. */
> > - return;
> > + return 0;
> > if ( len != sizeof(u32) && len != sizeof(u64) )
> > {
> > printk("linux,initrd-start property has invalid length %d\n", len);
> > - return;
> > + return -EINVAL;
> This change breaks the current behavior and will result in triggering the
> printk in early_scan_node for parsing failure.
> Is this intended? If so, you could mention this in the commit msg.
I think I will follow your advice above for the return type so here we won't
have any changes in v2.
>
> > }
> > start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> >
> > @@ -318,12 +344,12 @@ static void __init process_chosen_node(const
> void *fdt, int node,
> > if ( !prop )
> > {
> > printk("linux,initrd-end not present but -start was\n");
> > - return;
> > + return -EINVAL;
> > }
> > if ( len != sizeof(u32) && len != sizeof(u64) )
> > {
> > printk("linux,initrd-end property has invalid length %d\n", len);
> > - return;
> > + return -EINVAL;
> > }
> > end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> >
> > @@ -331,12 +357,14 @@ static void __init process_chosen_node(const
> void *fdt, int node,
> > {
> > printk("linux,initrd limits invalid: %"PRIpaddr" >= %"PRIpaddr"\n",
> > start, end);
> > - return;
> > + return -EINVAL;
> > }
> >
> > printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
> >
> > add_boot_module(BOOTMOD_RAMDISK, start, end-start, false);
> > +
> > + return 0;
> > }
> >
> > static int __init process_domain_node(const void *fdt, int node,
> > @@ -358,7 +386,8 @@ static int __init process_domain_node(const void
> *fdt, int node,
> > "#xen,static-mem-size-cells", 0);
> >
> > return device_tree_get_meminfo(fdt, node, "xen,static-mem",
> address_cells,
> > - size_cells, &bootinfo.reserved_mem,
> > true);
> > + size_cells, &bootinfo.reserved_mem,
> > true,
> > + false);
> > }
> >
> > static int __init early_scan_node(const void *fdt,
> > @@ -383,7 +412,7 @@ static int __init early_scan_node(const void *fdt,
> > device_tree_node_compatible(fdt, node, "multiboot,module" )))
> > process_multiboot_node(fdt, node, name, address_cells, size_cells);
> > else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
> > - process_chosen_node(fdt, node, name, address_cells, size_cells);
> > + rc = process_chosen_node(fdt, node, name, address_cells,
> > size_cells);
> > else if ( depth == 2 && device_tree_node_compatible(fdt, node,
> "xen,domain") )
> > rc = process_domain_node(fdt, node, name, address_cells,
> > size_cells);
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 3fd1186b53..6f97f5f06a 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1038,8 +1038,9 @@ static int __init make_memory_node(const struct
> domain *d,
> > if ( mem->nr_banks == 0 )
> > return -ENOENT;
> >
> > - /* find first memory range not bound to a Xen domain */
> > - for ( i = 0; i < mem->nr_banks && mem->bank[i].xen_domain; i++ )
> > + /* find first memory range not bound to neither a Xen domain nor heap
> */
> > + for ( i = 0; i < mem->nr_banks &&
> > + (mem->bank[i].xen_domain || mem->bank[i].xen_heap); i++ )
> > ;
> Could you please add an empty line here to improve readability?
Sure.
Kind regards,
Henry
