[Qemu-devel] [PATCH] target-i386: fix iret emulation correctness

[Qemu-devel] [PATCH] target-i386: fix iret emulation correctness

[poletaev]

From: Dmitry Poletaev <Dmitry.Poletaev@ispras.ru>

Subject: [PATCH] target-i386: fix iret emulation correctness

Signed-off-by: Dmitry Poletaev <Dmitry.Poletaev@ispras.ru>

 

According to Intel manual: "If the NMI handler is a virtual-8086 task with
an IOPL of less than 3, an IRET instruction issued from the handler
generates a general-protection

exception, the NMI is unmasked before the general-protection exception
handler is invoked."

 

QEMU does not reset NMI-blocking in such situation. This patch fixes it.

---

target-i386/cc_helper.c | 5 +++++

target-i386/helper.h    | 1 +

target-i386/translate.c | 1 +

3 files changed, 7 insertions(+)

 

diff --git a/target-i386/cc_helper.c b/target-i386/cc_helper.c

index 83af223..74851ae 100644

--- a/target-i386/cc_helper.c

+++ b/target-i386/cc_helper.c

@@ -383,3 +383,8 @@ void helper_sti_vm(CPUX86State *env)

     }

}

#endif

+

+void helper_reset_nmi_blocking(CPUX86State *env)

+{

+    env->hflags2 &= ~HF2_NMI_MASK;

+}

\ No newline at end of file

diff --git a/target-i386/helper.h b/target-i386/helper.h

index 1320edc..7718d2f 100644

--- a/target-i386/helper.h

+++ b/target-i386/helper.h

@@ -70,6 +70,7 @@ DEF_HELPER_1(cli, void, env)

DEF_HELPER_1(sti, void, env)

DEF_HELPER_1(clac, void, env)

DEF_HELPER_1(stac, void, env)

+DEF_HELPER_1(reset_nmi_blocking, void, env)

DEF_HELPER_3(boundw, void, env, tl, int)

DEF_HELPER_3(boundl, void, env, tl, int)

DEF_HELPER_1(rsm, void, env)

diff --git a/target-i386/translate.c b/target-i386/translate.c

index f010022..c409baf 100644

--- a/target-i386/translate.c

+++ b/target-i386/translate.c

@@ -6319,6 +6319,7 @@ static target_ulong disas_insn(CPUX86State *env,
DisasContext *s,

             set_cc_op(s, CC_OP_EFLAGS);

         } else if (s->vm86) {

             if (s->iopl != 3) {

+                gen_helper_reset_nmi_blocking(cpu_env);

                 gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);

             } else {

                 gen_helper_iret_real(cpu_env, tcg_const_i32(dflag - 1));

-- 

1.8.4.msysgit.0

 

 

Best regards, 

Dmitry Poletaev.

 

Re: [Qemu-devel] [PATCH] target-i386: fix iret emulation correctness

[Paolo Bonzini]

On 07/06/2016 10:07, poletaev wrote:
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index f010022..c409baf 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -6319,6 +6319,7 @@ static target_ulong disas_insn(CPUX86State *env,
> DisasContext *s,
>              set_cc_op(s, CC_OP_EFLAGS);
>          } else if (s->vm86) {
>              if (s->iopl != 3) {
> +                gen_helper_reset_nmi_blocking(cpu_env);
>                  gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);

I am afraid that the solution is more complicated.  The exception will
be handled before the NMI, while the opposite should be done according
to the manual.

So, first, you need to move HF2_NMI_MASK from hflags2 to hflags, so that
different NMI masking states cause the guest code to be retranslated.
Second, an IRET with HF_NMI_MASK set can be translated to _only_ the
reset of NMI mask followed by end of basic block.  An IRET without
HF_NMI_MASK instead can be translated the same way as now.

Paolo

> 
>              } else {
> 
>                  gen_helper_iret_real(cpu_env, tcg_const_i32(dflag - 1));
> 

Re: [Qemu-devel] [PATCH] target-i386: fix iret emulation correctness

[poletaev]

> Second, an IRET with HF_NMI_MASK set can be translated to _only_ the 
> reset of NMI mask followed by end of basic block.  An IRET without 
> HF_NMI_MASK instead can be translated the same way as now.

I want to make it like this, but it seems to me it can lead to zero tb size
and introduce bad side effect. What I do wrong?

case 0xcf: /* iret */
    gen_svm_check_intercept(s, pc_start, SVM_EXIT_IRET);
    if (!s->pe) {
        /* real mode */
        gen_helper_iret_real(cpu_env, tcg_const_i32(dflag - 1));
        set_cc_op(s, CC_OP_EFLAGS);
    } else if (s->vm86) {
        if (s->iopl != 3) {
            if (s->flags & HF_NMI_MASK) {
                gen_reset_hflag(s, HF_NMI_MASK);
                s->pc = pc_start;
                gen_jmp_im(pc_start - s->cs_base);
            } else {
                gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
            }
        } else {
            gen_helper_iret_real(cpu_env, tcg_const_i32(dflag - 1));
            set_cc_op(s, CC_OP_EFLAGS);
        }
    } else {
        gen_helper_iret_protected(cpu_env, tcg_const_i32(dflag - 1),
                                  tcg_const_i32(s->pc - s->cs_base));
        set_cc_op(s, CC_OP_EFLAGS);
    }
    gen_eob(s);
    break;

Best regards, 
Dmitry Poletaev.

Re: [Qemu-devel] [PATCH] target-i386: fix iret emulation correctness

[Paolo Bonzini]

On 07/06/2016 14:07, poletaev wrote:
>> Second, an IRET with HF_NMI_MASK set can be translated to _only_ the 
>> reset of NMI mask followed by end of basic block.  An IRET without 
>> HF_NMI_MASK instead can be translated the same way as now.
> 
> I want to make it like this, but it seems to me it can lead to zero tb size
> and introduce bad side effect. What I do wrong?
> 
> case 0xcf: /* iret */
>     gen_svm_check_intercept(s, pc_start, SVM_EXIT_IRET);
>     if (!s->pe) {
>         /* real mode */
>         gen_helper_iret_real(cpu_env, tcg_const_i32(dflag - 1));
>         set_cc_op(s, CC_OP_EFLAGS);
>     } else if (s->vm86) {
>         if (s->iopl != 3) {
>             if (s->flags & HF_NMI_MASK) {
>                 gen_reset_hflag(s, HF_NMI_MASK);
>                 s->pc = pc_start;
>                 gen_jmp_im(pc_start - s->cs_base);
>             } else {
>                 gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
>             }
>         } else {
>             gen_helper_iret_real(cpu_env, tcg_const_i32(dflag - 1));
>             set_cc_op(s, CC_OP_EFLAGS);
>         }
>     } else {
>         gen_helper_iret_protected(cpu_env, tcg_const_i32(dflag - 1),
>                                   tcg_const_i32(s->pc - s->cs_base));
>         set_cc_op(s, CC_OP_EFLAGS);
>     }
>     gen_eob(s);
>     break;

I think it's okay to make it count as a one instruction tb, just like a
tb with a "jmp ." instruction.

Thanks,

Paolo