[PATCH v5 0/5] semihosting cleanups (plus minor tests/tcg tweak)

[PATCH v5 0/5] semihosting cleanups (plus minor tests/tcg tweak)

[Alex Bennée]

Hi Peter,

I've restored the #ifndef CONFIG_USER_ONLY to ensure A-profile
linux-user still works. I've cleaned up the now unused linux-user code
and added a simple smoke test to make sure we don't break it my
accident. There is a tests/tcg patch at the beginning of the series
which was just fixing some obvious errors in the tests/tcg comments
which I couldn't leave untouched while I was adding the smoke test. I
can put it in my next testing PR but I'm happy enough for you to take
it through your tree if you want.

Alex Bennée (5):
  tests/tcg: clean-up some comments after the de-tangling
  target/arm: handle M-profile semihosting at translate time
  target/arm: handle A-profile semihosting at translate time
  target/arm: remove run time semihosting checks
  target/arm: remove run-time semihosting checks for linux-user

 linux-user/arm/cpu_loop.c         |  3 -
 linux-user/arm/target_syscall.h   |  3 -
 target/arm/helper.c               | 96 +++++++------------------------
 target/arm/m_helper.c             | 18 ++----
 target/arm/translate.c            | 27 +++++++--
 tests/tcg/Makefile.target         |  7 ++-
 tests/tcg/aarch64/Makefile.target |  8 ++-
 tests/tcg/arm/Makefile.target     | 20 ++++---
 tests/tcg/arm/semihosting.c       | 45 +++++++++++++++
 9 files changed, 120 insertions(+), 107 deletions(-)
 create mode 100644 tests/tcg/arm/semihosting.c

-- 
2.20.1

[Qemu-devel] [PATCH v5 0/5] semihosting cleanups (plus minor tests/tcg tweak)

[Alex Bennée]

Hi Peter,

I've restored the #ifndef CONFIG_USER_ONLY to ensure A-profile
linux-user still works. I've cleaned up the now unused linux-user code
and added a simple smoke test to make sure we don't break it my
accident. There is a tests/tcg patch at the beginning of the series
which was just fixing some obvious errors in the tests/tcg comments
which I couldn't leave untouched while I was adding the smoke test. I
can put it in my next testing PR but I'm happy enough for you to take
it through your tree if you want.

Alex Bennée (5):
  tests/tcg: clean-up some comments after the de-tangling
  target/arm: handle M-profile semihosting at translate time
  target/arm: handle A-profile semihosting at translate time
  target/arm: remove run time semihosting checks
  target/arm: remove run-time semihosting checks for linux-user

 linux-user/arm/cpu_loop.c         |  3 -
 linux-user/arm/target_syscall.h   |  3 -
 target/arm/helper.c               | 96 +++++++------------------------
 target/arm/m_helper.c             | 18 ++----
 target/arm/translate.c            | 27 +++++++--
 tests/tcg/Makefile.target         |  7 ++-
 tests/tcg/aarch64/Makefile.target |  8 ++-
 tests/tcg/arm/Makefile.target     | 20 ++++---
 tests/tcg/arm/semihosting.c       | 45 +++++++++++++++
 9 files changed, 120 insertions(+), 107 deletions(-)
 create mode 100644 tests/tcg/arm/semihosting.c

-- 
2.20.1

[PATCH v5 1/5] tests/tcg: clean-up some comments after the de-tangling

[Alex Bennée]

These were missed in the recent de-tangling so have been updated to be
more actuate. I've also built up ARM_TESTS in a manner similar to
AARCH64_TESTS for better consistency.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/Makefile.target         |  7 +++++--
 tests/tcg/aarch64/Makefile.target |  3 ++-
 tests/tcg/arm/Makefile.target     | 15 ++++++++-------
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index 8808beaf74b..679eb56bd37 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -74,8 +74,11 @@ TIMEOUT=15
 endif
 
 ifdef CONFIG_USER_ONLY
-# The order we include is important. We include multiarch, base arch
-# and finally arch if it's not the same as base arch.
+# The order we include is important. We include multiarch first and
+# then the target. If there are common tests shared between
+# sub-targets (e.g. ARM & AArch64) then it is up to
+# $(TARGET_NAME)/Makefile.target to include the common parent
+# architecture in its VPATH.
 -include $(SRC_PATH)/tests/tcg/multiarch/Makefile.target
 -include $(SRC_PATH)/tests/tcg/$(TARGET_NAME)/Makefile.target
 
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index e763dd9da37..9758f89f905 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -8,7 +8,7 @@ VPATH 		+= $(ARM_SRC)
 AARCH64_SRC=$(SRC_PATH)/tests/tcg/aarch64
 VPATH 		+= $(AARCH64_SRC)
 
-# we don't build any other ARM test
+# Float-convert Tests
 AARCH64_TESTS=fcvt
 
 fcvt: LDFLAGS+=-lm
@@ -17,6 +17,7 @@ run-fcvt: fcvt
 	$(call run-test,$<,$(QEMU) $<, "$< on $(TARGET_NAME)")
 	$(call diff-out,$<,$(AARCH64_SRC)/fcvt.ref)
 
+# Pauth Tests
 AARCH64_TESTS += pauth-1 pauth-2
 run-pauth-%: QEMU_OPTS += -cpu max
 
diff --git a/tests/tcg/arm/Makefile.target b/tests/tcg/arm/Makefile.target
index aa4e4e3782c..7347d3d0adb 100644
--- a/tests/tcg/arm/Makefile.target
+++ b/tests/tcg/arm/Makefile.target
@@ -8,25 +8,26 @@ ARM_SRC=$(SRC_PATH)/tests/tcg/arm
 # Set search path for all sources
 VPATH 		+= $(ARM_SRC)
 
-ARM_TESTS=hello-arm test-arm-iwmmxt
-
-TESTS += $(ARM_TESTS) fcvt
-
+# Basic Hello World
+ARM_TESTS = hello-arm
 hello-arm: CFLAGS+=-marm -ffreestanding
 hello-arm: LDFLAGS+=-nostdlib
 
