* [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode @ 2020-07-02 22:12 Abhishek Bhardwaj 2020-07-02 23:16 ` Randy Dunlap ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Abhishek Bhardwaj @ 2020-07-02 22:12 UTC (permalink / raw) To: LKML Cc: Abhishek Bhardwaj, Anthony Steinhauser, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Mike Rapoport, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Waiman Long, Wanpeng Li, kvm, x86 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-02 22:12 [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode Abhishek Bhardwaj @ 2020-07-02 23:16 ` Randy Dunlap 2020-07-03 0:17 ` Waiman Long 2020-07-03 14:14 ` kernel test robot 2 siblings, 0 replies; 15+ messages in thread From: Randy Dunlap @ 2020-07-02 23:16 UTC (permalink / raw) To: Abhishek Bhardwaj, LKML Cc: Anthony Steinhauser, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Mike Rapoport, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Waiman Long, Wanpeng Li, kvm, x86 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-02 22:12 [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode Abhishek Bhardwaj 2020-07-02 23:16 ` Randy Dunlap @ 2020-07-03 0:17 ` Waiman Long 2020-07-03 3:17 ` Anthony Steinhauser 2020-07-03 14:14 ` kernel test robot 2 siblings, 1 reply; 15+ messages in thread From: Waiman Long @ 2020-07-03 0:17 UTC (permalink / raw) To: Abhishek Bhardwaj, LKML Cc: Anthony Steinhauser, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Mike Rapoport, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-03 0:17 ` Waiman Long @ 2020-07-03 3:17 ` Anthony Steinhauser 2020-07-03 6:43 ` Abhishek Bhardwaj 0 siblings, 1 reply; 15+ messages in thread From: Anthony Steinhauser @ 2020-07-03 3:17 UTC (permalink / raw) To: Abhishek Bhardwaj Cc: LKML, Waiman Long, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Mike Rapoport, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 Yes, this probably requires an explanation why the change is necessary or useful. Without that it is difficult to give some meaningful feedback. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-03 3:17 ` Anthony Steinhauser @ 2020-07-03 6:43 ` Abhishek Bhardwaj 2020-07-03 11:40 ` Mike Rapoport 0 siblings, 1 reply; 15+ messages in thread From: Abhishek Bhardwaj @ 2020-07-03 6:43 UTC (permalink / raw) To: Anthony Steinhauser Cc: LKML, Waiman Long, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Mike Rapoport, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86, Doug Anderson 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-03 6:43 ` Abhishek Bhardwaj @ 2020-07-03 11:40 ` Mike Rapoport 2020-07-03 14:00 ` Doug Anderson 0 siblings, 1 reply; 15+ messages in thread From: Mike Rapoport @ 2020-07-03 11:40 UTC (permalink / raw) To: Abhishek Bhardwaj Cc: Anthony Steinhauser, LKML, Waiman Long, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86, Doug Anderson 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-03 11:40 ` Mike Rapoport @ 2020-07-03 14:00 ` Doug Anderson 2020-07-05 15:23 ` Mike Rapoport 0 siblings, 1 reply; 15+ messages in thread From: Doug Anderson @ 2020-07-03 14:00 UTC (permalink / raw) To: Mike Rapoport Cc: Abhishek Bhardwaj, Anthony Steinhauser, LKML, Waiman Long, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-03 14:00 ` Doug Anderson @ 2020-07-05 15:23 ` Mike Rapoport 2020-07-05 15:56 ` Waiman Long 2020-07-05 17:33 ` Doug Anderson 0 siblings, 2 replies; 15+ messages in thread From: Mike Rapoport @ 2020-07-05 15:23 UTC (permalink / raw) To: Doug Anderson Cc: Abhishek Bhardwaj, Anthony Steinhauser, LKML, Waiman Long, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-05 15:23 ` Mike Rapoport @ 2020-07-05 15:56 ` Waiman Long 2020-07-05 18:22 ` Abhishek Bhardwaj 2020-07-05 17:33 ` Doug Anderson 1 sibling, 1 reply; 15+ messages in thread From: Waiman Long @ 2020-07-05 15:56 UTC (permalink / raw) To: Mike Rapoport, Doug Anderson Cc: Abhishek Bhardwaj, Anthony Steinhauser, LKML, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-05 15:56 ` Waiman Long @ 2020-07-05 18:22 ` Abhishek Bhardwaj 2020-07-05 18:48 ` Waiman Long 0 siblings, 1 reply; 15+ messages in thread From: Abhishek Bhardwaj @ 2020-07-05 18:22 UTC (permalink / raw) To: Waiman Long Cc: Mike Rapoport, Doug Anderson, Anthony Steinhauser, LKML, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-05 18:22 ` Abhishek Bhardwaj @ 2020-07-05 18:48 ` Waiman Long 2020-07-05 21:51 ` Abhishek Bhardwaj 0 siblings, 1 reply; 15+ messages in thread From: Waiman Long @ 2020-07-05 18:48 UTC (permalink / raw) To: Abhishek Bhardwaj Cc: Mike Rapoport, Doug Anderson, Anthony Steinhauser, LKML, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-05 18:48 ` Waiman Long @ 2020-07-05 21:51 ` Abhishek Bhardwaj [not found] ` <f8e84764-1cc7-c4e5-4e4f-4b907204a374@redhat.com> 0 siblings, 1 reply; 15+ messages in thread From: Abhishek Bhardwaj @ 2020-07-05 21:51 UTC (permalink / raw) To: Waiman Long Cc: Mike Rapoport, Doug Anderson, Anthony Steinhauser, LKML, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <f8e84764-1cc7-c4e5-4e4f-4b907204a374@redhat.com>]
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode [not found] ` <f8e84764-1cc7-c4e5-4e4f-4b907204a374@redhat.com> @ 2020-07-08 19:32 ` Abhishek Bhardwaj 0 siblings, 0 replies; 15+ messages in thread From: Abhishek Bhardwaj @ 2020-07-08 19:32 UTC (permalink / raw) To: Waiman Long Cc: Mike Rapoport, Doug Anderson, Anthony Steinhauser, LKML, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-05 15:23 ` Mike Rapoport 2020-07-05 15:56 ` Waiman Long @ 2020-07-05 17:33 ` Doug Anderson 1 sibling, 0 replies; 15+ messages in thread From: Doug Anderson @ 2020-07-05 17:33 UTC (permalink / raw) To: Mike Rapoport Cc: Abhishek Bhardwaj, Anthony Steinhauser, LKML, Waiman Long, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross, Paolo Bonzini, Pawan Gupta, Peter Zijlstra, Sean Christopherson, Thomas Gleixner, Tony Luck, Vitaly Kuznetsov, Wanpeng Li, kvm, x86 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode 2020-07-02 22:12 [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode Abhishek Bhardwaj 2020-07-02 23:16 ` Randy Dunlap 2020-07-03 0:17 ` Waiman Long @ 2020-07-03 14:14 ` kernel test robot 2 siblings, 0 replies; 15+ messages in thread From: kernel test robot @ 2020-07-03 14:14 UTC (permalink / raw) To: Abhishek Bhardwaj, LKML Cc: kbuild-all, Abhishek Bhardwaj, Anthony Steinhauser, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Jim Mattson, Joerg Roedel, Josh Poimboeuf, Mark Gross [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-07-08 19:33 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-02 22:12 [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode Abhishek Bhardwaj 2020-07-02 23:16 ` Randy Dunlap 2020-07-03 0:17 ` Waiman Long 2020-07-03 3:17 ` Anthony Steinhauser 2020-07-03 6:43 ` Abhishek Bhardwaj 2020-07-03 11:40 ` Mike Rapoport 2020-07-03 14:00 ` Doug Anderson 2020-07-05 15:23 ` Mike Rapoport 2020-07-05 15:56 ` Waiman Long 2020-07-05 18:22 ` Abhishek Bhardwaj 2020-07-05 18:48 ` Waiman Long 2020-07-05 21:51 ` Abhishek Bhardwaj [not found] ` <f8e84764-1cc7-c4e5-4e4f-4b907204a374@redhat.com> 2020-07-08 19:32 ` Abhishek Bhardwaj 2020-07-05 17:33 ` Doug Anderson 2020-07-03 14:14 ` kernel test robot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).