Re: [Qemu-devel] [PATCH 9/9] target-arm: Wire up HLT 0xf000 as the A64 semihosting instruction

[Christopher Covington]

Hi Peter,

On 08/27/2015 02:35 PM, Peter Maydell wrote:
> On 13 August 2015 at 17:35, Peter Maydell  wrote:
>> For the A64 instruction set, the semihosting call instruction
>> is 'HLT 0xf000'. Wire this up to call do_arm_semihosting()
>> if semihosting is enabled.
>>
>> Signed-off-by: Peter Maydell 
>> ---
> 
>> @@ -1553,8 +1554,17 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>>  unallocated_encoding(s);
>>  break;
>>  }
>> -/* HLT */
>> -unsupported_encoding(s, insn);
>> +/* HLT. This has two purposes.
>> + * Architecturally, it is an external halting debug instruction.
>> + * Since QEMU doesn't implement external debug, we treat this as
>> + * it is required for halting debug disabled: it will UNDEF.
>> + * Secondly, "HLT 0xf000" is the A64 semihosting syscall 
>> instruction.
>> + */
>> +if (semihosting_enabled() && imm16 == 0xf000) {
>> +gen_exception_internal_insn(s, 0, EXCP_SEMIHOST);
>> +} else {
>> +unsupported_encoding(s, insn);
>> +}
> 
> Christopher pointed out to me at KVM Forum that this isn't
> consistent with how we do 32-bit ARM semihosting, which has a
> check to prevent its use from userspace in system emulation.
> (The idea is that semihosting is basically a "guest can pwn
> your host" API, so giving access to it to guest userspace is
> kind of brave.)

> There is a usecase for allowing unfettered access to semihosting
> in system emulation mode (basically, running bare metal test
> binaries). I think we should deal with that by having a separate
> command line option for "userspace semihosting access is OK",
> which changes the behaviour for both 32-bit and 64-bit semihosting
> APIs. Alternatively, we could instead allow userspace to use
> "safe" parts of the semihosting API, like "print to stdout",
> but not the less safe parts like "open and write to arbitrary
> host files". Or we could decide that this safety check isn't
> actually very useful (no other model/debug environment has it
> that I know of) and drop it entirely; but that makes me a little
> nervous.

I find allowing trusted guests to access host files to be a very useful
feature. To me it is very similar to passing through / (root) via VirtIO-9P.
Perhaps a useful way of making sure the user knows what files their guest is
gaining access to would be to have a semihosting path prefix option. That way
access could be allowed nowhere; clearly allow everywhere (/); or clearly be
restricted to, and relative to, a certain sysroot directory
(/home/user/my-sysroot).

Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Qemu-devel] [PATCH 9/9] target-arm: Wire up HLT 0xf000 as the A64 semihosting instruction

[Peter Maydell]

On 13 August 2015 at 17:35, Peter Maydell peter.mayd...@linaro.org wrote:
 For the A64 instruction set, the semihosting call instruction
 is 'HLT 0xf000'. Wire this up to call do_arm_semihosting()
 if semihosting is enabled.

 Signed-off-by: Peter Maydell peter.mayd...@linaro.org
 ---

 @@ -1553,8 +1554,17 @@ static void disas_exc(DisasContext *s, uint32_t insn)
  unallocated_encoding(s);
  break;
  }
 -/* HLT */
 -unsupported_encoding(s, insn);
 +/* HLT. This has two purposes.
 + * Architecturally, it is an external halting debug instruction.
 + * Since QEMU doesn't implement external debug, we treat this as
 + * it is required for halting debug disabled: it will UNDEF.
 + * Secondly, HLT 0xf000 is the A64 semihosting syscall instruction.
 + */
 +if (semihosting_enabled()  imm16 == 0xf000) {
 +gen_exception_internal_insn(s, 0, EXCP_SEMIHOST);
 +} else {
 +unsupported_encoding(s, insn);
 +}

Christopher pointed out to me at KVM Forum that this isn't
consistent with how we do 32-bit ARM semihosting, which has a
check to prevent its use from userspace in system emulation.
(The idea is that semihosting is basically a guest can pwn
your host API, so giving access to it to guest userspace is
kind of brave.)

I propose to squash the following change into this patch as I
put it into target-arm.next.


