From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] ARM/multiboot: use more flexible node naming Date: Fri, 06 Sep 2013 12:11:29 +0100 Message-ID: <5229B861.8080000@linaro.org> References: <1378388623-9853-1-git-send-email-andre.przywara@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1378388623-9853-1-git-send-email-andre.przywara@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andre Przywara Cc: stefano.stabellini@eu.citrix.com, Ian.Campbell@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > --- > 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", > + NULL Xen uses spaces instead of tabulations. > +}; > + > +static const char * const initrd_module_names[] = { __initconst > + "xen,linux-initrd", > + "xen,ramdisk", > + "boot,ramdisk", > + NULL tabulations > +}; > + > 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