On 18 September 2015 at 16:25, Heyi Guo <heyi....@linaro.org> 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 <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
