From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
Andy Lutomirski <luto@amacapital.net>,
x86@kernel.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org,
David Laight <David.Laight@aculab.com>
Subject: Re: [PATCH v2 6/6] x86: vmx: virtualize split lock detection
Date: Mon, 3 Feb 2020 21:35:52 -0800 [thread overview]
Message-ID: <20200204053552.GA31665@linux.intel.com> (raw)
In-Reply-To: <addf50c8-f683-9176-d6e4-51bc217dcc92@intel.com>
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.
prev parent reply other threads:[~2020-02-04 5:35 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-03 15:16 [PATCH v2 0/6] kvm/split_lock: Add feature split lock detection support in kvm Xiaoyao Li
2020-02-03 15:16 ` [PATCH v2 1/6] x86/split_lock: Add and export get_split_lock_detect_state() Xiaoyao Li
2020-02-03 21:45 ` Sean Christopherson
2020-02-03 15:16 ` [PATCH v2 2/6] x86/split_lock: Add and export split_lock_detect_set() Xiaoyao Li
2020-02-03 15:16 ` [PATCH v2 3/6] kvm: x86: Emulate split-lock access as a write Xiaoyao Li
2020-02-03 20:54 ` Sean Christopherson
2020-02-04 2:55 ` Xiaoyao Li
2020-02-11 12:20 ` Paolo Bonzini
2020-02-11 13:22 ` Thomas Gleixner
2020-02-11 13:34 ` Paolo Bonzini
2020-02-11 14:02 ` Xiaoyao Li
2020-02-11 14:34 ` David Laight
2020-02-27 0:11 ` Sean Christopherson
2020-03-12 11:42 ` Xiaoyao Li
2020-03-12 15:00 ` Paolo Bonzini
2020-02-03 15:16 ` [PATCH v2 4/6] kvm: vmx: Extend VMX's #AC handding for split lock in guest Xiaoyao Li
2020-02-03 21:14 ` Sean Christopherson
2020-02-04 6:46 ` Xiaoyao Li
2020-02-10 21:30 ` Sean Christopherson
2020-02-03 15:16 ` [PATCH v2 5/6] kvm: x86: Emulate MSR IA32_CORE_CAPABILITIES Xiaoyao Li
2020-02-03 21:43 ` Sean Christopherson
2020-02-04 9:19 ` Xiaoyao Li
2020-02-04 9:37 ` Peter Zijlstra
2020-02-11 3:52 ` Andy Lutomirski
2020-02-11 12:38 ` Peter Zijlstra
2020-02-03 15:16 ` [PATCH v2 6/6] x86: vmx: virtualize split lock detection Xiaoyao Li
2020-02-03 15:58 ` Xiaoyao Li
2020-02-03 18:52 ` Andy Lutomirski
2020-02-03 21:42 ` Sean Christopherson
2020-02-04 2:52 ` Xiaoyao Li
2020-02-04 5:35 ` Sean Christopherson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200204053552.GA31665@linux.intel.com \
--to=sean.j.christopherson@intel.com \
--cc=David.Laight@aculab.com \
--cc=bp@alien8.de \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
--cc=xiaoyao.li@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).