Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode

[Doug Anderson]

Hi,

On Sun, Jul 5, 2020 at 8:23 AM Mike Rapoport <rppt@xxxxxxxxxxxxx> wrote:
>
> On Fri, Jul 03, 2020 at 07:00:11AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Jul 3, 2020 at 4:40 AM Mike Rapoport <rppt@xxxxxxxxxxxxx> wrote:
> > >
> > > On Thu, Jul 02, 2020 at 11:43:47PM -0700, Abhishek Bhardwaj wrote:
> > > > We have tried to steer away from kernel command line args for a few reasons.
> > > >
> > > > I am paraphrasing my colleague Doug's argument here (CC'ed him as well) -
> > > >
> > > > - The command line args are getting unwieldy. Kernel command line
> > > > parameters are not a scalable way to set kernel config. It's intended
> > > > as a super limited way for the bootloader to pass info to the kernel
> > > > and also as a way for end users who are not compiling the kernel
> > > > themselves to tweak kernel behavior.
> > >
> > > Why cannot you simply add this option to CONFIG_CMDLINE at your kernel build
> > > scripts?
> >
> > At least in the past I've seen that 'CONFIG_CMDLINE' interacts badly
> > with the bootloader provided command line in some architectures.  In
> > days of yore I tried to post a patch to fix this, at least on ARM
> > targets, but it never seemed to go anywhere upstream.  I'm going to
> > assume this is still a problem because I still see an ANDROID tagged
> > patch in the Chrome OS 5.4 tree:
>
> I presume a patch subject should have been here :)
> Anyway, bad iteraction of CONFIG_CMDLINE with bootloader command line
> seems like a bug to me and a bug need to be fixed.

Sure.  In the Chrome OS 5.4 tree, this is commit 016c3a09a573
("ANDROID: arm64: copy CONFIG_CMDLINE_EXTEND from ARM").  Here's a
link too:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/016c3a09a573

...written in 2014.  :-)  Ah funny.  It looks like to make this work
we're carrying around something based on my old patch, too (from
2012)!  :-P  Commit 5eaa3f55d381 ("ANDROID: of: Support
CONFIG_CMDLINE_EXTEND config option"), or (as a link):

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/5eaa3f55d381

IIRC without stuff like those patches then, on ARM (and ARM64)
hardware anyway, you get _either_ the command line from the bootloader
or the command line from the kernel.  So I guess I wasn't quite
remembering it correctly and it wasn't a bad interaction with the
bootloader but just generally that on ARM kernels on mainline there
just isn't a concept of extending the command line.

Certainly someone could make an extra effort to get the above patches
landed in mainline, but (trying to remember from ~8 years ago) I think
I dropped trying to do that because it was pointed out to me that it
was better not to jam so much stuff into the command line.

...and yes, the fact that we're carrying those patches in the Chrome
OS tree means we _could_ just use them on Chrome OS, but I'd rather
not.  Right now we only have them there because we merged in the
ANDROID tree and I'm not aware of any Chrome OS users.  In general I
prefer not to rely on patches that are not in mainline.


> > In any case, as per my previous arguments, stuffing lots of config
> > into the cmdline is a bit clunky and doesn't scale well.  You end up
> > with a really long run on command line and it's hard to tell where one
> > config option ends and the next one starts and if the same concept is
> > there more than one time it's hard to tell and something might cancel
> > out a previous config option or maybe it won't and by the time you end
> > up finishing this it's hard to tell where you started.  :-)
>
> Configuration options may also have weird interactions between them and
> addition of #ifdef means that most of the non-default paths won't get as
> good test coverage as the default one.
>
> And the proposed #ifdef maze does not look pretty at all...
>
> > > > - Also, we know we want this setting from the start. This is a
> > > > definite smell that it deserves to be a compile time thing rather than
> > > > adding extra code + whatever miniscule time at runtime to pass an
> > > > extra arg.
> > >
> > > This might be a compile time thing in your environment, but not
> > > necessarily it must be the same in others. For instance, what option
> > > should distro kernels select?
> >
> > Nothing prevents people from continuing to use the command line
> > options if they want, right?  This just allows a different default.
> > So if a distro is security focused and decided that it wanted a slower
> > / more secure default then it could ship that way but individual users
> > could still override, right?
>
> Well, nothing prevents you from continuing to use the command line as
> well ;-)
>
> I can see why whould you want an ability to select compile time default
> for an option, but I'm really not thrilled by the added ifdefery.

Sounds like the right solution here is to clean up the patch to make
it not so hard to follow and it looks like there's already a
suggestion for that.  :-)

-Doug