RE: [PATCH v5 2/7] xen/arm: introduce domain on Static Allocation

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Stefano

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Sent: Friday, September 3, 2021 5:31 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx;
> Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [PATCH v5 2/7] xen/arm: introduce domain on Static Allocation
> 
> On Tue, 24 Aug 2021, Penny Zheng wrote:
> > Static Allocation refers to system or sub-system(domains) for which
> > memory areas are pre-defined by configuration using physical address
> ranges.
> > Those pre-defined memory, -- Static Memory, as parts of RAM reserved
> > in the beginning, shall never go to heap allocator or boot allocator for any
> use.
> >
> > Memory can be statically allocated to a domain using the property
> > "xen,static- mem" defined in the domain configuration. The number of
> > cells for the address and the size must be defined using respectively
> > the properties "#xen,static-mem-address-cells" and "#xen,static-mem-size-
> cells".
> >
> > This patch introduces this new `xen,static-mem` feature, and also
> > documents and parses this new attribute at boot time.
> >
> > This patch also introduces a new field "bool xen_domain" in "struct
> membank"
> > to tell the difference of one memory bank is reserved as the whole
> > hardware resource, or bind to one specific xen domain node, through
> > "xen,static-mem"
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> > v5 changes:
> > - check the node using the Xen domain binding whether contains the
> > property "xen,static-mem", not the property itself
> > - add "rc = ..." to get the error propagated
> > - introduce new field "bool xen_domain", then static memory shall be
> > also stored as reserved memory(bootinfo.reserved_mem), but being bind
> > to one specific Xen domain node.
> > - doc refinement
> > ---
> >  docs/misc/arm/device-tree/booting.txt | 33 ++++++++++++++++++++++++
> >  xen/arch/arm/bootfdt.c                | 36 +++++++++++++++++++++++++--
> >  xen/include/asm-arm/setup.h           |  1 +
> >  3 files changed, 68 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 5243bc7fd3..95b20ddc3a 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -268,3 +268,36 @@ The DTB fragment is loaded at 0xc000000 in the
> > example above. It should  follow the convention explained in
> > docs/misc/arm/passthrough.txt. The  DTB fragment will be added to the
> > guest device tree, so that the guest  kernel will be able to discover the 
> > device.
> > +
> > +
> > +Static Allocation
> > +=============
> > +
> > +Static Allocation refers to system or sub-system(domains) for which
> > +memory areas are pre-defined by configuration using physical address
> ranges.
> > +
> > +Memory can be statically allocated to a domain using the property
> > +"xen,static- mem" defined in the domain configuration. The number of
> > +cells for the address and the size must be defined using respectively
> > +the properties "#xen,static-mem-address-cells" and "#xen,static-mem-size-
> cells".
> > +
> > +Below is an example on how to specify the static memory region in the
> > +device-tree:
> > +
> > +    / {
> > +        chosen {
> > +            domU1 {
> > +                compatible = "xen,domain";
> > +                #address-cells = <0x2>;
> > +                #size-cells = <0x2>;
> > +                cpus = <2>;
> > +                #xen,static-mem-address-cells = <0x1>;
> > +                #xen,static-mem-size-cells = <0x1>;
> > +                xen,static-mem = <0x30000000 0x20000000>;
> > +                ...
> > +            };
> > +        };
> > +    };
> > +
> > +This will reserve a 512MB region starting at the host physical
> > +address
> > +0x30000000 to be exclusively used by DomU1
> 
> This binding is OK.  We might want to clarify what is the purpose of the
> "memory" property when "xen,static-mem" is present. I would suggest to write
> that when "xen,static-mem" is present, the "memory" property becomes
> optional. Or even better:
> 
> """
> When present, the xen,static-mem property supersedes the memory property.
> """
> 

oh, "supersede" learned! ;)

