[PATCH 0/2] RC: rework U-boot arguments handling

[PATCH 0/2] RC: rework U-boot arguments handling

[Eugeniy Paltsev]

Reworking U-boot args handling and enable uboot support
unconditionally.

Changes RFC->v1:
 * Don't add new ABI contract between kernel and uboot
 * Eliminate CONFIG_ARC_UBOOT_SUPPORT Kconfig option and
   enable uboot support unconditionally
 * Skip invalid U-boot args instead of panic
 * Check existing U-boot magic value
 * Improve uboot_arg validating
 * Minor code changes

Eugeniy Paltsev (2):
  ARC: U-boot: check arguments paranoidly
  ARC: enable uboot support unconditionally

 arch/arc/Kconfig                        | 12 -----
 arch/arc/configs/nps_defconfig          |  1 -
 arch/arc/configs/vdk_hs38_defconfig     |  1 -
 arch/arc/configs/vdk_hs38_smp_defconfig |  2 -
 arch/arc/kernel/head.S                  |  7 ++-
 arch/arc/kernel/setup.c                 | 96 +++++++++++++++++++++++----------
 6 files changed, 70 insertions(+), 49 deletions(-)

-- 
2.14.5

[PATCH 1/2] ARC: U-boot: check arguments paranoidly

[Eugeniy Paltsev]

Handle U-boot arguments paranoidly:
 * don't allow to pass unknown tag.
 * try to use external device tree blob only if corresponding tag
   (TAG_DTB) is set.
 * check that magic number is correct.
 * don't check uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.

NOTE:
If U-boot args are invalid we skip them and try to use embedded device
tree blob. We can't panic on invalid U-boot args as we really pass
invalid args due to bug in U-boot code.
This happens if we don't provide external DTB to U-boot and
don't set 'bootargs' U-boot environment variable (which is default
case at least for HSDK board) In that case we will pass
{r0 = 1 (bootargs in r2); r1 = 0; r2 = 0;} to linux which is invalid.

NOTE:
We can safely check U-boot magic value (0x0) in linux passed via
r1 register as U-boot pass it from the beginning.

While I'm at it refactor U-boot arguments handling code.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 arch/arc/kernel/head.S  |  5 +--
 arch/arc/kernel/setup.c | 92 +++++++++++++++++++++++++++++++++++--------------
 2 files changed, 69 insertions(+), 28 deletions(-)

diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
index 8b90d25a15cc..fccea361e896 100644
--- a/arch/arc/kernel/head.S
+++ b/arch/arc/kernel/head.S
@@ -93,10 +93,11 @@ ENTRY(stext)
 #ifdef CONFIG_ARC_UBOOT_SUPPORT
 	; Uboot - kernel ABI
 	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
-	;    r1 = magic number (board identity, unused as of now
+	;    r1 = magic number (always zero as of now)
 	;    r2 = pointer to uboot provided cmdline or external DTB in mem
-	; These are handled later in setup_arch()
+	; These are handled later in handle_uboot_args()
 	st	r0, [@uboot_tag]
+	st      r1, [@uboot_magic]
 	st	r2, [@uboot_arg]
 #endif
 
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index feb90093e6b1..84d394a37e79 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -36,7 +36,8 @@ unsigned int intr_to_DE_cnt;
 
 /* Part of U-boot ABI: see head.S */
 int __initdata uboot_tag;
-char __initdata *uboot_arg;
+int __initdata uboot_magic;
+unsigned int __initdata uboot_arg;
 
 const struct machine_desc *machine_desc;
 
@@ -462,43 +463,82 @@ void setup_processor(void)
 	arc_chk_core_config();
 }
 
-static inline int is_kernel(unsigned long addr)
+static inline bool uboot_arg_invalid(unsigned int addr)
 {
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
-		return 1;
-	return 0;
+	/*
+	 * Check that it is a untranslated address (although MMU is not enabled
+	 * yet, it being a high address ensures this is not by fluke)
+	 */
+	if (addr < PAGE_OFFSET)
+		return true;
+
+	/* Check that address doesn't clobber resident kernel image */
+	return addr >= (unsigned int)_stext && addr <= (unsigned int)_end;
 }
 
