Re: [PULL 13/42] target/i386: use gen_writeback() within gen_POP()

[Clément Chigot]

On Wed, Jul 10, 2024 at 12:43 PM Paolo Bonzini <pbonzini@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 <pbonzini@redhat.com> wrote:
> >>
> >> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>
> >> Instead of directly implementing the writeback using gen_op_st_v(), use the
> >> existing gen_writeback() function.
> >>
> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> Message-ID: <20240606095319.229650-3-mark.cave-ayland@ilande.co.uk>
> >> Signed-off-by: Paolo Bonzini <pbonzini@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
> >>
> >>
> >
> >
>