On Tue, Feb 04, 2020 at 10:52:01AM +0800, Xiaoyao Li wrote:
> On 2/4/2020 5:42 AM, Sean Christopherson wrote:
> >On Mon, Feb 03, 2020 at 11:16:08PM +0800, Xiaoyao Li wrote:
> >>
> >>Only when host is sld_off, can guest control the hardware value of
> >>MSR_TEST_CTL, i.e., KVM loads guest's value into hardware when vcpu is
> >>running.
...
> Right, SLD is exposed to the guest only when host is sld_off makes thing
> much simpler. But this seems only meaning for using guest for debugging or
> testing?
Ah, I misunderstood. I thought the above quote was saying SLD would be
exposed to the guest if it's off in the host, i.e. intended only to reword
the changelog.
Per our offline discussion:
sld_fatal - MSR_TEST_CTL.SDL is forced on and is sticky from the guest's
perspective (so the guest can detect a forced fatal mode).
sld_warn - SLD is exposed to the guest. MSR_TEST_CTL.SDL is left on
until an #AC is intercepted with MSR_TEST_CTL.SDL=0 in the
guest, at which point normal sld_warn rules apply. If a vCPU
associated with the task does VM-Enter with MSR_TEST_CTL.SDL=1,
TIF_SLD is reset and the cycle begins anew.
sld_off - When set by the guest, MSR_TEST_CTL.SLD is set on VM-Entry
and cleared on VM-Exit.
Side topic, this means we need more than is_split_lock_detect_enabled(),
but it's probably still a good idea to hide the enum, e.g. have
is_sld_enabled() and is_sld_fatal() wrappers.
> >Reiterating everything that was implemented in previous patches does more
> >harm than good.
...
> >>@@ -1934,6 +1960,15 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >> u32 index;
> >> switch (msr_index) {
> >>+ case MSR_TEST_CTRL:
> >>+ if (!msr_info->host_initiated &&
> >>+ (!guest_has_feature_split_lock_detect(vcpu) ||
> >>+ data & ~vmx_msr_test_ctrl_valid_bits(vcpu)))
> >>+ return 1;
> >>+ if (data & MSR_TEST_CTRL_SPLIT_LOCK_DETECT)
> >>+ vmx->disable_split_lock_detect = false;
> >
> >Pretty sure disable_split_lock_detect won't exist, but if it does, don't
> >reuse it for emulating guest behavior. Keep the two things separate, i.e.
> >use vmx->msr_test_ctrl to track guest state and use the disable_sld to
> >track when the feature has been disabled for an ignorant guest.
>
> My thought was that when both host and guest are sld_warn.
> If there is a split lock in guest user space,
> 1. #AC trapped in kvm, and re-injected to guest due to guest's MSR bit set;
> 2. Guest clears MSR bit but hardware bit not cleared, re-execute the
> instruction
> 3. #AC trapped again, vmx->disable_sld set to true, vm-enter to guest with
> hardware MSR bit cleared, re-execute the instruction
> 4. After guest user space application finishes/ or scheduled, guest set MSR
> bit, here we'd better clear vmx->disable_sld, otherwise hardware MSR bit
> keeps cleared for this vcpu thread.
Ya, all that works. But I don't think KVM needs to context switch
MSR_TEST_CTRL in any mode except sld_off. For sld_fatal, it's simply on.
For sld_warn, it's only disabled when TIF_SLD=1, i.e. after a warning #AC.
I suppose there's a corner case where userspace is multiplexing vCPUs on
tasks, in which case we could end up with TIF_SLD=1 and MSR_TEST_CTRL.SLD=1.
KVM still doesn't need a separate flag, e.g.:
if (static_cpu_has(...) && vmx->msr_test_control) {
if (test_thread_flag(TIF_SLD))
sld_turn_back_on();
else if (!is_split_lock_detect_enabled())
wrmsrl(MSR_TEST_CTL,
this_cpu_read(msr_test_ctl_val) |
vmx->msr_test_ctl);
}
__vmx_vcpu_run();
if (static_cpu_has(...) && vmx->msr_test_control &&
!is_split_lock_detect_enabled())
wrmsrl(MSR_TEST_CTL, this_cpu_read(msr_test_ctl_val));
> Also, this makes a difference for guest user space application that when it
> scheduled out then scheduled in, the MSR bit is set again while in bare
> metal it keeps cleared. That's why I use pr_warn_ratelimited() in #AC
> interceptor.
