Re: [Xen-devel] [PATCH 1/4] dtb: correct handling of #address-cells and #size-cells.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-02-15 at 13:18 +0000, Ian Campbell wrote:
> On Fri, 2013-02-15 at 12:43 +0000, Stefano Stabellini wrote:
> > On Wed, 30 Jan 2013, Ian Campbell wrote:
> > > If a node does not have #*-cells then the parent's value should be
> > > used. Currently we were asssuming zero which is useless.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > ---
> > >  xen/arch/arm/domain_build.c   |    6 ++++--
> > >  xen/common/device_tree.c      |   12 ++++++++----
> > >  xen/include/xen/device_tree.h |    3 ++-
> > >  3 files changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 7403f1a..bfbe7c7 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -198,8 +198,10 @@ static int write_nodes(struct domain *d, struct 
> > > kernel_info *kinfo,
> > >          while ( last_depth-- >= depth )
> > >              fdt_end_node(kinfo->fdt);
> > >  
> > > -        address_cells[depth] = device_tree_get_u32(fdt, node, 
> > > "#address-cells");
> > > -        size_cells[depth] = device_tree_get_u32(fdt, node, 
> > > "#size-cells");
> > > +        address_cells[depth] = device_tree_get_u32(fdt, node, 
> > > "#address-cells",
> > > +                                    depth > 0 ? address_cells[depth-1] : 
> > > 0);
> > > +        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells",
> > > +                                    depth > 0 ? size_cells[depth-1] : 0);
> > >  
> > >          fdt_begin_node(kinfo->fdt, name);
> > 
> > The depth is always increasing by steps of 1 in this loop, right?
> > Because retrieving address-cells and size-cells should be recursive: if
> > n-1 doesn't have them, let's look at n-2, etc. Of course if we start from
> > depth = 0 and go from there without missing any levels the results will
> > be the same.
> 
> That was what I thought too. Perhaps it is too subtle?
> 
> I bet my "xen: strip xen,multiboot-module nodes from dom0 device tree"
> patch changes this invariant. Better to make it explicitly walk
> backwards now I think. (or maybe set things for level in
> last_depth..depth). I'll change things along these lines.

I looked into this and even with that patch the invariant that we cannot
skip levels is present. This turns out to be obviously the case because
you have actually open the node and name it... I'll add an ASSERT here
though just in case.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel