Messages in this thread | | | From | Arvind Sankar <> | Date | Wed, 22 Jan 2020 19:45:08 -0500 | Subject | Re: [PATCH v12] x86/split_lock: Enable split lock detection by kernel |
| |
On Wed, Jan 22, 2020 at 11:24:34PM +0000, Luck, Tony wrote: > >> +static enum split_lock_detect_state sld_state = sld_warn; > >> + > > > > This sets sld_state to sld_warn even on CPUs that don't support > > split-lock detection. split_lock_init will then try to read/write the > > MSR to turn it on. Would it be better to initialize it to sld_off and > > set it to sld_warn in split_lock_setup instead, which is only called if > > the CPU supports the feature? > > I've lost some bits of this patch series somewhere along the way :-( There > was once code to decide whether the feature was supported (either with > x86_match_cpu() for a couple of models, or using the architectural test > based on some MSR bits. I need to dig that out and put it back in. Then > stuff can check X86_FEATURE_SPLIT_LOCK before wandering into code > that messes with MSRs
That code is still there (cpu_set_core_cap_bits). The issue is that with the initialization here, nothing ever sets sld_state to sld_off if the feature isn't supported.
v10 had a corresponding split_lock_detect_enabled that was 0-initialized, but Peter's patch as he sent out had the flag initialized to sld_warn.
> > >> + if (!split_lock_detect_enabled()) > >> + return; > > > > This misses one comment from Sean [1] that this check should be dropped, > > otherwise user-space alignment check via EFLAGS.AC will get ignored when > > split lock detection is disabled. > > Ah yes. Good catch. Will fix. > > Thanks for the review. > > -Tony
| |