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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 4 Jun 2021, Penny Zheng wrote:
> > > > 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>;
> > >
> > > This is pretty similar to what Penny suggested. But I dislike it
> > > because of the amount of code that needs to be duplicated with the
> > > reserved memory.
> > 
> > Where is the code duplication? In the parsing itself?
> > 
> > If there is code duplication, can we find a way to share some of the code to
> > avoid the duplication?
> 
> Both your opinions are so convincing... :/
> 
> Correct me if I am wrong:
> I think the duplication which Julien means are here, See commit 
> https://patchew.org/Xen/20210518052113.725808-1-penny.zheng@xxxxxxx/20210518052113.725808-3-penny.zheng@xxxxxxx/
> I added a another similar loop in dt_unreserved_regions to unreserve static 
> memory.
> For this part, I could try to extract common codes.
> 
> But another part I think is just this commit, where I added another check for 
> static memory
> in early_scan_node:
> 
> +    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);
> 
> TBH, I don't know how to fix here....

Is it only one loop in dt_unreserved_regions and another call to
process_static_memory? If you can make the code common in
dt_unreserved_regions there wouldn't be much duplication left.


To explain my point of view a bit better, I think we have a lot more
freedom in the Xen implementation compared to the device tree
specification. For the sake of an example, let's say that we wanted Xen
to reuse bootinfo.reserved_mem for both reserved-memory and
static-memory. I don't know if it is even a good idea (I haven't looked
into it, it is just an example) but I think it would be OK, we could
decide to do that.

We have less room for flexibility in the device tree specification.
/reserved-memory is for special configurations of 1 domain only. I don't
think we could add domain static memory allocations to it.