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

[Julien Grall <julien@xxxxxxx>]

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").
> 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