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

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

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@zentific.com>
> Suggested-by: Julien Grall <julien.grall@arm.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
>   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.

> +    {
> +        advance_pc(regs, hsr);
> +        return;
> +    }
> +
> +    inject_undef_exception(regs, hsr);
> +}
> +
>   static void enter_hypervisor_head(struct cpu_user_regs *regs)
>   {
>       if ( guest_mode(regs) )
> @@ -2582,7 +2593,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>            */
>           GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
>           perfc_incr(trap_smc32);
> -        inject_undef32_exception(regs);
> +        do_trap_smc(regs, hsr);
>           break;
>       case HSR_EC_HVC32:
>           GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> @@ -2615,7 +2626,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>            */
>           GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
>           perfc_incr(trap_smc64);
> -        inject_undef64_exception(regs, hsr.len);
> +        do_trap_smc(regs, hsr);
>           break;
>       case HSR_EC_SYSREG:
>           GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
>

Regards,

-- 
Julien Grall

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