Re: Need arm64/qemu help

[Victor Kamensky <kamensky@cisco.com>]

On Mon, 19 Mar 2018, Peter Maydell wrote:

> On 19 March 2018 at 17:46, Victor Kamensky <kamensky@cisco.com> wrote:
>> In v2.11.1 of qemu, that we use, we already have
>> b29fd33db578decacd14f34933b29aece3e7c25e. Previous testing
>> and collected log was done with it present.
>>
>> But my understanding that eret would happen when target exits
>> an interrupt, here I don't think it enters one.
>>
>> Consider that target explicitely disables interrupts and while it is
>> disabled, arm_cpu_exec_interrupt function calls arm_excp_unmasked
>> and it returns false, so arm_cpu_do_interrupt is not called. Main
>> loop resume execution, and one of the block explicitely
>> reenables interrupt and sequence continues without ever returning to
>> main loop.
>>
>> For example, if I apply below patch, it boots fine. But I am not sure
>> in what other places similar thing is needed, and whether below
>> is complete and correct:
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 91a9300..19128c5 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -2948,6 +2948,14 @@ static CPAccessResult aa64_daif_access(CPUARMState
>> *env, const ARMCPRegInfo *ri,
>>  static void aa64_daif_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>                              uint64_t value)
>>  {
>> +    if (env->daif & ~(value & PSTATE_DAIF)) {
>> +        /* reenabling interrupts */
>> +        CPUState *cs = CPU(arm_env_get_cpu(env));
>> +        if (cs->interrupt_request) {
>> +            /* there is pending one, let's drop back into main loop */
>> +            cs->icount_decr.u16.high = -1;
>> +        }
>> +    }
>>      env->daif = value & PSTATE_DAIF;
>>  }
>
> target/arm/translate-a64.c:handle_sys() is setting
>    s->base.is_jmp = DISAS_UPDATE;
> which it thinks will end the TB, specifically because system
> register writes might do things like unmask interrupts or
> otherwise require main loop processing.
>
> The changes that prompted b29fd33db578dec stopped this working.
> I suspect what we want is for the case DISAS_UPDATE in
> aarch64_tr_tb_stop() to fall through into DISAS_EXIT, not
> DISAS_JUMP. (The AArch32 code gets this right, amazingly.)

Peter, thank you. I can confirm that change you suggested
like one below, boots fine.

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 625ef2d..c381091 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -11384,12 +11384,12 @@ static void aarch64_tr_tb_stop(DisasContextBase 
*dcbase, CPUState *cpu)
          case DISAS_UPDATE:
              gen_a64_set_pc_im(dc->pc);
              /* fall through */
-        case DISAS_JUMP:
-            tcg_gen_lookup_and_goto_ptr();
-            break;
          case DISAS_EXIT:
              tcg_gen_exit_tb(0);
              break;
+        case DISAS_JUMP:
+            tcg_gen_lookup_and_goto_ptr();
+            break;
          case DISAS_NORETURN:
          case DISAS_SWI:
              break;

- Victor

> thanks
> -- PMM
>