Re: [PATCH 3/3] xen/arm: Don't crash the domain on invalid HVC immediate

[Julien Grall <julien.grall@arm.com>]

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
>>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel