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

[Philippe Mathieu-Daudé]

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.