Re: [Xen-devel] [PATCH v2] ARM: parse separate DT properties for different commandlines

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 20 August 2013 13:09, Andre Przywara <andre.przywara@xxxxxxxxxx> wrote:
> On 07/10/2013 01:48 PM, Julien Grall wrote:
>>
>> On 3 June 2013 14:43, Andre Przywara <andre.przywara@xxxxxxxxxxx> 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.
>>> I will post the u-boot patch to their ML later.
>>>
>>> Changes from v1:
>>> - fix whitespace issues
>>
>>
>> Any news about this patch ? :)
>
>
> Sorry for the lag ;-)
>
>
>>
>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxxx>
>>> ---
>>>   xen/arch/arm/domain_build.c | 13 ++++++++++---
>>>   xen/common/device_tree.c    |  7 ++++++-
>>>   2 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index b92c64b..5809489 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,11 +170,17 @@ 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;
>>
>>
>> Here, you overwrite the previous "bootargs". This variable is set if
>> the module node contains "bootargs" property
>> (see process_multiboot_node in common/device_tree.c)
>
>
> I'd say that is intended. I think those command lines directly under /chosen
> should have the highest priority. If someone has
> /chosen/xen,dom0-bootargs, that should be used instead of a most likely
> hard-coded value under modules.

I don't  think people use bootargs in module. So we can get a rid of this code.
Ian, what do you think?

Cheers,

-- 
Julien Grall

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