Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Joerg Roedel <joro@8bytes.org>]

On Thu, Oct 01, 2015 at 06:31:27PM -0400, Bandan Das wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 01/10/2015 13:43, Dirk Müller 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 = to_svm(vcpu);
> >>  
> >>  	if (svm->vmcb->control.next_rip != 0) {
> >> -		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
> >> +		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
> >>  		svm->next_rip = svm->vmcb->control.next_rip;
> >>  	}
> >>  
> >
> > Bandan, what was the reason for warning here?
> 
> 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, we
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.

So what's the point in adding a guest-triggerable warning at all?



	Joerg