[Qemu-devel] arm: semihosting: Preliminary AArch64 support

[Qemu-devel] arm: semihosting: Preliminary AArch64 support

[Christopher Covington]

Hi,

Here are a few patches preparing for and adding AArch64 Angel
semihosting support. I've been testing them with some simple tests from
the following repository. This series only adds support for exit, but
support for the rest of the calls will hopefully follow shortly.

http://git.linaro.org/people/christopher.covington/angel-semihosting.git

Thanks,
Chris

[Qemu-devel] [PATCH 1/3] arm: semihosting: Improve debug prints

[Christopher Covington]

Print semihosting debugging information before the
do_arm_semihosting() call so that angel_SWIreason_ReportException,
which causes the function to not return, gets the same debug prints as
other semihosting calls. Also print out the semihosting call number.

Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
---
 target-arm/helper.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 10886c5..3088a18 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4397,8 +4397,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
             nr = arm_lduw_code(env, env->regs[15], env->bswap_code) & 0xff;
             if (nr == 0xab) {
                 env->regs[15] += 2;
+                qemu_log_mask(CPU_LOG_INT,
+                              "...handling as semihosting call 0x%x\n",
+                              env->regs[0]);
                 env->regs[0] = do_arm_semihosting(env);
-                qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
                 return;
             }
         }
@@ -4718,8 +4720,10 @@ void arm_cpu_do_interrupt(CPUState *cs)
             if (((mask == 0x123456 && !env->thumb)
                     || (mask == 0xab && env->thumb))
                   && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
+                qemu_log_mask(CPU_LOG_INT,
+                              "...handling as semihosting call 0x%x\n",
+                              env->regs[0]);
                 env->regs[0] = do_arm_semihosting(env);
-                qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
                 return;
             }
         }
@@ -4736,8 +4740,10 @@ void arm_cpu_do_interrupt(CPUState *cs)
             if (mask == 0xab
                   && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
                 env->regs[15] += 2;
+                qemu_log_mask(CPU_LOG_INT,
+                              "...handling as semihosting call 0x%x\n",
+                              env->regs[0]);
                 env->regs[0] = do_arm_semihosting(env);
-                qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
                 return;
             }
         }
-- 
1.9.1

[Qemu-devel] [PATCH 2/3] arm: semihosting: Create unsupported call function

[Christopher Covington]

This will allow the print-error-and-exit sequence to be called from a
second location in a subsequent patch. The type of the nr variable is
changed from int to uint32_t since I'm unaware of semihosting call
numbers needing more than 32 bits, even on AArch64. Also generalize
the wording of the unsupported semihosting call error message so that
it will make sense on AArch64 as well.

Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
---
 target-arm/arm-semi.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
index a8b83e6..d915123 100644
--- a/target-arm/arm-semi.c
+++ b/target-arm/arm-semi.c
@@ -174,6 +174,13 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
 #endif
 }
 
+static void QEMU_NORETURN unsupported_semihosting(uint32_t nr, CPUState *cs)
+{
+        fprintf(stderr, "qemu: Unsupported semihosting call 0x%02x\n", nr);
+        cpu_dump_state(cs, stderr, fprintf, 0);
+        abort();
+}
+
 /* Read the input value from the argument block; fail the semihosting
  * call if the memory read fails.
  */
@@ -191,7 +198,7 @@ uint32_t do_arm_semihosting(CPUARMState *env)
     target_ulong args;
     target_ulong arg0, arg1, arg2, arg3;
     char * s;
-    int nr;
+    uint32_t nr;
     uint32_t ret;
     uint32_t len;
 #ifdef CONFIG_USER_ONLY
@@ -561,8 +568,6 @@ uint32_t do_arm_semihosting(CPUARMState *env)
         gdb_exit(env, ret);
         exit(ret);
     default:
-        fprintf(stderr, "qemu: Unsupported SemiHosting SWI 0x%02x\n", nr);
-        cpu_dump_state(cs, stderr, fprintf, 0);
-        abort();
+        unsupported_semihosting(nr, cs);
     }
 }
-- 
1.9.1

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

[Christopher Covington]

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 <christopher.covington@linaro.org>
---
 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) {
+            unsupported_semihosting(nr, cs);
+        }
+    } else {
+        nr = env->regs[0];
+        args = env->regs[1];
+    }
     switch (nr) {
     case TARGET_SYS_OPEN:
         GET_ARG(0);
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) {
+            gen_exception_insn(s, 4, EXCP_SEMI, 0);
+        } else {
+            unsupported_encoding(s, insn);
+        }
         break;
     case 5:
         if (op2_ll < 1 || op2_ll > 3) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/3] arm: semihosting: Improve debug prints

