[Xen-devel,v3,4/4] 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 a better
position to provide helpful information (stack trace...).

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---

    We potentially want to return -1 instead. This would make Xen more
    future-proof if we decide to implement the other HVC immediate.

    Changes in v2:
        - Add Stefano's reviewed-by
---
 xen/arch/arm/traps.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Hi,

On 02/02/18 10:14, 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 a better
> position to provide helpful information (stack trace...).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> ---
> 
>     We potentially want to return -1 instead. This would make Xen more
>     future-proof if we decide to implement the other HVC immediate.
> 
>     Changes in v2:
>         - Add Stefano's reviewed-by
> ---
>  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 1e85f99ec1..1cba7e584d 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1471,14 +1471,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);

Why is this UNDEF?
UNDEF is the exception used when either HVC is disabled or if it's
called from EL0.
I couldn't find anything that talks about how non-implemented immediates
should be handled, I guess they would just be ignored?
Do you have any sources that recommend UNDEF?

The rest looks fine.

Cheers,
Andre.

> +    }
>  
>      if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
>      {
> @@ -2109,7 +2112,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;
>      }
> @@ -2123,7 +2126,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:
>          /*
>

On 02/02/18 14:37, Andre Przywara wrote:
> Hi,

Hi,

> On 02/02/18 10:14, 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 a better
>> position to provide helpful information (stack trace...).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> ---
>>
>>      We potentially want to return -1 instead. This would make Xen more
>>      future-proof if we decide to implement the other HVC immediate.
>>
>>      Changes in v2:
>>          - Add Stefano's reviewed-by
>> ---
>>   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 1e85f99ec1..1cba7e584d 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1471,14 +1471,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);
> 
> Why is this UNDEF?
> UNDEF is the exception used when either HVC is disabled or if it's
> called from EL0.
> I couldn't find anything that talks about how non-implemented immediates
> should be handled, I guess they would just be ignored?
> Do you have any sources that recommend UNDEF?

Per the SMCCC, all nonzero immediate for HVC are designated for use by 
the hypervisors. So I take as we can implement the way we want.

We have few solutions here:
	1) Ignore. This means that a guest may wrongly call an HVC and will 
never notice it until implemented. This would lead to some trouble with 
introduce new ABI.
	2) Return a value. We could return -1 (or -ENOSYS). But that would need 
to be defined.
	3) Undefined. The guest should not be allow to touch it, so it makes sense.

1# is not a solution because catching them wrong usage on old Xen would 
be a nightmare.

2# might be doable, thought I am not sure we want to commit on a return 
value now. But old Xen would see crash the domain.

So I choose #3 which still crash the domain but by injecting an undefined.

Cheers,