Re: [Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

2019-09-06 Thread Richard Henderson 
On 9/6/19 8:47 AM, Alex Bennée wrote:
> +if (semihosting_enabled() &&
> +#ifndef CONFIG_USER_ONLY
> +s->current_el != 0 &&
> +#endif

!IS_USER(s).


r~

Re: [Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

2019-09-06 Thread Peter Maydell 
On Fri, 6 Sep 2019 at 14:16, Alex Bennée  wrote:
>
>
> Peter Maydell  writes:
>
> > On Fri, 6 Sep 2019 at 13:47, Alex Bennée  wrote:
>
> >
> > Doesn't this accidentally enable semihosting via SVC for
> > M-profile ?
>
> We must have done that before then.

No, we didn't do it before, because we were handling it in
arm_cpu_do_interrupt(), which is A/R-profile only. The
M-profile code goes via arm_v7m_cpu_do_interrupt() which doesn't
check for semihosting when it sees an EXCP_SWI.

> Just gate it with && !arm_dc_feature(s, ARM_FEATURE_M) then?

Yes, I think that's the right fix.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

2019-09-06 Thread Alex Bennée 


Peter Maydell  writes:

> On Fri, 6 Sep 2019 at 13:47, Alex Bennée  wrote:
>>
>> As for the other semihosting calls we can resolve this at translate
>> time.
>>
>> Signed-off-by: Alex Bennée 
>>
>> ---
>> 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
>> ---
>>  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 4cda7812bcb..ed4a97cfb44 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -10222,14 +10222,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 (semihosting_enabled() &&
>> +#ifndef CONFIG_USER_ONLY
>> +s->current_el != 0 &&
>> +#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;
>>  }
>
> Doesn't this accidentally enable semihosting via SVC for
> M-profile ?

We must have done that before then. Just gate it with &&
!arm_dc_feature(s, ARM_FEATURE_M) then?

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

2019-09-06 Thread Peter Maydell 
On Fri, 6 Sep 2019 at 13:47, Alex Bennée  wrote:
>
> As for the other semihosting calls we can resolve this at translate
> time.
>
> Signed-off-by: Alex Bennée 
>
> ---
> 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
> ---
>  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 4cda7812bcb..ed4a97cfb44 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10222,14 +10222,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 (semihosting_enabled() &&
> +#ifndef CONFIG_USER_ONLY
> +s->current_el != 0 &&
> +#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;
>  }

Doesn't this accidentally enable semihosting via SVC for
M-profile ?

thanks
-- PMM

[Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

2019-09-06 Thread Alex Bennée 
As for the other semihosting calls we can resolve this at translate
time.

Signed-off-by: Alex Bennée 

---
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
---
 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 4cda7812bcb..ed4a97cfb44 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10222,14 +10222,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 (semihosting_enabled() &&
+#ifndef CONFIG_USER_ONLY
+s->current_el != 0 &&
+#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