Re: [PATCH] ARM/multiboot: use more flexible node naming

[Julien Grall <julien.grall@linaro.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 <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",
> +	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