+# IWMXT floating point extensions
+ARM_TESTS += test-arm-iwmmxt
 test-arm-iwmmxt: CFLAGS+=-marm -march=iwmmxt -mabi=aapcs -mfpu=fpv4-sp-d16
 test-arm-iwmmxt: test-arm-iwmmxt.S
 	$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
 
-ifeq ($(TARGET_NAME), arm)
+# Float-convert Tests
+ARM_TESTS += fcvt
 fcvt: LDFLAGS+=-lm
 # fcvt: CFLAGS+=-march=armv8.2-a+fp16 -mfpu=neon-fp-armv8
-
 run-fcvt: fcvt
 	$(call run-test,fcvt,$(QEMU) $<,"$< on $(TARGET_NAME)")
 	$(call diff-out,fcvt,$(ARM_SRC)/fcvt.ref)
-endif
+
+TESTS += $(ARM_TESTS)
 
 # On ARM Linux only supports 4k pages
 EXTRA_RUNS+=run-test-mmap-4096
-- 
2.20.1

[Qemu-devel] [PATCH v5 1/5] tests/tcg: clean-up some comments after the de-tangling

[Alex Bennée]

These were missed in the recent de-tangling so have been updated to be
more actuate. I've also built up ARM_TESTS in a manner similar to
AARCH64_TESTS for better consistency.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/Makefile.target         |  7 +++++--
 tests/tcg/aarch64/Makefile.target |  3 ++-
 tests/tcg/arm/Makefile.target     | 15 ++++++++-------
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index 8808beaf74b..679eb56bd37 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -74,8 +74,11 @@ TIMEOUT=15
 endif
 
 ifdef CONFIG_USER_ONLY
-# The order we include is important. We include multiarch, base arch
-# and finally arch if it's not the same as base arch.
+# The order we include is important. We include multiarch first and
+# then the target. If there are common tests shared between
+# sub-targets (e.g. ARM & AArch64) then it is up to
+# $(TARGET_NAME)/Makefile.target to include the common parent
+# architecture in its VPATH.
 -include $(SRC_PATH)/tests/tcg/multiarch/Makefile.target
 -include $(SRC_PATH)/tests/tcg/$(TARGET_NAME)/Makefile.target
 
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index e763dd9da37..9758f89f905 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -8,7 +8,7 @@ VPATH 		+= $(ARM_SRC)
 AARCH64_SRC=$(SRC_PATH)/tests/tcg/aarch64
 VPATH 		+= $(AARCH64_SRC)
 
-# we don't build any other ARM test
+# Float-convert Tests
 AARCH64_TESTS=fcvt
 
 fcvt: LDFLAGS+=-lm
@@ -17,6 +17,7 @@ run-fcvt: fcvt
 	$(call run-test,$<,$(QEMU) $<, "$< on $(TARGET_NAME)")
 	$(call diff-out,$<,$(AARCH64_SRC)/fcvt.ref)
 
+# Pauth Tests
 AARCH64_TESTS += pauth-1 pauth-2
 run-pauth-%: QEMU_OPTS += -cpu max
 
diff --git a/tests/tcg/arm/Makefile.target b/tests/tcg/arm/Makefile.target
index aa4e4e3782c..7347d3d0adb 100644
--- a/tests/tcg/arm/Makefile.target
+++ b/tests/tcg/arm/Makefile.target
@@ -8,25 +8,26 @@ ARM_SRC=$(SRC_PATH)/tests/tcg/arm
 # Set search path for all sources
 VPATH 		+= $(ARM_SRC)
 
-ARM_TESTS=hello-arm test-arm-iwmmxt
-
-TESTS += $(ARM_TESTS) fcvt
-
+# Basic Hello World
+ARM_TESTS = hello-arm
 hello-arm: CFLAGS+=-marm -ffreestanding
 hello-arm: LDFLAGS+=-nostdlib
 
+# IWMXT floating point extensions
+ARM_TESTS += test-arm-iwmmxt
 test-arm-iwmmxt: CFLAGS+=-marm -march=iwmmxt -mabi=aapcs -mfpu=fpv4-sp-d16
 test-arm-iwmmxt: test-arm-iwmmxt.S
 	$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
 
-ifeq ($(TARGET_NAME), arm)
+# Float-convert Tests
+ARM_TESTS += fcvt
 fcvt: LDFLAGS+=-lm
 # fcvt: CFLAGS+=-march=armv8.2-a+fp16 -mfpu=neon-fp-armv8
-
 run-fcvt: fcvt
 	$(call run-test,fcvt,$(QEMU) $<,"$< on $(TARGET_NAME)")
 	$(call diff-out,fcvt,$(ARM_SRC)/fcvt.ref)
-endif
+
+TESTS += $(ARM_TESTS)
 
 # On ARM Linux only supports 4k pages
 EXTRA_RUNS+=run-test-mmap-4096
-- 
2.20.1

Re: [PATCH v5 1/5] tests/tcg: clean-up some comments after the de-tangling

[Peter Maydell]

On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> These were missed in the recent de-tangling so have been updated to be
> more actuate. I've also built up ARM_TESTS in a manner similar to
> AARCH64_TESTS for better consistency.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 1/5] tests/tcg: clean-up some comments after the de-tangling

[Peter Maydell]

On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> These were missed in the recent de-tangling so have been updated to be
> more actuate. I've also built up ARM_TESTS in a manner similar to
> AARCH64_TESTS for better consistency.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

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

thanks
-- PMM

[PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Alex Bennée]

We do this for other semihosting calls so we might as well do it for
M-profile as well.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---
v2
  - update for change to gen_exception_internal_insn API
v3
  - update for decode tree
v4
  - use !IS_USER
