Joerg Roedel <[email protected]> writes: > On Thu, Oct 01, 2015 at 06:31:27PM -0400, Bandan Das wrote: >> Paolo Bonzini <[email protected]> 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.
The problems is that the next_rip field could be stale. If the processor 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 incorrect 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
