Re: [Qemu-arm] [PATCH v9 23/25] target-arm: introduce ARM_CP_EXIT_PC

[Peter Maydell]

On 1 February 2017 at 15:05, Alex Bennée <address@hidden> wrote:
> Some helpers may trigger an immediate exit of the cpu_loop. If this
> happens the PC need to be rectified to ensure the restart will begin
> on the next instruction.
>
> Signed-off-by: Alex Bennée <address@hidden>
> ---
>  target/arm/cpu.h           | 3 ++-
>  target/arm/translate-a64.c | 4 ++++
>  target/arm/translate.c     | 4 ++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index d61793ca06..a3c4d07817 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1465,7 +1465,8 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
>  #define ARM_CP_NZCV            (ARM_CP_SPECIAL | (3 << 8))
>  #define ARM_CP_CURRENTEL       (ARM_CP_SPECIAL | (4 << 8))
>  #define ARM_CP_DC_ZVA          (ARM_CP_SPECIAL | (5 << 8))
> -#define ARM_LAST_SPECIAL       ARM_CP_DC_ZVA
> +#define ARM_CP_EXIT_PC         (ARM_CP_SPECIAL | (6 << 8))
> +#define ARM_LAST_SPECIAL       ARM_CP_EXIT_PC

This shouldn't be a "special", because those are for
"this is a special case that is handled entirely in the translate
code", not "I need some extra behaviour on the code generated
for calling the helper functions" (which is what the
plain non-special ARM_CP flags do). Notice that all the other
"special" cases completely define the behaviour of the cp that
uses them, and the code implementing them ends the case
statement with "return", not "break".

Missing documentation comment change.

That said, I'm definitely becoming more strongly of the
opinion that longjumping out of this helper is not the
best way to implement this, so these remarks are a bit moot.

>  /* Used only as a terminator for ARMCPRegInfo lists */
>  #define ARM_CP_SENTINEL 0xffff
>  /* Mask of only the flag bits in a type field */
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 7e7131fe2f..98d4fac070 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1561,6 +1561,10 @@ static void handle_sys(DisasContext *s, uint32_t insn, 
> bool isread,
>          tcg_rt = cpu_reg(s, rt);
>          gen_helper_dc_zva(cpu_env, tcg_rt);
>          return;
> +    case ARM_CP_EXIT_PC:
> +        /* The helper may exit the cpu_loop so ensure PC is correct */
> +        gen_a64_set_pc_im(s->pc);
> +        break;
>      default:
>          break;
>      }
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 24faa7c60c..e1f4a48720 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -7510,6 +7510,10 @@ static int disas_coproc_insn(DisasContext *s, uint32_t 
> insn)
>              gen_set_pc_im(s, s->pc);
>              s->is_jmp = DISAS_WFI;
>              return 0;
> +        case ARM_CP_EXIT_PC:
> +            /* The helper may exit the cpu_loop so ensure PC is correct */
> +            gen_set_pc_im(s, s->pc);
> +            break;
>          default:
>              break;
>          }

thanks
-- PMM