[Peter Maydell]

On 27 March 2015 at 16:22, Christopher Covington
<christopher.covington@linaro.org> wrote:
> Print semihosting debugging information before the
> do_arm_semihosting() call so that angel_SWIreason_ReportException,
> which causes the function to not return, gets the same debug prints as
> other semihosting calls. Also print out the semihosting call number.
>
> Signed-off-by: Christopher Covington <christopher.covington@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

On 27 March 2015 at 16:22, Christopher Covington
<christopher.covington@linaro.org> 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 <christopher.covington@linaro.org>
> ---
>  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...)

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

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

> +            gen_exception_insn(s, 4, EXCP_SEMI, 0);

> +        } else {
> +            unsupported_encoding(s, insn);
> +        }
>          break;
>      case 5:
>          if (op2_ll < 1 || op2_ll > 3) {
> --
> 1.9.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] arm: semihosting: Create unsupported call function

[Peter Maydell]

On 27 March 2015 at 16:22, Christopher Covington
<christopher.covington@linaro.org> wrote:
> This will allow the print-error-and-exit sequence to be called from a
> second location in a subsequent patch. The type of the nr variable is
> changed from int to uint32_t since I'm unaware of semihosting call
> numbers needing more than 32 bits, even on AArch64. Also generalize
> the wording of the unsupported semihosting call error message so that
> it will make sense on AArch64 as well.
>
> Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
> ---
>  target-arm/arm-semi.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
> index a8b83e6..d915123 100644
> --- a/target-arm/arm-semi.c
> +++ b/target-arm/arm-semi.c
> @@ -174,6 +174,13 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
>  #endif
>  }
>
> +static void QEMU_NORETURN unsupported_semihosting(uint32_t nr, CPUState *cs)
> +{
> +        fprintf(stderr, "qemu: Unsupported semihosting call 0x%02x\n", nr);
> +        cpu_dump_state(cs, stderr, fprintf, 0);
> +        abort();
> +}
> +
>  /* Read the input value from the argument block; fail the semihosting
>   * call if the memory read fails.
>   */
> @@ -191,7 +198,7 @@ uint32_t do_arm_semihosting(CPUARMState *env)
>      target_ulong args;
>      target_ulong arg0, arg1, arg2, arg3;
>      char * s;
> -    int nr;
> +    uint32_t nr;
>      uint32_t ret;
>      uint32_t len;
>  #ifdef CONFIG_USER_ONLY
> @@ -561,8 +568,6 @@ uint32_t do_arm_semihosting(CPUARMState *env)
>          gdb_exit(env, ret);
>          exit(ret);
>      default:
> -        fprintf(stderr, "qemu: Unsupported SemiHosting SWI 0x%02x\n", nr);
> -        cpu_dump_state(cs, stderr, fprintf, 0);
> -        abort();
> +        unsupported_semihosting(nr, cs);
>      }
>  }
> --
> 1.9.1

OK, I guess, though I think the need to call from multiple places
goes away as soon as we've implemented all the calls, so I would
personally just have implemented them all...

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] arm: semihosting: Preliminary AArch64 support

[Liviu Ionescu]

> On 27 Mar 2015, at 18:22, Christopher Covington <christopher.covington@linaro.org> wrote:
> 
> 
> Hi,
> 
> Here are a few patches preparing for and adding AArch64 Angel
> semihosting support.

please note that the semihosting support is still incomplete, the code to pass the semihosting command line is not in; we discussed a lot about this, I fixed it in my fork (GNU ARM Eclipse QEMU), I prepared some patches, but finally they were not included.

the code I currently use reflects the latest discussed solution, to add the command line to -semihosting-config:

"-semihosting-config [enable=on|off,]target=native|gdb|auto[,cmdline=string]   semihosting configuration\n",

it is functional, but I'm not sure the usability is ok, requiring to use double commas in the command string (if I remember right).


regards,

Liviu


p.s. these days I finally was able to allocate some time to my GNU ARM Eclipse QEMU subproject; right now I'm updating the build procedures and then I plan to continue work on the generic Cortex-M code.

Re: [Qemu-devel] arm: semihosting: Preliminary AArch64 support

[Peter Maydell]

