On 23 August 2015 at 17:59, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
> On 23 August 2015 at 15:39, Heyi Guo <heyi....@linaro.org> wrote:
>>
>>
>> On 08/17/2015 05:52 PM, Ard Biesheuvel wrote:
>>>
>>> On 13 August 2015 at 05:10, Heyi Guo<heyi....@linaro.org>  wrote:
>>>>
>>>> Interrupt must be disabled before we storing ELR and other system
>>>> registers, or else ELR will be overridden by interrupt reentrance.
>>>>
>>>> This bug is critical as we may get occasional exception or dead loop
>>>> when interrupt reentrance occurs:
>>>>
>>>>    After increasing SP ... Before popping out registers
>>>> Or
>>>>    After restoring ELR
>>>>
>>>> The 1st circumstance could also be resolved by optimizing SP operation
>>>> (Pop out registers before adding SP back), but the 2nd could not be
>>>> resolved by disabling interrupt.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Heyi Guo<heyi....@linaro.org>
>>>> Cc: Leif Lindholm<leif.lindh...@linaro.org>
>>>> Cc: Ard Biesheuvel<ard.biesheu...@linaro.org>
>>>> ---
>>>>   ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S
>>>> b/ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S
>>>> index 2682f4f..ca6c9a1 100644
>>>> --- a/ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S
>>>> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S
>>>> @@ -358,6 +358,14 @@ ASM_PFX(AsmCommonExceptionEntry):
>>>>   #define REG_PAIR(REG1, REG2, OFFSET, CONTEXT_SIZE)    ldp  REG1, REG2,
>>>> [sp, #(OFFSET-CONTEXT_SIZE)]
>>>>   #define REG_ONE(REG1, OFFSET, CONTEXT_SIZE)           ldur REG1, [sp,
>>>> #(OFFSET-CONTEXT_SIZE)]
>>>>
>>>> +  //
>>>> +  // Disable interrupt(IRQ and FIQ) before restoring context,
>>>> +  // or else the context will be corrupted by interrupt reentrance.
>>>> +  // Interrupt mask will be restored from spsr by hardware when we call
>>>> eret
>>>> +  //
>>>> +  msr   daifset, #3
>>>> +  isb
>>>> +
>>>
>>> Are you sure this is necessary? According to the ARM ARM
>>>
>>> """
>>> On taking any exception to an Exception level using AArch64, all of
>>> PSTATE.{A, I, F} are set to 1, masking all
>>> interrupts that target that Exception level.
>>> """
>>
>> Yes. However, in timer interrupt handler, we will raise TPL to HIGH_LEVEL
>> and then restore TPL, while restore TPL will enable interrupt.
>>
>
> Is that in the generic ARM timer code? Perhaps we should raise and
> lower the TPL in the common interrupt entry/exit path, since the
> architecture implicitly raises the TPL by entering the interrupt
> handler with interrupts disabled. In general, I don't think it is
> correct for TPL lowering code to enable interrupts if it did not find
> the enabled when it raised the TPL.
>
>> So I think we can't avoid interrupt reentering in UEFI architecture and need
>> protection when restoring interrupt context.
>>

Yes, it seems you are right. Even though I am not happy with the idea
that we are supporting nested interrupts without exactly understanding
the implication and without there being a real need (since we use
interrupts for the timer tick only), the core does not really allow us
to change that for AARCH64 only.

So I think your patch is correct. There are still two issues to resolve, though:

1. Could you update the comment in the code to mention that interrupts
have been re-enabled by the TPL lowering code, and that we need to
disable them again only to protect the critical section that restores
the interrupted context from the stack?
2. The 32-bit ARM code appears to suffer from the same problem, so we
should fix that as well.

-- 
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to