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: Mon, 05 Oct 2015 12:54:43 -0400 Message-ID: References: <560D1C6E.2060803@suse.com> <560D278F.10801@redhat.com> <20151005095022.GX3036@8bytes.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Paolo Bonzini , kvm@vger.kernel.org, Dirk =?utf-8?Q?M=C3=BCller?= To: Joerg Roedel Return-path: Received: from mx1.redhat.com ([209.132.183.28]:52724 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751740AbbJEQyp convert rfc822-to-8bit (ORCPT ); Mon, 5 Oct 2015 12:54:45 -0400 In-Reply-To: <20151005095022.GX3036@8bytes.org> (Joerg Roedel's message of "Mon, 5 Oct 2015 11:50:22 +0200") Sender: kvm-owner@vger.kernel.org List-ID: Joerg Roedel writes: > On Thu, Oct 01, 2015 at 06:31:27PM -0400, Bandan Das wrote: >> Paolo Bonzini writes: >>=20 >> > 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? >>=20 >> I added the warning so that we catch if the next_rip field is being = written >> to (even if the feature isn't supported) by a buggy L1 hypervisor. > > Even if the L1 hypervisor writes to the next_rip field in the VMCB, w= e > would never see it in this code path, as we access the shadow VMCB in > this statement. > > We don't even care if the L1 hypervisor writes to its next_rip field > because we only write to this field on an emulatated VMEXIT and never > read it back. The problems is that the next_rip field could be stale. If the processo= r supports next_rip, then it will clear it out on the next entry. If it doesn't, an old value just sits there (no matter who wrote it) and the problem happens when skip_emulated_instruction advances the rip with an incorre= ct value. > So what's the point in adding a guest-triggerable warning at all? So, yes, maybe this doesn't have to be a guest specific warning but we = still need to warn if this unsupported field is being written to. > > > Joerg