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

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

[Ian Campbell]

On Tue, 2013-09-17 at 14:48 +0000, patchbot@xen.org wrote:
> commit a8992d62362e0755d3a1929b059769bc3343135d
> Author:     Andre Przywara <andre.przywara@linaro.org>
> AuthorDate: Fri Sep 13 13:49:34 2013 +0100
> Commit:     Ian Campbell <ian.campbell@citrix.com>
> CommitDate: Tue Sep 17 15:29:18 2013 +0100
> 
>     ARM: parse separate DT properties for different commandlines
>     
>     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.


So this doesn't seem to be working like I expected...

I have a "bootargs" which contains my hypervisor bootargs, I have
neither xen,xen-bootargs nor xen,dom0-bootargs. dom0 args are in the
module bootargs property.

But Xen isn't seeing a command line. Is my configuration wrong or is
this supposed to work as it used to? I suppose the missing logic is to
take into account whether the dom0 bootargs are in a module?

FYI I'm seeing this on fastmodle with bootwrapper (from my xenbits arm32
branch).

> @@ -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);
> +    }

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

[Andre Przywara]

On 09/18/2013 10:47 PM, Ian Campbell wrote:
> On Tue, 2013-09-17 at 14:48 +0000, patchbot@xen.org wrote:
>> commit a8992d62362e0755d3a1929b059769bc3343135d
>> Author:     Andre Przywara <andre.przywara@linaro.org>
>> AuthorDate: Fri Sep 13 13:49:34 2013 +0100
>> Commit:     Ian Campbell <ian.campbell@citrix.com>
>> CommitDate: Tue Sep 17 15:29:18 2013 +0100
>>
>>      ARM: parse separate DT properties for different commandlines
>>
>>      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.
>
>
> So this doesn't seem to be working like I expected...
>
> I have a "bootargs" which contains my hypervisor bootargs, I have
> neither xen,xen-bootargs nor xen,dom0-bootargs. dom0 args are in the
> module bootargs property.
>
> But Xen isn't seeing a command line.

The same experience I had yesterday. Using the explicit xen,xen-bootargs 
for the time being works, though.
I am not sure if that due to that module node patch of mine, which also 
uses bootargs, though one level deeper. This may be triggered by one of 
Julien's DTB patches.

I will look into this this afternoon.

Regards,
Andre.

> Is my configuration wrong or is
> this supposed to work as it used to? I suppose the missing logic is to
> take into account whether the dom0 bootargs are in a module?
>
> FYI I'm seeing this on fastmodle with bootwrapper (from my xenbits arm32
> branch).
>
>> @@ -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);
>> +    }
>
>
>

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

[Julien Grall]

[-- Attachment #1.1: Type: text/plain, Size: 759 bytes --]

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.

-- 
Julien Grall
Sent from my phone

[-- Attachment #1.2: Type: text/html, Size: 1152 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Ian Campbell]

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!

commit ae37a08fa4776ba8ca3bd5554d7c042e8502fa95
Author: Ian Campbell <ijc@hellion.org.uk>
Date:   Fri Sep 20 23:45:34 2013 +0100

    HACK: bootargs fixup

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 27ee708..d66e392 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -252,7 +252,7 @@ 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))
+        //if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL))
             prop = fdt_get_property(fdt, node, "bootargs", NULL);
     }
     if ( prop == NULL )

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

[Ian Campbell]

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.

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 )
-- 
1.7.10.4

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

[Julien Grall]

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

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

[Ian Campbell]

On Mon, 2013-10-07 at 13:23 +0100, Julien Grall wrote:
> 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?

You mean the patch is good but the docs need updating? Sure.

> 
> > 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 )
> > 
> 
> 

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

[Julien Grall]

On 10/07/2013 01:44 PM, Ian Campbell wrote:
> On Mon, 2013-10-07 at 13:23 +0100, Julien Grall wrote:
>> 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?
> 
> You mean the patch is good but the docs need updating? Sure.

Yes.

-- 
Julien Grall