Re: [Xen-devel] [PATCH 9/9] xen: strip /chosen/modules/module@<N>/* from dom0 device tree

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-12-10 at 13:10 +0000, Stefano Stabellini wrote:
> On Mon, 10 Dec 2012, Ian Campbell wrote:
> > On Fri, 2012-12-07 at 17:35 +0000, Stefano Stabellini wrote:
> > > On Thu, 6 Dec 2012, Ian Campbell wrote:
> > > > These nodes are used by Xen to find the initial modules.
> > > > 
> > > > Only drop the "xen,multiboot-module" compatible nodes in case someone
> > > > else has a similar idea.
> > > > 
> > > > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > > ---
> > > > v4 - /chosen/modules/modules@N not /chosen/module@N
> > > > v3 - use a helper to filter out DT elements which are not for dom0.
> > > >      Better than an ad-hoc break in the middle of a loop.
> > > > ---
> > > >  xen/arch/arm/domain_build.c |   40 
> > > > ++++++++++++++++++++++++++++++++++++++--
> > > >  1 files changed, 38 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > > index 7a964f7..27e02e4 100644
> > > > --- a/xen/arch/arm/domain_build.c
> > > > +++ b/xen/arch/arm/domain_build.c
> > > > @@ -172,6 +172,40 @@ static int write_properties(struct domain *d, 
> > > > struct kernel_info *kinfo,
> > > >      return prop;
> > > >  }
> > > >  
> > > > +/* Returns the next node in fdt (starting from offset) which should be
> > > > + * passed through to dom0.
> > > > + */
> > > > +static int fdt_next_dom0_node(const void *fdt, int node,
> > > > +                              int *depth_out,
> > > > +                              int parents[DEVICE_TREE_MAX_DEPTH])
> > > > +{
> > > > +    int depth = *depth_out;
> > > > +
> > > > +    while ( (node = fdt_next_node(fdt, node, &depth)) &&
> > > > +            node >= 0 && depth >= 0 )
> > > > +    {
> > > > +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> > > > +            break;
> > > > +
> > > > +        parents[depth] = node;
> > > > +
> > > > +        /* Skip /chosen/modules/module@<N>/ and all subnodes */
> > > > +        if ( depth >= 3 &&
> > > > +             device_tree_node_matches(fdt, parents[1], "chosen") &&
> > > > +             device_tree_node_matches(fdt, parents[2], "modules") &&
> > > > +             device_tree_node_matches(fdt, parents[3], "module") &&
> > > > +             fdt_node_check_compatible(fdt, parents[3],
> > > > +                                       "xen,multiboot-module" ) == 0 )
> > > > +            continue;
> > > > +
> > > > +        /* We've arrived at a node which dom0 is interested in. */
> > > > +        break;
> > > > +    }
> > > > +
> > > > +    *depth_out = depth;
> > > > +    return node;
> > > > +}
> > > 
> > > Can't we just skip the node if it is compatible with
> > > "xen,multiboot-module", no matter where it lives?  This should simplify
> > > this function greatly and you wouldn't need the parents parameter
> > > anymore.
> > 
> > As well as my previous query about the meaning of the tree structure I
> > think that even if we could get away with this in this particular case
> > we are going to need this sort of infrastructure once we start doing
> > proper filtering of dom0 vs xen nodes in the tree.
> 
> Maybe. However in that case we could just have a generic
> filter_device_tree_nodes function that takes an array of strings
> (compatible string? device tree paths? I would go for both in a struct,
> but the former would probably suffice), and filter the DT based on
> those. That would be very useful in the long run. This is very ad-hoc
> for the /chosen/modules/module@<N> path.

This function is precisely a filtering function as you are suggesting.
The only difference is that it does the filtering in code instead of
using a string/struct. There is nothing ad-hoc about it it just happens
that the only user right now is the module@N stuff.

I think you'd find that the code to filter based on a struct containing
a path would be more complicated and be a superset of this function,
since you would have to check the path against the same sort of parent
array.

Ian.


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