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>
---
 target/arm/helper.c | 84 +++++++++------------------------------------
 1 file changed, 17 insertions(+), 67 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index ad29dc4072..5c1f741380 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
                   new_el, env->pc, pstate_read(env));
 }
 
-static inline bool check_for_semihosting(CPUState *cs)
+/*
+ * Check whether this exception is a semihosting call; if so
+ * then handle it and return true; otherwise return false.
+ *
+ * All the permission and validity checks are done at translate time.
+ */
+static inline bool handle_semihosting(CPUState *cs)
 {
-    /* Check whether this exception is a semihosting call; if so
-     * then handle it and return true; otherwise return false.
-     */
     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.
-             */
+    if (cs->exception_index == EXCP_SEMIHOST) {
+        if (is_a64(env)) {
             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;
-    } 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;
+        } else {
+            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,
-                      "...handling as semihosting call 0x%x\n",
-                      env->regs[0]);
-        env->regs[0] = do_arm_semihosting(env);
         return true;
     }
+
+    return false;
 }
 
 /* Handle a CPU exception for A and R profile CPUs.
@@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
      * code that caused the exception, not the target exception level,
      * so must be handled here.
      */
-    if (check_for_semihosting(cs)) {
+    if (handle_semihosting(cs)) {
         return;
     }
 
-- 
2.20.1

On 7/3/19 5:52 PM, Alex Bennée wrote:
> 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>
> ---
>  target/arm/helper.c | 84 +++++++++------------------------------------
>  1 file changed, 17 insertions(+), 67 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ad29dc4072..5c1f741380 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>                    new_el, env->pc, pstate_read(env));
>  }
>  
> -static inline bool check_for_semihosting(CPUState *cs)
> +/*
> + * Check whether this exception is a semihosting call; if so
> + * then handle it and return true; otherwise return false.
> + *
> + * All the permission and validity checks are done at translate time.
> + */
> +static inline bool handle_semihosting(CPUState *cs)

Drop the inline, probably.

Since you are renaming away the "check", perhaps hoist

> +    if (cs->exception_index == EXCP_SEMIHOST) {

this check to the caller, change the return to void,

> @@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>       * code that caused the exception, not the target exception level,
>       * so must be handled here.
>       */
> -    if (check_for_semihosting(cs)) {
> +    if (handle_semihosting(cs)) {
>          return;
>      }

so we get

	if (cs->exception_index == EXCP_SEMIHOST) {
	    handle_semihosting(cs);
	    return;
	}


r~

Hi Alex, Peter.

On 7/3/19 5:52 PM, Alex Bennée wrote:
> 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>
> ---
>  target/arm/helper.c | 84 +++++++++------------------------------------
>  1 file changed, 17 insertions(+), 67 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ad29dc4072..5c1f741380 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>                    new_el, env->pc, pstate_read(env));
>  }
>  
> -static inline bool check_for_semihosting(CPUState *cs)
> +/*
> + * Check whether this exception is a semihosting call; if so
> + * then handle it and return true; otherwise return false.
> + *
> + * All the permission and validity checks are done at translate time.
> + */
> +static inline bool handle_semihosting(CPUState *cs)
>  {
> -    /* Check whether this exception is a semihosting call; if so
> -     * then handle it and return true; otherwise return false.
> -     */
>      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.
> -             */
> +    if (cs->exception_index == EXCP_SEMIHOST) {
> +        if (is_a64(env)) {
>              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;
> -    } 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;
> +        } else {
> +            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,
> -                      "...handling as semihosting call 0x%x\n",
> -                      env->regs[0]);
> -        env->regs[0] = do_arm_semihosting(env);
>          return true;
>      }
> +
> +    return false;
>  }
>  
>  /* Handle a CPU exception for A and R profile CPUs.
> @@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>       * code that caused the exception, not the target exception level,
>       * so must be handled here.
>       */
> -    if (check_for_semihosting(cs)) {
> +    if (handle_semihosting(cs)) {
>          return;
>      }
>  
> 

This patch clashes with this one:
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00343.html
that Peter already queued:
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00692.html

Peter, if you want this for 4.1, it might be easier to dequeue "Restrict
semi-hosting to TCG", apply Alex series, and re-apply "Restrict
semi-hosting to TCG", the conflicts should be trivial (function name
changed check_for_semihosting -> handle_semihosting).

Thanks,

