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

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

[Abhishek Bhardwaj]

This change adds a new kernel configuration that sets the l1d cache
flush setting at compile time rather than at run time.

Signed-off-by: Abhishek Bhardwaj <abhishekbh@google.com>

---

Changes in v3:
- Change depends on to only x86_64.
- Remove copy paste errors at the end of the KConfig.

Changes in v2:
- Fix typo in the help of the new KConfig.

 arch/x86/kernel/cpu/bugs.c |  8 ++++++++
 arch/x86/kvm/Kconfig       | 13 +++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 0b71970d2d3d2..1dcc875cf5547 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1406,7 +1406,15 @@ enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_FLUSH;
 #if IS_ENABLED(CONFIG_KVM_INTEL)
 EXPORT_SYMBOL_GPL(l1tf_mitigation);
 #endif
+#if (CONFIG_KVM_VMENTRY_L1D_FLUSH == 1)
+enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_NEVER;
+#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 2)
+enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_COND;
+#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 3)
+enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_ALWAYS;
+#else
 enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO;
+#endif
 EXPORT_SYMBOL_GPL(l1tf_vmx_mitigation);
 
 /*
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index b277a2db62676..1f85374a0b812 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -107,4 +107,17 @@ config KVM_MMU_AUDIT
 	 This option adds a R/W kVM module parameter 'mmu_audit', which allows
 	 auditing of KVM MMU events at runtime.
 
+config KVM_VMENTRY_L1D_FLUSH
+	int "L1D cache flush settings (1-3)"
+	range 1 3
+	default "2"
+	depends on KVM && X86_64
+	help
+	 This setting determines the L1D cache flush behavior before a VMENTER.
+	 This is similar to setting the option / parameter to
+	 kvm-intel.vmentry_l1d_flush.
+	 1 - Never flush.
+	 2 - Conditionally flush.
+	 3 - Always flush.
+
 endif # VIRTUALIZATION
-- 
2.27.0.212.ge8ba1cc988-goog

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

[Randy Dunlap]

On 7/2/20 3:12 PM, Abhishek Bhardwaj wrote:
> This change adds a new kernel configuration that sets the l1d cache
> flush setting at compile time rather than at run time.
> 
> Signed-off-by: Abhishek Bhardwaj <abhishekbh@google.com>
> 
> ---
> 
> Changes in v3:
> - Change depends on to only x86_64.
> - Remove copy paste errors at the end of the KConfig.
> 
> Changes in v2:
> - Fix typo in the help of the new KConfig.
> 
>  arch/x86/kernel/cpu/bugs.c |  8 ++++++++
>  arch/x86/kvm/Kconfig       | 13 +++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 0b71970d2d3d2..1dcc875cf5547 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1406,7 +1406,15 @@ enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_FLUSH;
>  #if IS_ENABLED(CONFIG_KVM_INTEL)
>  EXPORT_SYMBOL_GPL(l1tf_mitigation);
>  #endif
> +#if (CONFIG_KVM_VMENTRY_L1D_FLUSH == 1)
> +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_NEVER;
> +#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 2)
> +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_COND;
> +#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 3)
> +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_ALWAYS;
> +#else
>  enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO;
> +#endif
>  EXPORT_SYMBOL_GPL(l1tf_vmx_mitigation);
>  
>  /*
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index b277a2db62676..1f85374a0b812 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -107,4 +107,17 @@ config KVM_MMU_AUDIT
>  	 This option adds a R/W kVM module parameter 'mmu_audit', which allows
>  	 auditing of KVM MMU events at runtime.
>  
> +config KVM_VMENTRY_L1D_FLUSH
> +	int "L1D cache flush settings (1-3)"
> +	range 1 3
> +	default "2"
> +	depends on KVM && X86_64
> +	help
> +	 This setting determines the L1D cache flush behavior before a VMENTER.
> +	 This is similar to setting the option / parameter to
> +	 kvm-intel.vmentry_l1d_flush.
> +	 1 - Never flush.
> +	 2 - Conditionally flush.
> +	 3 - Always flush.

No hurry on this (wait for other comments), but help text
should be indented by one tab + 2 spaces, per Documentation/process/coding-style.rst:

  Lines under a ``config`` definition
  are indented with one tab, while help text is indented an additional two
  spaces.


> +
>  endif # VIRTUALIZATION
> 
-- 
~Randy

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

[Waiman Long]

On 7/2/20 6:12 PM, Abhishek Bhardwaj wrote:
> This change adds a new kernel configuration that sets the l1d cache
> flush setting at compile time rather than at run time.
>
> Signed-off-by: Abhishek Bhardwaj <abhishekbh@google.com>

Can you explain in the commit log why you need a compile time option 
whereas the desired mitigation can also be set via a boot command line 
option?

The code looks OK, but the question I have is why.

Cheers,
Longman

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

[Anthony Steinhauser]

Yes, this probably requires an explanation why the change is necessary
or useful. Without that it is difficult to give some meaningful
feedback.

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

[Abhishek Bhardwaj]

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.

- 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.

I think this was what CONFIGS were intended for. I'm happy to add all
this to the commit message once it's approved in spirit by the
maintainers.

On Thu, Jul 2, 2020 at 8:18 PM Anthony Steinhauser
<asteinhauser@google.com> wrote:
>
> Yes, this probably requires an explanation why the change is necessary
> or useful. Without that it is difficult to give some meaningful
> feedback.



-- 
Abhishek

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

[Mike Rapoport]

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?
 
> - 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?

> I think this was what CONFIGS were intended for. I'm happy to add all
> this to the commit message once it's approved in spirit by the
> maintainers.
> 
> On Thu, Jul 2, 2020 at 8:18 PM Anthony Steinhauser
> <asteinhauser@google.com> wrote:
> >
> > Yes, this probably requires an explanation why the change is necessary
> > or useful. Without that it is difficult to give some meaningful
> > feedback.
> 
> 
> 
> -- 
> Abhishek

-- 
Sincerely yours,
Mike.

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

[Doug Anderson]

Hi,

On Fri, Jul 3, 2020 at 4:40 AM Mike Rapoport <rppt@linux.ibm.com> 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:

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.  :-)


> > - 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?


> > I think this was what CONFIGS were intended for. I'm happy to add all
> > this to the commit message once it's approved in spirit by the
> > maintainers.
> >
> > On Thu, Jul 2, 2020 at 8:18 PM Anthony Steinhauser
> > <asteinhauser@google.com> wrote:
> > >
> > > Yes, this probably requires an explanation why the change is necessary
> > > or useful. Without that it is difficult to give some meaningful
> > > feedback.
> >
> >
> >
> > --
> > Abhishek
>
> --
> Sincerely yours,
> Mike.

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2582 bytes --]

Hi Abhishek,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on linux/master tip/x86/core kvm/linux-next linus/master v5.8-rc3 next-20200703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Abhishek-Bhardwaj/x86-speculation-l1tf-Add-KConfig-for-setting-the-L1D-cache-flush-mode/20200703-061509
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 7e44a91e0445a854af5d34ca0f5baceccd518e73
config: i386-randconfig-a004-20200701 (attached as .config)
compiler: gcc-6 (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> arch/x86/kernel/cpu/bugs.c:1402:6: warning: "CONFIG_KVM_VMENTRY_L1D_FLUSH" is not defined [-Wundef]
    #if (CONFIG_KVM_VMENTRY_L1D_FLUSH == 1)
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/x86/kernel/cpu/bugs.c:1404:8: warning: "CONFIG_KVM_VMENTRY_L1D_FLUSH" is not defined [-Wundef]
    #elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 2)
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/x86/kernel/cpu/bugs.c:1406:8: warning: "CONFIG_KVM_VMENTRY_L1D_FLUSH" is not defined [-Wundef]
    #elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 3)
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/CONFIG_KVM_VMENTRY_L1D_FLUSH +1402 arch/x86/kernel/cpu/bugs.c

  1396	
  1397	/* Default mitigation for L1TF-affected CPUs */
  1398	enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_FLUSH;
  1399	#if IS_ENABLED(CONFIG_KVM_INTEL)
  1400	EXPORT_SYMBOL_GPL(l1tf_mitigation);
  1401	#endif
