Add support for the arm breakpoint syscall.

---
 linux-user/arm/syscall.h | 1 +
 linux-user/main.c        | 4 ++++
 2 files changed, 5 insertions(+)

[cc'ing Riku as the linux-user maintainer.]

On 20 June 2014 10:14, Hunter Laux <hunterlaux@gmail.com> wrote:

Thanks for writing this patch. It mostly looks good
but there are some minor admin/format tweaks we need.

You need to provide a Signed-off-by: line here, or
we can't take this patch, I'm afraid. (We work the same
way as the Linux kernel for this; see
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297
for more details, but basically it says you wrote the code
and are willing to contribute it under our licensing terms.)

It could also use a little more explanation in the commit
message.

FWIW, this is an obscure syscall that dates back to before
the architecture provided an official breakpoint instruction;
apparently it's still used by Steel Bank Common Lisp.

This is the kernel implementation:
http://lxr.free-electrons.com/source/arch/arm/kernel/traps.c#L598

> ---
>  linux-user/arm/syscall.h | 1 +
>  linux-user/main.c        | 4 ++++
>  2 files changed, 5 insertions(+)
>
> diff --git a/linux-user/arm/syscall.h b/linux-user/arm/syscall.h
> index ce2c2a8..e0d2cc3 100644
> --- a/linux-user/arm/syscall.h
> +++ b/linux-user/arm/syscall.h
> @@ -29,6 +29,7 @@ struct target_pt_regs {
>  #define ARM_THUMB_SYSCALL      0
>
>  #define ARM_NR_BASE      0xf0000
> +#define ARM_NR_breakpoint (ARM_NR_BASE + 1)
>  #define ARM_NR_cacheflush (ARM_NR_BASE + 2)
>  #define ARM_NR_set_tls   (ARM_NR_BASE + 5)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index a87c6f7..831b363 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -807,6 +807,9 @@ void cpu_loop(CPUARMState *env)
>                              cpu_set_tls(env, env->regs[0]);
>                              env->regs[0] = 0;
>                              break;
> +                        case ARM_NR_breakpoint:
> +                            env->regs[15] -= env->thumb ? 2 : 4;
> +                            goto excp_debug;
>                          default:
>                              gemu_log("qemu: Unsupported ARM syscall: 0x%x\n",
>                                       n);
> @@ -850,6 +853,7 @@ void cpu_loop(CPUARMState *env)
>              }
>              break;
>          case EXCP_DEBUG:
> +            excp_debug:

The indent on this label isn't quite right; it should
be 4 columns to the left.

>              {
>                  int sig;

Those minor issues aside, the code looks right to me.

thanks
-- PMM

20.06.2014 13:51, Peter Maydell пишет:
> [cc'ing Riku as the linux-user maintainer.]
> 
> On 20 June 2014 10:14, Hunter Laux <hunterlaux@gmail.com> wrote:
> 
> Thanks for writing this patch. It mostly looks good
> but there are some minor admin/format tweaks we need.
> 
> You need to provide a Signed-off-by: line here, or
> we can't take this patch, I'm afraid. (We work the same
> way as the Linux kernel for this; see
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297
> for more details, but basically it says you wrote the code
> and are willing to contribute it under our licensing terms.)
> 
> It could also use a little more explanation in the commit
> message.
> 
> FWIW, this is an obscure syscall that dates back to before
> the architecture provided an official breakpoint instruction;
> apparently it's still used by Steel Bank Common Lisp.
> 
> This is the kernel implementation:
> http://lxr.free-electrons.com/source/arch/arm/kernel/traps.c#L598
> 
>> ---
>>  linux-user/arm/syscall.h | 1 +
>>  linux-user/main.c        | 4 ++++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/linux-user/arm/syscall.h b/linux-user/arm/syscall.h
>> index ce2c2a8..e0d2cc3 100644
>> --- a/linux-user/arm/syscall.h
>> +++ b/linux-user/arm/syscall.h
>> @@ -29,6 +29,7 @@ struct target_pt_regs {
>>  #define ARM_THUMB_SYSCALL      0
>>
>>  #define ARM_NR_BASE      0xf0000
>> +#define ARM_NR_breakpoint (ARM_NR_BASE + 1)
>>  #define ARM_NR_cacheflush (ARM_NR_BASE + 2)
>>  #define ARM_NR_set_tls   (ARM_NR_BASE + 5)
>>
>> diff --git a/linux-user/main.c b/linux-user/main.c
>> index a87c6f7..831b363 100644
>> --- a/linux-user/main.c
>> +++ b/linux-user/main.c
>> @@ -807,6 +807,9 @@ void cpu_loop(CPUARMState *env)
>>                              cpu_set_tls(env, env->regs[0]);
>>                              env->regs[0] = 0;
>>                              break;
>> +                        case ARM_NR_breakpoint:
>> +                            env->regs[15] -= env->thumb ? 2 : 4;
>> +                            goto excp_debug;
>>                          default:
>>                              gemu_log("qemu: Unsupported ARM syscall: 0x%x\n",
>>                                       n);
>> @@ -850,6 +853,7 @@ void cpu_loop(CPUARMState *env)
>>              }
>>              break;
>>          case EXCP_DEBUG:
>> +            excp_debug:
> 
> The indent on this label isn't quite right; it should
> be 4 columns to the left.

Isn't whole thing - using label and goto like this, intermixing
two switch statements with goto - a bit flawed?  At best it makes
the code unclean...

>>              {
>>                  int sig;
> 
> Those minor issues aside, the code looks right to me.

There was another submission of the same patch, but with
a S-o-b line.

Thanks,

/mjt