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

[Julien Grall <julien.grall@linaro.org>]

On 09/17/2013 03:53 PM, Ian Campbell wrote:
> 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@linaro.org>
>> ---
>>  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.

The function only works with the device tree structure not the FDT.

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


-- 
Julien Grall