> 1402	#if (CONFIG_KVM_VMENTRY_L1D_FLUSH == 1)
  1403	enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_NEVER;
  1404	#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 2)
  1405	enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_COND;
  1406	#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 3)
  1407	enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_ALWAYS;
  1408	#else
  1409	enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO;
  1410	#endif
  1411	EXPORT_SYMBOL_GPL(l1tf_vmx_mitigation);
  1412	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31323 bytes --]

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

[Mike Rapoport]

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@linux.ibm.com> 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.

> 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.
 
> > > I think this was what CONFIGS were intended for. I'm happy to add all
> > > this to the commit message once it's approved in spirit by the
> > > maintainers.
> > >

-- 
Sincerely yours,
Mike.

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

[Waiman Long]

On 7/5/20 11:23 AM, Mike Rapoport wrote:
>> 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.
>   

It turns out that CONFIG_KVM_VMENTRY_L1D_FLUSH values match the enum 
vmx_l1d_flush_state values. So one way to reduce the ifdefery is to do, 
for example,

+#ifdef CONFIG_KVM_VMENTRY_L1D_FLUSH
+#define VMENTER_L1D_FLUSH_DEFAULT CONFIG_KVM_VMENTRY_L1D_FLUSH
+#else
+#define VMENTER_L1D_FLUSH_DEFAULT      VMENTER_L1D_FLUSH_AUTO
#endif
-enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO;
+enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_DEFAULT;