---
 target/arm/m_helper.c  | 18 ++++++------------
 target/arm/translate.c |  8 +++++++-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 884d35d2b02..27cd2f3f964 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2114,19 +2114,13 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
             break;
         }
         break;
+    case EXCP_SEMIHOST:
+        qemu_log_mask(CPU_LOG_INT,
+                      "...handling as semihosting call 0x%x\n",
+                      env->regs[0]);
+        env->regs[0] = do_arm_semihosting(env);
+        return;
     case EXCP_BKPT:
-        if (semihosting_enabled()) {
-            int nr;
-            nr = arm_lduw_code(env, env->regs[15], arm_sctlr_b(env)) & 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);
-                return;
-            }
-        }
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_DEBUG, false);
         break;
     case EXCP_IRQ:
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 34bb280e3da..6689acc911e 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8424,7 +8424,13 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
     if (!ENABLE_ARCH_5) {
         return false;
     }
-    gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
+    if (arm_dc_feature(s, ARM_FEATURE_M) &&
+        semihosting_enabled() && !IS_USER(s) &&
+        (a->imm == 0xab)) {
+        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+    } else {
+        gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
+    }
     return true;
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Alex Bennée]

We do this for other semihosting calls so we might as well do it for
M-profile as well.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---
v2
  - update for change to gen_exception_internal_insn API
v3
  - update for decode tree
v4
  - use !IS_USER
---
 target/arm/m_helper.c  | 18 ++++++------------
 target/arm/translate.c |  8 +++++++-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 884d35d2b02..27cd2f3f964 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2114,19 +2114,13 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
             break;
         }
         break;
+    case EXCP_SEMIHOST:
+        qemu_log_mask(CPU_LOG_INT,
+                      "...handling as semihosting call 0x%x\n",
+                      env->regs[0]);
+        env->regs[0] = do_arm_semihosting(env);
+        return;
     case EXCP_BKPT:
-        if (semihosting_enabled()) {
-            int nr;
-            nr = arm_lduw_code(env, env->regs[15], arm_sctlr_b(env)) & 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);
-                return;
-            }
-        }
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_DEBUG, false);
         break;
     case EXCP_IRQ:
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 34bb280e3da..6689acc911e 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8424,7 +8424,13 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
     if (!ENABLE_ARCH_5) {
         return false;
     }
-    gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
+    if (arm_dc_feature(s, ARM_FEATURE_M) &&
+        semihosting_enabled() && !IS_USER(s) &&
+        (a->imm == 0xab)) {
+        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+    } else {
+        gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
+    }
     return true;
 }
 
-- 
2.20.1

Re: [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Peter Maydell]

On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> We do this for other semihosting calls so we might as well do it for
> M-profile as well.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> ---
> v2
>   - update for change to gen_exception_internal_insn API
> v3
>   - update for decode tree
> v4
>   - use !IS_USER
> ---

> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 34bb280e3da..6689acc911e 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8424,7 +8424,13 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
>      if (!ENABLE_ARCH_5) {
>          return false;
>      }
> -    gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
> +    if (arm_dc_feature(s, ARM_FEATURE_M) &&
> +        semihosting_enabled() && !IS_USER(s) &&
> +        (a->imm == 0xab)) {
> +        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
> +    } else {
> +        gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
> +    }
>      return true;
>  }

This is still disabling semihosting for the linux-user-mode
build for M-profile, isn't it ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Peter Maydell]

On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> We do this for other semihosting calls so we might as well do it for
> M-profile as well.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> ---
> v2
>   - update for change to gen_exception_internal_insn API
> v3
>   - update for decode tree
> v4
>   - use !IS_USER
> ---

> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 34bb280e3da..6689acc911e 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8424,7 +8424,13 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
>      if (!ENABLE_ARCH_5) {
>          return false;
>      }
> -    gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
> +    if (arm_dc_feature(s, ARM_FEATURE_M) &&
> +        semihosting_enabled() && !IS_USER(s) &&
> +        (a->imm == 0xab)) {
> +        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
> +    } else {
> +        gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
> +    }
>      return true;
>  }

This is still disabling semihosting for the linux-user-mode
build for M-profile, isn't it ?

thanks
-- PMM

Re: [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> We do this for other semihosting calls so we might as well do it for
>> M-profile as well.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> ---
>> v2
>>   - update for change to gen_exception_internal_insn API
>> v3
>>   - update for decode tree
>> v4
>>   - use !IS_USER
>> ---
>
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index 34bb280e3da..6689acc911e 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -8424,7 +8424,13 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
>>      if (!ENABLE_ARCH_5) {
>>          return false;
>>      }
>> -    gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
>> +    if (arm_dc_feature(s, ARM_FEATURE_M) &&
>> +        semihosting_enabled() && !IS_USER(s) &&
>> +        (a->imm == 0xab)) {
>> +        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
>> +    } else {
>> +        gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
>> +    }
>>      return true;
>>  }
>
> This is still disabling semihosting for the linux-user-mode
> build for M-profile, isn't it ?

Sure - as rth suggested. But m-profile doesn't run Linux so why would we
support it in linux-user?

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> We do this for other semihosting calls so we might as well do it for
>> M-profile as well.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> ---
>> v2
>>   - update for change to gen_exception_internal_insn API
>> v3
>>   - update for decode tree
>> v4
>>   - use !IS_USER
>> ---
>
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index 34bb280e3da..6689acc911e 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -8424,7 +8424,13 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
>>      if (!ENABLE_ARCH_5) {
>>          return false;
>>      }
>> -    gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
>> +    if (arm_dc_feature(s, ARM_FEATURE_M) &&
>> +        semihosting_enabled() && !IS_USER(s) &&
>> +        (a->imm == 0xab)) {
>> +        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
>> +    } else {
>> +        gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
>> +    }
>>      return true;
>>  }
>
> This is still disabling semihosting for the linux-user-mode
> build for M-profile, isn't it ?

Sure - as rth suggested. But m-profile doesn't run Linux so why would we
support it in linux-user?

