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

[Andre Przywara <andre.przywara@linaro.org>]

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.