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

[Alex Bennée]

Peter Maydell <address@hidden> writes:

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

I posted this before you commented on the last version. Anyway see
bellow.

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

Yep the tree:

  https://github.com/stsquad/qemu/commits/mttcg/base-patches-v10

Reverts the this change and changes the cputlb flush code to return and
let the guest translation code exit the normal way. I was hoping to get
some feedback from Paolo and Richard before I roll the fixes together
and post v10 which will be later today.

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


--
Alex Bennée