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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

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 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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