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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien
On 2017/3/30 21:31, Julien Grall wrote:
> 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@xxxxxxx>
>>
>> ---
>
> 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.
>

Sorry about that, I will add it to the note in next version.

>
>> 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.
>

Yes, that's what I hadn't considered. I would cover them in next version.


> Cheers,
>


-- 
Regards,
Wei Chen

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