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
