arm64: Support CMDLINE_EXTEND

  Support extending the bootloader provided command line for arm64
targets. This support is already present via generic DT/EFI code the
only thing required is for the architecture to make it selectable.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 arch/arm64/Kconfig | 6 ++++++
 1 file changed, 6 insertions(+)
  

Hello Chris,

On 3/21/23 02:44, Chris Packham wrote:
> Support extending the bootloader provided command line for arm64
> targets. This support is already present via generic DT/EFI code the
> only thing required is for the architecture to make it selectable.

Does this config really depend on given platform's active support or
it is just matter of selecting this for interested platforms ? Could
this config definition be unified in a single place i.e arch/Kconfig
and be selected (unconditionally or conditionally) on all subscribing
platforms. There seems to be a redundancy in defining the exact same
config the same way, on multiple platforms.

$git grep "config CMDLINE_EXTEND"

arch/arm/Kconfig:config CMDLINE_EXTEND
arch/loongarch/Kconfig:config CMDLINE_EXTEND
arch/powerpc/Kconfig:config CMDLINE_EXTEND
arch/riscv/Kconfig:config CMDLINE_EXTEND
arch/sh/Kconfig:config CMDLINE_EXTEND   

I guess this redundancy should be removed as a pre-requisite, before
enabling it on arm64 as proposed here, which in itself seems alright.

> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  arch/arm64/Kconfig | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1023e896d46b..3c837b085f21 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -2228,6 +2228,12 @@ config CMDLINE_FROM_BOOTLOADER
>  	  the boot loader doesn't provide any, the default kernel command
>  	  string provided in CMDLINE will be used.
>  
> +config CMDLINE_EXTEND
> +	bool "Extend bootloader kernel arguments"
> +	help
> +	  The command-line arguments provided by the boot loader will be
> +	  appended to the default kernel command string.
> +
>  config CMDLINE_FORCE
>  	bool "Always use the default kernel command string"
>  	help
  

Hi,

On 21/03/23 16:54, Anshuman Khandual wrote:
> Hello Chris,
>
> On 3/21/23 02:44, Chris Packham wrote:
>> Support extending the bootloader provided command line for arm64
>> targets. This support is already present via generic DT/EFI code the
>> only thing required is for the architecture to make it selectable.
> Does this config really depend on given platform's active support or
> it is just matter of selecting this for interested platforms ?
Most modern platforms using DT or UEFI should be able to use the common 
code. There are some older ones (arm and powerpc) that have other/legacy 
methods of passing the kernel command line so retaining support for that 
while at the same time providing support for the generic methods as well 
could get tricky. It looks as though it won't be too bad as the code 
seems to use the same Kconfig options.
> Could
> this config definition be unified in a single place i.e arch/Kconfig
> and be selected (unconditionally or conditionally) on all subscribing
> platforms. There seems to be a redundancy in defining the exact same
> config the same way, on multiple platforms.
>
> $git grep "config CMDLINE_EXTEND"
>
> arch/arm/Kconfig:config CMDLINE_EXTEND
> arch/loongarch/Kconfig:config CMDLINE_EXTEND
> arch/powerpc/Kconfig:config CMDLINE_EXTEND
> arch/riscv/Kconfig:config CMDLINE_EXTEND
> arch/sh/Kconfig:config CMDLINE_EXTEND

Same applies to CMDLINE_FROM_BOOTLOADER/CMDLINE_FORCE. Although sh uses 
CMDLINE_OVERWRITE instead of CMDLINE_FORCE. I guess it'd be possible to 
move it to some common place and have the arches source it like they do 
for things like power management.

> I guess this redundancy should be removed as a pre-requisite, before
> enabling it on arm64 as proposed here, which in itself seems alright.
I was kind of hoping it wouldn't be a pre-requisite mainly because it 
turns a quick patch I can actually test on hardware I have in front of 
me to something that I can't test on half the platforms that are 
affected. I also haven't had to co-ordinate a change across 6 maintainer 
trees before. But I guess I can give it a try if that's the only way of 
getting it in.
>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>   arch/arm64/Kconfig | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 1023e896d46b..3c837b085f21 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -2228,6 +2228,12 @@ config CMDLINE_FROM_BOOTLOADER
>>   	  the boot loader doesn't provide any, the default kernel command
>>   	  string provided in CMDLINE will be used.
>>   
>> +config CMDLINE_EXTEND
>> +	bool "Extend bootloader kernel arguments"
>> +	help
>> +	  The command-line arguments provided by the boot loader will be
>> +	  appended to the default kernel command string.
>> +
>>   config CMDLINE_FORCE
>>   	bool "Always use the default kernel command string"
>>   	help
  

