From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH] ARM: parse separate DT properties for different commandlines Date: Fri, 31 May 2013 19:55:46 +0200 Message-ID: <51A8E422.8030109@calxeda.com> References: <1370007940-16555-1-git-send-email-andre.przywara@calxeda.com> <1370010243.5199.175.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1370010243.5199.175.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xen.org" , "julien.grall@linaro.org" , "stefano.stabellini@eu.citrix.com" List-Id: xen-devel@lists.xenproject.org On 05/31/2013 04:24 PM, Ian Campbell wrote: > On Fri, 2013-05-31 at 15:45 +0200, 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. >> >> 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. >> If you like this approach, I will send the u-boot patch to their ML. > > I think I've traced through all 8 possibilities and the results seem to > make sense... Sorry about the coding style issues, I am doing u-boot/Linux/Xen context switching currently. Some of the braces are leftovers from debugging (to check all 8 possibilities ;-) Will send a fixed version. Thanks for the review, Andre. >> Signed-off-by: Andre Przywara >> --- >> xen/arch/arm/domain_build.c | 14 +++++++++++--- >> xen/common/device_tree.c | 7 ++++++- >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index b92c64b..952adb3 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,12 +170,19 @@ 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; >> + } > > Xen coding style doesn't require {} around single lines. > >> continue; >> } >> } >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >> index 84d704d..c25e6d4 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 ) { > > Coding style is { on the next line > >> + if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL)) { >> + prop = fdt_get_property(fdt, node, "bootargs", NULL); >> + } > > There's a hard tab here, but you don't need the {} anyway. To avoid > ambiguity I would stick with the outer one though. > >> + } >> if ( prop == NULL ) >> return NULL; >> > >