Re: [Xen-devel] [PATCH v2] ARM/multiboot: use more flexible node naming

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2013-09-11 at 16:06 +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 by providing a
> more specific name ("xen,dom0-kernel" and "xen,ramdisk", preferably).
> For compatibility reasons the older names above are still recognized.
> 
> Changes from v1:
> * whitespace / coding style fixes (sorry for that mess!)
> * removed module enumeration by using module@address
>   (this violates the EPAPR device tree spec).
> * added __initconst to names array
> 
> [1] http://lists.xen.org/archives/html/xen-devel/2013-09/msg00083.html
> 
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
>  xen/common/device_tree.c | 44 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index eed77ce..3ae593f 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -433,22 +433,50 @@ static void __init process_cpu_node(const void *fdt, 
> int node,
>      cpumask_set_cpu(start, &cpu_possible_map);
>  }
>  
> +static const char * const __initconst kernel_module_names[] = {
> +    "xen,linux-zimage",
> +    "xen,dom0-kernel",
> +    "boot,kernel",

I'm wondering about this..

The current "xen,linux-zimage" node does more than simply identifying
the location in memory of the kernel, it actually tells us that the boot
protocol used by that kernel is the Linux zImage protocol.

For "xen,dom0-kernel" and "boot,kernel" we don't get that -- so how do
we know how the kernel in question wants to be called?

I guess there is a magic number in the kernel itself, so maybe this is
OK? Any futyre kernel (I'm thinking *BSD here...) would either need to
be identifiable by magic number or use only a specific compatible value
which indicates this...

OK, I think I've convinced myself, I'll leave the commentary above in
case you can spot a flaw in my reasoning.

> +    NULL
> +};
> +
> +static const char * const __initconst 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 )

I've just pushed Julien's big DTB series which adds a helper for
matching against a list of compatible nodes.

> +        {
> +            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 )
> +    {
> +        early_printk("warning: %s not a known module type, ignoring\n", 
> name);
> +        return;
> +    }
>  
>      mod = &early_info.modules.module[nr];
>  
> @@ -486,6 +514,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" 
> ) )

Should we mark this as deprecated in the docs?

Speaking of which, this patch doesn't seem to ouch the docs tree ;-)

I know the doc should live somewhere else, but lets try and keep it up
to date until we move it...

>          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;
>  }



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