Phil.

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Hi Alex, Peter.
>
> On 7/3/19 5:52 PM, Alex Bennée wrote:
>> 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>
>> ---
>>  target/arm/helper.c | 84 +++++++++------------------------------------
>>  1 file changed, 17 insertions(+), 67 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index ad29dc4072..5c1f741380 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>>                    new_el, env->pc, pstate_read(env));
>>  }
>>
>> -static inline bool check_for_semihosting(CPUState *cs)
>> +/*
>> + * Check whether this exception is a semihosting call; if so
>> + * then handle it and return true; otherwise return false.
>> + *
>> + * All the permission and validity checks are done at translate time.
>> + */
>> +static inline bool handle_semihosting(CPUState *cs)
>>  {
>> -    /* Check whether this exception is a semihosting call; if so
>> -     * then handle it and return true; otherwise return false.
>> -     */
>>      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.
>> -             */
>> +    if (cs->exception_index == EXCP_SEMIHOST) {
>> +        if (is_a64(env)) {
>>              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;
>> -    } 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;
>> +        } else {
>> +            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,
>> -                      "...handling as semihosting call 0x%x\n",
>> -                      env->regs[0]);
>> -        env->regs[0] = do_arm_semihosting(env);
>>          return true;
>>      }
>> +
>> +    return false;
>>  }
>>
>>  /* Handle a CPU exception for A and R profile CPUs.
>> @@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>       * code that caused the exception, not the target exception level,
>>       * so must be handled here.
>>       */
>> -    if (check_for_semihosting(cs)) {
>> +    if (handle_semihosting(cs)) {
>>          return;
>>      }
>>
>>
>
> This patch clashes with this one:
> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00343.html
> that Peter already queued:
> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00692.html
>
> Peter, if you want this for 4.1, it might be easier to dequeue "Restrict
> semi-hosting to TCG", apply Alex series, and re-apply "Restrict
> semi-hosting to TCG", the conflicts should be trivial (function name
> changed check_for_semihosting -> handle_semihosting).

This isn't 4.1 material - unless I can figure out the weird single-step
while semihosting bug.

>
> Thanks,
>
> Phil.


--
Alex Bennée

On 7/3/19 6:44 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> Hi Alex, Peter.
>>
>> On 7/3/19 5:52 PM, Alex Bennée wrote:
>>> 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>
>>> ---
>>>  target/arm/helper.c | 84 +++++++++------------------------------------
>>>  1 file changed, 17 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>> index ad29dc4072..5c1f741380 100644
>>> --- a/target/arm/helper.c
>>> +++ b/target/arm/helper.c
>>> @@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>>>                    new_el, env->pc, pstate_read(env));
>>>  }
>>>
>>> -static inline bool check_for_semihosting(CPUState *cs)
>>> +/*
>>> + * Check whether this exception is a semihosting call; if so
>>> + * then handle it and return true; otherwise return false.
>>> + *
>>> + * All the permission and validity checks are done at translate time.
>>> + */
>>> +static inline bool handle_semihosting(CPUState *cs)
>>>  {
>>> -    /* Check whether this exception is a semihosting call; if so
>>> -     * then handle it and return true; otherwise return false.
>>> -     */
>>>      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.
>>> -             */
>>> +    if (cs->exception_index == EXCP_SEMIHOST) {
>>> +        if (is_a64(env)) {
>>>              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;
>>> -    } 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;
>>> +        } else {
>>> +            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,
>>> -                      "...handling as semihosting call 0x%x\n",
>>> -                      env->regs[0]);
>>> -        env->regs[0] = do_arm_semihosting(env);
>>>          return true;
>>>      }
>>> +
>>> +    return false;
>>>  }
>>>
>>>  /* Handle a CPU exception for A and R profile CPUs.
>>> @@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>>       * code that caused the exception, not the target exception level,
>>>       * so must be handled here.
>>>       */
>>> -    if (check_for_semihosting(cs)) {
>>> +    if (handle_semihosting(cs)) {
>>>          return;
>>>      }
>>>
>>>
>>
>> This patch clashes with this one:
>> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00343.html
>> that Peter already queued:
>> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00692.html
>>
>> Peter, if you want this for 4.1, it might be easier to dequeue "Restrict
>> semi-hosting to TCG", apply Alex series, and re-apply "Restrict
>> semi-hosting to TCG", the conflicts should be trivial (function name
>> changed check_for_semihosting -> handle_semihosting).
> 
> This isn't 4.1 material - unless I can figure out the weird single-step
> while semihosting bug.

Well I'm not sure it is ARM-specific... I remember something similar
with MIPS.

>>
>> Thanks,
>>
>> Phil.