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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 16 Mar 2017, Julien Grall wrote:
> 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.

Please address Julien's comments. The code looks good.


> > 
> > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel