Re: [PATCH v3 8/8] x86: vmx: virtualize split lock detection

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Thu, Mar 05, 2020 at 10:16:40PM +0800, Xiaoyao Li wrote:
> On 3/4/2020 3:30 AM, Sean Christopherson wrote:
> >On Thu, Feb 06, 2020 at 03:04:12PM +0800, Xiaoyao Li wrote:
> >>--- a/arch/x86/kvm/vmx/vmx.c
> >>+++ b/arch/x86/kvm/vmx/vmx.c
> >>@@ -1781,6 +1781,25 @@ static int vmx_get_msr_feature(struct kvm_msr_entry *msr)
> >>  	}
> >>  }
> >>+/*
> >>+ * Note: for guest, feature split lock detection can only be enumerated through
> >>+ * MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT bit. The FMS enumeration is invalid.
> >>+ */
> >>+static inline bool guest_has_feature_split_lock_detect(struct kvm_vcpu *vcpu)
> >>+{
> >>+	return vcpu->arch.core_capabilities & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT;
> >>+}
> >>+
> >>+static inline u64 vmx_msr_test_ctrl_valid_bits(struct kvm_vcpu *vcpu)
> >>+{
> >>+	u64 valid_bits = 0;
> >>+
> >>+	if (guest_has_feature_split_lock_detect(vcpu))
> >>+		valid_bits |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
> >>+
> >>+	return valid_bits;
> >>+}
> >>+
> >>  /*
> >>   * Reads an msr value (of 'msr_index') into 'pdata'.
> >>   * Returns 0 on success, non-0 otherwise.
> >>@@ -1793,6 +1812,12 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >>  	u32 index;
> >>  	switch (msr_info->index) {
> >>+	case MSR_TEST_CTRL:
> >>+		if (!msr_info->host_initiated &&
> >>+		    !guest_has_feature_split_lock_detect(vcpu))
> >>+			return 1;
> >>+		msr_info->data = vmx->msr_test_ctrl;
> >>+		break;
> >>  #ifdef CONFIG_X86_64
> >>  	case MSR_FS_BASE:
> >>  		msr_info->data = vmcs_readl(GUEST_FS_BASE);
> >>@@ -1934,6 +1959,13 @@ 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 &&
> >
> >Host initiated writes need to be validated against
> >kvm_get_core_capabilities(), otherwise userspace can enable SLD when it's
> >supported in hardware and the kernel, but can't be safely exposed to the
> >guest due to SMT being on.
> 
> How about making the whole check like this:
> 
> 	if (!msr_info->host_initiated &&
> 	    (!guest_has_feature_split_lock_detect(vcpu))
> 		return 1;
> 
> 	if (data & ~vmx_msr_test_ctrl_valid_bits(vcpu))

Whoops, the check on kvm_get_core_capabilities() should be done in
"case MSR_IA32_CORE_CAPS:", i.e. KVM shouldn't let host userspace advertise
split-lock support unless it's allowed by KVM.

Then this code doesn't need to do a check on host_initiated=true.

Back to the original code, I don't think we need to make the existence of
MSR_TEST_CTRL dependent on guest_has_feature_split_lock_detect(), i.e. this
check can simply be:

	if (!msr_info->host_initiated &&
	    (data & ~vmx_msr_test_ctrl_valid_bits(vcpu)))
		return 1;

and vmx_get_msr() doesn't need to check anything, i.e. RDMSR always
succeeds.  This is actually aligned with real silicon behavior because
MSR_TEST_CTRL exists on older processors, it's just wasn't documented until
we decided to throw in SPLIT_LOCK_AC, e.g. the LOCK# suppression bit is
marked for deprecation in the SDM, which wouldn't be necessary if it didn't
exist :-)

  Intel ISA/Feature                          Year of Removal
  TEST_CTRL MSR, bit 31 (MSR address 33H)    2019 onwards

  31 Disable LOCK# assertion for split locked access

On my Haswell box:

  $ rdmsr 0x33
  0
  $ wrmsr 0x33 0x20000000
  wrmsr: CPU 0 cannot set MSR 0x00000033 to 0x0000000020000000
  $ wrmsr 0x33 0x80000000
  $ rdmsr 0x33
  80000000
  $ wrmsr 0x33 0x00000000
  $ rdmsr 0x33
  0

That way the guest_has_feature_split_lock_detect() helper isn't needed
since its only user is vmx_msr_test_ctrl_valid_bits(), i.e. it can be
open coded there.

> >>+		    (!guest_has_feature_split_lock_detect(vcpu) ||
> >>+		     data & ~vmx_msr_test_ctrl_valid_bits(vcpu)))
> >>+			return 1;
> >>+		vmx->msr_test_ctrl = data;
> m>+		break;