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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 20 August 2013 14:39, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Tue, 2013-08-20 at 14:32 +0100, Julien Grall wrote:
>> On 20 August 2013 14:11, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
>> > On Tue, 2013-08-20 at 13:19 +0100, Julien Grall wrote:
>> >> On 20 August 2013 13:09, Andre Przywara <andre.przywara@xxxxxxxxxx> wrote:
>> >> > 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 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

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