Re: [Qemu-devel] [PATCH 3/3] arm: semihosting: Wire up A64 HLT 0xf000

[Christopher Covington]

Hi Peter,

On Fri, Mar 27, 2015 at 12:40 PM, Peter Maydell
<address@hidden> wrote:
> On 27 March 2015 at 16:22, Christopher Covington
> <address@hidden> wrote:
>> In AArch64, Angel semihosting uses HLT instructions with a special
>> immediate value, 0xf000. Use a new exception type EXCP_SEMI for this,
>> since I'm not aware of plans for full HLT support, and EXCP_HLT is
>> already defined as a QEMU internal exception type. Support just the
>> exit call in A64 to start with.
>>
>> Signed-off-by: Christopher Covington <address@hidden>
>> ---
>>  target-arm/arm-semi.c      | 14 +++++++++++---
>>  target-arm/cpu.h           |  3 ++-
>>  target-arm/helper-a64.c    | 11 +++++++++++
>>  target-arm/internals.h     |  1 +
>>  target-arm/translate-a64.c |  6 +++++-
>>  5 files changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
>> index d915123..c71d47a 100644
>> --- a/target-arm/arm-semi.c
>> +++ b/target-arm/arm-semi.c
>> @@ -191,7 +191,7 @@ static void QEMU_NORETURN 
>> unsupported_semihosting(uint32_t nr, CPUState *cs)
>>  } while (0)
>>
>>  #define SET_ARG(n, val) put_user_ual(val, args + (n) * 4)
>> -uint32_t do_arm_semihosting(CPUARMState *env)
>> +target_ulong do_arm_semihosting(CPUARMState *env)
>>  {
>>      ARMCPU *cpu = arm_env_get_cpu(env);
>>      CPUState *cs = CPU(cpu);
>> @@ -207,8 +207,16 @@ uint32_t do_arm_semihosting(CPUARMState *env)
>>      CPUARMState *ts = env;
>>  #endif
>>
>> -    nr = env->regs[0];
>> -    args = env->regs[1];
>> +    if (is_a64(env)) {
>> +        nr = env->xregs[0];
>
>> +        args = env->xregs[1];
>> +        if (nr != env->xregs[0] || nr != TARGET_SYS_EXIT) {
>
> What is the first part of this if condition intended to do?
> (Note that the semihosting API number is passed in W0,
> not X0...)

The intention was to check that none of bits 63 through 32 were set,
even if the lower half looked good. Yes, w0 as opposed to x0 makes the
most sense for moving the call number into its register, but I'd
prefer to double check. Maybe using target_ulong for args would be
better, as the default case of the switch statement would handle high
bits being set on A64.

>> +            unsupported_semihosting(nr, cs);
>> +        }
>> +    } else {
>> +        nr = env->regs[0];
>> +        args = env->regs[1];
>> +    }
>>      switch (nr) {
>>      case TARGET_SYS_OPEN:
>>          GET_ARG(0);
>
> This doesn't look right -- in the AArch64 version
> the SYS_EXIT (aka AngelSWI_Reason_ReportException)
> takes a parameter block, so you can't just fall through
> to the existing code without modifying it to support this.
>
> It's also a bit difficult to tell if this code is
> right without seeing some actual implementations of
> the semihosting calls.

Thanks for pointing that out. I'll update my hand-coded exit sequence
to better match what Newlib does. My plan for the time being is to
code against one of the recent Linaro GCC+Newlib toolchains, but once
most of the functionality is in place I may be able to check against
other toolchains and simulators/debuggers.

>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 083211c..d5c5cfa 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -56,6 +56,7 @@
>>  #define EXCP_SMC            13   /* Secure Monitor Call */
>>  #define EXCP_VIRQ           14
>>  #define EXCP_VFIQ           15
>> +#define EXCP_SEMI           16   /* Angel Semihosting Call */
>>
>>  #define ARMV7M_EXCP_RESET   1
>>  #define ARMV7M_EXCP_NMI     2
>> @@ -494,7 +495,7 @@ typedef struct CPUARMState {
>>
>>  ARMCPU *cpu_arm_init(const char *cpu_model);
>>  int cpu_arm_exec(CPUARMState *s);
>> -uint32_t do_arm_semihosting(CPUARMState *env);
>> +target_ulong do_arm_semihosting(CPUARMState *env);
>>  void aarch64_sync_32_to_64(CPUARMState *env);
>>  void aarch64_sync_64_to_32(CPUARMState *env);
>>
>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
>> index 7e0d038..9ecd488 100644
>> --- a/target-arm/helper-a64.c
>> +++ b/target-arm/helper-a64.c
>> @@ -514,6 +514,17 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>>      case EXCP_VFIQ:
>>          addr += 0x100;
>>          break;
>> +    case EXCP_SEMI:
>> +        if (semihosting_enabled) {
>> +            qemu_log_mask(CPU_LOG_INT,
>> +                          "...handling as semihosting call 0x%" PRIx64 "\n",
>> +                          env->xregs[0]);
>> +            env->xregs[0] = do_arm_semihosting(env);
>> +            break;
>> +        } else {
>> +            qemu_log_mask(CPU_LOG_INT,
>> +                          "...not handled because semihosting is 
>> disabled\n");
>> +        }
>>      default:
>>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>>      }
>> diff --git a/target-arm/internals.h b/target-arm/internals.h
>> index bb171a7..86b5854 100644
>> --- a/target-arm/internals.h
>> +++ b/target-arm/internals.h
>> @@ -58,6 +58,7 @@ static const char * const excnames[] = {
>>      [EXCP_SMC] = "Secure Monitor Call",
>>      [EXCP_VIRQ] = "Virtual IRQ",
>>      [EXCP_VFIQ] = "Virtual FIQ",
>> +    [EXCP_SEMI] = "Angel Semihosting Call",
>>  };
>>
>>  static inline void arm_log_exception(int idx)
>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> index 0b192a1..3b5b875 100644
>> --- a/target-arm/translate-a64.c
>> +++ b/target-arm/translate-a64.c
>> @@ -1544,7 +1544,11 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>>              break;
>>          }
>>          /* HLT */
>> -        unsupported_encoding(s, insn);
>> +        if (imm16 == 0xf000) {
>
> You need to have the semihosting_enabled check here rather
> than in the do_interrupt code, because otherwise we won't
> behave correctly in the disabled case.

I don't think that's what A32 does, but I like it.

Thanks,
Chrish