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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

I have not read most emails in this thread (sorry!) but I spotted this
discussion about device tree and I would like to reply to that as we
have discussed something very similar in the context of system device
tree.


On Thu, 3 Jun 2021, Julien Grall wrote:
> On 02/06/2021 11:09, Penny Zheng wrote:
> > 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.
> 
> Why are you using the #address-cells and #size-cells from the node domU1 to
> parse staticmemdomU1?
> 
> > 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 correct approach is to find the parent of staticmemdomU1 (i.e.
> reserved-memory) and use the #address-cells and #size-cells from there.

Julien is right about how to parse the static-memory.

But I have a suggestion on the new binding. The /reserved-memory node is
a weird node: it is one of the very few node (the only node aside from
/chosen) which is about software configurations rather than hardware
description.

For this reason, in a device tree with multiple domains /reserved-memory
doesn't make a lot of sense: for which domain is the memory reserved?

This was one of the first points raised by Rob Herring in reviewing
system device tree.

So the solution we went for is the following: if there is a default
domain /reserved-memory applies to the default domain. Otherwise, each
domain is going to have its own reserved-memory. Example:

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

            reserved-memory {
                #address-cells = <2>;
                #size-cells = <2>;
   
                static-memory@0x30000000 {
                    compatible = "xen,static-memory-domain";
                    reg = <0x0 0x30000000 0x0 0x20000000>;
                };
            };
        };


So I don't think we want to use reserved-memory for this, either
/reserved-memory or /chosen/domU1/reserved-memory. Instead it would be
good to align it with system device tree and define it as a new property
under domU1.

In system device tree we would use a property called "memory" to specify
one or more ranges, e.g.:

    domU1 {
        memory = <0x0 0x500000 0x0 0x7fb00000>;

Unfortunately for xen,domains we have already defined "memory" to
specify an amount, rather than a range. That's too bad because the most
natural way to do this would be:

    domU1 {
        size = <amount>;
        memory = <ranges>;

When we'll introduce native system device tree support in Xen we'll be
able to do that. For now, we need to come up with a different property.
For instance: "static-memory" (other names are welcome if you have a
better suggestion).

We use a new property called "static-memory" together with
#static-memory-address-cells and #static-memory-size-cells to define how
many cells to use for address and size.

Example:

    domU1 {
        #static-memory-address-cells = <2>;
        #static-memory-size-cells = <2>;
        static-memory = <0x0 0x500000 0x0 0x7fb00000>;



Another alternative would be to extend the definition of the existing
"memory" property to potentially handle not just sizes but also address
and size pairs. There are a couple of ways to do that, including using
#memory-address-cells = <0>; to specify when memory only has a size, or
changing compatible string to "xen,domain-v2". But actually I would
avoid it. I would keep it simple and just introduce a new property like
"static-memory" (we can come up with a better name).