Hi Ard,
Sorry for the late reply. However I think you've got what I meant :)
In my opinion, UEFI actually needs timer interrupt nesting, e.g. we have
a timer event with low TPL and long execution time, UEFI should support
a higher TPL timer event interrupt the execution of this low TPL event,
isn't it?
Also, I took a look at the code of Linux kernel for ARM64 interrupt
handler; it also disables IRQ before restoring context.
And, thanks for your comments; I'll send another version of the patch.
Heyi Guo
On 08/24/2015 07:05 PM, Ard Biesheuvel wrote:
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