>
> thanks
> -- PMM


--
Alex Bennée

Re: [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Peter Maydell]

On Fri, 13 Sep 2019 at 14:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > This is still disabling semihosting for the linux-user-mode
> > build for M-profile, isn't it ?
>
> Sure - as rth suggested. But m-profile doesn't run Linux so why would we
> support it in linux-user?

(a) Linux does support Cortex-M, since it has no-mmu uCLinux style
support these days
(b) gcc test case binaries, which tend to get run on linux-user
even though in theory they're not actually Linux binaries

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Peter Maydell]

On Fri, 13 Sep 2019 at 14:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > This is still disabling semihosting for the linux-user-mode
> > build for M-profile, isn't it ?
>
> Sure - as rth suggested. But m-profile doesn't run Linux so why would we
> support it in linux-user?

(a) Linux does support Cortex-M, since it has no-mmu uCLinux style
support these days
(b) gcc test case binaries, which tend to get run on linux-user
even though in theory they're not actually Linux binaries

thanks
-- PMM

Re: [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Peter Maydell]

On Fri, 13 Sep 2019 at 14:31, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 13 Sep 2019 at 14:28, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> > > This is still disabling semihosting for the linux-user-mode
> > > build for M-profile, isn't it ?
> >
> > Sure - as rth suggested. But m-profile doesn't run Linux so why would we
> > support it in linux-user?
>
> (a) Linux does support Cortex-M, since it has no-mmu uCLinux style
> support these days
> (b) gcc test case binaries, which tend to get run on linux-user
> even though in theory they're not actually Linux binaries

also I think (c) general principle -- don't change behaviour in what
looks like it's just a refactoring patchset.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Peter Maydell]

On Fri, 13 Sep 2019 at 14:31, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 13 Sep 2019 at 14:28, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> > > This is still disabling semihosting for the linux-user-mode
> > > build for M-profile, isn't it ?
> >
> > Sure - as rth suggested. But m-profile doesn't run Linux so why would we
> > support it in linux-user?
>
> (a) Linux does support Cortex-M, since it has no-mmu uCLinux style
> support these days
> (b) gcc test case binaries, which tend to get run on linux-user
> even though in theory they're not actually Linux binaries

also I think (c) general principle -- don't change behaviour in what
looks like it's just a refactoring patchset.

thanks
-- PMM

Re: [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 13 Sep 2019 at 14:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > This is still disabling semihosting for the linux-user-mode
>> > build for M-profile, isn't it ?
>>
>> Sure - as rth suggested. But m-profile doesn't run Linux so why would we
>> support it in linux-user?
>
> (a) Linux does support Cortex-M, since it has no-mmu uCLinux style
> support these days

TIL

> (b) gcc test case binaries, which tend to get run on linux-user
> even though in theory they're not actually Linux binaries

Ahh ok - that's fair. I'll add the ifndef for that then.

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH v5 2/5] target/arm: handle M-profile semihosting at translate time

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 13 Sep 2019 at 14:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > This is still disabling semihosting for the linux-user-mode
>> > build for M-profile, isn't it ?
>>
>> Sure - as rth suggested. But m-profile doesn't run Linux so why would we
>> support it in linux-user?
>
> (a) Linux does support Cortex-M, since it has no-mmu uCLinux style
> support these days

TIL

> (b) gcc test case binaries, which tend to get run on linux-user
> even though in theory they're not actually Linux binaries

Ahh ok - that's fair. I'll add the ifndef for that then.

>
> thanks
> -- PMM


--
Alex Bennée

[PATCH v5 3/5] target/arm: handle A-profile semihosting at translate time

[Alex Bennée]

As for the other semihosting calls we can resolve this at translate
time.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - update for change to gen_exception_internal_insn API
v3
  - update for decode tree, merge T32 & A32 commits
  - dropped r-b due to changes
v4
  - !IS_USER and !arm_dc_feature(s, ARM_FEATURE_M)
v5
  - only if !IS_USER for softmmu, linux-user is still allowed
---
 target/arm/translate.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6689acc911e..fac791c4b06 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10219,14 +10219,25 @@ static bool trans_CBZ(DisasContext *s, arg_CBZ *a)
 }
 
 /*
- * Supervisor call
+ * Supervisor call - both T32 & A32 come here so we need to check
+ * which mode we are in when checking for semihosting.
  */
 
 static bool trans_SVC(DisasContext *s, arg_SVC *a)
 {
-    gen_set_pc_im(s, s->base.pc_next);
-    s->svc_imm = a->imm;
-    s->base.is_jmp = DISAS_SWI;
+    const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
+
+    if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled() &&
+#ifndef CONFIG_USER_ONLY
+        !IS_USER(s) &&
+#endif
+        (a->imm == semihost_imm)) {
+        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+    } else {
+        gen_set_pc_im(s, s->base.pc_next);
+        s->svc_imm = a->imm;
+        s->base.is_jmp = DISAS_SWI;
+    }
     return true;
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH v5 3/5] target/arm: handle A-profile semihosting at translate time

[Alex Bennée]

As for the other semihosting calls we can resolve this at translate
time.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - update for change to gen_exception_internal_insn API
v3
  - update for decode tree, merge T32 & A32 commits
  - dropped r-b due to changes
v4
  - !IS_USER and !arm_dc_feature(s, ARM_FEATURE_M)
v5
  - only if !IS_USER for softmmu, linux-user is still allowed
