For the current "multiboot" on ARM support we look for a compatible
string of "xen,multiboot-module" in the device tree, and then
use "xen,linux-zimage" and "xen,linux-initrd" to
differentiate
between the two supported module types.
To meet the more generic multiboot proposal in the device tree [1],
allow Xen to be more flexible in the compatible naming and also use
the new generic base name "boot,module".
The mapping to either Dom0 kernel or RAM disk works either by
providing a more specific name ("xen,dom0-kernel" and
"xen,ramdisk"),
or by using the enumeration order of the device tree nodes
(module@0 = kernel, module@1 = initrd). This allows bootloaders
without any specific Xen knowledge to boot Xen anyways.
[1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html
Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
xen/common/device_tree.c | 57 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 50 insertions(+), 7 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index ec0d5e2..e10c035 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -439,22 +439,63 @@ static void __init process_cpu_node(const void *fdt, int
node,
cpumask_set_cpu(start, &cpu_possible_map);
}
+static const char * const kernel_module_names[] = {
+ "xen,linux-zimage",
+ "xen,dom0-kernel",
+ "boot,kernel",
+ NULL
+};
+
+static const char * const initrd_module_names[] = {
+ "xen,linux-initrd",
+ "xen,ramdisk",
+ "boot,ramdisk",
+ NULL
+};
+
static void __init process_multiboot_node(const void *fdt, int node,
const char *name,
u32 address_cells, u32 size_cells)
{
const struct fdt_property *prop;
const u32 *cell;
- int nr;
+ int nr = -1;
struct dt_mb_module *mod;
int len;
+ const char* const * name_list;
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") ==
0 )
- nr = MOD_KERNEL;
- else if ( fdt_node_check_compatible(fdt, node,
"xen,linux-initrd") == 0)
- nr = MOD_INITRD;
- else
- early_panic("%s not a known xen multiboot type\n", name);
+ for (name_list = kernel_module_names; *name_list != NULL; name_list++)
+ if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) {
+ nr = MOD_KERNEL;
+ break;
+ }
+
+ for (name_list = initrd_module_names; *name_list != NULL; name_list++)
+ if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) {
+ nr = MOD_INITRD;
+ break;
+ }
+
+ if (nr == -1) {
+ char *s;
+
+ if ( fdt_node_check_compatible(fdt, node, "boot,module") != 0
) {
+ early_panic("%s not a known xen multiboot type\n", name);
+ return;
+ }
+ s = strchr(name, ''@'');
+ if (s == NULL)
+ nr = early_info.modules.nr_mods + 1;
+ else {
+ nr = simple_strtoll(s + 1, NULL, 10) + 1;
+ }
+ }
+
+ if (nr >= NR_MODULES) {
+ early_panic("only supporting %d multiboot modules\n",
+ NR_MODULES - MOD_DISCARD_FIRST);
+ return;
+ }
mod = &early_info.modules.module[nr];
@@ -492,6 +533,8 @@ static int __init early_scan_node(const void *fdt,
process_cpu_node(fdt, node, name, address_cells, size_cells);
else if ( device_tree_node_compatible(fdt, node,
"xen,multiboot-module" ) )
process_multiboot_node(fdt, node, name, address_cells, size_cells);
+ else if ( device_tree_node_compatible(fdt, node, "boot,module" )
)
+ process_multiboot_node(fdt, node, name, address_cells, size_cells);
return 0;
}
--
1.7.12.1
Egger, Christoph
2013-Sep-05 13:54 UTC
Re: [PATCH] ARM/multiboot: use more flexible node naming
Is there a chance to get NetBSD Dom0 [1] to boot to justify that your approach is *really* generic. xen,linux-zimage and xen,linux-initrd do not sound generic but xen,dom0-kernel and xen,ramdisk do sound generic at least. Christoph [1] NetBSD Dom0 hasn''t been ported to ARM yet. On 05.09.13 15:43, Andre Przywara wrote:> For the current "multiboot" on ARM support we look for a compatible > string of "xen,multiboot-module" in the device tree, and then > use "xen,linux-zimage" and "xen,linux-initrd" to differentiate > between the two supported module types. > To meet the more generic multiboot proposal in the device tree [1], > allow Xen to be more flexible in the compatible naming and also use > the new generic base name "boot,module". > The mapping to either Dom0 kernel or RAM disk works either by > providing a more specific name ("xen,dom0-kernel" and "xen,ramdisk"), > or by using the enumeration order of the device tree nodes > (module@0 = kernel, module@1 = initrd). This allows bootloaders > without any specific Xen knowledge to boot Xen anyways. > > [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/common/device_tree.c | 57 ++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 50 insertions(+), 7 deletions(-) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index ec0d5e2..e10c035 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -439,22 +439,63 @@ static void __init process_cpu_node(const void *fdt, int node, > cpumask_set_cpu(start, &cpu_possible_map); > } > > +static const char * const kernel_module_names[] = { > + "xen,linux-zimage", > + "xen,dom0-kernel", > + "boot,kernel", > + NULL > +}; > + > +static const char * const initrd_module_names[] = { > + "xen,linux-initrd", > + "xen,ramdisk", > + "boot,ramdisk", > + NULL > +}; > + > static void __init process_multiboot_node(const void *fdt, int node, > const char *name, > u32 address_cells, u32 size_cells) > { > const struct fdt_property *prop; > const u32 *cell; > - int nr; > + int nr = -1; > struct dt_mb_module *mod; > int len; > + const char* const * name_list; > > - if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ) > - nr = MOD_KERNEL; > - else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0) > - nr = MOD_INITRD; > - else > - early_panic("%s not a known xen multiboot type\n", name); > + for (name_list = kernel_module_names; *name_list != NULL; name_list++) > + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { > + nr = MOD_KERNEL; > + break; > + } > + > + for (name_list = initrd_module_names; *name_list != NULL; name_list++) > + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { > + nr = MOD_INITRD; > + break; > + } > + > + if (nr == -1) { > + char *s; > + > + if ( fdt_node_check_compatible(fdt, node, "boot,module") != 0 ) { > + early_panic("%s not a known xen multiboot type\n", name); > + return; > + } > + s = strchr(name, ''@''); > + if (s == NULL) > + nr = early_info.modules.nr_mods + 1; > + else { > + nr = simple_strtoll(s + 1, NULL, 10) + 1; > + } > + } > + > + if (nr >= NR_MODULES) { > + early_panic("only supporting %d multiboot modules\n", > + NR_MODULES - MOD_DISCARD_FIRST); > + return; > + } > > mod = &early_info.modules.module[nr]; > > @@ -492,6 +533,8 @@ static int __init early_scan_node(const void *fdt, > process_cpu_node(fdt, node, name, address_cells, size_cells); > else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) > process_multiboot_node(fdt, node, name, address_cells, size_cells); > + else if ( device_tree_node_compatible(fdt, node, "boot,module" ) ) > + process_multiboot_node(fdt, node, name, address_cells, size_cells); > > return 0; > } >
Ian Campbell
2013-Sep-05 14:03 UTC
Re: [PATCH] ARM/multiboot: use more flexible node naming
On Thu, 2013-09-05 at 15:54 +0200, Egger, Christoph wrote:> Is there a chance to get NetBSD Dom0 [1] to boot to justify that your > approach is *really* generic. > > xen,linux-zimage and xen,linux-initrd do not sound generic > but xen,dom0-kernel and xen,ramdisk do sound generic at least.Does NetBSD follow the Linux zImage boot protocol (Documentation/arm/Booting.txt in a Linux source tree)? If not then you need to define (or link to) the NetBSD boot protocol and we probably will eventually need xen,netbsd-fooimg and suitable code in Xen to handle that format.> [1] NetBSD Dom0 hasn''t been ported to ARM yet.So surely you have answered your own question then, unless you are asking Andre to port NetBSD to ARM? I don''t think that is a reasonable prerequisite for this patch.> > > On 05.09.13 15:43, Andre Przywara wrote: > > For the current "multiboot" on ARM support we look for a compatible > > string of "xen,multiboot-module" in the device tree, and then > > use "xen,linux-zimage" and "xen,linux-initrd" to differentiate > > between the two supported module types. > > To meet the more generic multiboot proposal in the device tree [1], > > allow Xen to be more flexible in the compatible naming and also use > > the new generic base name "boot,module". > > The mapping to either Dom0 kernel or RAM disk works either by > > providing a more specific name ("xen,dom0-kernel" and "xen,ramdisk"), > > or by using the enumeration order of the device tree nodes > > (module@0 = kernel, module@1 = initrd). This allows bootloaders > > without any specific Xen knowledge to boot Xen anyways. > > > > [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html > > > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > > --- > > xen/common/device_tree.c | 57 ++++++++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 50 insertions(+), 7 deletions(-) > > > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > > index ec0d5e2..e10c035 100644 > > --- a/xen/common/device_tree.c > > +++ b/xen/common/device_tree.c > > @@ -439,22 +439,63 @@ static void __init process_cpu_node(const void *fdt, int node, > > cpumask_set_cpu(start, &cpu_possible_map); > > } > > > > +static const char * const kernel_module_names[] = { > > + "xen,linux-zimage", > > + "xen,dom0-kernel", > > + "boot,kernel", > > + NULL > > +}; > > + > > +static const char * const initrd_module_names[] = { > > + "xen,linux-initrd", > > + "xen,ramdisk", > > + "boot,ramdisk", > > + NULL > > +}; > > + > > static void __init process_multiboot_node(const void *fdt, int node, > > const char *name, > > u32 address_cells, u32 size_cells) > > { > > const struct fdt_property *prop; > > const u32 *cell; > > - int nr; > > + int nr = -1; > > struct dt_mb_module *mod; > > int len; > > + const char* const * name_list; > > > > - if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ) > > - nr = MOD_KERNEL; > > - else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0) > > - nr = MOD_INITRD; > > - else > > - early_panic("%s not a known xen multiboot type\n", name); > > + for (name_list = kernel_module_names; *name_list != NULL; name_list++) > > + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { > > + nr = MOD_KERNEL; > > + break; > > + } > > + > > + for (name_list = initrd_module_names; *name_list != NULL; name_list++) > > + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { > > + nr = MOD_INITRD; > > + break; > > + } > > + > > + if (nr == -1) { > > + char *s; > > + > > + if ( fdt_node_check_compatible(fdt, node, "boot,module") != 0 ) { > > + early_panic("%s not a known xen multiboot type\n", name); > > + return; > > + } > > + s = strchr(name, ''@''); > > + if (s == NULL) > > + nr = early_info.modules.nr_mods + 1; > > + else { > > + nr = simple_strtoll(s + 1, NULL, 10) + 1; > > + } > > + } > > + > > + if (nr >= NR_MODULES) { > > + early_panic("only supporting %d multiboot modules\n", > > + NR_MODULES - MOD_DISCARD_FIRST); > > + return; > > + } > > > > mod = &early_info.modules.module[nr]; > > > > @@ -492,6 +533,8 @@ static int __init early_scan_node(const void *fdt, > > process_cpu_node(fdt, node, name, address_cells, size_cells); > > else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) > > process_multiboot_node(fdt, node, name, address_cells, size_cells); > > + else if ( device_tree_node_compatible(fdt, node, "boot,module" ) ) > > + process_multiboot_node(fdt, node, name, address_cells, size_cells); > > > > return 0; > > } > > >
Egger, Christoph
2013-Sep-05 14:22 UTC
Re: [PATCH] ARM/multiboot: use more flexible node naming
On 05.09.13 16:03, Ian Campbell wrote:> On Thu, 2013-09-05 at 15:54 +0200, Egger, Christoph wrote: >> Is there a chance to get NetBSD Dom0 [1] to boot to justify that your >> approach is *really* generic. >> >> xen,linux-zimage and xen,linux-initrd do not sound generic >> but xen,dom0-kernel and xen,ramdisk do sound generic at least. > > Does NetBSD follow the Linux zImage boot protocol > (Documentation/arm/Booting.txt in a Linux source tree)? > > If not then you need to define (or link to) the NetBSD boot protocol and > we probably will eventually need xen,netbsd-fooimg and suitable code in > Xen to handle that format. > >> [1] NetBSD Dom0 hasn''t been ported to ARM yet. > > So surely you have answered your own question then, unless you are > asking Andre to port NetBSD to ARM? I don''t think that is a reasonable > prerequisite for this patch.No, it is not a prerequisite. My question is related to the design if it is "I allow Linux only to boot" or "I allow any Dom0 to boot". Christoph> >> >> >> On 05.09.13 15:43, Andre Przywara wrote: >>> For the current "multiboot" on ARM support we look for a compatible >>> string of "xen,multiboot-module" in the device tree, and then >>> use "xen,linux-zimage" and "xen,linux-initrd" to differentiate >>> between the two supported module types. >>> To meet the more generic multiboot proposal in the device tree [1], >>> allow Xen to be more flexible in the compatible naming and also use >>> the new generic base name "boot,module". >>> The mapping to either Dom0 kernel or RAM disk works either by >>> providing a more specific name ("xen,dom0-kernel" and "xen,ramdisk"), >>> or by using the enumeration order of the device tree nodes >>> (module@0 = kernel, module@1 = initrd). This allows bootloaders >>> without any specific Xen knowledge to boot Xen anyways. >>> >>> [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html >>> >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>> --- >>> xen/common/device_tree.c | 57 ++++++++++++++++++++++++++++++++++++++++++------ >>> 1 file changed, 50 insertions(+), 7 deletions(-) >>> >>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >>> index ec0d5e2..e10c035 100644 >>> --- a/xen/common/device_tree.c >>> +++ b/xen/common/device_tree.c >>> @@ -439,22 +439,63 @@ static void __init process_cpu_node(const void *fdt, int node, >>> cpumask_set_cpu(start, &cpu_possible_map); >>> } >>> >>> +static const char * const kernel_module_names[] = { >>> + "xen,linux-zimage", >>> + "xen,dom0-kernel", >>> + "boot,kernel", >>> + NULL >>> +}; >>> + >>> +static const char * const initrd_module_names[] = { >>> + "xen,linux-initrd", >>> + "xen,ramdisk", >>> + "boot,ramdisk", >>> + NULL >>> +}; >>> + >>> static void __init process_multiboot_node(const void *fdt, int node, >>> const char *name, >>> u32 address_cells, u32 size_cells) >>> { >>> const struct fdt_property *prop; >>> const u32 *cell; >>> - int nr; >>> + int nr = -1; >>> struct dt_mb_module *mod; >>> int len; >>> + const char* const * name_list; >>> >>> - if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ) >>> - nr = MOD_KERNEL; >>> - else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0) >>> - nr = MOD_INITRD; >>> - else >>> - early_panic("%s not a known xen multiboot type\n", name); >>> + for (name_list = kernel_module_names; *name_list != NULL; name_list++) >>> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { >>> + nr = MOD_KERNEL; >>> + break; >>> + } >>> + >>> + for (name_list = initrd_module_names; *name_list != NULL; name_list++) >>> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { >>> + nr = MOD_INITRD; >>> + break; >>> + } >>> + >>> + if (nr == -1) { >>> + char *s; >>> + >>> + if ( fdt_node_check_compatible(fdt, node, "boot,module") != 0 ) { >>> + early_panic("%s not a known xen multiboot type\n", name); >>> + return; >>> + } >>> + s = strchr(name, ''@''); >>> + if (s == NULL) >>> + nr = early_info.modules.nr_mods + 1; >>> + else { >>> + nr = simple_strtoll(s + 1, NULL, 10) + 1; >>> + } >>> + } >>> + >>> + if (nr >= NR_MODULES) { >>> + early_panic("only supporting %d multiboot modules\n", >>> + NR_MODULES - MOD_DISCARD_FIRST); >>> + return; >>> + } >>> >>> mod = &early_info.modules.module[nr]; >>> >>> @@ -492,6 +533,8 @@ static int __init early_scan_node(const void *fdt, >>> process_cpu_node(fdt, node, name, address_cells, size_cells); >>> else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) >>> process_multiboot_node(fdt, node, name, address_cells, size_cells); >>> + else if ( device_tree_node_compatible(fdt, node, "boot,module" ) ) >>> + process_multiboot_node(fdt, node, name, address_cells, size_cells); >>> >>> return 0; >>> } >>> >> > >
Ian Campbell
2013-Sep-05 14:59 UTC
Re: [PATCH] ARM/multiboot: use more flexible node naming
On Thu, 2013-09-05 at 16:22 +0200, Egger, Christoph wrote:> On 05.09.13 16:03, Ian Campbell wrote: > > On Thu, 2013-09-05 at 15:54 +0200, Egger, Christoph wrote: > >> Is there a chance to get NetBSD Dom0 [1] to boot to justify that your > >> approach is *really* generic. > >> > >> xen,linux-zimage and xen,linux-initrd do not sound generic > >> but xen,dom0-kernel and xen,ramdisk do sound generic at least. > > > > Does NetBSD follow the Linux zImage boot protocol > > (Documentation/arm/Booting.txt in a Linux source tree)? > > > > If not then you need to define (or link to) the NetBSD boot protocol and > > we probably will eventually need xen,netbsd-fooimg and suitable code in > > Xen to handle that format. > > > >> [1] NetBSD Dom0 hasn''t been ported to ARM yet. > > > > So surely you have answered your own question then, unless you are > > asking Andre to port NetBSD to ARM? I don''t think that is a reasonable > > prerequisite for this patch. > > No, it is not a prerequisite. My question is related to the design > if it is "I allow Linux only to boot" or "I allow any Dom0 to boot".The design allows for any dom0, since it includes a compatibility string which corresponds to the boot protocol to use. The implementation currently only covers Linux though.> Christoph > > > > >> > >> > >> On 05.09.13 15:43, Andre Przywara wrote: > >>> For the current "multiboot" on ARM support we look for a compatible > >>> string of "xen,multiboot-module" in the device tree, and then > >>> use "xen,linux-zimage" and "xen,linux-initrd" to differentiate > >>> between the two supported module types. > >>> To meet the more generic multiboot proposal in the device tree [1], > >>> allow Xen to be more flexible in the compatible naming and also use > >>> the new generic base name "boot,module". > >>> The mapping to either Dom0 kernel or RAM disk works either by > >>> providing a more specific name ("xen,dom0-kernel" and "xen,ramdisk"), > >>> or by using the enumeration order of the device tree nodes > >>> (module@0 = kernel, module@1 = initrd). This allows bootloaders > >>> without any specific Xen knowledge to boot Xen anyways. > >>> > >>> [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html > >>> > >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > >>> --- > >>> xen/common/device_tree.c | 57 ++++++++++++++++++++++++++++++++++++++++++------ > >>> 1 file changed, 50 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > >>> index ec0d5e2..e10c035 100644 > >>> --- a/xen/common/device_tree.c > >>> +++ b/xen/common/device_tree.c > >>> @@ -439,22 +439,63 @@ static void __init process_cpu_node(const void *fdt, int node, > >>> cpumask_set_cpu(start, &cpu_possible_map); > >>> } > >>> > >>> +static const char * const kernel_module_names[] = { > >>> + "xen,linux-zimage", > >>> + "xen,dom0-kernel", > >>> + "boot,kernel", > >>> + NULL > >>> +}; > >>> + > >>> +static const char * const initrd_module_names[] = { > >>> + "xen,linux-initrd", > >>> + "xen,ramdisk", > >>> + "boot,ramdisk", > >>> + NULL > >>> +}; > >>> + > >>> static void __init process_multiboot_node(const void *fdt, int node, > >>> const char *name, > >>> u32 address_cells, u32 size_cells) > >>> { > >>> const struct fdt_property *prop; > >>> const u32 *cell; > >>> - int nr; > >>> + int nr = -1; > >>> struct dt_mb_module *mod; > >>> int len; > >>> + const char* const * name_list; > >>> > >>> - if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ) > >>> - nr = MOD_KERNEL; > >>> - else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0) > >>> - nr = MOD_INITRD; > >>> - else > >>> - early_panic("%s not a known xen multiboot type\n", name); > >>> + for (name_list = kernel_module_names; *name_list != NULL; name_list++) > >>> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { > >>> + nr = MOD_KERNEL; > >>> + break; > >>> + } > >>> + > >>> + for (name_list = initrd_module_names; *name_list != NULL; name_list++) > >>> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { > >>> + nr = MOD_INITRD; > >>> + break; > >>> + } > >>> + > >>> + if (nr == -1) { > >>> + char *s; > >>> + > >>> + if ( fdt_node_check_compatible(fdt, node, "boot,module") != 0 ) { > >>> + early_panic("%s not a known xen multiboot type\n", name); > >>> + return; > >>> + } > >>> + s = strchr(name, ''@''); > >>> + if (s == NULL) > >>> + nr = early_info.modules.nr_mods + 1; > >>> + else { > >>> + nr = simple_strtoll(s + 1, NULL, 10) + 1; > >>> + } > >>> + } > >>> + > >>> + if (nr >= NR_MODULES) { > >>> + early_panic("only supporting %d multiboot modules\n", > >>> + NR_MODULES - MOD_DISCARD_FIRST); > >>> + return; > >>> + } > >>> > >>> mod = &early_info.modules.module[nr]; > >>> > >>> @@ -492,6 +533,8 @@ static int __init early_scan_node(const void *fdt, > >>> process_cpu_node(fdt, node, name, address_cells, size_cells); > >>> else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) > >>> process_multiboot_node(fdt, node, name, address_cells, size_cells); > >>> + else if ( device_tree_node_compatible(fdt, node, "boot,module" ) ) > >>> + process_multiboot_node(fdt, node, name, address_cells, size_cells); > >>> > >>> return 0; > >>> } > >>> > >> > > > > >
Andre Przywara
2013-Sep-06 08:44 UTC
Re: [PATCH] ARM/multiboot: use more flexible node naming
On 09/05/2013 04:59 PM, Ian Campbell wrote:> On Thu, 2013-09-05 at 16:22 +0200, Egger, Christoph wrote: >> On 05.09.13 16:03, Ian Campbell wrote: >>> On Thu, 2013-09-05 at 15:54 +0200, Egger, Christoph wrote: >>>> Is there a chance to get NetBSD Dom0 [1] to boot to justify that your >>>> approach is *really* generic. >>>> >>>> xen,linux-zimage and xen,linux-initrd do not sound generic >>>> but xen,dom0-kernel and xen,ramdisk do sound generic at least. >>> >>> Does NetBSD follow the Linux zImage boot protocol >>> (Documentation/arm/Booting.txt in a Linux source tree)? >>> >>> If not then you need to define (or link to) the NetBSD boot protocol and >>> we probably will eventually need xen,netbsd-fooimg and suitable code in >>> Xen to handle that format. >>> >>>> [1] NetBSD Dom0 hasn''t been ported to ARM yet. >>> >>> So surely you have answered your own question then, unless you are >>> asking Andre to port NetBSD to ARM? I don''t think that is a reasonable >>> prerequisite for this patch. >> >> No, it is not a prerequisite. My question is related to the design >> if it is "I allow Linux only to boot" or "I allow any Dom0 to boot". > > The design allows for any dom0, since it includes a compatibility string > which corresponds to the boot protocol to use. The implementation > currently only covers Linux though.I think the whole discussion is somewhat moot here, as NetBSD does not seem to support device trees. I see FreeBSD has support for it, but with some quick googling I cannot find anything in NetBSD. Regards, Andre.>>>> >>>> On 05.09.13 15:43, Andre Przywara wrote: >>>>> For the current "multiboot" on ARM support we look for a compatible >>>>> string of "xen,multiboot-module" in the device tree, and then >>>>> use "xen,linux-zimage" and "xen,linux-initrd" to differentiate >>>>> between the two supported module types. >>>>> To meet the more generic multiboot proposal in the device tree [1], >>>>> allow Xen to be more flexible in the compatible naming and also use >>>>> the new generic base name "boot,module". >>>>> The mapping to either Dom0 kernel or RAM disk works either by >>>>> providing a more specific name ("xen,dom0-kernel" and "xen,ramdisk"), >>>>> or by using the enumeration order of the device tree nodes >>>>> (module@0 = kernel, module@1 = initrd). This allows bootloaders >>>>> without any specific Xen knowledge to boot Xen anyways. >>>>> >>>>> [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html >>>>> >>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>>> --- >>>>> xen/common/device_tree.c | 57 ++++++++++++++++++++++++++++++++++++++++++------ >>>>> 1 file changed, 50 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >>>>> index ec0d5e2..e10c035 100644 >>>>> --- a/xen/common/device_tree.c >>>>> +++ b/xen/common/device_tree.c >>>>> @@ -439,22 +439,63 @@ static void __init process_cpu_node(const void *fdt, int node, >>>>> cpumask_set_cpu(start, &cpu_possible_map); >>>>> } >>>>> >>>>> +static const char * const kernel_module_names[] = { >>>>> + "xen,linux-zimage", >>>>> + "xen,dom0-kernel", >>>>> + "boot,kernel", >>>>> + NULL >>>>> +}; >>>>> + >>>>> +static const char * const initrd_module_names[] = { >>>>> + "xen,linux-initrd", >>>>> + "xen,ramdisk", >>>>> + "boot,ramdisk", >>>>> + NULL >>>>> +}; >>>>> + >>>>> static void __init process_multiboot_node(const void *fdt, int node, >>>>> const char *name, >>>>> u32 address_cells, u32 size_cells) >>>>> { >>>>> const struct fdt_property *prop; >>>>> const u32 *cell; >>>>> - int nr; >>>>> + int nr = -1; >>>>> struct dt_mb_module *mod; >>>>> int len; >>>>> + const char* const * name_list; >>>>> >>>>> - if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ) >>>>> - nr = MOD_KERNEL; >>>>> - else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0) >>>>> - nr = MOD_INITRD; >>>>> - else >>>>> - early_panic("%s not a known xen multiboot type\n", name); >>>>> + for (name_list = kernel_module_names; *name_list != NULL; name_list++) >>>>> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { >>>>> + nr = MOD_KERNEL; >>>>> + break; >>>>> + } >>>>> + >>>>> + for (name_list = initrd_module_names; *name_list != NULL; name_list++) >>>>> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { >>>>> + nr = MOD_INITRD; >>>>> + break; >>>>> + } >>>>> + >>>>> + if (nr == -1) { >>>>> + char *s; >>>>> + >>>>> + if ( fdt_node_check_compatible(fdt, node, "boot,module") != 0 ) { >>>>> + early_panic("%s not a known xen multiboot type\n", name); >>>>> + return; >>>>> + } >>>>> + s = strchr(name, ''@''); >>>>> + if (s == NULL) >>>>> + nr = early_info.modules.nr_mods + 1; >>>>> + else { >>>>> + nr = simple_strtoll(s + 1, NULL, 10) + 1; >>>>> + } >>>>> + } >>>>> + >>>>> + if (nr >= NR_MODULES) { >>>>> + early_panic("only supporting %d multiboot modules\n", >>>>> + NR_MODULES - MOD_DISCARD_FIRST); >>>>> + return; >>>>> + } >>>>> >>>>> mod = &early_info.modules.module[nr]; >>>>> >>>>> @@ -492,6 +533,8 @@ static int __init early_scan_node(const void *fdt, >>>>> process_cpu_node(fdt, node, name, address_cells, size_cells); >>>>> else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) >>>>> process_multiboot_node(fdt, node, name, address_cells, size_cells); >>>>> + else if ( device_tree_node_compatible(fdt, node, "boot,module" ) ) >>>>> + process_multiboot_node(fdt, node, name, address_cells, size_cells); >>>>> >>>>> return 0; >>>>> } >>>>> >>>> >>> >>> >> > >
Julien Grall
2013-Sep-06 11:11 UTC
Re: [PATCH] ARM/multiboot: use more flexible node naming
Hi, I have also modified this function with my patch series "Allow Xen to boot with a raw Device Tree". Can you rebase this patch on top of this serie? Other comments below. On 09/05/2013 02:43 PM, Andre Przywara wrote:> For the current "multiboot" on ARM support we look for a compatible > string of "xen,multiboot-module" in the device tree, and then > use "xen,linux-zimage" and "xen,linux-initrd" to differentiate > between the two supported module types. > To meet the more generic multiboot proposal in the device tree [1], > allow Xen to be more flexible in the compatible naming and also use > the new generic base name "boot,module". > The mapping to either Dom0 kernel or RAM disk works either by > providing a more specific name ("xen,dom0-kernel" and "xen,ramdisk"), > or by using the enumeration order of the device tree nodes > (module@0 = kernel, module@1 = initrd). This allows bootloaders > without any specific Xen knowledge to boot Xen anyways.The ePAR (section 2.2.1) requires the unit-address (name@unit-address) to match the first address of the "reg" property. I think, with this solution we doesn''t comply to the specification. If the bootloader is able to add "boot,kernel" and "boot,initramfs" to the compatible property, we should have a generic solution for multiboot.> [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/common/device_tree.c | 57 ++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 50 insertions(+), 7 deletions(-) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index ec0d5e2..e10c035 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -439,22 +439,63 @@ static void __init process_cpu_node(const void *fdt, int node, > cpumask_set_cpu(start, &cpu_possible_map); > } > > +static const char * const kernel_module_names[] = {These variable are only used durint the initialization, you should add __initconst.> + "xen,linux-zimage", > + "xen,dom0-kernel", > + "boot,kernel", > + NULLXen uses spaces instead of tabulations.> +}; > + > +static const char * const initrd_module_names[] = {__initconst> + "xen,linux-initrd", > + "xen,ramdisk", > + "boot,ramdisk", > + NULLtabulations> +}; > + > static void __init process_multiboot_node(const void *fdt, int node, > const char *name, > u32 address_cells, u32 size_cells) > { > const struct fdt_property *prop; > const u32 *cell; > - int nr; > + int nr = -1; > struct dt_mb_module *mod; > int len; > + const char* const * name_list; > > - if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ) > - nr = MOD_KERNEL; > - else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0) > - nr = MOD_INITRD; > - else > - early_panic("%s not a known xen multiboot type\n", name); > + for (name_list = kernel_module_names; *name_list != NULL; name_list++)the coding style for "for" is for ( ... )> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) {newline before {> + nr = MOD_KERNEL; > + break; > + } > + > + for (name_list = initrd_module_names; *name_list != NULL; name_list++)for ( ... )> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) {newline before {> + nr = MOD_INITRD; > + break; > + } > + > + if (nr == -1) {newline before {> + char *s;there is a tabulation here.> + > + if ( fdt_node_check_compatible(fdt, node, "boot,module") != 0 ) {newline before {> + early_panic("%s not a known xen multiboot type\n", name);early_panic is a bit tough, can we simply ignore the node?> + return; > + } > + s = strchr(name, ''@''); > + if (s == NULL)if ( ... )> + nr = early_info.modules.nr_mods + 1; > + else {newline before {> + nr = simple_strtoll(s + 1, NULL, 10) + 1; > + } > + } > + > + if (nr >= NR_MODULES) {if ( ... ) You should also check nr < 0. Strtoll could return a negative value.> + early_panic("only supporting %d multiboot modules\n", > + NR_MODULES - MOD_DISCARD_FIRST);indentation here.> + return; > + } > > mod = &early_info.modules.module[nr]; > > @@ -492,6 +533,8 @@ static int __init early_scan_node(const void *fdt, > process_cpu_node(fdt, node, name, address_cells, size_cells); > else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) > process_multiboot_node(fdt, node, name, address_cells, size_cells); > + else if ( device_tree_node_compatible(fdt, node, "boot,module" ) ) > + process_multiboot_node(fdt, node, name, address_cells, size_cells);Do we consider the multiboot node can live outside /chosen?> > return 0; > } >-- Julien Grall
Ian Campbell
2013-Sep-09 12:00 UTC
Re: [PATCH] ARM/multiboot: use more flexible node naming
On Thu, 2013-09-05 at 15:43 +0200, Andre Przywara wrote:> For the current "multiboot" on ARM support we look for a compatible > string of "xen,multiboot-module" in the device tree, and then > use "xen,linux-zimage" and "xen,linux-initrd" to differentiate > between the two supported module types. > To meet the more generic multiboot proposal in the device tree [1], > allow Xen to be more flexible in the compatible naming and also use > the new generic base name "boot,module". > The mapping to either Dom0 kernel or RAM disk works either by > providing a more specific name ("xen,dom0-kernel" and "xen,ramdisk"), > or by using the enumeration order of the device tree nodes > (module@0 = kernel, module@1 = initrd). This allows bootloaders > without any specific Xen knowledge to boot Xen anyways. > > [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/common/device_tree.c | 57 ++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 50 insertions(+), 7 deletions(-) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index ec0d5e2..e10c035 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -439,22 +439,63 @@ static void __init process_cpu_node(const void *fdt, int node, > cpumask_set_cpu(start, &cpu_possible_map); > } > > +static const char * const kernel_module_names[] = { > + "xen,linux-zimage", > + "xen,dom0-kernel", > + "boot,kernel", > + NULL > +}; > + > +static const char * const initrd_module_names[] = { > + "xen,linux-initrd", > + "xen,ramdisk", > + "boot,ramdisk", > + NULL > +}; > + > static void __init process_multiboot_node(const void *fdt, int node, > const char *name, > u32 address_cells, u32 size_cells) > { > const struct fdt_property *prop; > const u32 *cell; > - int nr; > + int nr = -1; > struct dt_mb_module *mod; > int len; > + const char* const * name_list; > > - if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ) > - nr = MOD_KERNEL; > - else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0) > - nr = MOD_INITRD; > - else > - early_panic("%s not a known xen multiboot type\n", name); > + for (name_list = kernel_module_names; *name_list != NULL; name_list++) > + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { > + nr = MOD_KERNEL; > + break; > + } > + > + for (name_list = initrd_module_names; *name_list != NULL; name_list++) > + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) {I think Julien''s big dtb series adds a helper which would make this simpler? That could be a later fix up, are there any other ordering constraints with that series?> + nr = MOD_INITRD; > + break; > + } > + > + if (nr == -1) { > + char *s; > + > + if ( fdt_node_check_compatible(fdt, node, "boot,module") != 0 ) { > + early_panic("%s not a known xen multiboot type\n", name); > + return; > + } > + s = strchr(name, ''@''); > + if (s == NULL) > + nr = early_info.modules.nr_mods + 1; > + else { > + nr = simple_strtoll(s + 1, NULL, 10) + 1; > + } > + } > + > + if (nr >= NR_MODULES) { > + early_panic("only supporting %d multiboot modules\n", > + NR_MODULES - MOD_DISCARD_FIRST); > + return; > + } > > mod = &early_info.modules.module[nr]; > > @@ -492,6 +533,8 @@ static int __init early_scan_node(const void *fdt, > process_cpu_node(fdt, node, name, address_cells, size_cells); > else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) > process_multiboot_node(fdt, node, name, address_cells, size_cells); > + else if ( device_tree_node_compatible(fdt, node, "boot,module" ) ) > + process_multiboot_node(fdt, node, name, address_cells, size_cells); > > return 0; > }
Andre Przywara
2013-Sep-09 13:52 UTC
Re: [PATCH] ARM/multiboot: use more flexible node naming
On 09/06/2013 01:11 PM, Julien Grall wrote:> Hi, > > I have also modified this function with my patch series "Allow Xen to > boot with a raw Device Tree". Can you rebase this patch on top of this > serie? > > Other comments below. > > On 09/05/2013 02:43 PM, Andre Przywara wrote: >> For the current "multiboot" on ARM support we look for a compatible >> string of "xen,multiboot-module" in the device tree, and then >> use "xen,linux-zimage" and "xen,linux-initrd" to differentiate >> between the two supported module types. >> To meet the more generic multiboot proposal in the device tree [1], >> allow Xen to be more flexible in the compatible naming and also use >> the new generic base name "boot,module". >> The mapping to either Dom0 kernel or RAM disk works either by >> providing a more specific name ("xen,dom0-kernel" and "xen,ramdisk"), >> or by using the enumeration order of the device tree nodes >> (module@0 = kernel, module@1 = initrd). This allows bootloaders >> without any specific Xen knowledge to boot Xen anyways. > > The ePAR (section 2.2.1) requires the unit-address (name@unit-address) > to match the first address of the "reg" property. I think, with this > solution we doesn''t comply to the specification.Yes, you are right. As it seems to be of no use to use the start address to do the enumeration (in my testing I load the initrd at a lower address for instance), I will drop this part of the patch and rely on proper, though generic naming of the nodes instead.> > If the bootloader is able to add "boot,kernel" and "boot,initramfs" to > the compatible property, we should have a generic solution for multiboot. > >> [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/common/device_tree.c | 57 >> ++++++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 50 insertions(+), 7 deletions(-) >> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >> index ec0d5e2..e10c035 100644 >> --- a/xen/common/device_tree.c >> +++ b/xen/common/device_tree.c >> @@ -439,22 +439,63 @@ static void __init process_cpu_node(const void >> *fdt, int node, >> cpumask_set_cpu(start, &cpu_possible_map); >> } >> >> +static const char * const kernel_module_names[] = { > These variable are only used durint the initialization, you should add > __initconst. >As already discussed on IRC, it seems like __initconst is not yet upstream and __initdata doesn''t work yet, either. So I will keep this part as it is for now with the option to fix this later.>> + "xen,linux-zimage", >> + "xen,dom0-kernel", >> + "boot,kernel", >> + NULL > > Xen uses spaces instead of tabulations.Sorry for all these whitespace issues. Seems like the switch between u-boot and Xen didn''t switch the whole context ;-) ....> >> + early_panic("%s not a known xen multiboot type\n", name); > > early_panic is a bit tough, can we simply ignore the node?In fact this was copied from the place before. Not sure if the situation has now changed with the wider, not necessarily Xen-centric scope of the modules. But in fact we could print a warning about this. If there is no kernel module specified, we will panic later anyways. ....>> @@ -492,6 +533,8 @@ static int __init early_scan_node(const void *fdt, >> process_cpu_node(fdt, node, name, address_cells, size_cells); >> else if ( device_tree_node_compatible(fdt, node, >> "xen,multiboot-module" ) ) >> process_multiboot_node(fdt, node, name, address_cells, >> size_cells); >> + else if ( device_tree_node_compatible(fdt, node, "boot,module" ) ) >> + process_multiboot_node(fdt, node, name, address_cells, >> size_cells); > > Do we consider the multiboot node can live outside /chosen?Did we care for the old node type? I just copied what was used before. Maybe for bootloaders it is technically easier to put it under the root node, so as long as it doesn''t hurt us, I''d like to keep it this way. Will send out a reworked version ASAP. Thanks for the review, Andre.
Andre Przywara
2013-Sep-09 14:01 UTC
Re: [PATCH] ARM/multiboot: use more flexible node naming
On 09/09/2013 02:00 PM, Ian Campbell wrote:> On Thu, 2013-09-05 at 15:43 +0200, Andre Przywara wrote: >> For the current "multiboot" on ARM support we look for a compatible >> string of "xen,multiboot-module" in the device tree, and then >> use "xen,linux-zimage" and "xen,linux-initrd" to differentiate >> between the two supported module types. >> To meet the more generic multiboot proposal in the device tree [1], >> allow Xen to be more flexible in the compatible naming and also use >> the new generic base name "boot,module". >> The mapping to either Dom0 kernel or RAM disk works either by >> providing a more specific name ("xen,dom0-kernel" and "xen,ramdisk"), >> or by using the enumeration order of the device tree nodes >> (module@0 = kernel, module@1 = initrd). This allows bootloaders >> without any specific Xen knowledge to boot Xen anyways. >> >> [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/common/device_tree.c | 57 ++++++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 50 insertions(+), 7 deletions(-) >> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >> index ec0d5e2..e10c035 100644 >> --- a/xen/common/device_tree.c >> +++ b/xen/common/device_tree.c >> @@ -439,22 +439,63 @@ static void __init process_cpu_node(const void *fdt, int node, >> cpumask_set_cpu(start, &cpu_possible_map); >> } >> >> +static const char * const kernel_module_names[] = { >> + "xen,linux-zimage", >> + "xen,dom0-kernel", >> + "boot,kernel", >> + NULL >> +}; >> + >> +static const char * const initrd_module_names[] = { >> + "xen,linux-initrd", >> + "xen,ramdisk", >> + "boot,ramdisk", >> + NULL >> +}; >> + >> static void __init process_multiboot_node(const void *fdt, int node, >> const char *name, >> u32 address_cells, u32 size_cells) >> { >> const struct fdt_property *prop; >> const u32 *cell; >> - int nr; >> + int nr = -1; >> struct dt_mb_module *mod; >> int len; >> + const char* const * name_list; >> >> - if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ) >> - nr = MOD_KERNEL; >> - else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0) >> - nr = MOD_INITRD; >> - else >> - early_panic("%s not a known xen multiboot type\n", name); >> + for (name_list = kernel_module_names; *name_list != NULL; name_list++) >> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { >> + nr = MOD_KERNEL; >> + break; >> + } >> + >> + for (name_list = initrd_module_names; *name_list != NULL; name_list++) >> + if ( fdt_node_check_compatible(fdt, node, *name_list) == 0 ) { > > I think Julien''s big dtb series adds a helper which would make this > simpler? > > That could be a later fix up, are there any other ordering constraints > with that series?Not that I am aware of. I think I will send out my new version so far for discussion and review and will happily rebase it later on if needed. Thanks, Andre.