lkml.org 
[lkml]   [2020]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 8/8] x86: vmx: virtualize split lock detection
    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;

    \
     
     \ /
      Last update: 2020-03-05 17:50    [W:3.979 / U:0.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site