[Xen-devel,3/3] xen/arm: Don't crash the domain on invalid HVC immediate

domain_crash_synchronous() should only be used when something went wrong
in Xen. It is better to inject to the guest as it will be in better
position to provide helpful information (stack trace...).

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

    We potentially want to return -1 instead. This would make Xen more
    future-proof if we decide to implement the other HVC immediate.
---
 xen/arch/arm/traps.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

On Tue, 30 Jan 2018, Julien Grall wrote:
> domain_crash_synchronous() should only be used when something went wrong
> in Xen. It is better to inject to the guest as it will be in better
                                                              ^ a better


> position to provide helpful information (stack trace...).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
>     We potentially want to return -1 instead. This would make Xen more
>     future-proof if we decide to implement the other HVC immediate.
> ---
>  xen/arch/arm/traps.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 843adf4959..18da45dff3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1473,14 +1473,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  #endif
>  
>  static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
> -                              unsigned long iss)
> +                              const union hsr hsr)
>  {
>      arm_hypercall_fn_t call = NULL;
>  
>      BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
>  
> -    if ( iss != XEN_HYPERCALL_TAG )
> -        domain_crash_synchronous();
> +    if ( hsr.iss != XEN_HYPERCALL_TAG )
> +    {
> +        gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
> +        return inject_undef_exception(regs, hsr);

It looks a bit weird, given that inject_undef_exception doesn't return
anything. This is just code style so anyway:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> +    }
>  
>      if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
>      {
> @@ -2150,7 +2153,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>          if ( hsr.iss == 0 )
>              return do_trap_hvc_smccc(regs);
>          nr = regs->r12;
> -        do_trap_hypercall(regs, &nr, hsr.iss);
> +        do_trap_hypercall(regs, &nr, hsr);
>          regs->r12 = (uint32_t)nr;
>          break;
>      }
> @@ -2164,7 +2167,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>  #endif
>          if ( hsr.iss == 0 )
>              return do_trap_hvc_smccc(regs);
> -        do_trap_hypercall(regs, &regs->x16, hsr.iss);
> +        do_trap_hypercall(regs, &regs->x16, hsr);
>          break;
>      case HSR_EC_SMC64:
>          /*
> -- 
> 2.11.0
>

On 30/01/18 18:24, Stefano Stabellini wrote:
> On Tue, 30 Jan 2018, Julien Grall wrote:
>> domain_crash_synchronous() should only be used when something went wrong
>> in Xen. It is better to inject to the guest as it will be in better
>                                                                ^ a better
> 
> 
>> position to provide helpful information (stack trace...).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>
>>      We potentially want to return -1 instead. This would make Xen more
>>      future-proof if we decide to implement the other HVC immediate.
>> ---
>>   xen/arch/arm/traps.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 843adf4959..18da45dff3 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1473,14 +1473,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>>   #endif
>>   
>>   static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
>> -                              unsigned long iss)
>> +                              const union hsr hsr)
>>   {
>>       arm_hypercall_fn_t call = NULL;
>>   
>>       BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
>>   
>> -    if ( iss != XEN_HYPERCALL_TAG )
>> -        domain_crash_synchronous();
>> +    if ( hsr.iss != XEN_HYPERCALL_TAG )
>> +    {
>> +        gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
>> +        return inject_undef_exception(regs, hsr);
> 
> It looks a bit weird, given that inject_undef_exception doesn't return
> anything. This is just code style so anyway:

I followed coding style in other part of the emulation (see do_sysreg 
for instance). I am happy to change that though.

Cheers,

> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>> +    }
>>   
>>       if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
>>       {
>> @@ -2150,7 +2153,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>>           if ( hsr.iss == 0 )
>>               return do_trap_hvc_smccc(regs);
>>           nr = regs->r12;
>> -        do_trap_hypercall(regs, &nr, hsr.iss);
>> +        do_trap_hypercall(regs, &nr, hsr);
>>           regs->r12 = (uint32_t)nr;
>>           break;
>>       }
>> @@ -2164,7 +2167,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>>   #endif
>>           if ( hsr.iss == 0 )
>>               return do_trap_hvc_smccc(regs);
>> -        do_trap_hypercall(regs, &regs->x16, hsr.iss);
>> +        do_trap_hypercall(regs, &regs->x16, hsr);
>>           break;
>>       case HSR_EC_SMC64:
>>           /*
>> -- 
>> 2.11.0
>>