Re: [SeaBIOS] [PATCH] x86emu: Correctly handle 0x66 prefix for some instructions

[Guillem Jover]

Hi!

On Mon, 2012-03-05 at 17:49:08 +0000, Julian Pidancet wrote:
> diff --git a/hw/xfree86/x86emu/ops.c b/hw/xfree86/x86emu/ops.c
> index 5d3cac1..440b8dc 100644
> --- a/hw/xfree86/x86emu/ops.c
> +++ b/hw/xfree86/x86emu/ops.c
> @@ -8787,11 +8795,16 @@ static void x86emuOp_enter(u8 X86EMU_UNUSED(op1))
>      frame_pointer = M.x86.R_SP;
>      if (nesting > 0) {
>          for (i = 1; i < nesting; i++) {
> -            M.x86.R_BP -= 2;
> -            push_word(fetch_data_word_abs(M.x86.R_SS, M.x86.R_BP));
> +            if (M.x86.mode & SYSMODE_PREFIX_DATA) {
> +                M.x86.R_EBP -= 4;
> +                push_long(fetch_data_long_abs(M.x86.R_SS, M.x86.R_BP));

Shouldn't this be:

  push_long(fetch_data_long_abs(M.x86.R_SS, M.x86.R_EBP))

?
> +            } else {
> +                M.x86.R_BP -= 2;
> +                push_word(fetch_data_word_abs(M.x86.R_SS, M.x86.R_BP));
>              }
> -        push_word(frame_pointer);
>          }
> +        push_word(frame_pointer);
> +    }
>      M.x86.R_BP = frame_pointer;
>      M.x86.R_SP = (u16)(M.x86.R_SP - local);
>      DECODE_CLEAR_SEGOVR();
> @@ -8827,8 +8844,13 @@ static void x86emuOp_ret_far_IMM(u8 X86EMU_UNUSED(op1))
>      DECODE_PRINTF2("%x\n", imm);
>   RETURN_TRACE("RETF",M.x86.saved_cs,M.x86.saved_ip);
>   TRACE_AND_STEP();
> -    M.x86.R_IP = pop_word();
> -    M.x86.R_CS = pop_word();
> +    if (M.x86.mode & SYSMODE_PREFIX_DATA) {
> +        M.x86.R_IP = pop_long();
> +        M.x86.R_CS = pop_long() & 0xffff;
> +    } else {
> +        M.x86.R_IP = pop_word();
> +        M.x86.R_CS = pop_word();
> +    }
>      M.x86.R_SP += imm;
>      DECODE_CLEAR_SEGOVR();
>      END_OF_INSTR();
> @@ -8844,8 +8866,13 @@ static void x86emuOp_ret_far(u8 X86EMU_UNUSED(op1))
>      DECODE_PRINTF("RETF\n");
>   RETURN_TRACE("RETF",M.x86.saved_cs,M.x86.saved_ip);
>   TRACE_AND_STEP();
> -    M.x86.R_IP = pop_word();
> -    M.x86.R_CS = pop_word();
> +    if (M.x86.mode & SYSMODE_PREFIX_DATA) {
> +        M.x86.R_IP = pop_long();
> +        M.x86.R_CS = pop_long() & 0xffff;
> +    } else {
> +        M.x86.R_IP = pop_word();
> +        M.x86.R_CS = pop_word();
> +    }
>      DECODE_CLEAR_SEGOVR();
>      END_OF_INSTR();
>  }
> @@ -8939,9 +8966,15 @@ static void x86emuOp_iret(u8 X86EMU_UNUSED(op1))
>
> TRACE_AND_STEP();
>
> -    M.x86.R_IP = pop_word();
> -    M.x86.R_CS = pop_word();
> -    M.x86.R_FLG = pop_word();
> +    if (M.x86.mode & SYSMODE_PREFIX_DATA) {
> +        M.x86.R_IP = pop_long();
> +        M.x86.R_CS = pop_long() & 0xffff;
> +        M.x86.R_FLG = pop_long();
> +    } else {
> +        M.x86.R_IP = pop_word();
> +        M.x86.R_CS = pop_word();
> +        M.x86.R_FLG = pop_word();
> +    }
>      DECODE_CLEAR_SEGOVR();
>      END_OF_INSTR();
>  }

On these three hunks, when on 32-bit mode shouldn't the registers be
M.x86.R_EIP and M.x86.R_EFLG?

regards,
guillem

[PATCH] x86emu: Correctly handle 0x66 prefix for some instructions

[Julian Pidancet]

