Mailing List Archive

[PATCH 2/9] xen: arm: parse modules from DT during early boot.
The bootloader should populate /chosen/modules/module@<N>/ for each
module it wishes to pass to the hypervisor. The content of these nodes
is described in docs/misc/arm/device-tree/booting.txt

The hypervisor parses for 2 types of module, linux zImages and linux
initrds. Currently we don't do anything with them.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v4: Use /chosen/modules/module@N
Identify module type by compatible property not number.
v3: Use a reg = < > property for the module address/length.
v2: Reserve the zeroeth module for Xen itself (not used yet)
Use a more idiomatic DT layout
Document said layout
---
docs/misc/arm/device-tree/booting.txt | 25 ++++++++++
xen/common/device_tree.c | 86 +++++++++++++++++++++++++++++++++
xen/include/xen/device_tree.h | 14 +++++
3 files changed, 125 insertions(+), 0 deletions(-)
create mode 100644 docs/misc/arm/device-tree/booting.txt

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
new file mode 100644
index 0000000..94cd3f1
--- /dev/null
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -0,0 +1,25 @@
+Xen is passed the dom0 kernel and initrd via a reference in the /chosen
+node of the device tree.
+
+Each node has the form /chosen/modules/module@<N> and contains the following
+properties:
+
+- compatible
+
+ Must be:
+
+ "xen,<type>", "xen,multiboot-module"
+
+ where <type> must be one of:
+
+ - "linux-zimage" -- the dom0 kernel
+ - "linux-initrd" -- the dom0 ramdisk
+
+- reg
+
+ Specifies the physical address of the module in RAM and the
+ length of the module.
+
+- bootargs (optional)
+
+ Command line associated with this module
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index da0af77..4bb640e 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -270,6 +270,90 @@ static void __init process_cpu_node(const void *fdt, int node,
cpumask_set_cpu(start, &cpu_possible_map);
}