---
 target/arm/translate.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6689acc911e..fac791c4b06 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10219,14 +10219,25 @@ static bool trans_CBZ(DisasContext *s, arg_CBZ *a)
 }
 
 /*
- * Supervisor call
+ * Supervisor call - both T32 & A32 come here so we need to check
+ * which mode we are in when checking for semihosting.
  */
 
 static bool trans_SVC(DisasContext *s, arg_SVC *a)
 {
-    gen_set_pc_im(s, s->base.pc_next);
-    s->svc_imm = a->imm;
-    s->base.is_jmp = DISAS_SWI;
+    const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
+
+    if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled() &&
+#ifndef CONFIG_USER_ONLY
+        !IS_USER(s) &&
+#endif
+        (a->imm == semihost_imm)) {
+        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+    } else {
+        gen_set_pc_im(s, s->base.pc_next);
+        s->svc_imm = a->imm;
+        s->base.is_jmp = DISAS_SWI;
+    }
     return true;
 }
 
-- 
2.20.1

Re: [PATCH v5 3/5] target/arm: handle A-profile semihosting at translate time

[Peter Maydell]

On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> As for the other semihosting calls we can resolve this at translate
> time.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - update for change to gen_exception_internal_insn API
> v3
>   - update for decode tree, merge T32 & A32 commits
>   - dropped r-b due to changes
> v4
>   - !IS_USER and !arm_dc_feature(s, ARM_FEATURE_M)
> v5
>   - only if !IS_USER for softmmu, linux-user is still allowed
> ---
>  target/arm/translate.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 6689acc911e..fac791c4b06 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10219,14 +10219,25 @@ static bool trans_CBZ(DisasContext *s, arg_CBZ *a)
>  }
>
>  /*
> - * Supervisor call
> + * Supervisor call - both T32 & A32 come here so we need to check
> + * which mode we are in when checking for semihosting.
>   */
>
>  static bool trans_SVC(DisasContext *s, arg_SVC *a)
>  {
> -    gen_set_pc_im(s, s->base.pc_next);
> -    s->svc_imm = a->imm;
> -    s->base.is_jmp = DISAS_SWI;
> +    const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
> +
> +    if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled() &&
> +#ifndef CONFIG_USER_ONLY
> +        !IS_USER(s) &&
> +#endif
> +        (a->imm == semihost_imm)) {
> +        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
> +    } else {
> +        gen_set_pc_im(s, s->base.pc_next);
> +        s->svc_imm = a->imm;
> +        s->base.is_jmp = DISAS_SWI;
> +    }
>      return true;
>  }

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 3/5] target/arm: handle A-profile semihosting at translate time

[Peter Maydell]

On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> As for the other semihosting calls we can resolve this at translate
> time.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - update for change to gen_exception_internal_insn API
> v3
>   - update for decode tree, merge T32 & A32 commits
>   - dropped r-b due to changes
> v4
>   - !IS_USER and !arm_dc_feature(s, ARM_FEATURE_M)
> v5
>   - only if !IS_USER for softmmu, linux-user is still allowed
> ---
>  target/arm/translate.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 6689acc911e..fac791c4b06 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10219,14 +10219,25 @@ static bool trans_CBZ(DisasContext *s, arg_CBZ *a)
>  }
>
>  /*
> - * Supervisor call
> + * Supervisor call - both T32 & A32 come here so we need to check
> + * which mode we are in when checking for semihosting.
>   */
>
>  static bool trans_SVC(DisasContext *s, arg_SVC *a)
>  {
> -    gen_set_pc_im(s, s->base.pc_next);
> -    s->svc_imm = a->imm;
> -    s->base.is_jmp = DISAS_SWI;
> +    const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
> +
> +    if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled() &&
> +#ifndef CONFIG_USER_ONLY
> +        !IS_USER(s) &&
> +#endif
> +        (a->imm == semihost_imm)) {
> +        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
> +    } else {
> +        gen_set_pc_im(s, s->base.pc_next);
> +        s->svc_imm = a->imm;
> +        s->base.is_jmp = DISAS_SWI;
> +    }
>      return true;
>  }

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

thanks
-- PMM

[PATCH v5 4/5] target/arm: remove run time semihosting checks

[Alex Bennée]

Now we do all our checking and use a common EXCP_SEMIHOST for
semihosting operations we can make helper code a lot simpler.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
v2
  - fix re-base conflicts
  - hoist EXCP_SEMIHOST check
  - comment cleanups
v5
  - move CONFIG_TCG ifdefs
---
 target/arm/helper.c | 96 +++++++++++----------------------------------
 1 file changed, 22 insertions(+), 74 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 507026c9154..a87ae6d46a1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8339,88 +8339,32 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
                   new_el, env->pc, pstate_read(env));
 }
 
-static inline bool check_for_semihosting(CPUState *cs)
-{
+/*
+ * Do semihosting call and set the appropriate return value. All the
+ * permission and validity checks have been done at translate time.
+ *
+ * We only see semihosting exceptions in TCG only as they are not
+ * trapped to the hypervisor in KVM.
+ */
 #ifdef CONFIG_TCG
-    /* Check whether this exception is a semihosting call; if so
-     * then handle it and return true; otherwise return false.
-     */
+static void handle_semihosting(CPUState *cs)
+{
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
 
     if (is_a64(env)) {
-        if (cs->exception_index == EXCP_SEMIHOST) {
-            /* This is always the 64-bit semihosting exception.
-             * The "is this usermode" and "is semihosting enabled"
-             * checks have been done at translate time.
-             */
-            qemu_log_mask(CPU_LOG_INT,
-                          "...handling as semihosting call 0x%" PRIx64 "\n",
-                          env->xregs[0]);
-            env->xregs[0] = do_arm_semihosting(env);
-            return true;
-        }
-        return false;
+        qemu_log_mask(CPU_LOG_INT,
+                      "...handling as semihosting call 0x%" PRIx64 "\n",
+                      env->xregs[0]);
+        env->xregs[0] = do_arm_semihosting(env);
     } else {
-        uint32_t imm;
-
-        /* Only intercept calls from privileged modes, to provide some
-         * semblance of security.
-         */
-        if (cs->exception_index != EXCP_SEMIHOST &&
-            (!semihosting_enabled() ||
-             ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_USR))) {
-            return false;
-        }
-
-        switch (cs->exception_index) {
-        case EXCP_SEMIHOST:
-            /* This is always a semihosting call; the "is this usermode"
-             * and "is semihosting enabled" checks have been done at
-             * translate time.
-             */
-            break;
-        case EXCP_SWI:
-            /* Check for semihosting interrupt.  */
-            if (env->thumb) {
-                imm = arm_lduw_code(env, env->regs[15] - 2, arm_sctlr_b(env))
-                    & 0xff;
-                if (imm == 0xab) {
-                    break;
-                }
-            } else {
-                imm = arm_ldl_code(env, env->regs[15] - 4, arm_sctlr_b(env))
-                    & 0xffffff;
-                if (imm == 0x123456) {
-                    break;
-                }
-            }
-            return false;
-        case EXCP_BKPT:
-            /* See if this is a semihosting syscall.  */
-            if (env->thumb) {
-                imm = arm_lduw_code(env, env->regs[15], arm_sctlr_b(env))
-                    & 0xff;
-                if (imm == 0xab) {
-                    env->regs[15] += 2;
-                    break;
-                }
-            }
-            return false;
-        default:
-            return false;
-        }
-
         qemu_log_mask(CPU_LOG_INT,
                       "...handling as semihosting call 0x%x\n",
                       env->regs[0]);
         env->regs[0] = do_arm_semihosting(env);
-        return true;
     }
-#else
-    return false;
-#endif
 }
