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

[Doug Anderson]

Hi,

On Thu, Jul 9, 2020 at 4:05 PM Abhishek Bhardwaj <abhishekbh@xxxxxxxxxx> wrote:
>
>
>
> On Thu, Jul 9, 2020 at 3:43 PM mark gross <mgross@xxxxxxxxxxxxxxx> wrote:
>>
>> On Thu, Jul 09, 2020 at 12:42:57PM -0700, Doug Anderson wrote:
>> > Hi,
>> >
>> > On Thu, Jul 9, 2020 at 3:51 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> > >
>> > > Abhishek Bhardwaj <abhishekbh@xxxxxxxxxx> writes:
>> > > > This change adds a new kernel configuration that sets the l1d cache
>> > > > flush setting at compile time rather than at run time.
>> > > >
>> > > > The reasons for this change are as follows -
>> > > >
>> > > >  - Kernel command line arguments are getting unwieldy. These parameters
>> > > >  are not a scalable way to set the kernel config. They're 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 the kernel behavior.
>> > > >
>> > > >  - Also, if a user wants this setting from the start. It's a definite
>> > > >  smell that it deserves to be a compile time thing rather than adding
>> > > >  extra code plus whatever miniscule time at runtime to pass an
>> > > >  extra argument.
>> > > >
>> > > >  - Finally, it doesn't preclude the runtime / kernel command line way.
>> > > >  Users are free to use those as well.
>> > >
>> > > TBH, I don't see why this is a good idea.
>> > >
>> > >  1) I'm not following your argumentation that the command line option is
>> > >     a poor Kconfig replacement. The L1TF mode is a boot time (module
>> > >     load time) decision and the command line parameter is there to
>> > >     override the carefully chosen and sensible default behaviour.
>> >
>> > When you say that the default behavior is carefully chosen and
>> > sensible, are you saying that (in your opinion) there would never be a
>> > good reason for someone distributing a kernel to others to change the
>> > default?  Certainly I agree that having the kernel command line
>> > parameter is nice to allow someone to override whatever the person
>> > building the kernel chose, but IMO it's not a good way to change the
>> > default built-in to the kernel.
>> >
>> > The current plan (as I understand it) is that we'd like to ship
>> > Chromebook kernels with this option changed from the default that's
>> > there now.  In your opinion, is that a sane thing to do?
>> >
>> >
>> > >  2) You can add the desired mode to the compiled in (partial) kernel
>> > >     command line today.
>> >
>> > This might be easier on x86 than it is on ARM.  ARM (and ARM64)
>> > kernels only have two modes: kernel provides cmdline and bootloader
>> > provides cmdline.  There are out-of-mainline ANDROID patches to
>> > address this but nothing in mainline.
>> >
>> > The patch we're discussing now is x86-only so it's not such a huge
>> > deal, but the fact that combining the kernel and bootloader
>> > commandline never landed in mainline for arm/arm64 means that this
>> > isn't a super common/expected thing to do.
>> >
>> >
>> > >  3) Boot loaders are well capable of handling large kernel command lines
>> > >     and the extra time spend for reading the parameter does not matter
>> > >     at all.
>> >
>> > Long command lines can still be a bit of a chore for humans to deal
>> > with.  Many times I've needed to look at "/proc/cmdline" and make
>> > sense of it.  The longer the command line is and the more cruft
>> > stuffed into it the more of a chore it is.  Yes, this is just one
>> > thing to put in the command line, but if 10 different drivers all have
>> > their "one thing" to put there it gets really long.  If 100 different
>> > drivers all want their one config option there it gets really really
>> > long.  IMO the command line should be a last resort place to put
>> > things and should just contain:
>>
>> This takes me back to my years doing android kernel work for Intel, I'm glad
>> those are over.  Yes, the android kernel command lines got hideous, I think we
>> even had patches to make the cmdline buffer bigger than the default was.
>>
>> From a practical point of view the command line was part of the boot image and
>> cryptography protected so it was a handy way to securely communicate parameters
>> from the platform to the kernel, drivers and even just user mode.  It got
>> pretty ugly but, it worked (mostly).
>>
>> What I don't get is why pick on l1tf in isolation?
>
> Because this is what we needed given the state of our projects.

Right.  Basically the flow is:

1. Someone comes up with a tweak that they think some people might
want to try out but it's not expected to be the default for anyone:
add a module parameter.

2. Someone comes around and says: I think it's sensible to change the
default for a whole class of devices, but another class of devices can
still use the old default: add a config option in addition to the
kernel parameter.


If #2 never happens then it can stay a module parameter forever.  If
#2 happens that's fine--it doesn't really hurt to add a config option
for it.


>> There are a bunch of
>> command line parameters similar to l1tf.  Would a more general option make
>> sense?
>
> Maybe. Given how much resistance this CONFIG has met, I can only imagine the resistance
> upon introducing a more reaching configuration. I also think there's a fine line between YAGNI (You Ain't
> Gonna Need It) vs planning for the future. I don't want to introduce a big CONFIG that won't be used for
> anything else. I'd rather we cross that bridge when someone else feels the same way about other options.

What are you envisioning?  I guess the "generic" solution is to add
cmdline extension (being able to combine kernel and bootloader
cmdline) to all platforms in a somewhat uniform way without breaking
any backward compatibility.  You'd still end up with a really ugly
"/proc/cmdline" if you had too many options there, but I guess it
wouldn't be the end of the world.

Getting this done on all platforms doesn't seem like it'd be an easy
task, though.


>> Anyway, I think there is a higher level issue you are poking at that might be
>> better addressed by talking about it directly.
>
>
> I am new to this process. I don't know who to "convince" or talk about these issues. Who is the final authority on
> this ?. I don't think appeasing every concern is going to be productive for anyone. I've offered to change the implementation,
> if that is what's required. However, if the final authority is against a CONFIG, I don't really know how to proceed.
>
>>
>>
>> --mark
>>
>>
>
>
> --
> Abhishek