PowerPC exceptions and critical interrupts

[Till Straumann]

Sebastian Huber wrote:
> Till Straumann wrote:
>> My idea was committing one of the SPRGs to holding the IRQ mask.
>> Currently, the TOS pointer of the interrupt stack is held in SPRG1
>> (for no really good reason). Using e.g., SPRG0 (the current purpose:
>> making sure that SPRG0 is not used for the IRQ_Nest_level (see PR288)
>> is probably obsolete by now).
>>   
> If we use SPRG0 we can reuse PPC_BSP_HAS_FIXED_PR288 as a quick and 
> dirty approach to adapt the PowerPC BSPs to new 
> rtems_interrupt_{enable, disable} functions.
>> The only caveat is that I'm not sure if every flavor of PPC features
>> SPRG0/SPRG1 but I suspect the ones we currently support do since
>> SPRG1 is used anyways.
>>   
> The SPRG0..3 are mentioned in the PowerPC Microprocessor Family: The 
> Programming Environments for 32-Bit Microprocessors for operating 
> system usage.
>> You don't need a run-time check orgie - the bspsupport already
>> has done it. The variable 'ppc_exc_msr_irq_mask' should hold the
>> correct value.
>>   
> Yes, but this is yet another magic place that the BSP developer has to 
> be aware of.
No. The cpu support takes care of this. If the CPU is one of the 
'BookE'-style CPUs then
the mask is changed to include CE and DE.
>
> Here are some constrains that I assume:
> C1: Three asynchronous exception types ME, CE and EE with priorities 
> ME > CE > EE. (Debug exceptions DE?)
Yes.
> C2: The exception handler code must not be interrupted by a lower 
> priority exception (mostly due to the thread dispatch disable level).
I don't think the thread-dispatch disable level is an issue here.
IIRC what can get messed up are the SRRs, i.e., it could be that

1)  EE saves IR and MSR into SRRs
2)  CE happens, saves IR and MSR into critical SRRs
3)  handler is dispatched; if the handler re-enables EE then
     the values saved in SRRs at 1) could be overwritten by a
     second EE.
> C3: To support RTEMS functions and the dispatcher the critical 
> sections in the system core must be protected by 
> rtems_interrupt_{enable, disable}. Thus if we want to use RTEMS within 
> machine check exceptions we have to add MSR_ME to the disable mask. 
> This may result in check stops. This is an inacceptable tradeoff.
I agree. MSR_ME should always be enabled. This severely limits what a ME 
handler
is allowed to do but IMO in many cases we don't have to recover from a 
ME which
can be considered fatal but we do want print diagnostic output. A 
check-stop is most
undesirable.
> C4: The asynchronous high level handler share a common stack.
Just so we can leverage the existing support code (and the whole 
configuration-table
business etc).
>
> The user of critical interrupts has to be very careful not to enable 
> external exceptions since a
>
> uint32_t level = _ISR_Get_level();
> _ISR_Set_level( 0);
> _ISR_Set_level( level);
>
> will not work.
If we support critical interrupts then the inline routine in irq_supp.h
could be modified accordingly. All BSPs using the inline routine would
not have to care.
>
> The ppc_exc_msr_irq_mask is currently used to protect the stack switch 
> and ISR nest level.
But the ultimate purpose was using that same mask for 
rtems_interrupt_enable/disable.
For efficiency reasons I'd suggest to cache the mask in a SPRG.
> Machine checks are not included in the disable mask (MSR_ME) that is 
> an inconsistency the current implementation for C4. 
Yes.
> After a review of the code I think that if C2 is true the disabling of 
> all asynchronous exceptions is superfluous. A stack switch would be 
> problematic only if we increment the ISR nest level and perform the 
> stack switch afterwards, because a higher priority exception may 
> assume that a stack switch is not necessary but actually has not occured.
Not sure I understand how you would make the operation of testing 
whether a switch is
necessary and doing the switch an atomic operation (you also have to 
take in consideration
the case where CE tries to switch just while EE switches back).

I believe I thought about it carefully and came to the conclusion that 
disabling all
interrupts was necessary (but I could, of course be proven wrong). OTOH,
I don't see any harm because the critical section is only 7 instructions 
long.
>
> The RTEMS support for critical interrupts has its obstacles but is 
> feasible.
I tried to do prepare for this and I believe the hardest part is already 
done.
What remains is :
  - change rtems_interrupt_enable()/disable so that all interrupts are 
masked
    (I propose to copy ppc_exc_msr_irq_mask -> SPRG0 and then use that to
    mask and unmask interrupts).
  - maybe introduce 2 levels of interrupts ('normal' and 'critical') and 
change
    the inline (irq_supp.h) so that it enables the current level - 1 
instead of enabling
    level 0.
> For machine checks I am against to add it due to the check stop 
> problematic.
Agreed.

I would like to make these changes soon, i.e., prior to 4.9 being cut

-- Till
>> What needs to be done is:
>>
>>  - eliminate current checks for SPRG0 contents
>>  - load SPRG0 with ppc_exc_msr_irq_mask (probably from
>>    initialize_exceptions)
>>  - convert _CPU_ISR_Set_level, _CPU_ISR_Disable, _CPU_ISR_Enable
>>    etc. to using SPRG0 instead of a static value.
>>  - convert the bspsupport to using SPRG0 instead of the 
>> ppc_exc_msr_irq_mask
>>    variable (optional but would be a little bit more efficient and 
>> consistent).
>>
>> -- Till
>>
>> Sebastian Huber wrote:
>>  
>>> Hi,
>>> I adapt the gen83xx BSP for the MPC8313E currently. I switched to 
>>> the new support code (libcpu/powerpc/new-exceptions/bspsupport) and 
>>> adjusted it for e300 cores. With this framework it is possible to 
>>> use RTEMS routines and the dispatcher from external and critical 
>>> exceptions (some sophisticated lock variables are involved for 
>>> this).  In order too test it I configured the interrupt controller 
>>> (IPIC) in a way so that for some interrupt sources a critical 
>>> interrupt exception will be generated. This works fine except that 
>>> the current rtems_interrupt_{enable,disable} functions are not aware 
>>> of critical interrupts. This leads to race conditions in the system 
>>> core. I guess that this is also true for Book E critical interrupts. 
>>> A solution is to add the appropriate bits to the disable mask 
>>> (MSR_CE or MSR_E300_CE), but this leads to a preprecessor or runtime 
>>> cpu check orgie. Do we really need RTEMS support in critical or 
>>> machine check exceptions?
>>>
>>> Ciao,
>>>     Sebastian
>>>
>>>       
>>
>> _______________________________________________
>> rtems-users mailing list
>> rtems-users at rtems.com
>> http://rtems.rtems.org/mailman/listinfo/rtems-users
>>   
>
>