From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bandan Das Subject: Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS Date: Thu, 01 Oct 2015 18:31:27 -0400 Message-ID: References: <560D1C6E.2060803@suse.com> <560D278F.10801@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, Dirk =?utf-8?Q?M=C3=BCller?= , Joerg Roedel To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:59595 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750823AbbJAWb3 convert rfc822-to-8bit (ORCPT ); Thu, 1 Oct 2015 18:31:29 -0400 In-Reply-To: <560D278F.10801@redhat.com> (Paolo Bonzini's message of "Thu, 1 Oct 2015 14:31:11 +0200") Sender: kvm-owner@vger.kernel.org List-ID: Paolo Bonzini writes: > On 01/10/2015 13:43, Dirk M=C3=BCller wrote: >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index 94b7d15..0a42859 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm= _vcpu *vcpu) >> struct vcpu_svm *svm =3D to_svm(vcpu); >> =20 >> if (svm->vmcb->control.next_rip !=3D 0) { >> - WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS)); >> + WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS)); >> svm->next_rip =3D svm->vmcb->control.next_rip; >> } >> =20 > > Bandan, what was the reason for warning here? I added the warning so that we catch if the next_rip field is being wri= tten to (even if the feature isn't supported) by a buggy L1 hypervisor. =46rom the commit: =20 - if (svm->vmcb->control.next_rip !=3D 0) + if (svm->vmcb->control.next_rip !=3D 0) { + WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS)); svm->next_rip =3D svm->vmcb->control.next_rip; + } =20 if (!svm->next_rip) { if (emulate_instruction(vcpu, EMULTYPE_SKIP) !=3D @@ -4317,7 +4319,9 @@ static int svm_check_intercept(struct kvm_vcpu *v= cpu, break; } =20 - vmcb->control.next_rip =3D info->next_rip; + /* TODO: Advertise NRIPS to guest hypervisor unconditionally */ + if (static_cpu_has(X86_FEATURE_NRIPS)) + vmcb->control.next_rip =3D info->next_rip; vmcb->control.exit_code =3D icpt_info.exit_code; vmexit =3D nested_svm_exit_handled(svm); =2E.. > Should we change the "if" condition to static_cpu_has(X86_FEATURE_NRI= PS) > instead of Dirk's patch? Yes, seems ok to me. If decodeassist isn't supported then it's mostly a stale value. It's interesting that that L1 still works even after we hit this warning! > Paolo