Re: [PATCHv2] KVM: inject #UD if instruction emulation fails and exit to userspace

[Mohammed Gamal <m.gamal005@gmail.com>]

On Mon, May 10, 2010 at 1:25 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Mon, May 10, 2010 at 11:16:56AM +0300, Gleb Natapov wrote:
>> Do not kill VM when instruction emulation fails. Inject #UD and report
>> failure to userspace instead. Userspace may choose to reenter guest if
>> vcpu is in userspace (cpl == 3) in which case guest OS will kill
>> offending process and continue running.
>>

I am curious to know what'd happen in case the vcpu is in kernel space
(cpl == 0). Is that case handled?

> Please use this one instead. Compilation warning fixed.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ed48904..5aa0944 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -575,7 +575,6 @@ enum emulation_result {
>  #define EMULTYPE_SKIP              (1 << 2)
>  int emulate_instruction(struct kvm_vcpu *vcpu,
>                        unsigned long cr2, u16 error_code, int emulation_type);
> -void kvm_report_emulation_failure(struct kvm_vcpu *cvpu, const char *context);
>  void realmode_lgdt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
>  void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 95bee9d..41d2de8 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2788,11 +2788,8 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code)
>                return 1;
>        case EMULATE_DO_MMIO:
>                ++vcpu->stat.mmio_exits;
> -               return 0;
> +               /* fall through */
>        case EMULATE_FAIL:
> -               vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -               vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> -               vcpu->run->internal.ndata = 0;
>                return 0;
>        default:
>                BUG();
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index cea916f..58c91f5 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1449,7 +1449,7 @@ static int io_interception(struct vcpu_svm *svm)
>        string = (io_info & SVM_IOIO_STR_MASK) != 0;
>        in = (io_info & SVM_IOIO_TYPE_MASK) != 0;
>        if (string || in)
> -               return !(emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO);
> +               return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
>
>        port = io_info >> 16;
>        size = (io_info & SVM_IOIO_SIZE_MASK) >> SVM_IOIO_SIZE_SHIFT;
> @@ -2300,16 +2300,12 @@ static int iret_interception(struct vcpu_svm *svm)
>
>  static int invlpg_interception(struct vcpu_svm *svm)
>  {
> -       if (emulate_instruction(&svm->vcpu, 0, 0, 0) != EMULATE_DONE)
> -               pr_unimpl(&svm->vcpu, "%s: failed\n", __func__);
> -       return 1;
> +       return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
>  }
>
>  static int emulate_on_interception(struct vcpu_svm *svm)
>  {
> -       if (emulate_instruction(&svm->vcpu, 0, 0, 0) != EMULATE_DONE)
> -               pr_unimpl(&svm->vcpu, "%s: failed\n", __func__);
> -       return 1;
> +       return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
>  }
>
>  static int cr8_write_interception(struct vcpu_svm *svm)
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 777e00d..e35c479 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3074,7 +3074,7 @@ static int handle_io(struct kvm_vcpu *vcpu)
>        ++vcpu->stat.io_exits;
>
>        if (string || in)
> -               return !(emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO);
> +               return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
>
>        port = exit_qualification >> 16;
>        size = (exit_qualification & 7) + 1;
> @@ -3331,22 +3331,7 @@ static int handle_wbinvd(struct kvm_vcpu *vcpu)
>
>  static int handle_apic_access(struct kvm_vcpu *vcpu)
>  {
> -       unsigned long exit_qualification;
> -       enum emulation_result er;
> -       unsigned long offset;
> -
> -       exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> -       offset = exit_qualification & 0xffful;
> -
> -       er = emulate_instruction(vcpu, 0, 0, 0);
> -
> -       if (er !=  EMULATE_DONE) {
> -               printk(KERN_ERR
> -                      "Fail to handle apic access vmexit! Offset is 0x%lx\n",
> -                      offset);
> -               return -ENOEXEC;
> -       }
> -       return 1;
> +       return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
>  }
>
>  static int handle_task_switch(struct kvm_vcpu *vcpu)
> @@ -3558,13 +3543,8 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
>                        goto out;
>                }
>
> -               if (err != EMULATE_DONE) {
> -                       vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -                       vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> -                       vcpu->run->internal.ndata = 0;
> -                       ret = 0;
> -                       goto out;
> -               }
> +               if (err != EMULATE_DONE)
> +                       return 0;
>
>                if (signal_pending(current))
>                        goto out;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cd8a606..eb845cf 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3587,24 +3587,6 @@ int emulator_set_dr(int dr, unsigned long value, struct kvm_vcpu *vcpu)
>        return __kvm_set_dr(vcpu, dr, value);
>  }
>
> -void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)
> -{
> -       u8 opcodes[4];
> -       unsigned long rip = kvm_rip_read(vcpu);
> -       unsigned long rip_linear;
> -
> -       if (!printk_ratelimit())
> -               return;
> -
> -       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
> -
> -       kvm_read_guest_virt(rip_linear, (void *)opcodes, 4, vcpu, NULL);
> -
> -       printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02x %02x %02x\n",
> -              context, rip, opcodes[0], opcodes[1], opcodes[2], opcodes[3]);
> -}
> -EXPORT_SYMBOL_GPL(kvm_report_emulation_failure);
> -
>  static u64 mk_cr_64(u64 curr_cr, u32 new_val)
>  {
>        return (curr_cr & ~((1ULL << 32) - 1)) | new_val;
> @@ -3811,6 +3793,17 @@ static void inject_emulated_exception(struct kvm_vcpu *vcpu)
>                kvm_queue_exception(vcpu, ctxt->exception);
>  }
>
> +static int handle_emulation_failure(struct kvm_vcpu *vcpu)
> +{
> +       ++vcpu->stat.insn_emulation_fail;
> +       trace_kvm_emulate_insn_failed(vcpu);
> +       vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +       vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> +       vcpu->run->internal.ndata = 0;
> +       kvm_queue_exception(vcpu, UD_VECTOR);
> +       return EMULATE_FAIL;
> +}
> +
>  int emulate_instruction(struct kvm_vcpu *vcpu,
>                        unsigned long cr2,
>                        u16 error_code,
> @@ -3879,11 +3872,11 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>
>                ++vcpu->stat.insn_emulation;
>                if (r)  {
> -                       ++vcpu->stat.insn_emulation_fail;
> -                       trace_kvm_emulate_insn_failed(vcpu);
>                        if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
>                                return EMULATE_DONE;
> -                       return EMULATE_FAIL;
> +                       if (emulation_type & EMULTYPE_SKIP)
> +                               return EMULATE_FAIL;
> +                       return handle_emulation_failure(vcpu);
>                }
>        }
>
> @@ -3908,9 +3901,7 @@ restart:
>                if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
>                        return EMULATE_DONE;
>
> -               trace_kvm_emulate_insn_failed(vcpu);
> -               kvm_report_emulation_failure(vcpu, "mmio");
> -               return EMULATE_FAIL;
> +               return handle_emulation_failure(vcpu);
>        }
>
>        toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
> @@ -4746,7 +4737,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>                vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>                r = emulate_instruction(vcpu, 0, 0, EMULTYPE_NO_DECODE);
>                srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> -               if (r == EMULATE_DO_MMIO) {
> +               if (r != EMULATE_DONE) {
>                        r = 0;
>                        goto out;
>                }
> --
>                        Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>