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 4:51 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,
> 
> On 20/05/2021 07:07, Penny Zheng wrote:
> >>> 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.
> 
> You may have misunderstood what I meant. "domU1" is the node that
> contains the property "xen,static-mem". The parent node would be the one
> above (in our case "chosen").
> 

I re-re-reconsider what you meant here, hope this time I get what you mean, 
correct me if I'm wrong,
List an example here:

    / {
        reserved-memory {
            #address-cells = <2>;
            #size-cells = <2>;

            staticmemdomU1: static-memory@0x30000000 {
                compatible = "xen,static-memory-domain";
                reg = <0x0 0x30000000 0x0 0x20000000>;
            };
        };

        chosen {
            domU1 {
                compatible = "xen,domain";
                #address-cells = <0x1>;
                #size-cells = <0x1>;
                cpus = <2>;
                xen,static-mem = <&staticmemdomU1>;

               ...
            };
        };
    };

If user gives two different #address-cells and #size-cells in reserved-memory 
and domU1, Then when parsing it
through `xen,static-mem`, it may have unexpected answers.
I could not think a way to fix it properly in codes, do you have any 
suggestion? Or we just put a warning in doc/commits.

> >
> > 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"?
> 
> I would possibly add "domain" in the name to make clear this is domain
> memory. Stefano, what do you think?
> 
> Cheers,
> 
> 
> Julien Grall

Cheers,

Penny Zheng