> 
> In the future when Xen will support direct mapping, I assume that we'll also
> add a direct-map property to enable it.  Something like:
> 
>     domU1 {
>         compatible = "xen,domain";
>         #address-cells = <0x2>;
>         #size-cells = <0x2>;
>         cpus = <2>;
>         #xen,static-mem-address-cells = <0x1>;
>         #xen,static-mem-size-cells = <0x1>;
>         xen,static-mem = <0x30000000 0x20000000>;
>         direct-map;
>         ...
>     };
> 
> Maybe I would add a statement to clarify it that xen,static-mem doesn't
> automatically imply direct mapping. Something like:
> 
> """
> The static memory will be mapped in the guest at the usual guest memory
> addresses (GUEST_RAM0_BASE, GUEST_RAM1_BASE) defined by
> xen/include/public/arch-arm.h.
> """
> 

Thanks for the detailed suggestion. I'll just take it.

> The rest of the patch looks OK. One minor NIT below.
> 
> 
> 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
> > 8c81be3379..00f34eec58 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -66,7 +66,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)
> > +                                          void *data, bool
> > + xen_domain)
> >  {
> >      const struct fdt_property *prop;
> >      unsigned int i, banks;
> > @@ -90,6 +90,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->nr_banks++;
> >      }
> >
> > @@ -184,7 +185,8 @@ static int __init process_memory_node(const void
> *fdt, int node,
> >          return -EINVAL;
> >      }
> >
> > -    return device_tree_get_meminfo(fdt, node, "reg", address_cells,
> size_cells, data);
> > +    return device_tree_get_meminfo(fdt, node, "reg", address_cells,
> size_cells,
> > +                                   data, false);
> >  }
> >
> >  static int __init process_reserved_memory_node(const void *fdt, int
> > node, @@ -338,6 +340,34 @@ static void __init process_chosen_node(const
> void *fdt, int node,
> >      add_boot_module(BOOTMOD_RAMDISK, start, end-start, false);  }
> >
> > +static int __init process_domain_node(const void *fdt, int node,
> > +                                       const char *name,
> > +                                       u32 address_cells, u32
> > +size_cells) {
> > +    const struct fdt_property *prop;
> > +
> > +    printk("Checking for \"xen,static-mem\" in domain node\n");
> > +
> > +    prop = fdt_get_property(fdt, node, "xen,static-mem", NULL);
> > +    if ( !prop )
> > +        /* No "xen,static-mem" present. */
> > +        return 0;
> > +
> > +    address_cells = device_tree_get_u32(fdt, node,
> > +                                        "#xen,static-mem-address-cells", 
> > 0);
> > +    size_cells = device_tree_get_u32(fdt, node,
> > +                                     "#xen,static-mem-size-cells", 0);
> > +    if ( address_cells < 1 || size_cells < 1 )
> > +    {
> > +        printk("fdt: node `%s': invalid #xen,static-mem-address-cells or
> #xen,static-mem-size-cells",
> > +               name);
> > +        return -EINVAL;
> > +    }
> > +
> > +    return device_tree_get_meminfo(fdt, node, "xen,static-mem",
> address_cells,
> > +                                   size_cells,
> > +&bootinfo.reserved_mem, true); }
> > +
> >  static int __init early_scan_node(const void *fdt,
> >                                    int node, const char *name, int depth,
> >                                    u32 address_cells, u32 size_cells,
> > @@ -356,6 +386,8 @@ static int __init early_scan_node(const void *fdt,
> >          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);
> > +    else if ( depth == 2 && device_tree_node_compatible(fdt, node,
> "xen,domain") )
> > +        rc = process_domain_node(fdt, node, name, address_cells,
> > + size_cells);
> >
> >      if ( rc < 0 )
> >          printk("fdt: node `%s': parsing failed\n", name); diff --git
> > a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index
> > c4b6af6029..6c3c16294b 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -24,6 +24,7 @@ typedef enum {
> >  struct membank {
> >      paddr_t start;
> >      paddr_t size;
> > +    bool xen_domain; /* whether memory bank is bind to Xen domain. */
>                                   ^ a or the      ^ bound to a
> 

Sure. Will fix it.

> 
> >  };
> >
> >  struct meminfo {
> > --
> > 2.25.1
> >