On 27 March 2015 at 16:57, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 27 Mar 2015, at 18:22, Christopher Covington <christopher.covington@linaro.org> wrote:
>>
>>
>> Hi,
>>
>> Here are a few patches preparing for and adding AArch64 Angel
>> semihosting support.
>
> please note that the semihosting support is still incomplete,

Christopher's patchset is just intended to bring 64-bit ARM
support into line with the 32-bit ARM support, so it's
orthogonal to any improvements to command line config switches.

> the code to pass the semihosting command line is not in; we
> discussed a lot about this, I fixed it in my fork (GNU ARM
> Eclipse QEMU), I prepared some patches, but finally they were
> not included.

We should pick those back up -- I think we sort of stalled
because it wasn't clear that you were happy with the double-comma
thing. It does fit in with the rest of QEMU's (ugly) command
line syntax, though...

-- PMM

Re: [Qemu-devel] arm: semihosting: Preliminary AArch64 support

[Leon Alrae]

Hi,

On 27/03/2015 17:05, Peter Maydell wrote:
> On 27 March 2015 at 16:57, Liviu Ionescu <ilg@livius.net> wrote:
>>
>>> On 27 Mar 2015, at 18:22, Christopher Covington <christopher.covington@linaro.org> wrote:
>>>
>>>
>>> Hi,
>>>
>>> Here are a few patches preparing for and adding AArch64 Angel
>>> semihosting support.
>>
>> please note that the semihosting support is still incomplete,
> 
> Christopher's patchset is just intended to bring 64-bit ARM
> support into line with the 32-bit ARM support, so it's
> orthogonal to any improvements to command line config switches.
> 
>> the code to pass the semihosting command line is not in; we
>> discussed a lot about this, I fixed it in my fork (GNU ARM
>> Eclipse QEMU), I prepared some patches, but finally they were
>> not included.
> 
> We should pick those back up -- I think we sort of stalled
> because it wasn't clear that you were happy with the double-comma
> thing. It does fit in with the rest of QEMU's (ugly) command
> line syntax, though...

Recently as a part of semihosting support for MIPS, I proposed
introducing separate "-semihosting-arg" option to pass input arguments
https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg05387.html

I'm not entirely sure about the name of this option (it is explicit at
least I think), but basically it makes the life easier. You can pass any
number of arguments without having to care about separators and escape
characters. Wouldn't it help also in this ugly double-commas case?

Thanks,
Leon

Re: [Qemu-devel] arm: semihosting: Preliminary AArch64 support

[Peter Maydell]

On 27 March 2015 at 17:15, Leon Alrae <leon.alrae@imgtec.com> wrote:
> On 27/03/2015 17:05, Peter Maydell wrote:
>> We should pick those back up -- I think we sort of stalled
>> because it wasn't clear that you were happy with the double-comma
>> thing. It does fit in with the rest of QEMU's (ugly) command
>> line syntax, though...
>
> Recently as a part of semihosting support for MIPS, I proposed
> introducing separate "-semihosting-arg" option to pass input arguments
> https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg05387.html
>
> I'm not entirely sure about the name of this option (it is explicit at
> least I think), but basically it makes the life easier. You can pass any
> number of arguments without having to care about separators and escape
> characters. Wouldn't it help also in this ugly double-commas case?

This is rather recapitulating the previous discussions. The
problem with extra ad-hoc top level command line arguments
is that they don't fit in with the structure we're trying
to impose on new QEMU options, which is that they should
have a particular syntax and sit inside option groups
(in this case, -semihosting-config).

-- PMM

Re: [Qemu-devel] arm: semihosting: Preliminary AArch64 support

[Liviu Ionescu]

> On 27 Mar 2015, at 19:15, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> ... introducing separate "-semihosting-arg" option to pass input arguments

if we'll ever go for this solution, I would call it "-semihosting-cmdline", since it should include the entire command line, starting with argv[0].

> ... but basically it makes the life easier.

this was my opinion too, and my first patches implemented this solution.

> On 27 Mar 2015, at 19:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> ... The
> problem with extra ad-hoc top level command line arguments
> is that they don't fit in with the structure we're trying
> to impose on new QEMU options, which is that they should
> have a particular syntax and sit inside option groups
> (in this case, -semihosting-config).

this is also true, and, although a bit more complicated to use, I implemented this solution.

> I think we sort of stalled
> because it wasn't clear that you were happy with the double-comma
> thing.

