Re: [Xen-devel] [PATCH v2 4/4] arm: traps: handle SMC32 in check_conditional_instr()

[Julien Grall <julien.grall@xxxxxxx>]

On 09/08/2017 20:44, Volodymyr Babchuk wrote:
> On ARMv8 architecture SMC instruction from aarch32 state can be conditional.
s/aarch32/AArch32/. But SMC instruction are conditional even on ARMv7. 
The main difference is the ARMv8 architecture allows conditional SMCs to 
be trapped even if they failed their condition check.
> (ARM DDI 0487A.k page D7-1949)
> Thus, we should not skip it while checking HSR.EC value.
>
> For this type of exception special coding of HSR.ISS is used. There is
> additional flag (CCKNOWNPASS) to check before perfoming standard handling
s/perfoming/performing/

> of CCVALID and COND fields.
>
> Because we can't distinguish ARMv7 from aarch32 state, we will always
See my comment on patch #2.

> check CCKNOWNPASS field. On ARMv7 it will be read as 0
> (ARM DDI 0406C.c page B3-1431), so there will be no problem.
Field are usually made RES{0,1} to allow later revision of the 
architecture to use them and not break previous version. There are now 
ARMv8.1, ARMv8.2, ARMv8.3. It does not mean we have 3 new modes. It just 
add more features on top of the earlier revision.
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
>
> - Added references to the ARM manuals.
> - Moved code from under #ifdef CONFIG_ARM_64
> - Added comment with rationale for this
>
> ---
>  xen/arch/arm/traps.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index eae2212..384219d 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1716,8 +1716,24 @@ static int check_conditional_instr(struct cpu_user_regs 
> *regs,
>      unsigned long cpsr, cpsr_cond;
>      int cond;
>
> +    /*
> +     * SMC32 instruction case is very special. Under SMC32 we mean
> +     * SMC instruction on ARMv7 or SMC instruction originating from
> +     * aarch32 state on ARMv8.
> +     * On ARMv7 it is uncoditional (ARM DDI 0406C.c page B3-1431),
> +     * but is conditional on ARMv8 (ARM DDI 0487A.k page D7-1949).
> +     * Problem is that we can't determine if XEN is running on
> +     * ARMv7 on ARMv8 in aarch32 mode. Fortunately, while running
See my comment on top.

> +     * on ARMv7 all conditonal bits of HSR will read as 0
> +     * (ARM DDI 0406C.c page B3-1431).
> +     * This includes CCKNOWNPASS bit of HSR. Thereby we will be okay,
> +     * if we'll check that bit.
> +     */
> +    if (hsr.ec == HSR_EC_SMC32 && hsr.smc32.ccknownpass == 0)
> +        return 1;
> +
>      /* Unconditional Exception classes */
> -    if ( hsr.ec == HSR_EC_UNKNOWN || hsr.ec >= 0x10 )
> +    if ( hsr.ec == HSR_EC_UNKNOWN || (hsr.ec >= 0x10 && hsr.ec != 
> HSR_EC_SMC32))
>          return 1;
>
>      /* Check for valid condition in hsr */
Cheers,

--
Julien Grall

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