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

[Julian Pidancet]

On Wed, Mar 7, 2012 at 1:46 PM, Guillem Jover <guillem at hadrons.org> 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