+#endif
 
 /* Handle a CPU exception for A and R profile CPUs.
  * Do any appropriate logging, handle PSCI calls, and then hand off
@@ -8451,13 +8395,17 @@ void arm_cpu_do_interrupt(CPUState *cs)
         return;
     }
 
-    /* Semihosting semantics depend on the register width of the
-     * code that caused the exception, not the target exception level,
-     * so must be handled here.
+    /*
+     * Semihosting semantics depend on the register width of the code
+     * that caused the exception, not the target exception level, so
+     * must be handled here.
      */
-    if (check_for_semihosting(cs)) {
+#ifdef CONFIG_TCG
+    if (cs->exception_index == EXCP_SEMIHOST) {
+        handle_semihosting(cs);
         return;
     }
+#endif
 
     /* Hooks may change global state so BQL should be held, also the
      * BQL needs to be held for any modification of
-- 
2.20.1

[Qemu-devel] [PATCH v5 4/5] target/arm: remove run time semihosting checks

[Alex Bennée]

Now we do all our checking and use a common EXCP_SEMIHOST for
semihosting operations we can make helper code a lot simpler.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
v2
  - fix re-base conflicts
  - hoist EXCP_SEMIHOST check
  - comment cleanups
v5
  - move CONFIG_TCG ifdefs
---
 target/arm/helper.c | 96 +++++++++++----------------------------------
 1 file changed, 22 insertions(+), 74 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 507026c9154..a87ae6d46a1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8339,88 +8339,32 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
                   new_el, env->pc, pstate_read(env));
 }
 
-static inline bool check_for_semihosting(CPUState *cs)
-{
+/*
+ * Do semihosting call and set the appropriate return value. All the
+ * permission and validity checks have been done at translate time.
+ *
+ * We only see semihosting exceptions in TCG only as they are not
+ * trapped to the hypervisor in KVM.
+ */
 #ifdef CONFIG_TCG
-    /* Check whether this exception is a semihosting call; if so
-     * then handle it and return true; otherwise return false.
-     */
+static void handle_semihosting(CPUState *cs)
+{
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
 
     if (is_a64(env)) {
-        if (cs->exception_index == EXCP_SEMIHOST) {
-            /* This is always the 64-bit semihosting exception.
-             * The "is this usermode" and "is semihosting enabled"
-             * checks have been done at translate time.
-             */
-            qemu_log_mask(CPU_LOG_INT,
-                          "...handling as semihosting call 0x%" PRIx64 "\n",
-                          env->xregs[0]);
-            env->xregs[0] = do_arm_semihosting(env);
-            return true;
-        }
-        return false;
+        qemu_log_mask(CPU_LOG_INT,
+                      "...handling as semihosting call 0x%" PRIx64 "\n",
+                      env->xregs[0]);
+        env->xregs[0] = do_arm_semihosting(env);
     } else {
-        uint32_t imm;
-
-        /* Only intercept calls from privileged modes, to provide some
-         * semblance of security.
-         */
-        if (cs->exception_index != EXCP_SEMIHOST &&
-            (!semihosting_enabled() ||
-             ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_USR))) {
-            return false;
-        }
-
-        switch (cs->exception_index) {
-        case EXCP_SEMIHOST:
-            /* This is always a semihosting call; the "is this usermode"
-             * and "is semihosting enabled" checks have been done at
-             * translate time.
-             */
-            break;
-        case EXCP_SWI:
-            /* Check for semihosting interrupt.  */
-            if (env->thumb) {
-                imm = arm_lduw_code(env, env->regs[15] - 2, arm_sctlr_b(env))
-                    & 0xff;
-                if (imm == 0xab) {
-                    break;
-                }
-            } else {
-                imm = arm_ldl_code(env, env->regs[15] - 4, arm_sctlr_b(env))
-                    & 0xffffff;
-                if (imm == 0x123456) {
-                    break;
-                }
-            }
-            return false;
-        case EXCP_BKPT:
-            /* See if this is a semihosting syscall.  */
-            if (env->thumb) {
-                imm = arm_lduw_code(env, env->regs[15], arm_sctlr_b(env))
-                    & 0xff;
-                if (imm == 0xab) {
-                    env->regs[15] += 2;
-                    break;
-                }
-            }
-            return false;
-        default:
-            return false;
-        }
-
         qemu_log_mask(CPU_LOG_INT,
                       "...handling as semihosting call 0x%x\n",
                       env->regs[0]);
         env->regs[0] = do_arm_semihosting(env);
-        return true;
     }
-#else
-    return false;
-#endif
 }