On 3/21/23 02:44, Chris Packham wrote:
> Support extending the bootloader provided command line for arm64
> targets. This support is already present via generic DT/EFI code the
> only thing required is for the architecture to make it selectable.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  arch/arm64/Kconfig | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1023e896d46b..3c837b085f21 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -2228,6 +2228,12 @@ config CMDLINE_FROM_BOOTLOADER
>  	  the boot loader doesn't provide any, the default kernel command
>  	  string provided in CMDLINE will be used.
>  
> +config CMDLINE_EXTEND
> +	bool "Extend bootloader kernel arguments"
> +	help
> +	  The command-line arguments provided by the boot loader will be
> +	  appended to the default kernel command string.
> +
>  config CMDLINE_FORCE
>  	bool "Always use the default kernel command string"
>  	help

LGTM, FWIW

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
  

On Tue, Mar 21, 2023 at 10:14:51AM +1300, Chris Packham wrote:
> Support extending the bootloader provided command line for arm64
> targets. This support is already present via generic DT/EFI code the
> only thing required is for the architecture to make it selectable.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

We deliberately dropped support for CMDLINE_EXTEND in commit:

  cae118b6acc3 ("arm64: Drop support for CMDLINE_EXTEND")

... which was mentioned the last time somone tried to re-add it:

  https://lore.kernel.org/linux-arm-kernel/ZAh8dWvbNkVQT11C@arm.com/

Has something changes such that those issues no longer apply? If so, please
call that out explicitly in the commit message. If not, I do not think we
should take this patch.

Thanks,
Mark.

> ---
>  arch/arm64/Kconfig | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1023e896d46b..3c837b085f21 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -2228,6 +2228,12 @@ config CMDLINE_FROM_BOOTLOADER
>  	  the boot loader doesn't provide any, the default kernel command
>  	  string provided in CMDLINE will be used.
>  
> +config CMDLINE_EXTEND
> +	bool "Extend bootloader kernel arguments"
> +	help
> +	  The command-line arguments provided by the boot loader will be
> +	  appended to the default kernel command string.
> +
>  config CMDLINE_FORCE
>  	bool "Always use the default kernel command string"
>  	help
> -- 
> 2.40.0
>
  

On Tue, Mar 21, 2023 at 11:19:55AM +0000, Mark Rutland wrote:

> We deliberately dropped support for CMDLINE_EXTEND in commit:

>   cae118b6acc3 ("arm64: Drop support for CMDLINE_EXTEND")

> ... which was mentioned the last time somone tried to re-add it:

>   https://lore.kernel.org/linux-arm-kernel/ZAh8dWvbNkVQT11C@arm.com/

> Has something changes such that those issues no longer apply? If so, please
> call that out explicitly in the commit message. If not, I do not think we
> should take this patch.

Given that there have been multiple attempts to readd it is it worth
documenting this in the code?
  

On 22/03/23 08:06, Mark Brown wrote:
> On Tue, Mar 21, 2023 at 11:19:55AM +0000, Mark Rutland wrote:
>
>> We deliberately dropped support for CMDLINE_EXTEND in commit:
>>    cae118b6acc3 ("arm64: Drop support for CMDLINE_EXTEND")
>> ... which was mentioned the last time somone tried to re-add it:
>>    https://lore.kernel.org/linux-arm-kernel/ZAh8dWvbNkVQT11C@arm.com/
>> Has something changes such that those issues no longer apply?

I'm not sure I see any "issues" (although perhaps that's just me not 
being able to find the rest of the series on lore).

My specific use case is that I want to have the  sbsa_gwdt watchdog 
driver built-in to the kernel but I also want it to produce panic output 
instead of silently resetting the board so I want to have 
sbsa_gwdt.action=1 in my kernel command line. Either appending or 
prepending that on the command line would work for me.

>>   If so, please
>> call that out explicitly in the commit message. If not, I do not think we
>> should take this patch.

To save a click here is the relevant part from cae118b6acc3

     The documented behaviour for CMDLINE_EXTEND is that the arguments from
     the bootloader are appended to the built-in kernel command line. This
     also matches the option parsing behaviour for the EFI stub and early ID
     register overrides.

     Bizarrely, the fdt behaviour is the other way around: appending the
     built-in command line to the bootloader arguments, resulting in a
     command-line that doesn't necessarily line-up with the parsing 
order and
     definitely doesn't line-up with the documented behaviour.

This appears to be a current problem for arm an powerpc. If it weren't 
for the fact that EFI version had different behaviour I'd be suggesting 
just updating the help text. Maybe that should be done anyway regardless 
of any unification so that the documentation reflects reality.

> Given that there have been multiple attempts to readd it is it worth
> documenting this in the code?

The proposed CMDLINE_APPEND would work well for my use-case but two 
attempts at landing such generic support seem to have failed. I could 
attempt to resurrect [1] or the alternative [2] but I'm worried I'll end 
up with the same road blocks. Also looks like I just found a 3rd [3].

I hope I haven't kicked a hornets nest here.

--

[1] - 
https://lore.kernel.org/lkml/20190319232448.45964-2-danielwa@cisco.com/
[2] - 
https://patchwork.ozlabs.org/project/linuxppc-dev/cover/cover.1554195798.git.christophe.leroy@c-s.fr/
[3] - 
https://lore.kernel.org/lkml/20210416040924.2882771-1-danielwa@cisco.com/