-void __init setup_arch(char **cmdline_p)
+#define IGNORE_ARGS		"Ignore U-boot args: "
+
+/* uboot_{tag, magic} values for U-boot - kernel ABI revision 0; see head.S */
+#define UBOOT_TAG_NONE		0
+#define UBOOT_TAG_CMDLINE	1
+#define UBOOT_TAG_DTB		2
+/* We always pass 0 as magic from U-boot */
+#define UBOOT_MAGIC_VAL		0
+
+void __init handle_uboot_args(void)
 {
+	bool use_embedded_dtb = true;
+	bool append_cmdline = false;
+
 #ifdef CONFIG_ARC_UBOOT_SUPPORT
-	/* make sure that uboot passed pointer to cmdline/dtb is valid */
-	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
-		panic("Invalid uboot arg\n");
+	/* check that we know this tag */
+	if (uboot_tag != UBOOT_TAG_NONE &&
+	    uboot_tag != UBOOT_TAG_CMDLINE &&
+	    uboot_tag != UBOOT_TAG_DTB) {
+		pr_warn(IGNORE_ARGS "invalid uboot tag: '%08x'\n", uboot_tag);
+		goto ignore_uboot_args;
+	}
+
+	if (uboot_magic != UBOOT_MAGIC_VAL) {
+		pr_warn(IGNORE_ARGS "non zero uboot magic\n");
+		goto ignore_uboot_args;
+	}
+
+	if (uboot_tag != UBOOT_TAG_NONE && uboot_arg_invalid(uboot_arg)) {
+		pr_warn(IGNORE_ARGS "invalid uboot arg: '%08x'\n", uboot_arg);
+		goto ignore_uboot_args;
+	}
+
+	/* see if U-boot passed an external Device Tree blob */
+	if (uboot_tag == UBOOT_TAG_DTB) {
+		machine_desc = setup_machine_fdt((void *)uboot_arg);
+
+		/* external Device Tree blob is invalid - use embedded one */
+		use_embedded_dtb = !machine_desc;
+	}
+
+	if (uboot_tag == UBOOT_TAG_CMDLINE)
+		append_cmdline = true;
 
-	/* See if u-boot passed an external Device Tree blob */
-	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
-	if (!machine_desc)
+ignore_uboot_args:
 #endif
-	{
-		/* No, so try the embedded one */
+
+	if (use_embedded_dtb) {
 		machine_desc = setup_machine_fdt(__dtb_start);
 		if (!machine_desc)
 			panic("Embedded DT invalid\n");
+	}
 
-		/*
-		 * If we are here, it is established that @uboot_arg didn't
-		 * point to DT blob. Instead if u-boot says it is cmdline,
-		 * append to embedded DT cmdline.
-		 * setup_machine_fdt() would have populated @boot_command_line
-		 */
-		if (uboot_tag == 1) {
-			/* Ensure a whitespace between the 2 cmdlines */
-			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
-			strlcat(boot_command_line, uboot_arg,
-				COMMAND_LINE_SIZE);
-		}
+	if (append_cmdline) {
+		/* Ensure a whitespace between the 2 cmdlines */
+		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
+		strlcat(boot_command_line, (char *)uboot_arg, COMMAND_LINE_SIZE);
 	}
+}
+
+void __init setup_arch(char **cmdline_p)
+{
+	handle_uboot_args();
 
 	/* Save unparsed command line copy for /proc/cmdline */
 	*cmdline_p = boot_command_line;
-- 
2.14.5

[PATCH 2/2] ARC: enable uboot support unconditionally

[Eugeniy Paltsev]

After reworking U-boot args handling code and adding paranoid
arguments check we can eliminate CONFIG_ARC_UBOOT_SUPPORT and
enable uboot support unconditionally.

For JTAG case we can assume that core registers will come up
reset value of 0 or in worst case we rely on user passing
'-on=clear_regs' to Metaware debugger.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 arch/arc/Kconfig                        | 12 ------------
 arch/arc/configs/nps_defconfig          |  1 -
 arch/arc/configs/vdk_hs38_defconfig     |  1 -
 arch/arc/configs/vdk_hs38_smp_defconfig |  2 --
 arch/arc/kernel/head.S                  |  2 --
 arch/arc/kernel/setup.c                 |  2 --
 6 files changed, 20 deletions(-)

diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index 376366a7db81..f9534417b201 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -191,7 +191,6 @@ config NR_CPUS
 
 config ARC_SMP_HALT_ON_RESET
 	bool "Enable Halt-on-reset boot mode"
-	default y if ARC_UBOOT_SUPPORT
 	help
 	  In SMP configuration cores can be configured as Halt-on-reset
 	  or they could all start at same time. For Halt-on-reset, non
@@ -515,17 +514,6 @@ config ARC_DBG_TLB_PARANOIA
 
 endif
 
-config ARC_UBOOT_SUPPORT
-	bool "Support uboot arg Handling"
-	help
-	  ARC Linux by default checks for uboot provided args as pointers to
-	  external cmdline or DTB. This however breaks in absence of uboot,
-	  when booting from Metaware debugger directly, as the registers are
-	  not zeroed out on reset by mdb and/or ARCv2 based cores. The bogus
-	  registers look like uboot args to kernel which then chokes.
-	  So only enable the uboot arg checking/processing if users are sure
-	  of uboot being in play.
-
 config ARC_BUILTIN_DTB_NAME
 	string "Built in DTB"
 	help
diff --git a/arch/arc/configs/nps_defconfig b/arch/arc/configs/nps_defconfig
index 6e84060e7c90..621f59407d76 100644
--- a/arch/arc/configs/nps_defconfig
+++ b/arch/arc/configs/nps_defconfig
@@ -31,7 +31,6 @@ CONFIG_ARC_CACHE_LINE_SHIFT=5
 # CONFIG_ARC_HAS_LLSC is not set
 CONFIG_ARC_KVADDR_SIZE=402
 CONFIG_ARC_EMUL_UNALIGNED=y
-CONFIG_ARC_UBOOT_SUPPORT=y
 CONFIG_PREEMPT=y
 CONFIG_NET=y
 CONFIG_UNIX=y
diff --git a/arch/arc/configs/vdk_hs38_defconfig b/arch/arc/configs/vdk_hs38_defconfig
index 1e59a2e9c602..e447ace6fa1c 100644
--- a/arch/arc/configs/vdk_hs38_defconfig
+++ b/arch/arc/configs/vdk_hs38_defconfig
@@ -13,7 +13,6 @@ CONFIG_PARTITION_ADVANCED=y
 CONFIG_ARC_PLAT_AXS10X=y
 CONFIG_AXS103=y
 CONFIG_ISA_ARCV2=y
-CONFIG_ARC_UBOOT_SUPPORT=y
 CONFIG_ARC_BUILTIN_DTB_NAME="vdk_hs38"
 CONFIG_PREEMPT=y
 CONFIG_NET=y
diff --git a/arch/arc/configs/vdk_hs38_smp_defconfig b/arch/arc/configs/vdk_hs38_smp_defconfig
index b5c3f6c54b03..c82cdb10aaf4 100644
--- a/arch/arc/configs/vdk_hs38_smp_defconfig
+++ b/arch/arc/configs/vdk_hs38_smp_defconfig
@@ -15,8 +15,6 @@ CONFIG_AXS103=y
 CONFIG_ISA_ARCV2=y
 CONFIG_SMP=y
 # CONFIG_ARC_TIMERS_64BIT is not set
-# CONFIG_ARC_SMP_HALT_ON_RESET is not set
-CONFIG_ARC_UBOOT_SUPPORT=y
 CONFIG_ARC_BUILTIN_DTB_NAME="vdk_hs38_smp"
 CONFIG_PREEMPT=y
 CONFIG_NET=y
diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
index fccea361e896..4b0deaff001c 100644
--- a/arch/arc/kernel/head.S
+++ b/arch/arc/kernel/head.S
@@ -90,7 +90,6 @@ ENTRY(stext)
 	st.ab   0, [r5, 4]
 1:
 
-#ifdef CONFIG_ARC_UBOOT_SUPPORT
 	; Uboot - kernel ABI
 	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
 	;    r1 = magic number (always zero as of now)
@@ -99,7 +98,6 @@ ENTRY(stext)
 	st	r0, [@uboot_tag]
 	st      r1, [@uboot_magic]
 	st	r2, [@uboot_arg]
-#endif
 
 	; setup "current" tsk and optionally cache it in dedicated r25
 	mov	r9, @init_task
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 84d394a37e79..fff946b0ab4f 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -490,7 +490,6 @@ void __init handle_uboot_args(void)
 	bool use_embedded_dtb = true;
 	bool append_cmdline = false;
 
-#ifdef CONFIG_ARC_UBOOT_SUPPORT
 	/* check that we know this tag */
 	if (uboot_tag != UBOOT_TAG_NONE &&
 	    uboot_tag != UBOOT_TAG_CMDLINE &&
@@ -521,7 +520,6 @@ void __init handle_uboot_args(void)
 		append_cmdline = true;
 
 ignore_uboot_args:
-#endif
 
 	if (use_embedded_dtb) {
 		machine_desc = setup_machine_fdt(__dtb_start);
-- 
2.14.5

Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly

[LABBE Corentin]

On Tue, Feb 12, 2019 at 06:39:31PM +0300, Eugeniy Paltsev wrote:
> Handle U-boot arguments paranoidly:
>  * don't allow to pass unknown tag.
>  * try to use external device tree blob only if corresponding tag
>    (TAG_DTB) is set.
>  * check that magic number is correct.
>  * don't check uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
> 
> NOTE:
> If U-boot args are invalid we skip them and try to use embedded device
> tree blob. We can't panic on invalid U-boot args as we really pass
> invalid args due to bug in U-boot code.
> This happens if we don't provide external DTB to U-boot and
> don't set 'bootargs' U-boot environment variable (which is default
> case at least for HSDK board) In that case we will pass
> {r0 = 1 (bootargs in r2); r1 = 0; r2 = 0;} to linux which is invalid.
> 
> NOTE:
> We can safely check U-boot magic value (0x0) in linux passed via
> r1 register as U-boot pass it from the beginning.
> 
> While I'm at it refactor U-boot arguments handling code.
> 

Hello

I have tried to test this serie, but this patch does not apply anymore on current next tree.
It conflicts with "ARC: boot: robustify u-boot arg referencing".

Regards

Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly

[Vineet Gupta]

On 2/12/19 8:39 AM, LABBE Corentin wrote:
>> While I'm at it refactor U-boot arguments handling code.
>>
> Hello
>
> I have tried to test this serie, but this patch does not apply anymore on current next tree.
> It conflicts with "ARC: boot: robustify u-boot arg referencing".

I was carrying that patch in the interim - now dropped and pushed.

-Vineet

Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly

[Vineet Gupta]

On 2/12/19 7:39 AM, Eugeniy Paltsev wrote:
> Handle U-boot arguments paranoidly:
>  * don't allow to pass unknown tag.
>  * try to use external device tree blob only if corresponding tag
>    (TAG_DTB) is set.
>  * check that magic number is correct.
>  * don't check uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
>
> NOTE:
> If U-boot args are invalid we skip them and try to use embedded device
> tree blob. We can't panic on invalid U-boot args as we really pass
> invalid args due to bug in U-boot code.
> This happens if we don't provide external DTB to U-boot and
> don't set 'bootargs' U-boot environment variable (which is default
> case at least for HSDK board) In that case we will pass
> {r0 = 1 (bootargs in r2); r1 = 0; r2 = 0;} to linux which is invalid.
>
> NOTE:
> We can safely check U-boot magic value (0x0) in linux passed via
> r1 register as U-boot pass it from the beginning.
>
> While I'm at it refactor U-boot arguments handling code.
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  arch/arc/kernel/head.S  |  5 +--
>  arch/arc/kernel/setup.c | 92 +++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 69 insertions(+), 28 deletions(-)
>
> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> index 8b90d25a15cc..fccea361e896 100644
> --- a/arch/arc/kernel/head.S
> +++ b/arch/arc/kernel/head.S
> @@ -93,10 +93,11 @@ ENTRY(stext)
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
>  	; Uboot - kernel ABI
>  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
> -	;    r1 = magic number (board identity, unused as of now
> +	;    r1 = magic number (always zero as of now)

This is technically changing the ABI - I think we don't need to enforce this -
keep ignoring this

>  	;    r2 = pointer to uboot provided cmdline or external DTB in mem
> -	; These are handled later in setup_arch()
> +	; These are handled later in handle_uboot_args()
>  	st	r0, [@uboot_tag]
> +	st      r1, [@uboot_magic]
>  	st	r2, [@uboot_arg]
>  #endif
>  
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index feb90093e6b1..84d394a37e79 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -36,7 +36,8 @@ unsigned int intr_to_DE_cnt;
>  
>  /* Part of U-boot ABI: see head.S */
>  int __initdata uboot_tag;
> -char __initdata *uboot_arg;
> +int __initdata uboot_magic;
> +unsigned int __initdata uboot_arg;
>  
>  const struct machine_desc *machine_desc;
>  
> @@ -462,43 +463,82 @@ void setup_processor(void)
>  	arc_chk_core_config();
>  }
>  
> -static inline int is_kernel(unsigned long addr)
> +static inline bool uboot_arg_invalid(unsigned int addr)
>  {
> -	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
> -		return 1;
> -	return 0;
> +	/*
> +	 * Check that it is a untranslated address (although MMU is not enabled
> +	 * yet, it being a high address ensures this is not by fluke)
> +	 */
> +	if (addr < PAGE_OFFSET)
> +		return true;
> +
> +	/* Check that address doesn't clobber resident kernel image */
> +	return addr >= (unsigned int)_stext && addr <= (unsigned int)_end;
>  }
>  
> -void __init setup_arch(char **cmdline_p)
> +#define IGNORE_ARGS		"Ignore U-boot args: "
> +
> +/* uboot_{tag, magic} values for U-boot - kernel ABI revision 0; see head.S */
> +#define UBOOT_TAG_NONE		0
> +#define UBOOT_TAG_CMDLINE	1
> +#define UBOOT_TAG_DTB		2
> +/* We always pass 0 as magic from U-boot */
> +#define UBOOT_MAGIC_VAL		0
> +
> +void __init handle_uboot_args(void)
>  {
> +	bool use_embedded_dtb = true;
> +	bool append_cmdline = false;
> +
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
> -	/* make sure that uboot passed pointer to cmdline/dtb is valid */
> -	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
> -		panic("Invalid uboot arg\n");
> +	/* check that we know this tag */
> +	if (uboot_tag != UBOOT_TAG_NONE &&
> +	    uboot_tag != UBOOT_TAG_CMDLINE &&
> +	    uboot_tag != UBOOT_TAG_DTB) {
> +		pr_warn(IGNORE_ARGS "invalid uboot tag: '%08x'\n", uboot_tag);
> +		goto ignore_uboot_args;
> +	}
> +
> +	if (uboot_magic != UBOOT_MAGIC_VAL) {
> +		pr_warn(IGNORE_ARGS "non zero uboot magic\n");
> +		goto ignore_uboot_args;
> +	}

Not needed per above.

> +
> +	if (uboot_tag != UBOOT_TAG_NONE && uboot_arg_invalid(uboot_arg)) {
> +		pr_warn(IGNORE_ARGS "invalid uboot arg: '%08x'\n", uboot_arg);
> +		goto ignore_uboot_args;
> +	}
> +
> +	/* see if U-boot passed an external Device Tree blob */
> +	if (uboot_tag == UBOOT_TAG_DTB) {
> +		machine_desc = setup_machine_fdt((void *)uboot_arg);
> +
> +		/* external Device Tree blob is invalid - use embedded one */
> +		use_embedded_dtb = !machine_desc;
> +	}
> +
> +	if (uboot_tag == UBOOT_TAG_CMDLINE)
> +		append_cmdline = true;
>  
> -	/* See if u-boot passed an external Device Tree blob */
> -	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
> -	if (!machine_desc)
> +ignore_uboot_args:
>  #endif
> -	{
> -		/* No, so try the embedded one */
> +
> +	if (use_embedded_dtb) {
>  		machine_desc = setup_machine_fdt(__dtb_start);
>  		if (!machine_desc)
>  			panic("Embedded DT invalid\n");
> +	}
>  
> -		/*
> -		 * If we are here, it is established that @uboot_arg didn't
> -		 * point to DT blob. Instead if u-boot says it is cmdline,
> -		 * append to embedded DT cmdline.
> -		 * setup_machine_fdt() would have populated @boot_command_line
> -		 */

Don't drop this comment, specially the last line. If was tempted to move the cmd
line processing before but this saved me since we rely on setup_machine_fdt()
being called aprioiri.
> -		if (uboot_tag == 1) {
> -			/* Ensure a whitespace between the 2 cmdlines */
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -			strlcat(boot_command_line, uboot_arg,
> -				COMMAND_LINE_SIZE);
> -		}
> +	if (append_cmdline) {
> +		/* Ensure a whitespace between the 2 cmdlines */
> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(boot_command_line, (char *)uboot_arg, COMMAND_LINE_SIZE);
>  	}
> +}
> +
> +void __init setup_arch(char **cmdline_p)
> +{
> +	handle_uboot_args();
>  
>  	/* Save unparsed command line copy for /proc/cmdline */
>  	*cmdline_p = boot_command_line;

Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly

[Eugeniy Paltsev]

On Tue, 2019-02-12 at 16:45 +0000, Vineet Gupta wrote:
> On 2/12/19 7:39 AM, Eugeniy Paltsev wrote:
> > Handle U-boot arguments paranoidly:
> >  * don't allow to pass unknown tag.
> >  * try to use external device tree blob only if corresponding tag
> >    (TAG_DTB) is set.
> >  * check that magic number is correct.
> >  * don't check uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
> > 
> > NOTE:
> > If U-boot args are invalid we skip them and try to use embedded device
> > tree blob. We can't panic on invalid U-boot args as we really pass
> > invalid args due to bug in U-boot code.
> > This happens if we don't provide external DTB to U-boot and
> > don't set 'bootargs' U-boot environment variable (which is default
> > case at least for HSDK board) In that case we will pass
> > {r0 = 1 (bootargs in r2); r1 = 0; r2 = 0;} to linux which is invalid.
> > 
> > NOTE:
> > We can safely check U-boot magic value (0x0) in linux passed via
> > r1 register as U-boot pass it from the beginning.
> > 
> > While I'm at it refactor U-boot arguments handling code.
> > 
> > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> > ---
> >  arch/arc/kernel/head.S  |  5 +--
> >  arch/arc/kernel/setup.c | 92 +++++++++++++++++++++++++++++++++++--------------
> >  2 files changed, 69 insertions(+), 28 deletions(-)
> > 
> > diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> > index 8b90d25a15cc..fccea361e896 100644
> > --- a/arch/arc/kernel/head.S
> > +++ b/arch/arc/kernel/head.S
> > @@ -93,10 +93,11 @@ ENTRY(stext)
> >  #ifdef CONFIG_ARC_UBOOT_SUPPORT
> >  	; Uboot - kernel ABI
> >  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
> > -	;    r1 = magic number (board identity, unused as of now
> > +	;    r1 = magic number (always zero as of now)
> 
> This is technically changing the ABI - I think we don't need to enforce this -
> keep ignoring this

I think it's better to add this check:
 * This check doesn't break backward compatibility. ARC U-boot pass zero to r1
   from the beginnings, I specially checked this. So we doesn't change ABI,
   we only document it ;) 
 * By adding this check we can cheap and easily minimize problems in JTAG case.

> > +
> > +	if (use_embedded_dtb) {
> >  		machine_desc = setup_machine_fdt(__dtb_start);
> >  		if (!machine_desc)
> >  			panic("Embedded DT invalid\n");
> > +	}
> >  
> > -		/*
> > -		 * If we are here, it is established that @uboot_arg didn't
> > -		 * point to DT blob. Instead if u-boot says it is cmdline,
> > -		 * append to embedded DT cmdline.
> > -		 * setup_machine_fdt() would have populated @boot_command_line
> > -		 */
> 
> Don't drop this comment, specially the last line. If was tempted to move the cmd
> line processing before but this saved me since we rely on setup_machine_fdt()
> being called aprioiri.

Ok, will fix in v2

> > -		if (uboot_tag == 1) {
> > -			/* Ensure a whitespace between the 2 cmdlines */
> > -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> > -			strlcat(boot_command_line, uboot_arg,
> > -				COMMAND_LINE_SIZE);
> > -		}
> > +	if (append_cmdline) {
> > +		/* Ensure a whitespace between the 2 cmdlines */
> > +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> > +		strlcat(boot_command_line, (char *)uboot_arg, COMMAND_LINE_SIZE);
> >  	}
> > +}
> > +
> > +void __init setup_arch(char **cmdline_p)
> > +{
> > +	handle_uboot_args();
> >  
> >  	/* Save unparsed command line copy for /proc/cmdline */
> >  	*cmdline_p = boot_command_line;
> 
> 
-- 
 Eugeniy Paltsev

Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly

[Vineet Gupta]

On 2/12/19 9:25 AM, Eugeniy Paltsev wrote:
>> This is technically changing the ABI - I think we don't need to enforce this -
>> keep ignoring this
> I think it's better to add this check:
>  * This check doesn't break backward compatibility. ARC U-boot pass zero to r1
>    from the beginnings, I specially checked this. So we doesn't change ABI,
>    we only document it ;) 

OK good.

>  * By adding this check we can cheap and easily minimize problems in JTAG case.

Prior to your patch this register was irrelevant - it didn't matter for jtag or
uboot cause what its value was since it was not being checked at all. Now you
enforce this be 0. Simple reasoning tells me it will likely cause problems, if
any, but won't reduce them at all. So I'd insist we keep ignoring it even if uboot
was zeroing it out. Atleast this leaves the door open any future changes.