[0/2] Fix CMDLINE_EXTEND handling for FDT "bootargs"

Message

Will Deacon Feb. 25, 2021, 12:59 p.m. UTC

Hi folks,

We recently [1] enabled support for CMDLINE_EXTEND on arm64, however
when I started looking at replacing Android's out-of-tree implementation [2]
with the upstream version, I noticed that the two behave significantly
differently: Android follows the Kconfig help text of appending the
bootloader arguments to the kernel command line, whereas upstream appends
the kernel command line to the bootloader arguments. That is, except for
the EFI stub, which follows the documented behaviour.

I think the documented behaviour is more useful, so this patch series
reworks the FDT code to follow that and updates the very recently merged
arm64 idreg early command-line parsing as well.

I'd like to take the first patch as a fix via the arm64 tree.

Cheers,

Will

[1] https://lore.kernel.org/r/[e-mail]
[2] https://android-review.googlesource.com/c/kernel/common/+/841045

--->8

Cc: Max Uvarov <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Ard Biesheuvel <[e-mail]>
Cc: Marc Zyngier <[e-mail]>
Cc: Doug Anderson <[e-mail]>
Cc: Tyler Hicks <[e-mail]>
Cc: Frank Rowand <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Palmer Dabbelt <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: Catalin Marinas <[e-mail]>
Cc: <[e-mail]>
Cc: <[e-mail]>
Cc: <[e-mail]>

Will Deacon (2):
  arm64: cpufeatures: Fix handling of CONFIG_CMDLINE for idreg overrides
  of/fdt: Append bootloader arguments when CMDLINE_EXTEND=y

 arch/arm64/kernel/idreg-override.c | 44 +++++++++++---------
 drivers/of/fdt.c                   | 64 ++++++++++++++++++------------
 2 files changed, 64 insertions(+), 44 deletions(-)

Comments

Rob Herring (Arm) March 1, 2021, 2:19 p.m. UTC | #1

On Thu, Feb 25, 2021 at 6:59 AM Will Deacon <[e-mail]> wrote:
>
> Hi folks,
>
> We recently [1] enabled support for CMDLINE_EXTEND on arm64, however
> when I started looking at replacing Android's out-of-tree implementation [2]

Did anyone go read the common, reworked version of all this I
referenced that supports prepend and append. Here it is again[1].
Maybe I should have been more assertive there and said 'extend' is
ambiguous.

> with the upstream version, I noticed that the two behave significantly
> differently: Android follows the Kconfig help text of appending the
> bootloader arguments to the kernel command line, whereas upstream appends
> the kernel command line to the bootloader arguments. That is, except for
> the EFI stub, which follows the documented behaviour.
>
> I think the documented behaviour is more useful, so this patch series
> reworks the FDT code to follow that and updates the very recently merged
> arm64 idreg early command-line parsing as well.

I can just as easily argue that the kernel having the last say makes
sense. Regardless, I'm pretty sure there's someone out there relying
on current behavior. What is the impact of this change to other
arches?

Rob

[1] https://lore.kernel.org/lkml/[e-mail]/

Will Deacon March 1, 2021, 2:41 p.m. UTC | #2

On Mon, Mar 01, 2021 at 08:19:32AM -0600, Rob Herring wrote:
> On Thu, Feb 25, 2021 at 6:59 AM Will Deacon <[e-mail]> wrote:
> > We recently [1] enabled support for CMDLINE_EXTEND on arm64, however
> > when I started looking at replacing Android's out-of-tree implementation [2]
> 
> Did anyone go read the common, reworked version of all this I
> referenced that supports prepend and append. Here it is again[1].
> Maybe I should have been more assertive there and said 'extend' is
> ambiguous.

I tried reading that, but (a) most of the series is not in the mailing list
archives and (b) the patch that _is_ doesn't touch CMDLINE_EXTEND at all.
Right now the code in mainline does the opposite of what it's documented to
do.

