On Thu, Aug 08, 2019 at 07:30:46PM +0200, Vitaly Kuznetsov wrote: > On AMD, kvm_x86_ops->skip_emulated_instruction(vcpu) can, in theory, > fail: in !nrips case we call kvm_emulate_instruction(EMULTYPE_SKIP). > Currently, we only do printk(KERN_DEBUG) when this happens and this > is not ideal. Propagate the error up the stack. > > On VMX, skip_emulated_instruction() doesn't fail, we have two call > sites calling it explicitly: handle_exception_nmi() and > handle_task_switch(), we can just ignore the result. > > On SVM, we also have two explicit call sites: > svm_queue_exception() and it seems we don't need to do anything there as > we check if RIP was advanced or not. In task_switch_interception(), > however, we are better off not proceeding to kvm_task_switch() in case > skip_emulated_instruction() failed. > > Suggested-by: Sean Christopherson <sean.j.christopher...@intel.com> > Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com> > ---
... > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 074385c86c09..2579e7a6d59d 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1473,7 +1473,7 @@ static int vmx_rtit_ctl_check(struct kvm_vcpu *vcpu, > u64 data) > } > > > -static void skip_emulated_instruction(struct kvm_vcpu *vcpu) > +static int skip_emulated_instruction(struct kvm_vcpu *vcpu) > { > unsigned long rip; > > @@ -1483,6 +1483,8 @@ static void skip_emulated_instruction(struct kvm_vcpu > *vcpu) > > /* skipping an emulated instruction also counts */ > vmx_set_interrupt_shadow(vcpu, 0); > + > + return EMULATE_DONE; > } > > static void vmx_clear_hlt(struct kvm_vcpu *vcpu) > @@ -4547,7 +4549,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu) > vcpu->arch.dr6 &= ~DR_TRAP_BITS; > vcpu->arch.dr6 |= dr6 | DR6_RTM; > if (is_icebp(intr_info)) > - skip_emulated_instruction(vcpu); > + (void)skip_emulated_instruction(vcpu); > > kvm_queue_exception(vcpu, DB_VECTOR); > return 1; > @@ -5057,7 +5059,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu) > if (!idt_v || (type != INTR_TYPE_HARD_EXCEPTION && > type != INTR_TYPE_EXT_INTR && > type != INTR_TYPE_NMI_INTR)) > - skip_emulated_instruction(vcpu); > + (void)skip_emulated_instruction(vcpu); Maybe a silly idea, but what if we squash the return value in a dedicated helper, with a big "DO NOT USE" comment above the int-returning function, e.g.: static int __skip_emulated_instruction(struct kvm_vcpu *vcpu) { unsigned long rip; rip = kvm_rip_read(vcpu); rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN); kvm_rip_write(vcpu, rip); /* skipping an emulated instruction also counts */ vmx_set_interrupt_shadow(vcpu, 0); return EMULATE_DONE; } static inline void skip_emulated_instruction(struct kvm_vcpu *vcpu) { (void)__skip_emulated_instruction(vcpu); } Alternatively, the inner function could be void, but on my system that adds an extra call in the wrapper, i.e. in the kvm_skip_emulated...() path. The above approach generates the same code as your patch, e.g. allows the compiler to decide whether or not to inline the meat of the code. > if (kvm_task_switch(vcpu, tss_selector, > type == INTR_TYPE_SOFT_INTR ? idt_index : -1, > reason, > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index c6d951cbd76c..a97818b1111d 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6383,9 +6383,11 @@ static void kvm_vcpu_do_singlestep(struct kvm_vcpu > *vcpu, int *r) > int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu) > { > unsigned long rflags = kvm_x86_ops->get_rflags(vcpu); > - int r = EMULATE_DONE; > + int r; > > - kvm_x86_ops->skip_emulated_instruction(vcpu); > + r = kvm_x86_ops->skip_emulated_instruction(vcpu); > + if (r != EMULATE_DONE) This should probably be wrapped with unlikely. > + return 0; > > /* > * rflags is the old, "raw" value of the flags. The new value has > -- > 2.20.1 >