Re: [Xen-devel] [PATCH v8 2/6] arm: filter SMC exceptions with failed condition checks

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Wed, Jul 6, 2016 at 11:31 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
> On 05/07/16 19:37, Tamas K Lengyel wrote:
>>
>> In AArch32 state, the ARMv8-A architecture permits, but does not require,
>> this trap to apply to conditional SMC instructions that fail their
>> condition
>> code check, in the same way as with traps on other conditional
>> instructions.
>
>
> Please add a quote to the spec.
>
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>> Suggested-by: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>>   xen/arch/arm/traps.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 44926ca..627e8c9 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2507,6 +2507,17 @@ bad_data_abort:
>>       inject_dabt_exception(regs, info.gva, hsr.len);
>>   }
>>
>> +static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
>> +{
>> +    if ( !check_conditional_instr(regs, hsr) )
>
>
> This function is checking the EC, it considers that EC > 0x10 will be
> unconditional. However, the SMC exception class is 0x13 when taken from
> AArch32 and 0x17 when taken from AArch64.
>
> Furthermore, for ARMv7, the register is Reserved UNK/SBZP (see B3-1431 in
> ARM DDI 0406C.c). I.e the software should not rely on the field reading as
> all 0s (see Glossary-2736).
>
> For ARMv8, when the SMC is taken from AArch64 (see D7-1942 in ARM DDI
> 0487A.j), the register is RES0 which means the software should not rely on
> the value to always be 0 (see Glossary-5734). When the SMC is taken from
> AArch32, the field CV is only valid if CCKNOWNPASS is 1 (this field does not
> exist for the other exception class).
>
> So this code need more extra care. It would also be nice to have a
> description in the code explaining what's going on.

Ack, there seems to be enough corner-cases here that I'm unfamiliar
with that I would rather not attempt implementing this as I likely
would miss something. I'll wait for your patch with the rest of my
series as that route will likely have less overhead anyway.

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel