Re: [Qemu-devel] [PATCH 11/26] target/arm: Rearrange decode in disas_uncond_b_reg

[Richard Henderson]

On 12/11/18 9:40 AM, Peter Maydell wrote:
> On Fri, 7 Dec 2018 at 10:36, Richard Henderson
> <address@hidden> wrote:
>>
>> This will enable PAuth decode in a subsequent patch.
>>
>> Signed-off-by: Richard Henderson <address@hidden>
>> ---
>>  target/arm/translate-a64.c | 34 +++++++++++++++++++++++-----------
>>  1 file changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>> index c84c2dbb66..5fa2647771 100644
>> --- a/target/arm/translate-a64.c
>> +++ b/target/arm/translate-a64.c
>> @@ -1989,32 +1989,41 @@ static void disas_uncond_b_reg(DisasContext *s, 
>> uint32_t insn)
>>      rn = extract32(insn, 5, 5);
>>      op4 = extract32(insn, 0, 5);
>>
>> -    if (op4 != 0x0 || op3 != 0x0 || op2 != 0x1f) {
>> -        unallocated_encoding(s);
>> -        return;
>> +    if (op2 != 0x1f) {
>> +        goto do_unallocated;
>>      }
>>
>>      switch (opc) {
>>      case 0: /* BR */
>>      case 1: /* BLR */
>>      case 2: /* RET */
>> -        gen_a64_set_pc(s, cpu_reg(s, rn));
>> +        if (op3 == 0 && op4 == 0) {
>> +            dst = cpu_reg(s, rn);
>> +        } else {
>> +            goto do_unallocated;
>> +        }
>> +        gen_a64_set_pc(s, dst);
>>          /* BLR also needs to load return address */
>>          if (opc == 1) {
>>              tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
>>          }
>>          break;
>> +
>>      case 4: /* ERET */
>>          if (s->current_el == 0) {
>> -            unallocated_encoding(s);
>> -            return;
>> -        }
>> -        if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
>> -            gen_io_start();
>> +            goto do_unallocated;
>>          }
>>          dst = tcg_temp_new_i64();
>>          tcg_gen_ld_i64(dst, cpu_env,
>>                         offsetof(CPUARMState, elr_el[s->current_el]));
>> +        if (op3 == 0 && op4 == 0) {
>> +            ;
>> +        } else {
>> +            goto do_unallocated;
> 
> 
> This decode check should go before any code has been
> emittede (ie before the tcg_gen_ld_i64 above it).

Well, it could, but only if we duplicate the ld_i64 in the various branches
that require it.  E.g.

  if (op3 == 0 && op4 == 0) {
      tcg_gen_ld_i64(...);
  } else if (dc_ir_feature(aa64_pauth, s) && ...) {
      tcg_gen_ld_i64(...);
      if (s->pauth_active) {
          gen_helper_auti*(...);
      }
  } else {
      goto do_unallocated;
  }

which I suppose isn't so bad.

What I have isn't an error because the ld_i64 will simply be deleted as dead
code by the tcg optimizer.  But I'll rearrange anyway.


r~