Of course, we may need to add a comment on enum vmx_l1d_flush_state 
definition to highlight the dependency of CONFIG_KVM_VMENTRY_L1D_FLUSH 
on it to avoid future mismatch.

Cheers,
Longman

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@linux.ibm.com> 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@linux.ibm.com> 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

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

[Abhishek Bhardwaj]

On Sun, Jul 5, 2020 at 8:57 AM Waiman Long <longman@redhat.com> wrote:
>
> On 7/5/20 11:23 AM, Mike Rapoport wrote:
> >> 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.
> >
>
> It turns out that CONFIG_KVM_VMENTRY_L1D_FLUSH values match the enum
> vmx_l1d_flush_state values. So one way to reduce the ifdefery is to do,
> for example,
>
> +#ifdef CONFIG_KVM_VMENTRY_L1D_FLUSH
> +#define VMENTER_L1D_FLUSH_DEFAULT CONFIG_KVM_VMENTRY_L1D_FLUSH
> +#else
> +#define VMENTER_L1D_FLUSH_DEFAULT      VMENTER_L1D_FLUSH_AUTO
> #endif
> -enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO;
> +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_DEFAULT;
>
> Of course, we may need to add a comment on enum vmx_l1d_flush_state
> definition to highlight the dependency of CONFIG_KVM_VMENTRY_L1D_FLUSH
> on it to avoid future mismatch.

I explicitly wanted to avoid doing that for this very reason. In my
opinion this is brittle and bound to be missed
sooner or later.

I'd rather have the value assignment be explicit rather than some
clever hack. Notice, this wouldn't work if the enum
values were not contiguous. We just got lucky.

Do people feel strongly against this ifdef ladder ?

>
> Cheers,
> Longman
>



--
Abhishek

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

[Waiman Long]

On 7/5/20 2:22 PM, Abhishek Bhardwaj wrote:
> On Sun, Jul 5, 2020 at 8:57 AM Waiman Long <longman@redhat.com> wrote:
>> On 7/5/20 11:23 AM, Mike Rapoport wrote:
>>>> 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.
>>>
>> It turns out that CONFIG_KVM_VMENTRY_L1D_FLUSH values match the enum
>> vmx_l1d_flush_state values. So one way to reduce the ifdefery is to do,
>> for example,
>>
>> +#ifdef CONFIG_KVM_VMENTRY_L1D_FLUSH
>> +#define VMENTER_L1D_FLUSH_DEFAULT CONFIG_KVM_VMENTRY_L1D_FLUSH
>> +#else
>> +#define VMENTER_L1D_FLUSH_DEFAULT      VMENTER_L1D_FLUSH_AUTO
>> #endif
>> -enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO;
>> +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_DEFAULT;
>>
>> Of course, we may need to add a comment on enum vmx_l1d_flush_state
>> definition to highlight the dependency of CONFIG_KVM_VMENTRY_L1D_FLUSH
>> on it to avoid future mismatch.
> I explicitly wanted to avoid doing that for this very reason. In my
> opinion this is brittle and bound to be missed
> sooner or later.
>
That is why I said a comment will have to be added to highlight this 
dependency. For instance,

+/*
+ * Three of the enums are explicitly assigned as the KVM_VMENTRY_L1D_FLUSH
+ * config entry in arch/x86/kvm/Kconfig depends on these values.
+ */
  enum vmx_l1d_flush_state {
         VMENTER_L1D_FLUSH_AUTO,
-       VMENTER_L1D_FLUSH_NEVER,
-       VMENTER_L1D_FLUSH_COND,
-       VMENTER_L1D_FLUSH_ALWAYS,
+       VMENTER_L1D_FLUSH_NEVER = 1,
+       VMENTER_L1D_FLUSH_COND = 2,
+       VMENTER_L1D_FLUSH_ALWAYS = 3,
         VMENTER_L1D_FLUSH_EPT_DISABLED,
         VMENTER_L1D_FLUSH_NOT_REQUIRED,
  };

Of course, this is just a suggestion.

Cheers,
Longman

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

[Abhishek Bhardwaj]