> > with the upstream version, I noticed that the two behave significantly
> > differently: Android follows the Kconfig help text of appending the
> > bootloader arguments to the kernel command line, whereas upstream appends
> > the kernel command line to the bootloader arguments. That is, except for
> > the EFI stub, which follows the documented behaviour.
> >
> > I think the documented behaviour is more useful, so this patch series
> > reworks the FDT code to follow that and updates the very recently merged
> > arm64 idreg early command-line parsing as well.
> 
> I can just as easily argue that the kernel having the last say makes
> sense.

Dunno, I'd say that's what CMDLINE_FORCE is for. Plus you'd be arguing
against both the documentation and the EFI stub implementation.

> Regardless, I'm pretty sure there's someone out there relying on current
> behavior. What is the impact of this change to other arches?

On arm64, I doubt it, as Android is the main user of this (where it's been
supported for 9 years with the documented behaviour).

The other option, then, is reverting CMDLINE_EXTEND from arm64 until this is
figured out. I think that's preferable to having divergent behaviour.

As for other architectures, I think the ATAGs-based solution on arch/arm/
gets it right:

  static int __init parse_tag_cmdline(const struct tag *tag)
  {
  #if defined(CONFIG_CMDLINE_EXTEND)
          strlcat(default_command_line, " ", COMMAND_LINE_SIZE);
          strlcat(default_command_line, tag->u.cmdline.cmdline,
                  COMMAND_LINE_SIZE);

For now I think we have two options for arm64: either fix the fdt code,
or revert CMDLINE_EXTEND until the PREPEND/APPEND series is merged. Which
do you prefer?

Will

Rob Herring (Arm) March 1, 2021, 5:26 p.m. UTC | #3

+PPC folks and Daniel W

On Mon, Mar 1, 2021 at 8:42 AM Will Deacon <[e-mail]> wrote:
>
> On Mon, Mar 01, 2021 at 08:19:32AM -0600, Rob Herring wrote:
> > On Thu, Feb 25, 2021 at 6:59 AM Will Deacon <[e-mail]> wrote:
> > > We recently [1] enabled support for CMDLINE_EXTEND on arm64, however
> > > when I started looking at replacing Android's out-of-tree implementation [2]
> >
> > Did anyone go read the common, reworked version of all this I
> > referenced that supports prepend and append. Here it is again[1].
> > Maybe I should have been more assertive there and said 'extend' is
> > ambiguous.
>
> I tried reading that, but (a) most of the series is not in the mailing list
> archives and (b) the patch that _is_ doesn't touch CMDLINE_EXTEND at all.
> Right now the code in mainline does the opposite of what it's documented to
> do.

Actually, there is a newer version I found:

https://lore.kernel.org/linuxppc-dev/[e-mail]/
https://lore.kernel.org/linuxppc-dev/[e-mail]/
https://lore.kernel.org/linuxppc-dev/[e-mail]/

(Once again, there's some weird threading going on)

> > > with the upstream version, I noticed that the two behave significantly
> > > differently: Android follows the Kconfig help text of appending the
> > > bootloader arguments to the kernel command line, whereas upstream appends
> > > the kernel command line to the bootloader arguments. That is, except for
> > > the EFI stub, which follows the documented behaviour.
> > >
> > > I think the documented behaviour is more useful, so this patch series
> > > reworks the FDT code to follow that and updates the very recently merged
> > > arm64 idreg early command-line parsing as well.
> >
> > I can just as easily argue that the kernel having the last say makes
> > sense.
>
> Dunno, I'd say that's what CMDLINE_FORCE is for. Plus you'd be arguing
> against both the documentation and the EFI stub implementation.

CMDLINE_FORCE is a complete override, not a merging of command lines.

> > Regardless, I'm pretty sure there's someone out there relying on current
> > behavior. What is the impact of this change to other arches?
>
> On arm64, I doubt it, as Android is the main user of this (where it's been
> supported for 9 years with the documented behaviour).
>
> The other option, then, is reverting CMDLINE_EXTEND from arm64 until this is
> figured out. I think that's preferable to having divergent behaviour.
>
> As for other architectures, I think the ATAGs-based solution on arch/arm/
> gets it right:
>
>   static int __init parse_tag_cmdline(const struct tag *tag)
>   {
>   #if defined(CONFIG_CMDLINE_EXTEND)
>           strlcat(default_command_line, " ", COMMAND_LINE_SIZE);
>           strlcat(default_command_line, tag->u.cmdline.cmdline,
>                   COMMAND_LINE_SIZE);

The question is really whether any arm32 DT based platform depends on
the current behavior. RiscV could also be relying on current behavior.
Powerpc also uses the current behavior (and the documentation is also
wrong there). Changing the behavior in the FDT code means the powerpc
early PROM code and the FDT code do the opposite.

Arm32 has had current behaviour for 5 years. Powerpc for 1.5 years and
Risc-V for 2 years. Then there's MIPS which has its own Kconfig
symbols for this and is its own kind of mess. Either we assume
existing users didn't really care about the order or we have to
support both prepend and append.

> For now I think we have two options for arm64: either fix the fdt code,
> or revert CMDLINE_EXTEND until the PREPEND/APPEND series is merged. Which
> do you prefer?

Like anything copied across arches, I want someone to look at this
across all architectures and make this common instead of just copying
to new arches. The prepend/append series is the closest we've come.

Rob

Christophe Leroy March 1, 2021, 5:45 p.m. UTC | #4

Le 01/03/2021 à 18:26, Rob Herring a écrit :
> +PPC folks and Daniel W
> 
> On Mon, Mar 1, 2021 at 8:42 AM Will Deacon <[e-mail]> wrote:
>>
>> On Mon, Mar 01, 2021 at 08:19:32AM -0600, Rob Herring wrote:
>>> On Thu, Feb 25, 2021 at 6:59 AM Will Deacon <[e-mail]> wrote:
>>>> We recently [1] enabled support for CMDLINE_EXTEND on arm64, however
>>>> when I started looking at replacing Android's out-of-tree implementation [2]
>>>
>>> Did anyone go read the common, reworked version of all this I
>>> referenced that supports prepend and append. Here it is again[1].
>>> Maybe I should have been more assertive there and said 'extend' is
>>> ambiguous.
>>
>> I tried reading that, but (a) most of the series is not in the mailing list
>> archives and (b) the patch that _is_ doesn't touch CMDLINE_EXTEND at all.
>> Right now the code in mainline does the opposite of what it's documented to
>> do.
> 
> Actually, there is a newer version I found:
> 
> https://lore.kernel.org/linuxppc-dev/[e-mail]/
> https://lore.kernel.org/linuxppc-dev/[e-mail]/
> https://lore.kernel.org/linuxppc-dev/[e-mail]/

This was seen as too much intrusive into powerpc.

I proposed an alternative at 
https://patchwork.ozlabs.org/project/linuxppc-dev/cover/[e-mail]/ but 
never got any feedback.


> 
> (Once again, there's some weird threading going on)
> 
>>>> with the upstream version, I noticed that the two behave significantly
>>>> differently: Android follows the Kconfig help text of appending the
>>>> bootloader arguments to the kernel command line, whereas upstream appends
>>>> the kernel command line to the bootloader arguments. That is, except for
>>>> the EFI stub, which follows the documented behaviour.
>>>>
>>>> I think the documented behaviour is more useful, so this patch series
>>>> reworks the FDT code to follow that and updates the very recently merged
>>>> arm64 idreg early command-line parsing as well.
>>>
>>> I can just as easily argue that the kernel having the last say makes
>>> sense.
>>
>> Dunno, I'd say that's what CMDLINE_FORCE is for. Plus you'd be arguing
>> against both the documentation and the EFI stub implementation.
> 
> CMDLINE_FORCE is a complete override, not a merging of command lines.
> 
>>> Regardless, I'm pretty sure there's someone out there relying on current
>>> behavior. What is the impact of this change to other arches?
>>
>> On arm64, I doubt it, as Android is the main user of this (where it's been
>> supported for 9 years with the documented behaviour).
>>
>> The other option, then, is reverting CMDLINE_EXTEND from arm64 until this is
>> figured out. I think that's preferable to having divergent behaviour.
>>
>> As for other architectures, I think the ATAGs-based solution on arch/arm/
>> gets it right:
>>
>>    static int __init parse_tag_cmdline(const struct tag *tag)
>>    {
>>    #if defined(CONFIG_CMDLINE_EXTEND)
>>            strlcat(default_command_line, " ", COMMAND_LINE_SIZE);
>>            strlcat(default_command_line, tag->u.cmdline.cmdline,
>>                    COMMAND_LINE_SIZE);
> 
> The question is really whether any arm32 DT based platform depends on
> the current behavior. RiscV could also be relying on current behavior.
> Powerpc also uses the current behavior (and the documentation is also
> wrong there). Changing the behavior in the FDT code means the powerpc
> early PROM code and the FDT code do the opposite.
> 
> Arm32 has had current behaviour for 5 years. Powerpc for 1.5 years and
> Risc-V for 2 years. Then there's MIPS which has its own Kconfig
> symbols for this and is its own kind of mess. Either we assume
> existing users didn't really care about the order or we have to
> support both prepend and append.
> 
>> For now I think we have two options for arm64: either fix the fdt code,
>> or revert CMDLINE_EXTEND until the PREPEND/APPEND series is merged. Which
>> do you prefer?
> 
> Like anything copied across arches, I want someone to look at this
> across all architectures and make this common instead of just copying
> to new arches. The prepend/append series is the closest we've come.
> 
> Rob
> 

Christophe

Rob Herring (Arm) March 2, 2021, 2:56 p.m. UTC | #5

On Mon, Mar 1, 2021 at 11:45 AM Christophe Leroy
<[e-mail]> wrote:
>
>
>
> Le 01/03/2021 à 18:26, Rob Herring a écrit :
> > +PPC folks and Daniel W
> >
> > On Mon, Mar 1, 2021 at 8:42 AM Will Deacon <[e-mail]> wrote:
> >>
> >> On Mon, Mar 01, 2021 at 08:19:32AM -0600, Rob Herring wrote:
> >>> On Thu, Feb 25, 2021 at 6:59 AM Will Deacon <[e-mail]> wrote:
> >>>> We recently [1] enabled support for CMDLINE_EXTEND on arm64, however
> >>>> when I started looking at replacing Android's out-of-tree implementation [2]
> >>>
> >>> Did anyone go read the common, reworked version of all this I
> >>> referenced that supports prepend and append. Here it is again[1].
> >>> Maybe I should have been more assertive there and said 'extend' is
> >>> ambiguous.
> >>
> >> I tried reading that, but (a) most of the series is not in the mailing list
> >> archives and (b) the patch that _is_ doesn't touch CMDLINE_EXTEND at all.
> >> Right now the code in mainline does the opposite of what it's documented to
> >> do.
> >
> > Actually, there is a newer version I found:
> >
> > https://lore.kernel.org/linuxppc-dev/[e-mail]/
> > https://lore.kernel.org/linuxppc-dev/[e-mail]/
> > https://lore.kernel.org/linuxppc-dev/[e-mail]/
>
> This was seen as too much intrusive into powerpc.

It looked like the main issue was string functions for KASAN?

As far as being too complex, I think that will be needed if you look
at all architectures and non-DT cases.

> I proposed an alternative at
> https://patchwork.ozlabs.org/project/linuxppc-dev/cover/[e-mail]/ but
> never got any feedback.

Didn't go to a list I subscribe to. In particular, if it had gone to
DT list and into PW you would have gotten a reply from me.

Rob

Christophe Leroy March 2, 2021, 3:16 p.m. UTC | #6

Le 02/03/2021 à 15:56, Rob Herring a écrit :
> On Mon, Mar 1, 2021 at 11:45 AM Christophe Leroy
> <[e-mail]> wrote:
>>
>>
>>
>> Le 01/03/2021 à 18:26, Rob Herring a écrit :
>>> +PPC folks and Daniel W
>>>
>>> On Mon, Mar 1, 2021 at 8:42 AM Will Deacon <[e-mail]> wrote:
>>>>
>>>> On Mon, Mar 01, 2021 at 08:19:32AM -0600, Rob Herring wrote:
>>>>> On Thu, Feb 25, 2021 at 6:59 AM Will Deacon <[e-mail]> wrote:
>>>>>> We recently [1] enabled support for CMDLINE_EXTEND on arm64, however
>>>>>> when I started looking at replacing Android's out-of-tree implementation [2]
>>>>>
>>>>> Did anyone go read the common, reworked version of all this I
>>>>> referenced that supports prepend and append. Here it is again[1].
>>>>> Maybe I should have been more assertive there and said 'extend' is
>>>>> ambiguous.
>>>>
>>>> I tried reading that, but (a) most of the series is not in the mailing list
>>>> archives and (b) the patch that _is_ doesn't touch CMDLINE_EXTEND at all.
>>>> Right now the code in mainline does the opposite of what it's documented to
>>>> do.
>>>
>>> Actually, there is a newer version I found:
>>>
>>> https://lore.kernel.org/linuxppc-dev/[e-mail]/
>>> https://lore.kernel.org/linuxppc-dev/[e-mail]/
>>> https://lore.kernel.org/linuxppc-dev/[e-mail]/
>>
>> This was seen as too much intrusive into powerpc.
> 
> It looked like the main issue was string functions for KASAN?

This is one issue yes,

> 
> As far as being too complex, I think that will be needed if you look
> at all architectures and non-DT cases.

As far as I remember, I could't understand why we absolutely need to define the command line string 
in the common part of the code, leading to being obliged to use macros in order to allow the 
architecture to specify in which section it wants the string.

Why not leave the definition of the string to the architecture and just declare it in the common 
code, allowing the architecture to put it where it suits it and reducing opacity and allowing use of 
standard static inline functions instead of uggly macros.


> 
>> I proposed an alternative at
>> https://patchwork.ozlabs.org/project/linuxppc-dev/cover/[e-mail]/ but
>> never got any feedback.
> 
> Didn't go to a list I subscribe to. In particular, if it had gone to
> DT list and into PW you would have gotten a reply from me.
> 

Sorry for that. Original series from Daniel 
(https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[e-mail]/) 
was sent only to linuxppc-dev list, and Michael suggested to also send it to linux-arch list, and I 
also always copy linux-kernel.

If there is new interest for that functionnality, I can try and rebase my series.

Christophe

Daniel Walker (danielwa) March 2, 2021, 5:12 p.m. UTC | #7

On Mon, Mar 01, 2021 at 11:26:14AM -0600, Rob Herring wrote:
> +PPC folks and Daniel W
> 
> On Mon, Mar 1, 2021 at 8:42 AM Will Deacon <[e-mail]> wrote:
> >
> > On Mon, Mar 01, 2021 at 08:19:32AM -0600, Rob Herring wrote:
> > > On Thu, Feb 25, 2021 at 6:59 AM Will Deacon <[e-mail]> wrote:
> > > > We recently [1] enabled support for CMDLINE_EXTEND on arm64, however
> > > > when I started looking at replacing Android's out-of-tree implementation [2]
> > >
> > > Did anyone go read the common, reworked version of all this I
> > > referenced that supports prepend and append. Here it is again[1].
> > > Maybe I should have been more assertive there and said 'extend' is
> > > ambiguous.
> >
> > I tried reading that, but (a) most of the series is not in the mailing list
> > archives and (b) the patch that _is_ doesn't touch CMDLINE_EXTEND at all.
> > Right now the code in mainline does the opposite of what it's documented to
> > do.
> 
> Actually, there is a newer version I found:
> 
> https://lore.kernel.org/linuxppc-dev/[e-mail]/
> https://lore.kernel.org/linuxppc-dev/[e-mail]/
> https://lore.kernel.org/linuxppc-dev/[e-mail]/
> 
> (Once again, there's some weird threading going on)
> 

I'm happy to work with anyone to resubmit the changes. We currently use the
changes in Cisco, and we have used them for many years.

I was planning to update and resubmit since someone has recently inquired about
why it wasn't upstream.

Daniel