Mailing List Archive: [PATCH 9/9] xen: strip /chosen/modules/module@/* from dom0 device tree

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

Dec 6, 2012, 5:10 AM

Post #1 of 5 (704 views)

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@citrix.com>
---
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;
+}
+
static int write_nodes(struct domain *d, struct kernel_info *kinfo,
const void *fdt)
{
@@ -179,11 +213,12 @@ static int write_nodes(struct domain *d, struct kernel_info *kinfo,
int depth = 0, last_depth = -1;
u32 address_cells[DEVICE_TREE_MAX_DEPTH];
u32 size_cells[DEVICE_TREE_MAX_DEPTH];
+ int parents[DEVICE_TREE_MAX_DEPTH];
int ret;

for ( node = 0, depth = 0;
node >= 0 && depth >= 0;
- node = fdt_next_node(fdt, node, &depth) )
+ node = fdt_next_dom0_node(fdt, node, &depth, parents) )
{
const char *name;

@@ -191,7 +226,8 @@ static int write_nodes(struct domain *d, struct kernel_info *kinfo,

if ( depth >= DEVICE_TREE_MAX_DEPTH )
{
- printk("warning: node `%s' is nested too deep\n", name);
+ printk("warning: node `%s' is nested too deep (%d)\n",
+ name, depth);
continue;
}

--
1.7.9.1

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

Re: [PATCH 9/9] xen: strip /chosen/modules/module@<N>/* from dom0 device tree [ In reply to ]

stefano.stabellini at eu

Dec 7, 2012, 9:35 AM

Post #2 of 5 (685 views)

Permalink

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@citrix.com>
> ---
> 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.
This way we could have a simple node blacklist based on the compatible
node all in a single function.

> static int write_nodes(struct domain *d, struct kernel_info *kinfo,
> const void *fdt)
> {
> @@ -179,11 +213,12 @@ static int write_nodes(struct domain *d, struct kernel_info *kinfo,
> int depth = 0, last_depth = -1;
> u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> + int parents[DEVICE_TREE_MAX_DEPTH];
> int ret;
>
> for ( node = 0, depth = 0;
> node >= 0 && depth >= 0;
> - node = fdt_next_node(fdt, node, &depth) )
> + node = fdt_next_dom0_node(fdt, node, &depth, parents) )
> {
> const char *name;
>
> @@ -191,7 +226,8 @@ static int write_nodes(struct domain *d, struct kernel_info *kinfo,
>
> if ( depth >= DEVICE_TREE_MAX_DEPTH )
> {
> - printk("warning: node `%s' is nested too deep\n", name);
> + printk("warning: node `%s' is nested too deep (%d)\n",
> + name, depth);
> continue;
> }
>
> --
> 1.7.9.1
>

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

Re: [PATCH 9/9] xen: strip /chosen/modules/module@<N>/* from dom0 device tree [ In reply to ]

Ian.Campbell at citrix

Dec 10, 2012, 1:42 AM

Post #3 of 5 (675 views)

Permalink

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@citrix.com>
> > ---
> > 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.

> This way we could have a simple node blacklist based on the compatible
> node all in a single function.

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

Re: [PATCH 9/9] xen: strip /chosen/modules/module@<N>/* from dom0 device tree [ In reply to ]

stefano.stabellini at eu

Dec 10, 2012, 5:10 AM

Post #4 of 5 (675 views)

Permalink

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@citrix.com>
> > > ---
> > > 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.

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

Re: [PATCH 9/9] xen: strip /chosen/modules/module@<N>/* from dom0 device tree [ In reply to ]

Ian.Campbell at citrix

Dec 10, 2012, 5:20 AM

Post #5 of 5 (674 views)

Permalink

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@citrix.com>
> > > > ---
> > > > 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@lists.xen.org
http://lists.xen.org/xen-devel