Re: [PATCH] target/i386/translate.c: always write 32-bits for SGDT and SIDT

[Richard Henderson]

On 4/19/24 12:51, Mark Cave-Ayland wrote:
> The various Intel CPU manuals claim that SGDT and SIDT can write either 24-bits
> or 32-bits depending upon the operand size, but this is incorrect. Not only do
> the Intel CPU manuals give contradictory information between processor
> revisions, but this information doesn't even match real-life behaviour.
>
> In fact, tests on real hardware show that the CPU always writes 32-bits for SGDT
> and SIDT, and this behaviour is required for at least OS/2 Warp and WFW 3.11 
> with
> Win32s to function correctly. Remove the masking applied due to the operand size
> for SGDT and SIDT so that the TCG behaviour matches the behaviour on real
> hardware.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2198
>
> --
> MCA: Whilst I don't have a copy of OS/2 Warp handy, I've confirmed that this
> patch fixes the issue in WFW 3.11 with Win32s. For more technical information I
> highly recommend the excellent write-up at
> https://www.os2museum.com/wp/sgdtsidt-fiction-and-reality/.
> ---
>   target/i386/tcg/translate.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index 76a42c679c..3026eb6774 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -5846,9 +5846,10 @@ static bool disas_insn(DisasContext *s, CPUState *cpu)
>               gen_op_st_v(s, MO_16, s->T0, s->A0);
>               gen_add_A0_im(s, 2);
>               tcg_gen_ld_tl(s->T0, tcg_env, offsetof(CPUX86State, gdt.base));
> -            if (dflag == MO_16) {
> -                tcg_gen_andi_tl(s->T0, s->T0, 0xffffff);
> -            }
> +            /*
> +             * NB: Despite claims to the contrary in Intel CPU documentation,
> +             *     all 32-bits are written regardless of operand size.
> +             */

Current documentation agrees that all 32 bits are written, so I don't think you need this 
comment:

  IF OperandSize =16 or OperandSize = 32 (* Legacy or Compatibility Mode *)
    THEN
      DEST[0:15] := GDTR(Limit);
      DEST[16:47] := GDTR(Base); (* Full 32-bit base address stored *)
  FI;


Anyway,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~