+static int __init process_chosen_modules_node(const void *fdt, int node,
+ const char *name, int *depth,
+ u32 address_cells, u32 size_cells)
+{
+ const struct fdt_property *prop;
+ const u32 *cell;
+ int nr, nr_modules = 0;
+ struct dt_mb_module *mod;
+ int len;
+
+ for ( *depth = 1;
+ *depth >= 1;
+ node = fdt_next_node(fdt, node, depth) )
+ {
+ name = fdt_get_name(fdt, node, NULL);
+ if ( strncmp(name, "module@", strlen("module@")) == 0 ) {
+
+ if ( fdt_node_check_compatible(fdt, node,
+ "xen,multiboot-module" ) != 0 )
+ early_panic("%s not a compatible module node\n", name);
+
+ if ( fdt_node_check_compatible(fdt, node,
+ "xen,linux-zimage") == 0 )
+ nr = 1;
+ else if ( fdt_node_check_compatible(fdt, node,
+ "xen,linux-initrd") == 0)
+ nr = 2;
+ else
+ early_panic("%s not a known xen multiboot byte\n");
+
+ if ( nr > nr_modules )
+ nr_modules = nr;
+
+ mod = &early_info.modules.module[nr];
+
+ prop = fdt_get_property(fdt, node, "reg", NULL);
+ if ( !prop )
+ early_panic("node %s missing `reg' property\n", name);
+
+ cell = (const u32 *)prop->data;
+ device_tree_get_reg(&cell, address_cells, size_cells,
+ &mod->start, &mod->size);
+
+ prop = fdt_get_property(fdt, node, "bootargs", &len);
+ if ( prop )
+ {
+ if ( len > sizeof(mod->cmdline) )
+ early_panic("module %d command line too long\n", nr);
+
+ safe_strcpy(mod->cmdline, prop->data);
+ }
+ else
+ mod->cmdline[0] = 0;
+ }
+ }
+
+ for ( nr = 1 ; nr < nr_modules ; nr++ )
+ {
+ mod = &early_info.modules.module[nr];
+ if ( !mod->start || !mod->size )
+ early_panic("module %d missing / invalid\n", nr);
+ }
+
+ early_info.modules.nr_mods = nr_modules;
+ return node;
+}
+
+static void __init process_chosen_node(const void *fdt, int node,
+ const char *name,
+ u32 address_cells, u32 size_cells)
+{
+ int depth;
+
+ for ( depth = 0;
+ depth >= 0;
+ node = fdt_next_node(fdt, node, &depth) )
+ {
+ name = fdt_get_name(fdt, node, NULL);
+ if ( depth == 1 && strcmp(name, "modules") == 0 )
+ node = process_chosen_modules_node(fdt, node, name, &depth,
+ address_cells, size_cells);
+ }
+}
+
static int __init early_scan_node(const void *fdt,
int node, const char *name, int depth,
u32 address_cells, u32 size_cells,
@@ -279,6 +363,8 @@ static int __init early_scan_node(const void *fdt,
process_memory_node(fdt, node, name, address_cells, size_cells);
else if ( device_tree_type_matches(fdt, node, "cpu") )
process_cpu_node(fdt, node, name, address_cells, size_cells);
+ else if ( device_tree_node_matches(fdt, node, "chosen") )
+ process_chosen_node(fdt, node, name, address_cells, size_cells);

return 0;
}
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 4d010c0..c383677 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -15,6 +15,7 @@
#define DEVICE_TREE_MAX_DEPTH 16

#define NR_MEM_BANKS 8
+#define NR_MODULES 2

struct membank {
paddr_t start;
@@ -26,8 +27,21 @@ struct dt_mem_info {
struct membank bank[NR_MEM_BANKS];
};

+struct dt_mb_module {
+ paddr_t start;
+ paddr_t size;
+ char cmdline[1024];
+};
+
+struct dt_module_info {
+ int nr_mods;
+ /* Module 0 is Xen itself, followed by the provided modules-proper */
+ struct dt_mb_module module[NR_MODULES + 1];
+};
+
struct dt_early_info {
struct dt_mem_info mem;
+ struct dt_module_info modules;
};

typedef int (*device_tree_node_func)(const void *fdt,
--
1.7.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 2/9] xen: arm: parse modules from DT during early boot. [ In reply to ]
On Thu, 6 Dec 2012, Ian Campbell wrote:
> The bootloader should populate /chosen/modules/module@<N>/ for each
> module it wishes to pass to the hypervisor. The content of these nodes
> is described in docs/misc/arm/device-tree/booting.txt
>
> The hypervisor parses for 2 types of module, linux zImages and linux
> initrds. Currently we don't do anything with them.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v4: Use /chosen/modules/module@N
> Identify module type by compatible property not number.
> v3: Use a reg = < > property for the module address/length.
> v2: Reserve the zeroeth module for Xen itself (not used yet)
> Use a more idiomatic DT layout
> Document said layout
> ---
> docs/misc/arm/device-tree/booting.txt | 25 ++++++++++
> xen/common/device_tree.c | 86 +++++++++++++++++++++++++++++++++
> xen/include/xen/device_tree.h | 14 +++++
> 3 files changed, 125 insertions(+), 0 deletions(-)
> create mode 100644 docs/misc/arm/device-tree/booting.txt
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> new file mode 100644
> index 0000000..94cd3f1
> --- /dev/null
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -0,0 +1,25 @@
> +Xen is passed the dom0 kernel and initrd via a reference in the /chosen
> +node of the device tree.
> +
> +Each node has the form /chosen/modules/module@<N> and contains the following
> +properties:
> +
> +- compatible
> +
> + Must be:
> +
> + "xen,<type>", "xen,multiboot-module"
> +
> + where <type> must be one of:
> +
> + - "linux-zimage" -- the dom0 kernel
> + - "linux-initrd" -- the dom0 ramdisk
> +
> +- reg
> +
> + Specifies the physical address of the module in RAM and the
> + length of the module.
> +
> +- bootargs (optional)
> +
> + Command line associated with this module
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index da0af77..4bb640e 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -270,6 +270,90 @@ static void __init process_cpu_node(const void *fdt, int node,
> cpumask_set_cpu(start, &cpu_possible_map);
> }
>
> +static int __init process_chosen_modules_node(const void *fdt, int node,
> + const char *name, int *depth,
> + u32 address_cells, u32 size_cells)
> +{
> + const struct fdt_property *prop;
> + const u32 *cell;
> + int nr, nr_modules = 0;
> + struct dt_mb_module *mod;
> + int len;
> +
> + for ( *depth = 1;
> + *depth >= 1;
> + node = fdt_next_node(fdt, node, depth) )
> + {
> + name = fdt_get_name(fdt, node, NULL);
> + if ( strncmp(name, "module@", strlen("module@")) == 0 ) {
> +
> + if ( fdt_node_check_compatible(fdt, node,
> + "xen,multiboot-module" ) != 0 )
> + early_panic("%s not a compatible module node\n", name);
> +
> + if ( fdt_node_check_compatible(fdt, node,
> + "xen,linux-zimage") == 0 )
> + nr = 1;
> + else if ( fdt_node_check_compatible(fdt, node,
> + "xen,linux-initrd") == 0)
> + nr = 2;
> + else
> + early_panic("%s not a known xen multiboot byte\n");
> +
> + if ( nr > nr_modules )
> + nr_modules = nr;
> +
> + mod = &early_info.modules.module[nr];
> +
> + prop = fdt_get_property(fdt, node, "reg", NULL);
> + if ( !prop )
> + early_panic("node %s missing `reg' property\n", name);
> +
> + cell = (const u32 *)prop->data;
> + device_tree_get_reg(&cell, address_cells, size_cells,
> + &mod->start, &mod->size);
> +
> + prop = fdt_get_property(fdt, node, "bootargs", &len);
> + if ( prop )
> + {
> + if ( len > sizeof(mod->cmdline) )
> + early_panic("module %d command line too long\n", nr);
> +
> + safe_strcpy(mod->cmdline, prop->data);
> + }
> + else
> + mod->cmdline[0] = 0;
> + }
> + }
> +
> + for ( nr = 1 ; nr < nr_modules ; nr++ )
> + {
> + mod = &early_info.modules.module[nr];
> + if ( !mod->start || !mod->size )
> + early_panic("module %d missing / invalid\n", nr);
> + }
> +
> + early_info.modules.nr_mods = nr_modules;
> + return node;
> +}
> +
> +static void __init process_chosen_node(const void *fdt, int node,
> + const char *name,
> + u32 address_cells, u32 size_cells)
> +{
> + int depth;
> +
> + for ( depth = 0;
> + depth >= 0;
> + node = fdt_next_node(fdt, node, &depth) )
> + {
> + name = fdt_get_name(fdt, node, NULL);
> + if ( depth == 1 && strcmp(name, "modules") == 0 )
> + node = process_chosen_modules_node(fdt, node, name, &depth,
> + address_cells, size_cells);
> + }
> +}
> +
> static int __init early_scan_node(const void *fdt,
> int node, const char *name, int depth,
> u32 address_cells, u32 size_cells,
> @@ -279,6 +363,8 @@ static int __init early_scan_node(const void *fdt,
> process_memory_node(fdt, node, name, address_cells, size_cells);
> else if ( device_tree_type_matches(fdt, node, "cpu") )
> process_cpu_node(fdt, node, name, address_cells, size_cells);
> + else if ( device_tree_node_matches(fdt, node, "chosen") )
> + process_chosen_node(fdt, node, name, address_cells, size_cells);
>
> return 0;
> }

You have really written a lot of code here!
I would have thought that just matching on the compatible string would
be enough:

else if ( device_tree_node_matches(fdt, node, "linux-zimage") )
process_linuxzimage_node(fdt, node, name, address_cells, size_cells);
else if ( device_tree_node_matches(fdt, node, "linux-initrd") )
process_linuxinitrd_node(fdt, node, name, address_cells, size_cells);

so that your process_linuxzimage_node and process_linuxinitrd_node start
from the right node and have everything they need to parse it



> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 4d010c0..c383677 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -15,6 +15,7 @@
> #define DEVICE_TREE_MAX_DEPTH 16
>
> #define NR_MEM_BANKS 8
> +#define NR_MODULES 2
>
> struct membank {
> paddr_t start;
> @@ -26,8 +27,21 @@ struct dt_mem_info {
> struct membank bank[NR_MEM_BANKS];
> };
>
> +struct dt_mb_module {
> + paddr_t start;
> + paddr_t size;
> + char cmdline[1024];
> +};
> +
> +struct dt_module_info {
> + int nr_mods;
> + /* Module 0 is Xen itself, followed by the provided modules-proper */
> + struct dt_mb_module module[NR_MODULES + 1];
> +};
> +
> struct dt_early_info {
> struct dt_mem_info mem;
> + struct dt_module_info modules;
> };
>
> typedef int (*device_tree_node_func)(const void *fdt,
> --
> 1.7.9.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 2/9] xen: arm: parse modules from DT during early boot. [ In reply to ]
On Fri, 2012-12-07 at 17:30 +0000, Stefano Stabellini wrote:
> On Thu, 6 Dec 2012, Ian Campbell wrote:
> > The bootloader should populate /chosen/modules/module@<N>/ for each
> > module it wishes to pass to the hypervisor. The content of these nodes
> > is described in docs/misc/arm/device-tree/booting.txt
> >
> > The hypervisor parses for 2 types of module, linux zImages and linux
> > initrds. Currently we don't do anything with them.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > v4: Use /chosen/modules/module@N
> > Identify module type by compatible property not number.
> > v3: Use a reg = < > property for the module address/length.
> > v2: Reserve the zeroeth module for Xen itself (not used yet)
> > Use a more idiomatic DT layout
> > Document said layout
> > ---
> > docs/misc/arm/device-tree/booting.txt | 25 ++++++++++
> > xen/common/device_tree.c | 86 +++++++++++++++++++++++++++++++++
> > xen/include/xen/device_tree.h | 14 +++++
> > 3 files changed, 125 insertions(+), 0 deletions(-)
> > create mode 100644 docs/misc/arm/device-tree/booting.txt
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > new file mode 100644
> > index 0000000..94cd3f1
> > --- /dev/null
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -0,0 +1,25 @@
> > +Xen is passed the dom0 kernel and initrd via a reference in the /chosen
> > +node of the device tree.
> > +
> > +Each node has the form /chosen/modules/module@<N> and contains the following
> > +properties:
> > +
> > +- compatible
> > +
> > + Must be:
> > +
> > + "xen,<type>", "xen,multiboot-module"
> > +
> > + where <type> must be one of:
> > +
> > + - "linux-zimage" -- the dom0 kernel
> > + - "linux-initrd" -- the dom0 ramdisk
> > +
> > +- reg
> > +
> > + Specifies the physical address of the module in RAM and the
> > + length of the module.
> > +
> > +- bootargs (optional)
> > +
> > + Command line associated with this module
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index da0af77..4bb640e 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -270,6 +270,90 @@ static void __init process_cpu_node(const void *fdt, int node,
> > cpumask_set_cpu(start, &cpu_possible_map);
> > }
> >
> > +static int __init process_chosen_modules_node(const void *fdt, int node,
> > + const char *name, int *depth,
> > + u32 address_cells, u32 size_cells)
> > +{
> > + const struct fdt_property *prop;
> > + const u32 *cell;
> > + int nr, nr_modules = 0;
> > + struct dt_mb_module *mod;
> > + int len;
> > +
> > + for ( *depth = 1;
> > + *depth >= 1;
> > + node = fdt_next_node(fdt, node, depth) )
> > + {
> > + name = fdt_get_name(fdt, node, NULL);
> > + if ( strncmp(name, "module@", strlen("module@")) == 0 ) {
> > +
> > + if ( fdt_node_check_compatible(fdt, node,
> > + "xen,multiboot-module" ) != 0 )
> > + early_panic("%s not a compatible module node\n", name);
> > +
> > + if ( fdt_node_check_compatible(fdt, node,
> > + "xen,linux-zimage") == 0 )
> > + nr = 1;
> > + else if ( fdt_node_check_compatible(fdt, node,
> > + "xen,linux-initrd") == 0)
> > + nr = 2;
> > + else
> > + early_panic("%s not a known xen multiboot byte\n");
> > +
> > + if ( nr > nr_modules )
> > + nr_modules = nr;
> > +
> > + mod = &early_info.modules.module[nr];
> > +
> > + prop = fdt_get_property(fdt, node, "reg", NULL);
> > + if ( !prop )
> > + early_panic("node %s missing `reg' property\n", name);
> > +
> > + cell = (const u32 *)prop->data;
> > + device_tree_get_reg(&cell, address_cells, size_cells,
> > + &mod->start, &mod->size);
> > +
> > + prop = fdt_get_property(fdt, node, "bootargs", &len);
> > + if ( prop )
> > + {
> > + if ( len > sizeof(mod->cmdline) )
> > + early_panic("module %d command line too long\n", nr);
> > +
> > + safe_strcpy(mod->cmdline, prop->data);
> > + }
> > + else
> > + mod->cmdline[0] = 0;
> > + }
> > + }
> > +
> > + for ( nr = 1 ; nr < nr_modules ; nr++ )
> > + {
> > + mod = &early_info.modules.module[nr];
> > + if ( !mod->start || !mod->size )
> > + early_panic("module %d missing / invalid\n", nr);
> > + }
> > +
> > + early_info.modules.nr_mods = nr_modules;
> > + return node;
> > +}
> > +
> > +static void __init process_chosen_node(const void *fdt, int node,
> > + const char *name,
> > + u32 address_cells, u32 size_cells)
> > +{
> > + int depth;
> > +
> > + for ( depth = 0;
> > + depth >= 0;
> > + node = fdt_next_node(fdt, node, &depth) )
> > + {
> > + name = fdt_get_name(fdt, node, NULL);
> > + if ( depth == 1 && strcmp(name, "modules") == 0 )
> > + node = process_chosen_modules_node(fdt, node, name, &depth,
> > + address_cells, size_cells);
> > + }
> > +}
> > +
> > static int __init early_scan_node(const void *fdt,
> > int node, const char *name, int depth,
> > u32 address_cells, u32 size_cells,
> > @@ -279,6 +363,8 @@ static int __init early_scan_node(const void *fdt,
> > process_memory_node(fdt, node, name, address_cells, size_cells);
> > else if ( device_tree_type_matches(fdt, node, "cpu") )
> > process_cpu_node(fdt, node, name, address_cells, size_cells);
> > + else if ( device_tree_node_matches(fdt, node, "chosen") )
> > + process_chosen_node(fdt, node, name, address_cells, size_cells);
> >
> > return 0;
> > }
>
> You have really written a lot of code here!
> I would have thought that just matching on the compatible string would
> be enough:
>
> else if ( device_tree_node_matches(fdt, node, "linux-zimage") )
> process_linuxzimage_node(fdt, node, name, address_cells, size_cells);
> else if ( device_tree_node_matches(fdt, node, "linux-initrd") )
> process_linuxinitrd_node(fdt, node, name, address_cells, size_cells);
>
> so that your process_linuxzimage_node and process_linuxinitrd_node start
> from the right node and have everything they need to parse it

Is the tree structure of Device Tree meaningless? I'd have thought that
a compatible node would only have meaning at a particular place in the
tree. Granted compatible nodes are often pretty specific and precise,
but is that inherent enough in DT that we can rely on it?


>
>
>
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 4d010c0..c383677 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -15,6 +15,7 @@
> > #define DEVICE_TREE_MAX_DEPTH 16
> >
> > #define NR_MEM_BANKS 8
> > +#define NR_MODULES 2
> >
> > struct membank {
> > paddr_t start;
> > @@ -26,8 +27,21 @@ struct dt_mem_info {
> > struct membank bank[NR_MEM_BANKS];
> > };
> >
> > +struct dt_mb_module {
> > + paddr_t start;
> > + paddr_t size;
> > + char cmdline[1024];
> > +};
> > +
> > +struct dt_module_info {
> > + int nr_mods;
> > + /* Module 0 is Xen itself, followed by the provided modules-proper */
> > + struct dt_mb_module module[NR_MODULES + 1];
> > +};
> > +
> > struct dt_early_info {
> > struct dt_mem_info mem;
> > + struct dt_module_info modules;
> > };
> >
> > typedef int (*device_tree_node_func)(const void *fdt,
> > --
> > 1.7.9.1
> >



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 2/9] xen: arm: parse modules from DT during early boot. [ In reply to ]
On Mon, 10 Dec 2012, Ian Campbell wrote:
> On Fri, 2012-12-07 at 17:30 +0000, Stefano Stabellini wrote:
> > On Thu, 6 Dec 2012, Ian Campbell wrote:
> > > The bootloader should populate /chosen/modules/module@<N>/ for each
> > > module it wishes to pass to the hypervisor. The content of these nodes
> > > is described in docs/misc/arm/device-tree/booting.txt
> > >
> > > The hypervisor parses for 2 types of module, linux zImages and linux
> > > initrds. Currently we don't do anything with them.
> > >
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > > v4: Use /chosen/modules/module@N
> > > Identify module type by compatible property not number.
> > > v3: Use a reg = < > property for the module address/length.
> > > v2: Reserve the zeroeth module for Xen itself (not used yet)
> > > Use a more idiomatic DT layout
> > > Document said layout
> > > ---
> > > docs/misc/arm/device-tree/booting.txt | 25 ++++++++++
> > > xen/common/device_tree.c | 86 +++++++++++++++++++++++++++++++++
> > > xen/include/xen/device_tree.h | 14 +++++
> > > 3 files changed, 125 insertions(+), 0 deletions(-)
> > > create mode 100644 docs/misc/arm/device-tree/booting.txt
> > >
> > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > > new file mode 100644
> > > index 0000000..94cd3f1
> > > --- /dev/null
> > > +++ b/docs/misc/arm/device-tree/booting.txt
> > > @@ -0,0 +1,25 @@
> > > +Xen is passed the dom0 kernel and initrd via a reference in the /chosen
> > > +node of the device tree.
> > > +
> > > +Each node has the form /chosen/modules/module@<N> and contains the following
> > > +properties:
> > > +
> > > +- compatible
> > > +
> > > + Must be:
> > > +
> > > + "xen,<type>", "xen,multiboot-module"
> > > +
> > > + where <type> must be one of:
> > > +
> > > + - "linux-zimage" -- the dom0 kernel
> > > + - "linux-initrd" -- the dom0 ramdisk
> > > +
> > > +- reg
> > > +
> > > + Specifies the physical address of the module in RAM and the
> > > + length of the module.
> > > +
> > > +- bootargs (optional)
> > > +
> > > + Command line associated with this module
> > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > > index da0af77..4bb640e 100644
> > > --- a/xen/common/device_tree.c
> > > +++ b/xen/common/device_tree.c
> > > @@ -270,6 +270,90 @@ static void __init process_cpu_node(const void *fdt, int node,
> > > cpumask_set_cpu(start, &cpu_possible_map);
> > > }
> > >
> > > +static int __init process_chosen_modules_node(const void *fdt, int node,
> > > + const char *name, int *depth,
> > > + u32 address_cells, u32 size_cells)
> > > +{
> > > + const struct fdt_property *prop;
> > > + const u32 *cell;
> > > + int nr, nr_modules = 0;
> > > + struct dt_mb_module *mod;
> > > + int len;
> > > +
> > > + for ( *depth = 1;
> > > + *depth >= 1;
> > > + node = fdt_next_node(fdt, node, depth) )
> > > + {
> > > + name = fdt_get_name(fdt, node, NULL);
> > > + if ( strncmp(name, "module@", strlen("module@")) == 0 ) {
> > > +
> > > + if ( fdt_node_check_compatible(fdt, node,
> > > + "xen,multiboot-module" ) != 0 )
> > > + early_panic("%s not a compatible module node\n", name);
> > > +
> > > + if ( fdt_node_check_compatible(fdt, node,
> > > + "xen,linux-zimage") == 0 )
> > > + nr = 1;
> > > + else if ( fdt_node_check_compatible(fdt, node,
> > > + "xen,linux-initrd") == 0)
> > > + nr = 2;
> > > + else
> > > + early_panic("%s not a known xen multiboot byte\n");
> > > +
> > > + if ( nr > nr_modules )
> > > + nr_modules = nr;
> > > +
> > > + mod = &early_info.modules.module[nr];
> > > +
> > > + prop = fdt_get_property(fdt, node, "reg", NULL);
> > > + if ( !prop )
> > > + early_panic("node %s missing `reg' property\n", name);
> > > +
> > > + cell = (const u32 *)prop->data;
> > > + device_tree_get_reg(&cell, address_cells, size_cells,
> > > + &mod->start, &mod->size);
> > > +
> > > + prop = fdt_get_property(fdt, node, "bootargs", &len);
> > > + if ( prop )
> > > + {
> > > + if ( len > sizeof(mod->cmdline) )
> > > + early_panic("module %d command line too long\n", nr);
> > > +
> > > + safe_strcpy(mod->cmdline, prop->data);
> > > + }
> > > + else
> > > + mod->cmdline[0] = 0;
> > > + }
> > > + }
> > > +
> > > + for ( nr = 1 ; nr < nr_modules ; nr++ )
> > > + {
> > > + mod = &early_info.modules.module[nr];
> > > + if ( !mod->start || !mod->size )
> > > + early_panic("module %d missing / invalid\n", nr);
> > > + }
> > > +
> > > + early_info.modules.nr_mods = nr_modules;
> > > + return node;
> > > +}
> > > +
> > > +static void __init process_chosen_node(const void *fdt, int node,
> > > + const char *name,
> > > + u32 address_cells, u32 size_cells)
> > > +{
> > > + int depth;
> > > +
> > > + for ( depth = 0;
> > > + depth >= 0;
> > > + node = fdt_next_node(fdt, node, &depth) )
> > > + {
> > > + name = fdt_get_name(fdt, node, NULL);
> > > + if ( depth == 1 && strcmp(name, "modules") == 0 )
> > > + node = process_chosen_modules_node(fdt, node, name, &depth,
> > > + address_cells, size_cells);
> > > + }
> > > +}
> > > +
> > > static int __init early_scan_node(const void *fdt,
> > > int node, const char *name, int depth,
> > > u32 address_cells, u32 size_cells,
> > > @@ -279,6 +363,8 @@ static int __init early_scan_node(const void *fdt,
> > > process_memory_node(fdt, node, name, address_cells, size_cells);
> > > else if ( device_tree_type_matches(fdt, node, "cpu") )
> > > process_cpu_node(fdt, node, name, address_cells, size_cells);
> > > + else if ( device_tree_node_matches(fdt, node, "chosen") )
> > > + process_chosen_node(fdt, node, name, address_cells, size_cells);
> > >
> > > return 0;
> > > }
> >
> > You have really written a lot of code here!
> > I would have thought that just matching on the compatible string would
> > be enough:
> >
> > else if ( device_tree_node_matches(fdt, node, "linux-zimage") )
> > process_linuxzimage_node(fdt, node, name, address_cells, size_cells);
> > else if ( device_tree_node_matches(fdt, node, "linux-initrd") )
> > process_linuxinitrd_node(fdt, node, name, address_cells, size_cells);
> >
> > so that your process_linuxzimage_node and process_linuxinitrd_node start
> > from the right node and have everything they need to parse it
>
> Is the tree structure of Device Tree meaningless? I'd have thought that
> a compatible node would only have meaning at a particular place in the
> tree. Granted compatible nodes are often pretty specific and precise,
> but is that inherent enough in DT that we can rely on it?

I don't know if it is entirely meaningless, but surely the compatible
string is regarded as a much more reliable way to identify a node AFAIK.
More often than not Linux drivers just use of_find_compatible_node to
find their DT node.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 2/9] xen: arm: parse modules from DT during early boot. [ In reply to ]
On Mon, 2012-12-10 at 13:04 +0000, Stefano Stabellini wrote:

> > > You have really written a lot of code here!
> > > I would have thought that just matching on the compatible string would
> > > be enough:
> > >
> > > else if ( device_tree_node_matches(fdt, node, "linux-zimage") )
> > > process_linuxzimage_node(fdt, node, name, address_cells, size_cells);
> > > else if ( device_tree_node_matches(fdt, node, "linux-initrd") )
> > > process_linuxinitrd_node(fdt, node, name, address_cells, size_cells);
> > >
> > > so that your process_linuxzimage_node and process_linuxinitrd_node start
> > > from the right node and have everything they need to parse it
> >
> > Is the tree structure of Device Tree meaningless? I'd have thought that
> > a compatible node would only have meaning at a particular place in the
> > tree. Granted compatible nodes are often pretty specific and precise,
> > but is that inherent enough in DT that we can rely on it?
>
> I don't know if it is entirely meaningless, but surely the compatible
> string is regarded as a much more reliable way to identify a node AFAIK.
> More often than not Linux drivers just use of_find_compatible_node to
> find their DT node.

Hrm, that sounds rather odd to me.

Anyway, there isn't much code needed to do it right -- there's only ~ a
dozen lines to do with actually walking the tree. The rest (i.e. the
majority of this patch) is the internals of process_chosen_modules_node,
the docs and the data structure which would be pretty much the same
regardless of walking the tree.

Ian.


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