Ian Campbell
2013-Sep-18 20:47 UTC
Re: [Xen-staging] [xen staging] ARM: parse separate DT properties for different commandlines
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); > + }
Andre Przywara
2013-Sep-19 08:24 UTC
Re: [Xen-staging] [xen staging] ARM: parse separate DT properties for different commandlines
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); >> + } > > >
Julien Grall
2013-Sep-19 09:44 UTC
Re: [Xen-staging] [xen staging] ARM: parse separate DT properties for different commandlines
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2013-Oct-01 09:56 UTC
Re: [Xen-staging] [xen staging] ARM: parse separate DT properties for different commandlines
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 )
Ian Campbell
2013-Oct-01 10:12 UTC
Re: [Xen-staging] [xen staging] ARM: parse separate DT properties for different commandlines
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
Julien Grall
2013-Oct-07 12:23 UTC
Re: [Xen-staging] [xen staging] ARM: parse separate DT properties for different commandlines
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
Ian Campbell
2013-Oct-07 12:44 UTC
Re: [Xen-staging] [xen staging] ARM: parse separate DT properties for different commandlines
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 ) > > > >
Julien Grall
2013-Oct-07 13:08 UTC
Re: [Xen-staging] [xen staging] ARM: parse separate DT properties for different commandlines
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