Re: [PATCH v3 2/8] x86/split_lock: Ensure X86_FEATURE_SPLIT_LOCK_DETECT means the existence of feature

[Sean Christopherson]

On Wed, Mar 04, 2020 at 09:49:14AM +0800, Xiaoyao Li wrote:
> On 3/4/2020 3:41 AM, Sean Christopherson wrote:
> >On Tue, Mar 03, 2020 at 10:55:24AM -0800, Sean Christopherson wrote:
> >>On Thu, Feb 06, 2020 at 03:04:06PM +0800, Xiaoyao Li wrote:
> >>>When flag X86_FEATURE_SPLIT_LOCK_DETECT is set, it should ensure the
> >>>existence of MSR_TEST_CTRL and MSR_TEST_CTRL.SPLIT_LOCK_DETECT bit.
> >>
> >>The changelog confused me a bit.  "When flag X86_FEATURE_SPLIT_LOCK_DETECT
> >>is set" makes it sound like the logic is being applied after the feature
> >>bit is set.  Maybe something like:
> >>
> >>```
> >>Verify MSR_TEST_CTRL.SPLIT_LOCK_DETECT can be toggled via WRMSR prior to
> >>setting the SPLIT_LOCK_DETECT feature bit so that runtime consumers,
> >>e.g. KVM, don't need to worry about WRMSR failure.
> >>```
> >>
> >>>Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> >>>---
> >>>  arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++----------------
> >>>  1 file changed, 23 insertions(+), 18 deletions(-)
> >>>
> >>>diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> >>>index 2b3874a96bd4..49535ed81c22 100644
> >>>--- a/arch/x86/kernel/cpu/intel.c
> >>>+++ b/arch/x86/kernel/cpu/intel.c
> >>>@@ -702,7 +702,8 @@ static void init_intel(struct cpuinfo_x86 *c)
> >>>  	if (tsx_ctrl_state == TSX_CTRL_DISABLE)
> >>>  		tsx_disable();
> >>>-	split_lock_init();
> >>>+	if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> >>>+		split_lock_init();
> >>>  }
> >>>  #ifdef CONFIG_X86_32
> >>>@@ -986,9 +987,26 @@ static inline bool match_option(const char *arg, int arglen, const char *opt)
> >>>  static void __init split_lock_setup(void)
> >>>  {
> >>>+	u64 test_ctrl_val;
> >>>  	char arg[20];
> >>>  	int i, ret;
> >>>+	/*
> >>>+	 * Use the "safe" versions of rdmsr/wrmsr here to ensure MSR_TEST_CTRL
> >>>+	 * and MSR_TEST_CTRL.SPLIT_LOCK_DETECT bit do exist. Because there may
> >>>+	 * be glitches in virtualization that leave a guest with an incorrect
> >>>+	 * view of real h/w capabilities.
> >>>+	 */
> >>>+	if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val))
> >>>+		return;
> >>>+
> >>>+	if (wrmsrl_safe(MSR_TEST_CTRL,
> >>>+			test_ctrl_val | MSR_TEST_CTRL_SPLIT_LOCK_DETECT))
> >>>+		return;
> >>>+
> >>>+	if (wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val))
> >>>+		return;a
> >>
> >>Probing the MSR should be skipped if SLD is disabled in sld_options, i.e.
> >>move this code (and setup_force_cpu_cap() etc...) down below the
> >>match_option() logic.  The above would temporarily enable SLD even if the
> >>admin has explicitly disabled it, e.g. makes the kernel param useless for
> >>turning off the feature due to bugs.
> >
> >Hmm, but this prevents KVM from exposing SLD to a guest when it's off in
> >the kernel, which would be a useful debug/testing scenario.
> >
> >Maybe add another SLD state to forcefully disable SLD?  That way the admin
> >can turn of SLD in the host kernel but still allow KVM to expose it to its
> >guests.  E.g.
> 
> I don't think we need do this.
> 
> IMO, this a the bug of split_lock_init(), which assume the initial value of
> MSR_TEST_CTRL is zero, at least bit SPLIT_LOCK of which is zero.
> This is problem, it's possible that BIOS has set this bit.

Hmm, yeah, that's a bug.  But it's a separate bug.
 
> split_lock_setup() here, is to check if the feature really exists. So
> probing MSR_TEST_CTRL and bit MSR_TEST_CTRL_SPLIT_LOCK_DETECT here. If there
> all exist, setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT) to indicate
> feature does exist.
> Only when feature exists, there is a need to parse the command line config
> of split_lock_detect.

Toggling SPLIT_LOCK before checking the kernel param is bad behavior, e.g.
if someone has broken silicon that causes explosions if SPLIT_LOCK=1.  The
behavior is especially bad because cpu_set_core_cap_bits() enumerates split
lock detection using FMS, i.e. clearcpuid to kill CORE_CAPABILITIES
wouldn't work either.