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: Thursday, May 20, 2021 2:27 AM
> 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
> 
> On 19/05/2021 03:22, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
> 
> >> -----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
> >>> +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/20
> > 210518052113.725808-11-penny.zheng@xxxxxxx/
> 
> Right. My point is an admin should not have to read the code in order to 
> figure
> out how the allocation works.
> 
> >
> > It will exhaust the GUEST_RAM0_SIZE, then seek to the GUEST_RAM1_BASE.
> > GUEST_RAM0 may take up several regions.
> 
> Can this be clarified in the commit message.
> 

Ok, I will expand commit to let it be clarified more clearly.

> > Yes, I may add the 1:1 direct-map scenario here to explain the
> > alternative possibility
> 
> Ok. So I would suggest to remove "by default" until the implementation for
> direct-map is added.
> 

Ok,  will do. Thx.

> > For the third point, are you suggesting that we could provide an
> > option that let user also define guest memory base address/size?
> 
> When reading the document, I originally thought that the first region would be
> mapped to bank1, and then the second region mapped to bank2...
> 
> But from your reply, this is not the expected behavior. Therefore, please 
> ignore
> my suggestion here.
> 
> > I'm confused on the fourth point, you mean the address cell and size cell 
> > for
> xen,static-mem = <...>?
> 
> Yes. This should be clarified in the document. See more below why?
> 
> > It will be consistent with the ones defined in the parent node, domUx.
> Hmmm... To take the example you provided, the parent would be chosen.
> However, from the example, I would expect the property #{address, size}-cells
> in domU1 to be used. What did I miss?
> 

Yeah, the property #{address, size}-cells in domU1 will be used. And the parent
node will be domU1.

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

> >>> +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.
> 
> All the code seems to be implemented in arm generic code. So isn't it already
> working?
> 
> >>>    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:
> 
> Yes, this is what I had in mind. Although I have one small remark below.
> 
> 
> > reserved-memory {
> >     #address-cells = <2>;
> >     #size-cells = <2>;
> >     ranges;
> >
> >     static-mem-domU1: static-mem@0x30000000{
> 
> I think the node would need to contain a compatible (name to be defined).
> 

Ok, maybe, hmmm, how about "xen,static-memory"?

> >        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
> 
> Cheers,
> 
> --
> Julien Grall

Cheers

Penny Zheng