Re: [Qemu-devel] [PATCH v2 06/17] target/mips: convert to DisasJumpType

[Richard Henderson]

On 04/07/2018 04:19 AM, Emilio G. Cota wrote:
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Cc: Aurelien Jarno <address@hidden>
> Cc: Yongbok Kim <address@hidden>
> Signed-off-by: Emilio G. Cota <address@hidden>
> ---
>  target/mips/translate.c | 186 
> +++++++++++++++++++++++-------------------------
>  1 file changed, 91 insertions(+), 95 deletions(-)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index d05ee67..a133205 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -36,6 +36,7 @@
>  
>  #include "target/mips/trace.h"
>  #include "trace-tcg.h"
> +#include "exec/translator.h"
>  #include "exec/log.h"
>  
>  #define MIPS_DEBUG_DISAS 0
> @@ -1439,7 +1440,7 @@ typedef struct DisasContext {
>      int mem_idx;
>      TCGMemOp default_tcg_memop_mask;
>      uint32_t hflags, saved_hflags;
> -    int bstate;
> +    DisasJumpType is_jmp;
>      target_ulong btarget;
>      bool ulri;
>      int kscrexist;
> @@ -1460,13 +1461,8 @@ typedef struct DisasContext {
>      bool abs2008;
>  } DisasContext;
>  
> -enum {
> -    BS_NONE     = 0, /* We go out of the TB without reaching a branch or an
> -                      * exception condition */
> -    BS_STOP     = 1, /* We want to stop translation for any reason */
> -    BS_BRANCH   = 2, /* We reached a branch condition     */
> -    BS_EXCP     = 3, /* We reached an exception condition */
> -};
> +#define DISAS_STOP       DISAS_TARGET_0
> +#define DISAS_EXCP       DISAS_TARGET_1

Ok, well, there are existing bugs within the MIPS translation here, and we
might as well fix them within this patch set.

(1) The description for BS_STOP says we want to stop, but (what will become)
mips_tr_tb_stop calls goto_tb.

That's not correct, since we use that after e.g. helper_mtc0_hwrena,
MIPS_HFLAG_HWRENA_ULR is included in tb->flags, and therefore the next TB is
not fixed but depends on the actual value stored into hwrena.

We should instead use lookup_and_goto_ptr, which does a full lookup of the
processor state every time through.

(2) The BS_EXCP in generate_exception_err should map to DISAS_NORETURN, because
we do not return after raising an exception.

(3) Otherwise, the use of BS_EXCP has nothing to do with an exception; e.g.

>         case 0:
>             save_cpu_state(ctx, 1);
>             gen_helper_mtc0_status(cpu_env, arg);
>             /* BS_STOP isn't good enough here, hflags may have changed. */
>             gen_save_pc(ctx->pc + 4);
>             ctx->bstate = BS_EXCP;
>             rn = "Status";
>             break;

where we are in fact relying on (what will become) mips_tr_tb_stop to emit
exit_tb.  It would be better to name these uses DISAS_EXIT, which would match
e.g. target/arm.



r~