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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> 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