Re: [Qemu-trivial] [Qemu-devel] [PATCH moxie] Fix bug in tlb_fill.

[Max Filippov]

On Tue, May 14, 2013 at 12:04 AM, Anthony Green <address@hidden> wrote:
> Fix a simple bug in tlb_fill for moxie.  The port was mostly working
> before, which is why I only really noticed it recently.  Thanks to
> @jcmvbkbc for tracking it down.
>
> Signed-off-by: Anthony Green <address@hidden>
> ---
>  target-moxie/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-moxie/helper.c b/target-moxie/helper.c
> index 6e0ac2a..6c36c49 100644
> --- a/target-moxie/helper.c
> +++ b/target-moxie/helper.c
> @@ -55,8 +55,8 @@ void tlb_fill(CPUMoxieState *env, target_ulong addr, int 
> is_write, int mmu_idx,
>          if (retaddr) {
>              cpu_restore_state(env, retaddr);
>          }
> +       cpu_loop_exit(env);
>      }
> -    cpu_loop_exit(env);
>  }


Hi Anthony,

that bug only revealed that some instructions (in that particular case jsra)
issue memory access while they have inconsistent registers state:

        case 0x03: /* jsra */
            {
                TCGv t1 = tcg_temp_new_i32();
                TCGv t2 = tcg_temp_new_i32();

                tcg_gen_movi_i32(t1, ctx->pc + 6);

                /* Make space for the static chain and return address.  */
                tcg_gen_subi_i32(t2, REG(1), 8);
                tcg_gen_mov_i32(REG(1), t2);
(1)-->                tcg_gen_qemu_st32(t1, REG(1), ctx->memidx);

                /* Push the current frame pointer.  */
                tcg_gen_subi_i32(t2, REG(1), 4);
                tcg_gen_mov_i32(REG(1), t2);
(2)-->                tcg_gen_qemu_st32(REG(0), REG(1), ctx->memidx);

                /* Set the pc and $fp.  */
                tcg_gen_mov_i32(REG(0), REG(1));

                gen_goto_tb(env, ctx, 0, cpu_ldl_code(env, ctx->pc+2));

                tcg_temp_free_i32(t1);
                tcg_temp_free_i32(t2);

                ctx->bstate = BS_BRANCH;
                length = 6;
            }

memory access at points (1) and (2) can abort the instruction (it did so
b/o the bug, but it may do so legitimately when you add MMU support),
but it has modified REG(1) at those points, which will not be restored.
It's probably worth carrying register modifications in some temporary
until after the point (2).

-- 
Thanks.
-- Max