--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1561,6 +1561,16 @@ static void disas_exc(DisasContext *s, uint32_t insn)
  * Secondly, HLT 0xf000 is the A64 semihosting syscall instruction.
  */
 if (semihosting_enabled()  imm16 == 0xf000) {
+#ifndef CONFIG_USER_ONLY
+/* In system mode, don't allow userspace access to semihosting,
+ * to provide some semblance of security (and for consistency
+ * with our 32-bit semihosting).
+ */
+if (s-current_el == 0) {
+unsupported_encoding(s, insn);
+break;
+}
+#endif
 gen_exception_internal_insn(s, 0, EXCP_SEMIHOST);
 } else {
 unsupported_encoding(s, insn);


This brings it into line with the 32-bit code.

There is a usecase for allowing unfettered access to semihosting
in system emulation mode (basically, running bare metal test
binaries). I think we should deal with that by having a separate
command line option for userspace semihosting access is OK,
which changes the behaviour for both 32-bit and 64-bit semihosting
APIs. Alternatively, we could instead allow userspace to use
safe parts of the semihosting API, like print to stdout,
but not the less safe parts like open and write to arbitrary
host files. Or we could decide that this safety check isn't
actually very useful (no other model/debug environment has it
that I know of) and drop it entirely; but that makes me a little
nervous.

(It would actually be nice to be able to say I'd like the
guest kernel to be able to do early printk via semihosting
without trusting it to open files etc, for that matter.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 9/9] target-arm: Wire up HLT 0xf000 as the A64 semihosting instruction

[Christopher Covington]

On Aug 13, 2015 9:35 AM, Peter Maydell peter.mayd...@linaro.org wrote:

 For the A64 instruction set, the semihosting call instruction
 is 'HLT 0xf000'. Wire this up to call do_arm_semihosting()
 if semihosting is enabled.

 Signed-off-by: Peter Maydell peter.mayd...@linaro.org

Reviewed-by: Christopher Covington christopher.coving...@linaro.org

[Qemu-devel] [PATCH 9/9] target-arm: Wire up HLT 0xf000 as the A64 semihosting instruction

[Peter Maydell]

For the A64 instruction set, the semihosting call instruction
is 'HLT 0xf000'. Wire this up to call do_arm_semihosting()
if semihosting is enabled.

Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
 linux-user/main.c  |  3 +++
 target-arm/cpu.h   |  1 +
 target-arm/helper-a64.c|  6 ++
 target-arm/internals.h |  2 ++
 target-arm/translate-a64.c | 14 --
 5 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index fdee981..56f452e 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -1054,6 +1054,9 @@ void cpu_loop(CPUARMState *env)
 queue_signal(env, info.si_signo, info);
 }
 break;
+case EXCP_SEMIHOST:
+env-xregs[0] = do_arm_semihosting(env);
+break;
 default:
 fprintf(stderr, qemu: unhandled CPU exception 0x%x - aborting\n,
 trapnr);
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index e067faa..fd91288 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -56,6 +56,7 @@
 #define EXCP_SMC13   /* Secure Monitor Call */
 #define EXCP_VIRQ   14
 #define EXCP_VFIQ   15
+#define EXCP_SEMIHOST   16   /* semihosting call (A64 only) */
 
 #define ARMV7M_EXCP_RESET   1
 #define ARMV7M_EXCP_NMI 2
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 08c95a3..02fc9b4 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -514,6 +514,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 case EXCP_VFIQ:
 addr += 0x100;
 break;
+case EXCP_SEMIHOST:
+qemu_log_mask(CPU_LOG_INT,
+  ...handling as semihosting call 0x% PRIx64 \n,
+  env-xregs[0]);
+env-xregs[0] = do_arm_semihosting(env);
+return;
 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 924aff9..36a56aa 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -36,6 +36,7 @@ static inline bool excp_is_internal(int excp)
 || excp == EXCP_HALTED
 || excp == EXCP_EXCEPTION_EXIT
 || excp == EXCP_KERNEL_TRAP
+|| excp == EXCP_SEMIHOST
 || excp == EXCP_STREX;
 }
 
@@ -58,6 +59,7 @@ static const char * const excnames[] = {
 [EXCP_SMC] = Secure Monitor Call,
 [EXCP_VIRQ] = Virtual IRQ,
 [EXCP_VFIQ] = Virtual FIQ,
+[EXCP_SEMIHOST] = 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 689f2be..8810760 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -30,6 +30,7 @@
 #include internals.h
 #include qemu/host-utils.h
 
+#include exec/semihost.h
 #include exec/gen-icount.h
 
 #include exec/helper-proto.h
@@ -1553,8 +1554,17 @@ static void disas_exc(DisasContext *s, uint32_t insn)
 unallocated_encoding(s);
 break;
 }
-/* HLT */
-unsupported_encoding(s, insn);
+/* HLT. This has two purposes.
+ * Architecturally, it is an external halting debug instruction.
+ * Since QEMU doesn't implement external debug, we treat this as
+ * it is required for halting debug disabled: it will UNDEF.
+ * Secondly, HLT 0xf000 is the A64 semihosting syscall instruction.
+ */
+if (semihosting_enabled()  imm16 == 0xf000) {
+gen_exception_internal_insn(s, 0, EXCP_SEMIHOST);
+} else {
+unsupported_encoding(s, insn);
+}
 break;
 case 5:
 if (op2_ll  1 || op2_ll  3) {
-- 
1.9.1