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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 22/4/24 21:03, Volker Rümelin wrote:
> Am 20.04.24 um 07:40 schrieb Mark Cave-Ayland:
>> On 20/04/2024 02:21, Richard Henderson wrote:
>>
>>> 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:
>>
>> Ah that's good to know the docs are now correct. I added the comment
>> as there was a lot of conflicting information around for older CPUs so
>> I thought it was worth an explicit mention.
>>
>> If everyone agrees a version without comments is preferable, I can
>> re-send an updated version without them included.
>>
> 
> Hi Mark,
> 
> I wouldn't remove the comment.
> 
> Quote from the Intel® 64 and IA-32 Architectures Software Developer’s
> Manual Volume 2B: Instruction Set Reference, M-U March 2024:
> 
> IA-32 Architecture Compatibility
> The 16-bit form of SGDT is compatible with the Intel 286 processor if
> the upper 8 bits are not referenced. The Intel 286 processor fills these
> bits with 1s; processor generations later than the Intel 286 processor
> fill these bits with 0s.

Is that that OS/2 Warp and WFW 3.11 expect a 286 CPU? QEMU starts
modelling the 486, do we want to consider down to the 286?

> Intel still claims the upper 8 bits are filled with 0s, but the
> Operation pseudo code below is correct. The same is true for SIDT.
> 
> With best regards,
> Volker
> 
>>>     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>
>>
>> Thanks!
>>
>>
>> ATB,
>>
>> Mark.
>>
>>
>>
> 
>