+#endif
 
 /* Handle a CPU exception for A and R profile CPUs.
  * Do any appropriate logging, handle PSCI calls, and then hand off
@@ -8451,13 +8395,17 @@ void arm_cpu_do_interrupt(CPUState *cs)
         return;
     }
 
-    /* Semihosting semantics depend on the register width of the
-     * code that caused the exception, not the target exception level,
-     * so must be handled here.
+    /*
+     * Semihosting semantics depend on the register width of the code
+     * that caused the exception, not the target exception level, so
+     * must be handled here.
      */
-    if (check_for_semihosting(cs)) {
+#ifdef CONFIG_TCG
+    if (cs->exception_index == EXCP_SEMIHOST) {
+        handle_semihosting(cs);
         return;
     }
+#endif
 
     /* Hooks may change global state so BQL should be held, also the
      * BQL needs to be held for any modification of
-- 
2.20.1

[PATCH v5 5/5] target/arm: remove run-time semihosting checks for linux-user

[Alex Bennée]

Now we do all our checking at translate time we can make cpu_loop a
little bit simpler. We also introduce a simple linux-user semihosting
test case to defend the functionality. The out-of-tree softmmu based
semihosting tests are still more comprehensive.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/arm/cpu_loop.c         |  3 ---
 linux-user/arm/target_syscall.h   |  3 ---
 tests/tcg/aarch64/Makefile.target |  5 ++++
 tests/tcg/arm/Makefile.target     |  5 ++++
 tests/tcg/arm/semihosting.c       | 45 +++++++++++++++++++++++++++++++
 5 files changed, 55 insertions(+), 6 deletions(-)
 create mode 100644 tests/tcg/arm/semihosting.c

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 8d65de5b9f4..e28c45cd4ab 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -325,9 +325,6 @@ void cpu_loop(CPUARMState *env)
 
                 if (n == ARM_NR_cacheflush) {
                     /* nop */
-                } else if (n == ARM_NR_semihosting
-                           || n == ARM_NR_thumb_semihosting) {
-                    env->regs[0] = do_arm_semihosting (env);
                 } else if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
                     /* linux syscall */
                     if (env->thumb || n == 0) {
diff --git a/linux-user/arm/target_syscall.h b/linux-user/arm/target_syscall.h
index afc0772e194..f85cbdaf56f 100644
--- a/linux-user/arm/target_syscall.h
+++ b/linux-user/arm/target_syscall.h
@@ -18,9 +18,6 @@ struct target_pt_regs {
 #define ARM_NR_set_tls	  (ARM_NR_BASE + 5)
 #define ARM_NR_get_tls    (ARM_NR_BASE + 6)
 
-#define ARM_NR_semihosting	  0x123456
-#define ARM_NR_thumb_semihosting  0xAB
-
 #if defined(TARGET_WORDS_BIGENDIAN)
 #define UNAME_MACHINE "armv5teb"
 #else
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 9758f89f905..509f1afa93d 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -21,4 +21,9 @@ run-fcvt: fcvt
 AARCH64_TESTS += pauth-1 pauth-2
 run-pauth-%: QEMU_OPTS += -cpu max
 
+# Semihosting smoke test for linux-user
+AARCH64_TESTS += semihosting
+run-semihosting: semihosting
+	$(call run-test,$<,$(QEMU) $< 2> $<.err, "$< on $(TARGET_NAME)")
+
 TESTS += $(AARCH64_TESTS)
diff --git a/tests/tcg/arm/Makefile.target b/tests/tcg/arm/Makefile.target
index 7347d3d0adb..3b7fc9a64be 100644
--- a/tests/tcg/arm/Makefile.target
+++ b/tests/tcg/arm/Makefile.target
@@ -27,6 +27,11 @@ run-fcvt: fcvt
 	$(call run-test,fcvt,$(QEMU) $<,"$< on $(TARGET_NAME)")
 	$(call diff-out,fcvt,$(ARM_SRC)/fcvt.ref)
 
+# Semihosting smoke test for linux-user
+ARM_TESTS += semihosting
+run-semihosting: semihosting
+	$(call run-test,$<,$(QEMU) $< 2> $<.err, "$< on $(TARGET_NAME)")
+
 TESTS += $(ARM_TESTS)
 
 # On ARM Linux only supports 4k pages
diff --git a/tests/tcg/arm/semihosting.c b/tests/tcg/arm/semihosting.c
new file mode 100644
index 00000000000..09c89cb481a
--- /dev/null
+++ b/tests/tcg/arm/semihosting.c
@@ -0,0 +1,45 @@
+/*
+ * linux-user semihosting checks
+ *
+ * Copyright (c) 2019
+ * Written by Alex Bennée <alex.bennee@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-3.0-or-later
+ */
+
+#include <stdint.h>
+
+#define SYS_WRITE0      0x04
+#define SYS_REPORTEXC   0x18
+
+void __semi_call(uintptr_t type, uintptr_t arg0)
+{
+#if defined(__arm__)
+    register uintptr_t t asm("r0") = type;
+    register uintptr_t a0 asm("r1") = arg0;
+    asm("svc 0xab"
+        : /* no return */
+        : "r" (t), "r" (a0));
+#else
+    register uintptr_t t asm("x0") = type;
+    register uintptr_t a0 asm("x1") = arg0;
+    asm("hlt 0xf000"
+        : /* no return */
+        : "r" (t), "r" (a0));
+#endif
+}
+
+int main(int argc, char *argv[argc])
+{
+#if defined(__arm__)
+    uintptr_t exit_code = 0x20026;
+#else
+    uintptr_t exit_block[2] = {0x20026, 0};
+    uintptr_t exit_code = (uintptr_t) &exit_block;
+#endif
+
+    __semi_call(SYS_WRITE0, (uintptr_t) "Hello World");
+    __semi_call(SYS_REPORTEXC, exit_code);
+    /* if we get here we failed */
+    return -1;
+}
-- 
2.20.1

[Qemu-devel] [PATCH v5 5/5] target/arm: remove run-time semihosting checks for linux-user

[Alex Bennée]

Now we do all our checking at translate time we can make cpu_loop a
little bit simpler. We also introduce a simple linux-user semihosting
test case to defend the functionality. The out-of-tree softmmu based
semihosting tests are still more comprehensive.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/arm/cpu_loop.c         |  3 ---
 linux-user/arm/target_syscall.h   |  3 ---
 tests/tcg/aarch64/Makefile.target |  5 ++++
 tests/tcg/arm/Makefile.target     |  5 ++++
 tests/tcg/arm/semihosting.c       | 45 +++++++++++++++++++++++++++++++
 5 files changed, 55 insertions(+), 6 deletions(-)
 create mode 100644 tests/tcg/arm/semihosting.c

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 8d65de5b9f4..e28c45cd4ab 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -325,9 +325,6 @@ void cpu_loop(CPUARMState *env)
 
                 if (n == ARM_NR_cacheflush) {
                     /* nop */
-                } else if (n == ARM_NR_semihosting
-                           || n == ARM_NR_thumb_semihosting) {
-                    env->regs[0] = do_arm_semihosting (env);
                 } else if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
                     /* linux syscall */
                     if (env->thumb || n == 0) {
diff --git a/linux-user/arm/target_syscall.h b/linux-user/arm/target_syscall.h
index afc0772e194..f85cbdaf56f 100644
--- a/linux-user/arm/target_syscall.h
+++ b/linux-user/arm/target_syscall.h
@@ -18,9 +18,6 @@ struct target_pt_regs {
 #define ARM_NR_set_tls	  (ARM_NR_BASE + 5)
 #define ARM_NR_get_tls    (ARM_NR_BASE + 6)
 
-#define ARM_NR_semihosting	  0x123456
-#define ARM_NR_thumb_semihosting  0xAB
-
 #if defined(TARGET_WORDS_BIGENDIAN)
 #define UNAME_MACHINE "armv5teb"
 #else
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 9758f89f905..509f1afa93d 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -21,4 +21,9 @@ run-fcvt: fcvt
 AARCH64_TESTS += pauth-1 pauth-2
 run-pauth-%: QEMU_OPTS += -cpu max
 
+# Semihosting smoke test for linux-user
+AARCH64_TESTS += semihosting
+run-semihosting: semihosting
+	$(call run-test,$<,$(QEMU) $< 2> $<.err, "$< on $(TARGET_NAME)")
+
 TESTS += $(AARCH64_TESTS)
diff --git a/tests/tcg/arm/Makefile.target b/tests/tcg/arm/Makefile.target
index 7347d3d0adb..3b7fc9a64be 100644
--- a/tests/tcg/arm/Makefile.target
+++ b/tests/tcg/arm/Makefile.target
@@ -27,6 +27,11 @@ run-fcvt: fcvt
 	$(call run-test,fcvt,$(QEMU) $<,"$< on $(TARGET_NAME)")
 	$(call diff-out,fcvt,$(ARM_SRC)/fcvt.ref)
 
+# Semihosting smoke test for linux-user
+ARM_TESTS += semihosting
+run-semihosting: semihosting
+	$(call run-test,$<,$(QEMU) $< 2> $<.err, "$< on $(TARGET_NAME)")
+
 TESTS += $(ARM_TESTS)
 
 # On ARM Linux only supports 4k pages
diff --git a/tests/tcg/arm/semihosting.c b/tests/tcg/arm/semihosting.c
new file mode 100644
index 00000000000..09c89cb481a
--- /dev/null
+++ b/tests/tcg/arm/semihosting.c
@@ -0,0 +1,45 @@
+/*
+ * linux-user semihosting checks
+ *
+ * Copyright (c) 2019
+ * Written by Alex Bennée <alex.bennee@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-3.0-or-later
+ */
+
+#include <stdint.h>
+
+#define SYS_WRITE0      0x04
+#define SYS_REPORTEXC   0x18
+
+void __semi_call(uintptr_t type, uintptr_t arg0)
+{
+#if defined(__arm__)
+    register uintptr_t t asm("r0") = type;
+    register uintptr_t a0 asm("r1") = arg0;
+    asm("svc 0xab"
+        : /* no return */
+        : "r" (t), "r" (a0));
+#else
+    register uintptr_t t asm("x0") = type;
+    register uintptr_t a0 asm("x1") = arg0;
+    asm("hlt 0xf000"
+        : /* no return */
+        : "r" (t), "r" (a0));
+#endif
+}
+
+int main(int argc, char *argv[argc])
+{
+#if defined(__arm__)
+    uintptr_t exit_code = 0x20026;
+#else
+    uintptr_t exit_block[2] = {0x20026, 0};
+    uintptr_t exit_code = (uintptr_t) &exit_block;
+#endif
+
+    __semi_call(SYS_WRITE0, (uintptr_t) "Hello World");
+    __semi_call(SYS_REPORTEXC, exit_code);
+    /* if we get here we failed */
+    return -1;
+}
-- 
2.20.1

Re: [PATCH v5 5/5] target/arm: remove run-time semihosting checks for linux-user

[Peter Maydell]

On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Now we do all our checking at translate time we can make cpu_loop a
> little bit simpler. We also introduce a simple linux-user semihosting
> test case to defend the functionality. The out-of-tree softmmu based
> semihosting tests are still more comprehensive.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Could you not put the code cleanup in the same commit as
the test case, please ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 5/5] target/arm: remove run-time semihosting checks for linux-user

[Peter Maydell]

On Wed, 11 Sep 2019 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Now we do all our checking at translate time we can make cpu_loop a
> little bit simpler. We also introduce a simple linux-user semihosting
> test case to defend the functionality. The out-of-tree softmmu based
> semihosting tests are still more comprehensive.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Could you not put the code cleanup in the same commit as
the test case, please ?

thanks
-- PMM