my fault for stalling it, I decided to wait until I have a chance to give it a try in a real use case (in my testing infrastructure, which is not yet ready... :-( )

but, as I said, although a bit more complicated, the current solution seems fully functional.


regards,

Liviu

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
<peter.maydell@linaro.org> wrote:
> On 27 March 2015 at 16:22, Christopher Covington
> <christopher.covington@linaro.org> 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 <christopher.covington@linaro.org>
>> ---
>>  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

Re: [Qemu-devel] arm: semihosting: Preliminary AArch64 support

[Leon Alrae]

On 27/03/2015 17:33, Liviu Ionescu wrote:
> 
>> On 27 Mar 2015, at 19:15, Leon Alrae <leon.alrae@imgtec.com> wrote:
>>
>> ... introducing separate "-semihosting-arg" option to pass input arguments
> 
> if we'll ever go for this solution, I would call it "-semihosting-cmdline", since it should include the entire command line, starting with argv[0].
> 
>> ... but basically it makes the life easier.
> 
> this was my opinion too, and my first patches implemented this solution.
> 
>> On 27 Mar 2015, at 19:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>> ... The
>> problem with extra ad-hoc top level command line arguments
>> is that they don't fit in with the structure we're trying
>> to impose on new QEMU options, which is that they should
>> have a particular syntax and sit inside option groups
>> (in this case, -semihosting-config).
> 
> this is also true, and, although a bit more complicated to use, I implemented this solution.

Avoiding top level ad-hoc arguments sounds reasonable. Unfortunately the
QEMU parser doesn't seem to support the same sub-argument used multiple
times (always the last value is used):
-semihosting-config arg="argument 1",arg="argument 2",arg="argument 3"
I may look into it to see how much is missing to make it work.

The reason I chose "arg" is because it naturally translates into argv[]
for the guest program. As far as "cmdline" option goes -- wouldn't "arg"
be more flexible so that you could use it to assemble cmdline?

Leon

Re: [Qemu-devel] arm: semihosting: Preliminary AArch64 support

[Liviu Ionescu]

> On 30 Mar 2015, at 14:44, Leon Alrae <leon.alrae@imgtec.com> wrote:
> 
> ... QEMU parser doesn't seem to support the same sub-argument used multiple
> times (always the last value is used):
> -semihosting-config arg="argument 1",arg="argument 2",arg="argument 3"

yes, unfortunately it is not supported.

please note that the actual code cannot pass the full path of the executable as argv[0], because it may be way too long, and semihosting applications usually have a tiny buffer to process args.

this is the reason why my code requires passing the entire command line, starting with argv[0]. this is also in line with other debugging tools, like the SEGGER J-Link GDB Server.

> I may look into it to see how much is missing to make it work.

ok, please do, but last year, when we analysed several options, altering the parser was considered too risky.

> The reason I chose "arg" is because it naturally translates into argv[]
> for the guest program. As far as "cmdline" option goes -- wouldn't "arg"
> be more flexible so that you could use it to assemble cmdline?

if you manage to parse multiple sub-arguments with the same name, arg is probably ok, but for the entire line I would vote for "cmdline=".

my changes are available from git://git.code.sf.net/p/gnuarmeclipse/qemu, the gnuarmeclipse-dev branch (work in progress).

btw, defining an array of strings is not very useful in this case, since the semihosting api does not allow to pass it as array, and the strings must be concatenated into a single string anyway (and parsed back inside the application). 


regards,

Liviu

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

[Peter Maydell]

On 28 March 2015 at 12:27, Christopher Covington
<christopher.covington@linaro.org> wrote:
> Hi Peter,
>
> On Fri, Mar 27, 2015 at 12:40 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 27 March 2015 at 16:22, Christopher Covington
>> <christopher.covington@linaro.org> wrote:
>>> +        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.

However the spec for this API says w0, so we should ignore
the upper bits.

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

target_ulong is a bit odd here, because for a 32-bit
CPU being run from qemu-system-aarch64 it will be a
64 bit type even though the semihosting ABI should be
using 32 bit types. I would be wary of using it...

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

For A32/T32 we always take the exception, because the
"not enabled" case can fall through to the standard
bkpt/SWI handling code. Because for A64 there is no
handling for HLT there's nothing to fall through to.
In theory you could make the do_interrupt code handle
EXCP_SEMI with semihosting disabled correctly, but it's
much easier to just not generate it in the first place.

-- PMM