Re: [Xen-devel] [PATCH v2 2/4] arm: processor: add new struct hsr_smc32 into hsr union

[Julien Grall <julien.grall@xxxxxxx>]

On 09/08/2017 20:44, Volodymyr Babchuk wrote:
> On ARMv8, one of conditional exceptions (SMC that originates
> from aarch32 state) have extra field in HCR.ISS encoding:
s/aarch32/AArch32/
s/have/has/

And the register is called HSR and not HCR.

> CCKNOWNPASS, bit [19]
> Indicates whether the instruction might have failed its condition
> code check.
>    0 - The instruction was unconditional, or was conditional and
>    passed  its condition code check.
>    1 - The instruction was conditional, and might have failed its
>    condition code check.
> (ARM DDI 0487A.k page D7-1949)
Please use the latest ARM ARM.

> This is instruction specific field, so better to add new structure
This is an instruction...

> to union hsr. This structure describes ISS encoding for an exception
> from SMC instruction execution in AArch32 state. But we define this
> struct for both ARMv7 and ARMv8. The reason is described in comment
> to the structure:
>
> "Nevertheless, we define this encoding for both ARMv7 and ARMv8,
> because check_conditional_inst() should properly handle SMC
> instruction in all modes: ARMv7, aarch32 and aarch64."
Hmmm. There are only two existing modes: AArch32 and AArch64. ARMv7 is 
just a version of the specification which happen to only support AArch32.
Actually Xen does not care about ARMv8 vs ARMv7. It only care about 
AArch32 vs AArch64.
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
>
> - Created new stucture for HSR_EC_SMC32 instead of extending
>   fields in hsr.cond.
> - Added references to ARM manual.
> - Wrote comment with rationale.
>
> ---
>  xen/include/asm-arm/processor.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index f640d54..af4a0f7 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -488,6 +488,25 @@ union hsr {
>          unsigned long ec:6;     /* Exception Class */
>      } cp; /* HSR_EC_CP */
>
> +    /*
> +     * This encoding is valid only for ARMv8 (ARM DDI 0487A.k pages D7-1949 and
> +     * G6-4405). On ARMv7, encoding ISS for EC=0x13 is defined as UNK/SBZP
> +     * (ARM DDI 0406C.c page B3-1431). UNK/SBZP means that hardware implements
> +     * this field as Read-As-Zero.
> +     *
> +     * Nevertheless, we define this encoding for both ARMv7 and ARMv8, because
> +     * check_conditional_inst() should properly handle SMC instruction in all
> +     * modes: ARMv7, aarch32 and aarch64.
See my comment above.

> +     */
> +    struct hsr_smc32 {
> +        unsigned long res0:19;  /* Reserved */
> +        unsigned long ccknownpass:1; /* Instruction passed conditional check */
> +        unsigned long cc:4;    /* Condition Code */
> +        unsigned long ccvalid:1;/* CC Valid */
> +        unsigned long len:1;   /* Instruction length */
> +        unsigned long ec:6;    /* Exception Class */
> +    } smc32; /* HSR_EC_SMC32 */
> +
>  #ifdef CONFIG_ARM_64
>      struct hsr_sysreg {
>          unsigned long read:1;   /* Direction */
Cheers,

--
Julien Grall

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