On Sun, Jul 5, 2020 at 11:48 AM Waiman Long <longman@redhat.com> wrote:
>
> On 7/5/20 2:22 PM, Abhishek Bhardwaj wrote:
> > On Sun, Jul 5, 2020 at 8:57 AM Waiman Long <longman@redhat.com> wrote:
> >> On 7/5/20 11:23 AM, Mike Rapoport wrote:
> >>>> 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.
> >>>
> >> It turns out that CONFIG_KVM_VMENTRY_L1D_FLUSH values match the enum
> >> vmx_l1d_flush_state values. So one way to reduce the ifdefery is to do,
> >> for example,
> >>
> >> +#ifdef CONFIG_KVM_VMENTRY_L1D_FLUSH
> >> +#define VMENTER_L1D_FLUSH_DEFAULT CONFIG_KVM_VMENTRY_L1D_FLUSH
> >> +#else
> >> +#define VMENTER_L1D_FLUSH_DEFAULT      VMENTER_L1D_FLUSH_AUTO
> >> #endif
> >> -enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO;
> >> +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_DEFAULT;
> >>
> >> Of course, we may need to add a comment on enum vmx_l1d_flush_state
> >> definition to highlight the dependency of CONFIG_KVM_VMENTRY_L1D_FLUSH
> >> on it to avoid future mismatch.
> > I explicitly wanted to avoid doing that for this very reason. In my
> > opinion this is brittle and bound to be missed
> > sooner or later.
> >
> That is why I said a comment will have to be added to highlight this
> dependency. For instance,
>
> +/*
> + * Three of the enums are explicitly assigned as the KVM_VMENTRY_L1D_FLUSH
> + * config entry in arch/x86/kvm/Kconfig depends on these values.
> + */
>   enum vmx_l1d_flush_state {
>          VMENTER_L1D_FLUSH_AUTO,
> -       VMENTER_L1D_FLUSH_NEVER,
> -       VMENTER_L1D_FLUSH_COND,
> -       VMENTER_L1D_FLUSH_ALWAYS,
> +       VMENTER_L1D_FLUSH_NEVER = 1,
> +       VMENTER_L1D_FLUSH_COND = 2,
> +       VMENTER_L1D_FLUSH_ALWAYS = 3,
>          VMENTER_L1D_FLUSH_EPT_DISABLED,
>          VMENTER_L1D_FLUSH_NOT_REQUIRED,
>   };
>
> Of course, this is just a suggestion.

I'd rather avoid this dependency. However, if people are okay with the
CONFIG option then I am happy to oblige with whatever people agree on.
Can a maintainer chime in ? Waiman if you're the final authority on
this, will you accept the patch if I incorporated your suggestion ?

>
> Cheers,
> Longman
>


-- 
Abhishek

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

[Abhishek Bhardwaj]

On Mon, Jul 6, 2020 at 8:14 AM Waiman Long <longman@redhat.com> wrote:
>
> On 7/5/20 5:51 PM, Abhishek Bhardwaj wrote:
>
> That is why I said a comment will have to be added to highlight this
> dependency. For instance,
>
> +/*
> + * Three of the enums are explicitly assigned as the KVM_VMENTRY_L1D_FLUSH
> + * config entry in arch/x86/kvm/Kconfig depends on these values.
> + */
>   enum vmx_l1d_flush_state {
>          VMENTER_L1D_FLUSH_AUTO,
> -       VMENTER_L1D_FLUSH_NEVER,
> -       VMENTER_L1D_FLUSH_COND,
> -       VMENTER_L1D_FLUSH_ALWAYS,
> +       VMENTER_L1D_FLUSH_NEVER = 1,
> +       VMENTER_L1D_FLUSH_COND = 2,
> +       VMENTER_L1D_FLUSH_ALWAYS = 3,
>          VMENTER_L1D_FLUSH_EPT_DISABLED,
>          VMENTER_L1D_FLUSH_NOT_REQUIRED,
>   };
>
> Of course, this is just a suggestion.
>
> I'd rather avoid this dependency. However, if people are okay with the
> CONFIG option then I am happy to oblige with whatever people agree on.
> Can a maintainer chime in ? Waiman if you're the final authority on
> this, will you accept the patch if I incorporated your suggestion ?
>
> It is fine if you think this is not what you want.
>
> BTW, I am not a maintainer. However, no maintainer will give pre-approval. At least, you have to give the reason why this patch is needed in the patch itself. Before that happens, I don't see how it will get merged upstream.

I just updated the patch with the reasoning in the commit message -
https://lkml.org/lkml/2020/7/8/1325


>
> Cheers,
> Longman



-- 
Abhishek