Hi Julien
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Thursday, May 20, 2021 2:27 AM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org
> Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen
> <wei.c...@arm.com>; nd <n...@arm.com>
> 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 <jul...@xen.org>
> >> Sent: Tuesday, May 18, 2021 4:58 PM
> >> To: Penny Zheng <penny.zh...@arm.com>;
> >> xen-devel@lists.xenproject.org; sstabell...@kernel.org
> >> Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen
> >> <wei.c...@arm.com>; nd <n...@arm.com>
> >> 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.zh...@arm.com/20
> > 210518052113.725808-11-penny.zh...@arm.com/
>
> 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
