Re: [edk2] [PATCH] ArmPkg/CpuDxe: Disable interrupt before restoring context
On 18 September 2015 at 16:25, Heyi Guo wrote: > Hi Ard, > > Sorry for the late reply. > > However, it seems V7 may not have the some problem, with the reasons below: > > 1. It always restores the context and then increase SP (V8 code increases SP > BEFORE restoring context); > 2. It stores PC and CPSR to srsfd stack slot: > ldr R1,[SP,#0x3c] @ EFI_SYSTEM_CONTEXT_ARM.PC > str R1,[SP,#0x58] @ Store it back to srsfd stack slot so > it can be restored > > ldr R1,[SP,#0x40] @ EFI_SYSTEM_CONTEXT_ARM.CPSR > str R1,[SP,#0x5c] @ Store it back to srsfd stack slot so > it can be restored > > 3. it restores PC and CPSR in the LAST ONE instruction (Is it atomic and not > able to be interrupted?): > rfefd SP! @ return from exception via srsfd stack > slot > > So I think V7 does not have the same problem as V8. After all, I'm NOT > familiar with V7 and not sure about it. > > Please let me know your comments. > As discussed, I think your analysis is correct. All manipulations of the stack should be safe to interruptions, and the actual return is done by a single instruction. IOW, the critical region is only one instruction, which [hopefully] means we don't have to disable interrupts when we execute it. So I am happy with the patch, and if Leif agrees, I will go ahead and apply it (I can fix up the comment myself) Leif? Thanks, Ard. > On 08/24/2015 07:05 PM, Ard Biesheuvel wrote: >> >> On 23 August 2015 at 17:59, Ard Biesheuvel >> wrote: >>> >>> On 23 August 2015 at 15:39, Heyi Guo wrote: On 08/17/2015 05:52 PM, Ard Biesheuvel wrote: > > On 13 August 2015 at 05:10, Heyi Guo 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 >> Cc: Leif Lindholm >> Cc: Ard Biesheuvel >> --- >>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-en
Re: [edk2] [PATCH] ArmPkg/CpuDxe: Disable interrupt before restoring context
Hi Ard, Sorry for the late reply. However, it seems V7 may not have the some problem, with the reasons below: 1. It always restores the context and then increase SP (V8 code increases SP BEFORE restoring context); 2. It stores PC and CPSR to srsfd stack slot: ldr R1,[SP,#0x3c] @ EFI_SYSTEM_CONTEXT_ARM.PC str R1,[SP,#0x58] @ Store it back to srsfd stack slot so it can be restored ldr R1,[SP,#0x40] @ EFI_SYSTEM_CONTEXT_ARM.CPSR str R1,[SP,#0x5c] @ Store it back to srsfd stack slot so it can be restored 3. it restores PC and CPSR in the LAST ONE instruction (Is it atomic and not able to be interrupted?): rfefd SP! @ return from exception via srsfd stack slot So I think V7 does not have the same problem as V8. After all, I'm NOT familiar with V7 and not sure about it. Please let me know your comments. Thanks Heyi On 08/24/2015 07:05 PM, Ard Biesheuvel wrote: On 23 August 2015 at 17:59, Ard Biesheuvel wrote: On 23 August 2015 at 15:39, Heyi Guo wrote: On 08/17/2015 05:52 PM, Ard Biesheuvel wrote: On 13 August 2015 at 05:10, Heyi Guo 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 Cc: Leif Lindholm Cc: Ard Biesheuvel --- 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
Re: [edk2] [PATCH] ArmPkg/CpuDxe: Disable interrupt before restoring context
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 wrote: On 23 August 2015 at 15:39, Heyi Guo wrote: On 08/17/2015 05:52 PM, Ard Biesheuvel wrote: On 13 August 2015 at 05:10, Heyi Guo 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 Cc: Leif Lindholm Cc: Ard Biesheuvel --- 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
Re: [edk2] [PATCH] ArmPkg/CpuDxe: Disable interrupt before restoring context
On 23 August 2015 at 17:59, Ard Biesheuvel wrote: > On 23 August 2015 at 15:39, Heyi Guo wrote: >> >> >> On 08/17/2015 05:52 PM, Ard Biesheuvel wrote: >>> >>> On 13 August 2015 at 05:10, Heyi Guo 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 Cc: Leif Lindholm Cc: Ard Biesheuvel --- 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
Re: [edk2] [PATCH] ArmPkg/CpuDxe: Disable interrupt before restoring context
On 23 August 2015 at 15:39, Heyi Guo wrote: > > > On 08/17/2015 05:52 PM, Ard Biesheuvel wrote: >> >> On 13 August 2015 at 05:10, Heyi Guo 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 >>> Cc: Leif Lindholm >>> Cc: Ard Biesheuvel >>> --- >>> 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. > I would like to understand this better, so perhaps you could cite some references in the code? Thanks, Ard. >> Since you are disabling interrupts after your exception handler >> returns, could it be the case that that function is reenabling >> interrupts too early? >> >> >>> // Adjust SP to pop system registers >>> add sp, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + >>> SYS_CONTEXT_SIZE) >>> ALL_SYS_REGS >>> -- >>> 2.1.4 >>> > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/CpuDxe: Disable interrupt before restoring context
On 08/17/2015 05:52 PM, Ard Biesheuvel wrote: On 13 August 2015 at 05:10, Heyi Guo 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 Cc: Leif Lindholm Cc: Ard Biesheuvel --- 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. So I think we can't avoid interrupt reentering in UEFI architecture and need protection when restoring interrupt context. Heyi Since you are disabling interrupts after your exception handler returns, could it be the case that that function is reenabling interrupts too early? // Adjust SP to pop system registers add sp, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE) ALL_SYS_REGS -- 2.1.4 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/CpuDxe: Disable interrupt before restoring context
On 13 August 2015 at 05:10, Heyi Guo 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 > Cc: Leif Lindholm > Cc: Ard Biesheuvel > --- > 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. """ Since you are disabling interrupts after your exception handler returns, could it be the case that that function is reenabling interrupts too early? >// Adjust SP to pop system registers >add sp, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE) >ALL_SYS_REGS > -- > 2.1.4 > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/CpuDxe: Disable interrupt before restoring context
On 08/13/2015 11:10 AM, Heyi Guo 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. Sorry, made a typo in commit log: the 2nd could *only* be resolved by disabling interrupt. Heyi Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Heyi Guo Cc: Leif Lindholm Cc: Ard Biesheuvel --- 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 + // Adjust SP to pop system registers add sp, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE) ALL_SYS_REGS ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] ArmPkg/CpuDxe: Disable interrupt before restoring context
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 Cc: Leif Lindholm Cc: Ard Biesheuvel --- 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 + // Adjust SP to pop system registers add sp, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE) ALL_SYS_REGS -- 2.1.4 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel