RE: [PATCH 01/10] xen/arm: introduce domain on Static Allocation

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Tuesday, May 18, 2021 4:58 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [PATCH 01/10] xen/arm: introduce domain on Static Allocation
> 
> Hi Penny,
> 
> On 18/05/2021 06:21, 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 Momery, as parts of RAM reserved
> > in the
> 
> s/Momery/Memory/

Oh, thx!

> 
> > beginning, shall never go to heap allocator or boot allocator for any use.
> >
> > Domains on Static Allocation is supported through device tree property
> > `xen,static-mem` specifying reserved RAM banks as this domain's guest
> RAM.
> > By default, they shall be mapped to the fixed guest RAM address
> > `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
> >
> > This patch introduces this new `xen,static-mem` property to define
> > static memory nodes in device tree file.
> > This patch also documents and parses this new attribute at boot time
> > and stores related info in static_mem for later initialization.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> >   docs/misc/arm/device-tree/booting.txt | 33 +++++++++++++++++
> >   xen/arch/arm/bootfdt.c                | 52 +++++++++++++++++++++++++++
> >   xen/include/asm-arm/setup.h           |  2 ++
> >   3 files changed, 87 insertions(+)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 5243bc7fd3..d209149d71 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.
> > +Those pre-defined memory, -- Static Momery, as parts of RAM reserved
> > +in the
> 
> s/Momery/Memory/
> 

Oh, thx

> > +beginning, shall never go to heap allocator or boot allocator for any use.
> > +
> > +Domains on Static Allocation is supported through device tree
> > +property `xen,static-mem` specifying reserved RAM banks as this domain's
> guest RAM.
> 
> I would suggest to use "physical RAM" when you refer to the host memory.
> 
> > +By default, they shall be mapped to the fixed guest RAM address
> > +`GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
> 
> There are a few bits that needs to clarified or part of the description:
>    1) "By default" suggests there is an alternative possibility.
> However, I don't see any.
>    2) Will the first region of xen,static-mem be mapped to GUEST_RAM0_BASE
> and the second to GUEST_RAM1_BASE? What if a third region is specificed?
>    3) We don't guarantee the base address and the size of the banks.
> Wouldn't it be better to let the admin select the region he/she wants?
>    4) How do you determine the number of cells for the address and the size?
> 

The specific implementation on this part could be traced to the last commit
https://patchew.org/Xen/20210518052113.725808-1-penny.zheng@xxxxxxx/20210518052113.725808-11-penny.zheng@xxxxxxx/

It will exhaust the GUEST_RAM0_SIZE, then seek to the GUEST_RAM1_BASE.
GUEST_RAM0 may take up several regions.

Yes, I may add the 1:1 direct-map scenario here to explain the alternative 
possibility

For the third point, are you suggesting that we could provide an option that 
let user
also define guest memory base address/size?

I'm confused on the fourth point, you mean the address cell and size cell for 
xen,static-mem = <...>?
It will be consistent with the ones defined in the parent node, domUx.

> > +Static Allocation is only supported on AArch64 for now.
> 
> The code doesn't seem to be AArch64 specific. So why can't this be used for
> 32-bit Arm?
> 

True, we have plans to make it also workable in AArch32 in the future.
Because we considered XEN on cortex-R52.

> > +
> > +The dtb property should look like as follows:
> > +
> > +        chosen {
> > +            domU1 {
> > +                compatible = "xen,domain";
> > +                #address-cells = <0x2>;
> > +                #size-cells = <0x2>;
> > +                cpus = <2>;
> > +                xen,static-mem = <0x0 0x30000000 0x0 0x20000000>;
> > +
> > +                ...
> > +            };
> > +        };
> > +
> > +DOMU1 on Static Allocation has reserved RAM bank at 0x30000000 of
> > +512MB size
> 
> Do you mean "DomU1 will have a static memory of 512MB reserved from the
> physical address..."?
>

Yes, yes. You phrase it more clearly, thx
 
> > +as guest RAM.
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
> > dcff512648..e9f14e6a44 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -327,6 +327,55 @@ static void __init process_chosen_node(const void
> *fdt, int node,
> >       add_boot_module(BOOTMOD_RAMDISK, start, end-start, false);
> >   }
> >
> > +static int __init process_static_memory(const void *fdt, int node,
> > +                                        const char *name,
> > +                                        u32 address_cells, u32 size_cells,
> > +                                        void *data) {
> > +    int i;
> > +    int banks;
> > +    const __be32 *cell;
> > +    paddr_t start, size;
> > +    u32 reg_cells = address_cells + size_cells;
> > +    struct meminfo *mem = data;
> > +    const struct fdt_property *prop;
> > +
> > +    if ( address_cells < 1 || size_cells < 1 )
> > +    {
> > +        printk("fdt: invalid #address-cells or #size-cells for static 
> > memory");
> > +        return -EINVAL;
> > +    }
> > +
> > +    /*
> > +     * Check if static memory property belongs to a specific domain, that 
> > is,
> > +     * its node `domUx` has compatible string "xen,domain".
> > +     */
> > +    if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
> > +        printk("xen,static-mem property can only locate under /domUx
> > + node.\n");
> > +
> > +    prop = fdt_get_property(fdt, node, name, NULL);
> > +    if ( !prop )
> > +        return -ENOENT;
> > +
> > +    cell = (const __be32 *)prop->data;
> > +    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> > +
> > +    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
> > +    {
> > +        device_tree_get_reg(&cell, address_cells, size_cells, &start, 
> > &size);
> > +        /* Some DT may describe empty bank, ignore them */
> > +        if ( !size )
> > +            continue;
> > +        mem->bank[mem->nr_banks].start = start;
> > +        mem->bank[mem->nr_banks].size = size;
> > +        mem->nr_banks++;
> > +    }
> > +
> > +    if ( i < banks )
> > +        return -ENOSPC;
> > +    return 0;
> > +}
> > +
> >   static int __init early_scan_node(const void *fdt,
> >                                     int node, const char *name, int depth,
> >                                     u32 address_cells, u32 size_cells,
> > @@ -345,6 +394,9 @@ 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 && fdt_get_property(fdt, node, "xen,static-mem",
> NULL) )
> > +        process_static_memory(fdt, node, "xen,static-mem", address_cells,
> > +                              size_cells, &bootinfo.static_mem);
> 
> I am a bit concerned to add yet another method to parse the DT and all the
> extra code it will add like in patch #2.
> 
>  From the host PoV, they are memory reserved for a specific purpose.
> Would it be possible to consider the reserve-memory binding for that
> purpose? This will happen outside of chosen, but we could use a phandle to
> refer the region.
> 

Correct me if I understand wrongly, do you mean what this device tree snippet 
looks like:

reserved-memory {
   #address-cells = <2>;
   #size-cells = <2>;
   ranges;
 
   static-mem-domU1: static-mem@0x30000000{
      reg = <0x0 0x30000000 0x0 0x20000000>;
   };
};

Chosen {
 ...
domU1 {
   xen,static-mem = <&static-mem-domU1>;
};
...
};

> >
> >       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
> > 5283244015..5e9f296760 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -74,6 +74,8 @@ struct bootinfo {
> >   #ifdef CONFIG_ACPI
> >       struct meminfo acpi;
> >   #endif
> > +    /* Static Memory */
> > +    struct meminfo static_mem;
> >   };
> >
> >   extern struct bootinfo bootinfo;
> >
> 
> Cheers,
> 
> --
> Julien Grall