Re: [PATCH 05/18] xen/arm: Save ESR_EL2 to avoid using mismatched value in syndrome check

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

Hi Wei,

On 03/13/2017 10:55 AM, Wei Chen wrote:
> Xen will do exception syndrome check while some types of exception
> take place in EL2. The syndrome check code read the ESR_EL2 register
> directly, but in some situation this register maybe overridden by
> nested exception.
>
> For example, if we re-enable IRQ before reading ESR_EL2 which means
> Xen will enter in IRQ exception mode and return the processor with

s/will/may/

> clobbered ESR_EL2 (See ARM ARM DDI 0487A.j D7.2.25)
>
> In this case the guest exception syndrome has been overridden, we will
> check the syndrome for guest sync exception with a mismatched ESR_EL2

s/mismatched/incorrect/ I think

> value. So we want to save ESR_EL2 to cpu_user_regs as soon as the
> exception takes place in EL2 to avoid using a mismatched syndrome value.

Ditto.

>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>  xen/arch/arm/arm32/asm-offsets.c      |  1 +
>  xen/arch/arm/arm32/entry.S            |  3 +++
>  xen/arch/arm/arm64/asm-offsets.c      |  1 +
>  xen/arch/arm/arm64/entry.S            | 13 +++++++++----
>  xen/arch/arm/traps.c                  |  2 +-
>  xen/include/asm-arm/arm32/processor.h |  2 +-
>  xen/include/asm-arm/arm64/processor.h | 10 ++++++++--
>  7 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
> index f8e6b53..5b543ab 100644
> --- a/xen/arch/arm/arm32/asm-offsets.c
> +++ b/xen/arch/arm/arm32/asm-offsets.c
> @@ -26,6 +26,7 @@ void __dummy__(void)
>     OFFSET(UREGS_lr, struct cpu_user_regs, lr);
>     OFFSET(UREGS_pc, struct cpu_user_regs, pc);
>     OFFSET(UREGS_cpsr, struct cpu_user_regs, cpsr);
> +   OFFSET(UREGS_hsr, struct cpu_user_regs, hsr);
>
>     OFFSET(UREGS_LR_usr, struct cpu_user_regs, lr_usr);
>     OFFSET(UREGS_SP_usr, struct cpu_user_regs, sp_usr);
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 2a6f4f0..2187226 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -23,6 +23,9 @@
>          add r11, sp, #UREGS_kernel_sizeof+4;                            \
>          str r11, [sp, #UREGS_sp];                                       \
>                                                                          \
> +        mrc CP32(r11, HSR);             /* Save exception syndrome */   \
> +        str r11, [sp, #UREGS_hsr];                                      \
> +                                                                        \
>          mrs r11, SPSR_hyp;                                              \
>          str r11, [sp, #UREGS_cpsr];                                     \
>          and r11, #PSR_MODE_MASK;                                        \
> diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
> index 69ea92a..ce24e44 100644
> --- a/xen/arch/arm/arm64/asm-offsets.c
> +++ b/xen/arch/arm/arm64/asm-offsets.c
> @@ -27,6 +27,7 @@ void __dummy__(void)
>     OFFSET(UREGS_SP, struct cpu_user_regs, sp);
>     OFFSET(UREGS_PC, struct cpu_user_regs, pc);
>     OFFSET(UREGS_CPSR, struct cpu_user_regs, cpsr);
> +   OFFSET(UREGS_ESR_el2, struct cpu_user_regs, hsr);
>
>     OFFSET(UREGS_SPSR_el1, struct cpu_user_regs, spsr_el1);
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index c181b5e..02802c0 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -121,9 +121,13 @@ lr      .req    x30             // link register
>
>          stp     lr, x21, [sp, #UREGS_LR]
>
> -        mrs     x22, elr_el2
> -        mrs     x23, spsr_el2
> -        stp     x22, x23, [sp, #UREGS_PC]
> +        mrs     x21, elr_el2
> +        str     x21, [sp, #UREGS_PC]

Please explain the commit message you modify the lines above ...

> +
> +        add     x21, sp, #UREGS_CPSR
> +        mrs     x22, spsr_el2
> +        mrs     x23, esr_el2
> +        stp     w22, w23, [x21]
>
>          .endm
>
> @@ -307,7 +311,8 @@ ENTRY(return_to_new_vcpu64)
>  return_from_trap:
>          msr     daifset, #2 /* Mask interrupts */
>
> -        ldp     x21, x22, [sp, #UREGS_PC]       // load ELR, SPSR
> +        ldr     x21, [sp, #UREGS_PC]            // load ELR
> +        ldr     w22, [sp, #UREGS_CPSR]          // load SPSR

as long as those one.

>
>          pop     x0, x1
>          pop     x2, x3

[...]

> diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
> index b0726ff..d381428 100644
> --- a/xen/include/asm-arm/arm64/processor.h
> +++ b/xen/include/asm-arm/arm64/processor.h
> @@ -65,9 +65,15 @@ struct cpu_user_regs
>
>      /* Return address and mode */
>      __DECL_REG(pc,           pc32);             /* ELR_EL2 */
> +    /*
> +     * Be careful for 32-bit registers, if we use xN to save 32-bit register
> +     * to stack, its next field on stack will be overridden.
> +     * For example, if we use xN to save SPSR_EL2 to stack will override the
> +     * hsr field on stack.
> +     * So, it's better to use wN to save 32-bit registers to stack.
> +     */

This comment is pointless. This is true for any 32-bit register, you 
should use wN unless you now that you have a padding after.

>      uint32_t cpsr;                              /* SPSR_EL2 */
> -
> -    uint32_t pad0; /* Align end of kernel frame. */
> +    uint32_t hsr;                               /* ESR_EL2 */
>
>      /* Outer guest frame only from here on... */
>
>

Cheers,

-- 
Julien Grall

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