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

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

On 09/09/2013 01:59 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> From: Andre Przywara <andre.przywara@linaro.org>
>>
>> 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 will send out a appropriate u-boot patch, which writes the content
>> of the "xen_bootargs" environment variable into the xen,xen-bootargs
>> dtb property.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Do we want/need to wait for the result of the bindings proposal Andre
> made?
>
> This is unrelated to Andre's "use more flexible node naming", isn't it?

Yes and no.

Unrelated: the code does not conflict and it adds flexibility in 
booting, allowing less capable boot loaders to start Xen.
Related: The flexible node naming solves this particular issue in a more 
generic manner.
But as I think one goal was to make Xen boot on every toaster, we may 
keep this patch (or a rebased version of it) for flexibility.

Regards,
Andre.

>> ---
>>      Changes in v2:
>>          - Add and rebase this patch to this patch series
>>
>>      Changes before the patch was added to this patch series:
>>      v1 .. v2:
>>       - fix whitespace issues
>>      v2 .. v3:
>>       - add documentation
>> ---
>>   docs/misc/arm/device-tree/booting.txt |   28 +++++++++++++++++++++++++++-
>>   xen/arch/arm/domain_build.c           |   15 +++++++++++----
>>   xen/common/device_tree.c              |    7 ++++++-
>>   3 files changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 94cd3f1..08ed775 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -1,3 +1,6 @@
>> +Dom0 kernel and ramdisk modules
>> +================================
>> +
>>   Xen is passed the dom0 kernel and initrd via a reference in the /chosen
>>   node of the device tree.
>>
>> @@ -22,4 +25,27 @@ properties:
>>
>>   - bootargs (optional)
>>
>> -	Command line associated with this module
>> +	Command line associated with this module. This is deprecated and should
>> +	be replaced by the bootargs variations described below.
>> +
>> +
>> +Command lines
>> +=============
>> +
>> +Xen also checks for properties directly under /chosen to find suitable command
>> +lines for Xen and Dom0. The logic is the following:
>> +
>> + - 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.
>> + - 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.
>> +
>> +Most of these cases is to make booting with Xen-unaware bootloaders easier.
>> +For those you would hardcode the Xen commandline in the DTB under
>> +/chosen/xen,xen-bootargs and would let the bootloader set the Dom0 command
>> +line by writing bootargs (as for native Linux).
>> +A Xen-aware bootloader would set xen,xen-bootargs for Xen, xen,dom0-bootargs
>> +for Dom0 and bootargs for native Linux.
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 1ac261e..e45e0e7 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -146,6 +146,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>       const char *bootargs = NULL;
>>       const struct dt_property *pp;
>>       int res = 0;
>> +    int had_dom0_bootargs = 0;
>>
>>       if ( early_info.modules.nr_mods >= MOD_KERNEL &&
>>            early_info.modules.module[MOD_KERNEL].cmdline[0] )
>> @@ -162,15 +163,21 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>            *
>>            * * remember xen,dom0-bootargs if we don't already have
>>            *   bootargs (from module #1, above).
>> -         * * remove bootargs and xen,dom0-bootargs.
>> +         * * remove bootargs,  xen,dom0-bootargs and xen,xen-bootargs.
>>            */
>>           if ( dt_node_path_is_equal(np, "/chosen") )
>>           {
>> -            if ( dt_property_name_is_equal(pp, "bootargs") )
>> +            if ( dt_property_name_is_equal(pp, "xen,xen-bootargs") )
>> +                continue;
>> +            if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") )
>> +            {
>> +                had_dom0_bootargs = 1;
>> +                bootargs = pp->value;
>>                   continue;
>> -            else if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") )
>> +            }
>> +            if ( dt_property_name_is_equal(pp, "bootargs") )
>>               {
>> -                if ( !bootargs )
>> +                if ( !bootargs  && !had_dom0_bootargs )
>>                       bootargs = pp->value;
>>                   continue;
>>               }
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 4bc1ce4..5620b23 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -261,7 +261,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 )
>> +    {
>> +        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL))
>> +            prop = fdt_get_property(fdt, node, "bootargs", NULL);
>> +    }
>>       if ( prop == NULL )
>>           return NULL;
>>
>
>