Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933164AbeAIP62 (ORCPT + 1 other); Tue, 9 Jan 2018 10:58:28 -0500 Received: from aserp2130.oracle.com ([141.146.126.79]:34438 "EHLO aserp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932850AbeAIP61 (ORCPT ); Tue, 9 Jan 2018 10:58:27 -0500 MIME-Version: 1.0 Message-ID: <901c4def-b0a4-42aa-9a88-cae3c3f64cf0@default> Date: Tue, 9 Jan 2018 07:58:09 -0800 (PST) From: Liran Alon To: Cc: , , , , , , , , Subject: Re: [PATCH 9/8] KVM: x86: limit MSR_IA32_SPEC_CTRL access based on CPUID availability X-Mailer: Zimbra on Oracle Beehive Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Content-Disposition: inline X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=8768 signatures=668652 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=1 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=925 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1801090226 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: ----- pbonzini@redhat.com wrote: > MSR_IA32_SPEC_CTRL is not available unless CPU[7,0].EDX[26] is 1. > Check that against host CPUID or guest CPUID, respectively for > host-initiated and guest-initiated accesses. > > Suggested-by: Jim Mattson > Signed-off-by: Paolo Bonzini > --- > This is for after X86_FEATURE_SPEC_CTRL is added to Linux, but > I still wanted to ack Jim's improvement. > > arch/x86/kvm/svm.c | 8 ++++++++ > arch/x86/kvm/vmx.c | 8 ++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 97126c2bd663..3a646580d7c5 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -3648,6 +3648,10 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, > struct msr_data *msr_info) > msr_info->data = svm->nested.vm_cr_msr; > break; > case MSR_IA32_SPEC_CTRL: > + if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) || > + (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))) > + return 1; > msr_info->data = svm->spec_ctrl; > break; > case MSR_IA32_UCODE_REV: > @@ -3806,6 +3810,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, > struct msr_data *msr) > vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, > data); > break; > case MSR_IA32_SPEC_CTRL: > + if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) || > + (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))) > + return 1; > svm->spec_ctrl = data; > break; > case MSR_IA32_APICBASE: > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 49b4a2d61603..42bc7ee293e4 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3368,6 +3368,10 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, > struct msr_data *msr_info) > msr_info->data = guest_read_tsc(vcpu); > break; > case MSR_IA32_SPEC_CTRL: > + if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) || > + (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))) > + return 1; > msr_info->data = to_vmx(vcpu)->spec_ctrl; > break; > case MSR_IA32_SYSENTER_CS: > @@ -3510,6 +3514,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, > struct msr_data *msr_info) > kvm_write_tsc(vcpu, msr_info); > break; > case MSR_IA32_SPEC_CTRL: > + if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) || > + (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))) > + return 1; > to_vmx(vcpu)->spec_ctrl = data; > break; > case MSR_IA32_CR_PAT: > -- > 1.8.3.1 Reviewed-by: Liran Alon The only thing I a bit dislike is that currently these MSRs are always pass-through to guest and therefore there is no case vmx_set_msr() is called with !msr_info->host_initiated. Don't you think we should BUG_ON(!msr_info->host_initiated)? -Liran