Re: [PATCH 1/1] x86/split_lock: check split lock feature on initialization

[Xiaoyao Li]

On 4/6/2020 4:23 PM, Benjamin Lamowski wrote:
> [...]
> > Calling split_lock_verify_msr() with X86_FEATURE_SPLIT_LOCK_DETECT=0 is
> > intentional, the idea is to ensure SLD is disabled on all CPUs, e.g. in the
> > unlikely scenario that BIOS enabled SLD.
>
> I was aware of the intention, but I still don't understand under which
> scenario this could be set by the BIOS although the earlier feature
> detection has failed, 

It's for the case that SLD is explicitly disabled by kernel params 
"split_lock_detect=off". You know, BIOS may turn SLD on for itself. So 
if user uses "split_lock_detect=off", we have to clear the SLD bit in 
case BIOS forgets to clear it.

> i.e. shouldn't the feature have been detected in
> all cases where SLD can actually be disabled? If so, checking for
> availability first instead of catching a #GP(0) if it is not implemented
> seems to be the cleaner solution.

I understand what you want. i.e., X86_FEATURE_SPLIT_LOCK_DETECT is 
independent from sld_off. I guess you have to make tglx accept it first.

> > The first rdmsrl_safe() should short circuit split_lock_verify_msr() if
> > the RDMSR faults, i.e. it might fault, but it shouldn't WARN.  Are you
> > seeing issues or was this found via code inspection?
>
> We stumbled across this issue because the x86 version of our VMM is not
> yet ready for production and when accessing unimplemented MSRs would
> simply return 0 and issue a warning. This is of course a deviation from
> rdmsr as defined in the SDM and the pieces are ours to keep, it will be
> changed to generate a #GP(0) once the last missing MSRs are emulated
> properly.

Got it. you are running linux guest in your own VMM and there is warning 
in the VMM.

If you really want to avoid the MSR access on the platform without SLD. 
You could make the default sld_state as sld_unsupported. It can only be 
changed to other value in split_lock_setup() when SLD is enumerated. So 
in split_lock_init(), we can use if (sld_state == sld_unsupported) to 
skip the MSR_TEST_CTRL access.