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

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

[Andre Przywara]

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

Signed-off-by: Andre Przywara <andre.przywara@calxeda.com>
---
 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;
                 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;
                 continue;
             }
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 84d704d..e22bd22 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 )
+    {
+        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL))
+            prop = fdt_get_property(fdt, node, "bootargs", NULL);
+    }
     if ( prop == NULL )
         return NULL;
 
-- 
1.7.12.1

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

[Julien Grall]

On 06/03/2013 02:43 PM, 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.

Could you add this logic in misc/arm/device-tree/booting.txt?

> 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
> 
> Signed-off-by: Andre Przywara <andre.przywara@calxeda.com>
> ---
>  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;

Could you complete the above comment to specify we are removing
xen,xen-bootargs?

Thanks,

Julien

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

[Julien Grall]

On 3 June 2013 14:43, Andre Przywara <andre.przywara@calxeda.com> 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 ? :)

> Signed-off-by: Andre Przywara <andre.przywara@calxeda.com>
> ---
>  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)

Could you either:
    1) Remove the possibility to set the command line via the custom multiboot
    2) Check if "bootargs" was not set

I think nobody uses the custom multiboot with the command line. So the
first solution seems to be the best.

--
Julien Grall

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

[Andre Przywara]

On 07/10/2013 01:48 PM, Julien Grall wrote:
> On 3 June 2013 14:43, Andre Przywara <andre.przywara@calxeda.com> 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@calxeda.com>
>> ---
>>   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 have incorporated your previous comments and will send out a new 
version ASAP.

Regards,
Andre.

>
> Could you either:
>      1) Remove the possibility to set the command line via the custom multiboot
>      2) Check if "bootargs" was not set
>
> I think nobody uses the custom multiboot with the command line. So the
> first solution seems to be the best.
>
> --
> Julien Grall
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

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

[Julien Grall]

On 20 August 2013 13:09, Andre Przywara <andre.przywara@linaro.org> wrote:
> On 07/10/2013 01:48 PM, Julien Grall wrote:
>>
>> On 3 June 2013 14:43, Andre Przywara <andre.przywara@calxeda.com> 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@calxeda.com>
>>> ---
>>>   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

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

[Ian Campbell]

On Tue, 2013-08-20 at 13:19 +0100, Julien Grall wrote:
> On 20 August 2013 13:09, Andre Przywara <andre.przywara@linaro.org> wrote:
> > On 07/10/2013 01:48 PM, Julien Grall wrote:
> >>
> >> On 3 June 2013 14:43, Andre Przywara <andre.przywara@calxeda.com> 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@calxeda.com>
> >>> ---
> >>>   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.

They are documented as existing in
docs/misc/arm/device-tree/booting.txt, so something would need to change
in either the code or the docs I think.

I don't think I mind which options take precedence so long as it is
clearly documented what is what. Although there is something nice about
having the stuff under modules be a complete consistent set of address
+cmdline I guess.

Ian.


>  So we can get a rid of this code.
> Ian, what do you think?
> 
> Cheers,
> 

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

[Julien Grall]

On 20 August 2013 14:11, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-08-20 at 13:19 +0100, Julien Grall wrote:
>> On 20 August 2013 13:09, Andre Przywara <andre.przywara@linaro.org> wrote:
>> > On 07/10/2013 01:48 PM, Julien Grall wrote:
>> >>
>> >> On 3 June 2013 14:43, Andre Przywara <andre.przywara@calxeda.com> 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@calxeda.com>
>> >>> ---
>> >>>   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.
>
> They are documented as existing in
> docs/misc/arm/device-tree/booting.txt, so something would need to change
> in either the code or the docs I think.

The wiki page only mentions xen,dom0-bootargs solution :).

BTW, Andre, can you document this patch at least in docs/misc/arm?

> I don't think I mind which options take precedence so long as it is
> clearly documented what is what. Although there is something nice about
> having the stuff under modules be a complete consistent set of address
> +cmdline I guess.

If we plan to use multiboot in the next few months, it's not a good
idea to keep 2 ways
(in fact even 3 now) to set Linux command line.

I guess people mostly follow the wiki page, so they don't you the
module@0/bootargs
solution.

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2013-08-20 at 14:32 +0100, Julien Grall wrote:
> On 20 August 2013 14:11, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2013-08-20 at 13:19 +0100, Julien Grall wrote:
> >> On 20 August 2013 13:09, Andre Przywara <andre.przywara@linaro.org> wrote:
> >> > On 07/10/2013 01:48 PM, Julien Grall wrote:
> >> >>
> >> >> On 3 June 2013 14:43, Andre Przywara <andre.przywara@calxeda.com> 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@calxeda.com>
> >> >>> ---
> >> >>>   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.
> >
> > They are documented as existing in
> > docs/misc/arm/device-tree/booting.txt, so something would need to change
> > in either the code or the docs I think.
> 
> The wiki page only mentions xen,dom0-bootargs solution :).

Do the wiki pages use the /modules stuff at all? If not then would not
expect they would use /module/N/bootargs either.

> BTW, Andre, can you document this patch at least in docs/misc/arm?

I think this is a must. In fact IIRC this was the main reason the patch
wasn't just applied.

Ian.

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

[Julien Grall]

On 20 August 2013 14:39, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-08-20 at 14:32 +0100, Julien Grall wrote:
>> On 20 August 2013 14:11, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Tue, 2013-08-20 at 13:19 +0100, Julien Grall wrote:
>> >> On 20 August 2013 13:09, Andre Przywara <andre.przywara@linaro.org> wrote:
>> >> > On 07/10/2013 01:48 PM, Julien Grall wrote:
>> >> >>
>> >> >> On 3 June 2013 14:43, Andre Przywara <andre.przywara@calxeda.com> 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@calxeda.com>
>> >> >>> ---
>> >> >>>   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.
>> >
>> > They are documented as existing in
>> > docs/misc/arm/device-tree/booting.txt, so something would need to change
>> > in either the code or the docs I think.
>>
>> The wiki page only mentions xen,dom0-bootargs solution :).
>
> Do the wiki pages use the /modules stuff at all? If not then would not
> expect they would use /module/N/bootargs either.

Nothing. The versatile express loads the Linux Image from the an
hardcoded address in the flash.

The arndale board uses /modules/... but it already set up in the provided
device tree.

That's why I think we can remove module@0/bootargs.

>> BTW, Andre, can you document this patch at least in docs/misc/arm?
>
> I think this is a must. In fact IIRC this was the main reason the patch
> wasn't just applied.

Indeed. I forgot that I have already asked few mails before :).

-- 
Julien Grall