On Wed, Mar 7, 2012 at 1:46 PM, Guillem Jover <[e-mail]> wrote:
> Hi!
>
> On Mon, 2012-03-05 at 17:49:08 +0000, Julian Pidancet wrote:
> > diff --git a/hw/xfree86/x86emu/ops.c b/hw/xfree86/x86emu/ops.c
> > index 5d3cac1..440b8dc 100644
> > --- a/hw/xfree86/x86emu/ops.c
> > +++ b/hw/xfree86/x86emu/ops.c
> > @@ -8787,11 +8795,16 @@ static void x86emuOp_enter(u8 X86EMU_UNUSED(op1))
> >      frame_pointer = M.x86.R_SP;
> >      if (nesting > 0) {
> >          for (i = 1; i < nesting; i++) {
> > -            M.x86.R_BP -= 2;
> > -            push_word(fetch_data_word_abs(M.x86.R_SS, M.x86.R_BP));
> > +            if (M.x86.mode & SYSMODE_PREFIX_DATA) {
> > +                M.x86.R_EBP -= 4;
> > +                push_long(fetch_data_long_abs(M.x86.R_SS, M.x86.R_BP));
>
> Shouldn't this be:
>
>  push_long(fetch_data_long_abs(M.x86.R_SS, M.x86.R_EBP))
>
> ?

> From the Intel® 64 and IA-32 Architectures Software Developer Manuals,
> here is the pseudo code associated with the enter instruction:

[...]
IF (NestingLevel > 1)
    THEN FOR i <- 1 to (NestingLevel - 1)
        DO
            IF 64-Bit Mode (StackSize = 64)
                THEN
                    RBP <- RBP - 8;
                    Push([RBP]); (* Quadword push *)
                ELSE IF OperandSize = 32
                    THEN
                        IF StackSize = 32
                            EBP <- EBP - 4;
                            Push([EBP]); (* Doubleword push *)
                        ELSE (* StackSize = 16 *)
                            BP <- BP - 4;
                            Push([BP]); (* Doubleword push *)
                        FI;
                    FI;
                ELSE (* OperandSize = 16 *)
                    IF StackSize = 32
                        THEN
                            EBP <- EBP - 2;
                            Push([EBP]); (* Word push *)
                        ELSE (* StackSize = 16 *)
                            BP <- BP - 2;
                            Push([BP]); (* Word push *)
                    FI;
            FI;
        OD;
FI;
[...]

I believe the 0x66 prefix only affects OperandSize, not StackSize. So
according to the manual, it should be BP, not EBP.
In any case, It won't be a problem, because the 16 high bits of EBP
will most likely be zero in real-mode code.
> > +            } else {
> > +                M.x86.R_BP -= 2;
> > +                push_word(fetch_data_word_abs(M.x86.R_SS, M.x86.R_BP));
> >              }
> > -        push_word(frame_pointer);
> >          }
> > +        push_word(frame_pointer);
> > +    }
> >      M.x86.R_BP = frame_pointer;
> >      M.x86.R_SP = (u16)(M.x86.R_SP - local);
> >      DECODE_CLEAR_SEGOVR();
> > @@ -8827,8 +8844,13 @@ static void x86emuOp_ret_far_IMM(u8 X86EMU_UNUSED(op1))
> >      DECODE_PRINTF2("%x\n", imm);
> >       RETURN_TRACE("RETF",M.x86.saved_cs,M.x86.saved_ip);
> >       TRACE_AND_STEP();
> > -    M.x86.R_IP = pop_word();
> > -    M.x86.R_CS = pop_word();
> > +    if (M.x86.mode & SYSMODE_PREFIX_DATA) {
> > +        M.x86.R_IP = pop_long();
> > +        M.x86.R_CS = pop_long() & 0xffff;
> > +    } else {
> > +        M.x86.R_IP = pop_word();
> > +        M.x86.R_CS = pop_word();
> > +    }
> >      M.x86.R_SP += imm;
> >      DECODE_CLEAR_SEGOVR();
> >      END_OF_INSTR();
> > @@ -8844,8 +8866,13 @@ static void x86emuOp_ret_far(u8 X86EMU_UNUSED(op1))
> >      DECODE_PRINTF("RETF\n");
> >       RETURN_TRACE("RETF",M.x86.saved_cs,M.x86.saved_ip);
> >       TRACE_AND_STEP();
> > -    M.x86.R_IP = pop_word();
> > -    M.x86.R_CS = pop_word();
> > +    if (M.x86.mode & SYSMODE_PREFIX_DATA) {
> > +        M.x86.R_IP = pop_long();
> > +        M.x86.R_CS = pop_long() & 0xffff;
> > +    } else {
> > +        M.x86.R_IP = pop_word();
> > +        M.x86.R_CS = pop_word();
> > +    }
> >      DECODE_CLEAR_SEGOVR();
> >      END_OF_INSTR();
> >  }
> > @@ -8939,9 +8966,15 @@ static void x86emuOp_iret(u8 X86EMU_UNUSED(op1))
> >
> >      TRACE_AND_STEP();
> >
> > -    M.x86.R_IP = pop_word();
> > -    M.x86.R_CS = pop_word();
> > -    M.x86.R_FLG = pop_word();
> > +    if (M.x86.mode & SYSMODE_PREFIX_DATA) {
> > +        M.x86.R_IP = pop_long();
> > +        M.x86.R_CS = pop_long() & 0xffff;
> > +        M.x86.R_FLG = pop_long();
> > +    } else {
> > +        M.x86.R_IP = pop_word();
> > +        M.x86.R_CS = pop_word();
> > +        M.x86.R_FLG = pop_word();
> > +    }
> >      DECODE_CLEAR_SEGOVR();
> >      END_OF_INSTR();
> >  }
>
> On these three hunks, when on 32-bit mode shouldn't the registers be
> M.x86.R_EIP and M.x86.R_EFLG?

You're absolutely right, and in addition, it seems that some of the
bits in EFLAGS must be preserved by the iret instruction, as described
in the manual:

[...]
REAL-ADDRESS-MODE;
    IF OperandSize = 32
        THEN
            IF top 12 bytes of stack not within stack limits
                THEN #SS; FI;
            tempEIP <- 4 bytes at end of stack
            IF tempEIP[31:16] is not zero THEN #GP(0); FI;
            EIP <- Pop();
            CS <- Pop(); (* 32-bit pop, high-order 16 bits discarded *)
            tempEFLAGS <- Pop();
            EFLAGS <- (tempEFLAGS AND 257FD5H) OR (EFLAGS AND 1A0000H);
        ELSE (* OperandSize = 16 *)
            IF top 6 bytes of stack are not within stack limits
                THEN #SS; FI;
            EIP <- Pop(); (* 16-bit pop; clear upper 16 bits *)
            CS <- Pop(); (* 16-bit pop *)
            EFLAGS[15:0] <- Pop();
    FI;
END;
[...]

I will respin a patch shortly.

-- 
Julian