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  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.