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.christopherson@xxxxxxxxx>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
...
> 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
>
