Re: [Xen-staging] [xen staging] ARM: parse separate DT properties for different commandlines

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

On 10/01/2013 11:12 AM, Ian Campbell wrote:
> On Tue, 2013-10-01 at 10:56 +0100, Ian Campbell wrote:
>> On Thu, 2013-09-19 at 10:44 +0100, Julien Grall wrote:
>>>
>>> On 19 Sep 2013 09:29, "Andre Przywara" <andre.przywara@linaro.org>
>>> wrote:
>>>>>
>>>>>> @@ -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);
>>>>>> +    }
>>>
>>> The logic seems wrong here, we returns bootargs only if the property
>>> "xen,dom0-bootargs" exists. We should also check if the user give the
>>> dom0
>>> command line via the multiboot module.
>>
>> Anyone investigating this? I've just been using the following, which is
>> obviously bogus!
> 
> How about this:
> 8<------------------------------------
> 
> From a158dee49bb59e76c0f9103f512bb4bf9489f770 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ijc@hellion.org.uk>
> Date: Fri, 20 Sep 2013 23:45:34 +0100
> Subject: [PATCH] xen: arm: fix usage of bootargs for Xen.
> 
> The chosen node's bootargs property should be used for Xen if there is a dom0
> kernel multiboot module with a command line, not just if xen,dom0-bootargs is
> present.

>From docs/misc/arm/device-tree/booting.txt:
If no Xen specific properties are present, bootargs is for Dom0.

The current behaviour seems logic. Can you update the documentation?

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/common/device_tree.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 27ee708..fe25508 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -242,7 +242,7 @@ static int __init device_tree_for_each_node(const void *fdt,
>   */
>  const char *device_tree_bootargs(const void *fdt)
>  {
> -    int node; 
> +    int node;
>      const struct fdt_property *prop;
>  
>      node = fdt_path_offset(fdt, "/chosen");
> @@ -252,7 +252,13 @@ const char *device_tree_bootargs(const void *fdt)
>      prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
>      if ( prop == NULL )
>      {
> -        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL))
> +        struct dt_mb_module *dom0_mod = NULL;
> +
> +        if ( early_info.modules.nr_mods >= MOD_KERNEL )
> +            dom0_mod = &early_info.modules.module[MOD_KERNEL];
> +
> +        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
> +            ( dom0_mod && dom0_mod->cmdline[0] ) )
>              prop = fdt_get_property(fdt, node, "bootargs", NULL);
>      }
>      if ( prop == NULL )
> 


-- 
Julien Grall