Re: [PATCH] ARM: parse separate DT properties for different commandlines

[Andre Przywara <andre.przywara@calxeda.com>]

On 05/31/2013 04:24 PM, Ian Campbell wrote:
> On Fri, 2013-05-31 at 15:45 +0200, Andre Przywara wrote:
>> Currently we use the chosen/bootargs property as the Xen commandline
>> and rely on xen,dom0-bootargs for Dom0. However this brings issues
>> with bootloaders, which usually build bootargs by bootscripts for a
>> Linux kernel - and not for the entirely different Xen hypervisor.
>> Introduce a new possible device tree property "xen,xen-bootargs"
>> explicitly for the Xen hypervisor and make the selection of which to
>> use more fine grained:
>> - If xen,xen-bootargs is present, it will be used for Xen.
>> - If xen,dom0-bootargs is present, it will be used for Dom0.
>> - If xen,xen-bootargs is _not_ present, but xen,dom0-bootargs is,
>>    bootargs will be used for Xen. Like the current situation.
>> - If no Xen specific properties are present, bootargs is for Dom0.
>> - If xen,xen-bootargs is present, but xen,dom0-bootargs is missing,
>>    bootargs will be used for Dom0.
>>
>> The aim is to allow common bootscripts to boot both Xen and native
>> Linux with the same device tree blob. If needed, one could hard-code
>> the Xen commandline into the DTB, leaving bootargs for Dom0 to be set
>> by the (non Xen-aware) bootloader.
>> I also have a simple patch for u-boot to transfer the content of the
>> "xen_bootargs" environment variable into the xen,xen-bootargs dtb
>> property.
>> If you like this approach, I will send the u-boot patch to their ML.
>
> I think I've traced through all 8 possibilities and the results seem to
> make sense...

Sorry about the coding style issues, I am doing u-boot/Linux/Xen context 
switching currently. Some of the braces are leftovers from debugging (to 
check all 8 possibilities ;-)
Will send a fixed version.

Thanks for the review,
Andre.

>> Signed-off-by: Andre Przywara <andre.przywara@calxeda.com>
>> ---
>>   xen/arch/arm/domain_build.c | 14 +++++++++++---
>>   xen/common/device_tree.c    |  7 ++++++-
>>   2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index b92c64b..952adb3 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -139,6 +139,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>                               u32 address_cells, u32 size_cells)
>>   {
>>       const char *bootargs = NULL;
>> +    int had_dom0_bootargs = 0;
>>       int prop;
>>
>>       if ( early_info.modules.nr_mods >= 1 &&
>> @@ -169,12 +170,19 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>            */
>>           if ( device_tree_node_matches(fdt, node, "chosen") )
>>           {
>> -            if ( strcmp(prop_name, "bootargs") == 0 )
>> +            if ( strcmp(prop_name, "xen,xen-bootargs") == 0 )
>> +                continue;
>> +            if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
>> +            {
>> +                had_dom0_bootargs = 1;
>> +                bootargs = prop_data;
>>                   continue;
>> -            else if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
>> +            }
>> +            if ( strcmp(prop_name, "bootargs") == 0 )
>>               {
>> -                if ( !bootargs )
>> +                if ( !bootargs  && !had_dom0_bootargs ) {
>>                       bootargs = prop_data;
>> +                }
>
> Xen coding style doesn't require {} around single lines.
>
>>                   continue;
>>               }
>>           }
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 84d704d..c25e6d4 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -325,7 +325,12 @@ const char *device_tree_bootargs(const void *fdt)
>>       if ( node < 0 )
>>           return NULL;
>>
>> -    prop = fdt_get_property(fdt, node, "bootargs", NULL);
>> +    prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
>> +    if ( prop == NULL ) {
>
> Coding style is { on the next line
>
>> +    	if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL)) {
>> +            prop = fdt_get_property(fdt, node, "bootargs", NULL);
>> +        }
>
> There's a hard tab here, but you don't need the {} anyway. To avoid
> ambiguity I would stick with the outer one though.
>
>> +    }
>>       if ( prop == NULL )
>>           return NULL;
>>
>
>