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

[Clément Chigot]

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.

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
>
>