Re: [PATCH v2 1/2] arm64: cpufeatures: Fix handling of CONFIG_CMDLINE for idreg overrides

[Will Deacon <will@kernel.org>]

On Wed, Mar 03, 2021 at 04:30:21PM -0600, Rob Herring wrote:
> On Wed, Mar 3, 2021 at 7:50 AM Will Deacon <will@kernel.org> wrote:
> >
> > The built-in kernel commandline (CONFIG_CMDLINE) can be configured in
> > three different ways:
> >
> >   1. CMDLINE_FORCE: Use CONFIG_CMDLINE instead of any bootloader args
> >   2. CMDLINE_EXTEND: Append the bootloader args to CONFIG_CMDLINE
> >   3. CMDLINE_FROM_BOOTLOADER: Only use CONFIG_CMDLINE if there aren't
> >      any bootloader args.
> >
> > The early cmdline parsing to detect idreg overrides gets (2) and (3)
> > slightly wrong: in the case of (2) the bootloader args are parsed first
> > and in the case of (3) the CMDLINE is always parsed.
> >
> > Fix these issues by moving the bootargs parsing out into a helper
> > function and following the same logic as that used by the EFI stub.
> >
> > Reviewed-by: Marc Zyngier <maz@kernel.org>
> > Fixes: 33200303553d ("arm64: cpufeature: Add an early command-line cpufeature override facility")
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/kernel/idreg-override.c | 44 +++++++++++++++++-------------
> >  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> It bothers me a bit having command line handling here. It means how
> the command line is assembled in 2 places.

It's more than 2 places, it's also done in the EFI stub; see efi_pe_entry()
in drivers/firmware/efi/libstub/efi-stub.c. That's why I'm pushing on this,
because at the moment the command-line reported by /proc/cmdline doesn't
match the command-line that was parsed there because they stitch it together
in opposite orders when CMDLINE_EXTEND is used.

> I guess if we get rid of ambiguous 'extend' then it's better, but perhaps
> a better implementation would be an api get a specific command line
> parameter.  The main downside would be searching the DT again for each
> parameter if we can't store any data in between calls, but there's ways
> around that. PowerPC also needs similar functionality in
> disabled_on_cmdline().

Christophe's patches at least aim to put the assembling all in one place,
although we'll have to see whether or not it can be used in the EFI stub
environment.

Will