On Wed, Jul 10, 2024 at 12:43 PM Paolo Bonzini <pbonz...@redhat.com> wrote: > > On 7/10/24 11:42, Clément Chigot wrote: > > Hi Mark, > > > > This patch introduces regressions in our x86_64 VxWorks kernels > > running over qemu. Some page faults are triggered randomly. > > > > Earlier to this patch, the MemOp `ot` passed to `gen_op_st_v` was the > > `gen_pop_T0` created a few lines above. > > Now, this is `op->ot` which comes from elsewhere. > > > > Adding `op->ot = ot` just before calling `gen_writeback` fixes my > > regressions. But I'm wondering if there could be some unexpected > > fallbacks, `op->ot` possibly being used afterwards. > > Mark's patch is correct and the (previously latent) mistake is in > the decoding table. > > The manual correctly says that POP has "default 64-bit" operand; > that is, it is 64-bit even without a REX.W prefix. It must be > marked as "d64" rather than "v". > > Can you test this patch?
Yes, it does work. Thanks a lot for it ! > diff --git a/target/i386/tcg/decode-new.c.inc > b/target/i386/tcg/decode-new.c.inc > index 0d846c32c22..d2da1d396d5 100644 > --- a/target/i386/tcg/decode-new.c.inc > +++ b/target/i386/tcg/decode-new.c.inc > @@ -1717,7 +1717,7 @@ static const X86OpEntry opcodes_root[256] = { > [0x8C] = X86_OP_ENTRYwr(MOV, E,v, S,w, op0_Mw), > [0x8D] = X86_OP_ENTRYwr(LEA, G,v, M,v, nolea), > [0x8E] = X86_OP_ENTRYwr(MOV, S,w, E,w), > - [0x8F] = X86_OP_GROUPw(group1A, E,v), > + [0x8F] = X86_OP_GROUPw(group1A, E,d64), > > [0x98] = X86_OP_ENTRY1(CBW, 0,v), /* rAX */ > [0x99] = X86_OP_ENTRYwr(CWD, 2,v, 0,v), /* rDX, rAX */ > diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc > index fc7477833bc..36bb308e449 100644 > --- a/target/i386/tcg/emit.c.inc > +++ b/target/i386/tcg/emit.c.inc > @@ -2788,6 +2788,8 @@ static void gen_POP(DisasContext *s, X86DecodedInsn > *decode) > X86DecodedOp *op = &decode->op[0]; > MemOp ot = gen_pop_T0(s); > > + /* Only 16/32-bit access in 32-bit mode, 16/64-bit access in long mode. > */ > + assert(ot == op->ot); > if (op->has_ea || op->unit == X86_OP_SEG) { > /* NOTE: order is important for MMU exceptions */ > gen_writeback(s, decode, 0, s->T0); > > Thanks (and it's reassuring that everything else has worked fine > for you!), > > Paolo > > > Thanks, > > Clément > > > > On Sat, Jun 8, 2024 at 10:36 AM Paolo Bonzini <pbonz...@redhat.com> wrote: > >> > >> From: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> > >> > >> Instead of directly implementing the writeback using gen_op_st_v(), use the > >> existing gen_writeback() function. > >> > >> Suggested-by: Paolo Bonzini <pbonz...@redhat.com> > >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> > >> Message-ID: <20240606095319.229650-3-mark.cave-ayl...@ilande.co.uk> > >> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > >> --- > >> target/i386/tcg/emit.c.inc | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc > >> index ca78504b6e4..6123235c000 100644 > >> --- a/target/i386/tcg/emit.c.inc > >> +++ b/target/i386/tcg/emit.c.inc > >> @@ -2580,9 +2580,9 @@ static void gen_POP(DisasContext *s, CPUX86State > >> *env, X86DecodedInsn *decode) > >> > >> if (op->has_ea) { > >> /* NOTE: order is important for MMU exceptions */ > >> - gen_op_st_v(s, ot, s->T0, s->A0); > >> - op->unit = X86_OP_SKIP; > >> + gen_writeback(s, decode, 0, s->T0); > >> } > >> + > >> /* NOTE: writing back registers after update is important for pop > >> %sp */ > >> gen_pop_update(s, ot); > >> } > >> -- > >> 2.45.1 > >> > >> > > > > >