Re: [PATCH v2 01/19] xen/arm: Save ESR_EL2 to avoid using mismatched value in syndrome check

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

Hi Wei,

On 30/03/17 10:13, 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 may enter in IRQ exception mode and return the processor with
> 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 an incorrect ESR_EL2
> value. So we want to save ESR_EL2 to cpu_user_regs as soon as the
> exception takes place in EL2 to avoid using an incorrect syndrome value.
>
> In order to save ESR_EL2, we added a 32-bit member hsr to cpu_user_regs.
> But while saving registers in trap entry, we use stp to save ELR and
> CPSR at the same time through 64-bit general registers. If we keep this
> code, the hsr will be overridden by upper 32-bit of CPSR. So adjust the
> code to use str to save ELR in a separate instruction and use stp to
> save CPSR and HSR at the same time through 32-bit general registers.
> This change affects the registers restore in trap exit, we can't use the
> ldp to restore ELR and CPSR from stack at the same time. We have to use
> ldr to restore them separately.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>
> ---

As mentioned in the internal review, it would have been helpful to 
mention this is a bug fix and should be backported on Xen 4.8 and Xen 4.7.


> v1->v2:
> 1. Use more accurate words in the commit message.
> 2. Remove pointless comment message in cpu_user_regs.
> 3. Explain the changes of the registers save/restore order in trap
>    entry/exit.
> ---
>  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 |  3 +--

A quick grep gives of ESR_EL2 in the code gives me:

include/asm-arm/cpregs.h
308:#define ESR_EL2                 HSR

arch/arm/arm64/traps.c
35:    union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };

arch/arm/traps.c
846:    printk("   ESR_EL2: %08"PRIx32"\n", READ_SYSREG32(ESR_EL2));
2424:     *  (the bit ESR_EL2.S1PTW is set)
2644:    const union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };

The problem you describe also apply for the other READ_SYSREG32(ESR_EL2) 
and I was expecting to see them updated in this patch.

Cheers,

-- 
Julien Grall

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