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

[Daniel P . Berrangé]

On Tue, Apr 23, 2024 at 10:15:57AM +0200, Philippe Mathieu-Daudé wrote:
> 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?

Depends on the versions you're talking about. From what I can gather,
OS/2 1.x targetted i286, OS/2 2.x & 3.x targetted i386, and OS/2 4.0
Warp targetted i486, while WFW 3.11 was i386.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|