[PATCH v2 2/2] libbsp/arm: Fix the local interrupt mask disable/enable calls.

Thu Aug 25 15:32:09 UTC 2016

On Thu, Aug 25, 2016 at 6:56 AM, Pavel Pisa <ppisa4lists at pikron.com> wrote:
> Hello Chris and others,
>
> I would like to remind Chris's patches (top-post to highlight important the first)
>
>   [PATCH v2 1/2] arm/cortex-a: Fix cache flush/invalidate after u-boot.
>   [PATCH v2 2/2] libbsp/arm: Fix the local interrupt mask disable/enable calls.
>
> I consider fix of Zynq (all arm-a9mpcore-start) boot
> through U-boot as important. But discussion has vanished.
>
> From my point of view, changes should/could be committed
> as they are. I consider locking in this place as unimportant
> in actual state (see my previous analysis in the thread) so
> the second patch can even replace rtems_interrupt_disable by comment
>
> -   rtems_interrupt_disable(level);
> +   /* FIXME: if concurrent update of same area is considered locking hat to be added */
>
> or even remove lines alltogether. The real need for locking
> has been eliminated by my previous patch
>
>   bsps/arm: do not disable MMU during translation table management operations.
>
> because set_translation_table_entries() manipulated with global core state before.
> It disabled cache and after update re-enabled it. Interleaving parallel execution
> of this sequence would lead to errors for sure. As I have analyzed, that sequence has
> been probably broken for SMP at all because invalidation complete cache
> (only realizable by set and way on ARMv7) can be implemented without broadcast to snoop
> subsystem on some cores so only reliable way is to ask all CPUs by IPI to stop,
> then run complete cache flush on each core one by one. But that is not case for now,
> so locking is not required except for case of contention for given region and this
> has to be solved somewhere else.
>
> So at the end, I suggest to remove locking alltogether there.
>
> There has been left my question unanswered
>
>   Is there some defined function/way to check if RTEMS executive
>   reached switch to multitasking mode?
>
_System_state_Is_before_multitasking()

> This has impact to think about correct locking in functions
> which are required to be used during startup when there is
> no concurrent execution but mutexes support and memory initialization
> is not finished yet but the same functions can be used later
> when scheduler is running for further update of system state.
>
> More complete analysis and context in previous communication.
>
I read prior discussion and had no problems, but Sebastian's concern
should be addressed whether other ARM targets are correct or not.

> Best wishes,
>
>               Pavel
>
>
> On Tuesday 16 of August 2016 15:28:42 Pavel Pisa wrote:
>> Hello Chris and Sebastian,
>>
>> On Tuesday 16 of August 2016 07:55:57 Sebastian Huber wrote:
>> > On 16/08/16 07:45, Chris Johns wrote:
>> > > ---
>> > >   c/src/lib/libbsp/arm/shared/arm-cp15-set-ttb-entries.c | 4 ++--
>> > >   1 file changed, 2 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/c/src/lib/libbsp/arm/shared/arm-cp15-set-ttb-entries.c
>> > > b/c/src/lib/libbsp/arm/shared/arm-cp15-set-ttb-entries.c index
>> > > f650009..cfad45f 100644
>> > > --- a/c/src/lib/libbsp/arm/shared/arm-cp15-set-ttb-entries.c
>> > > +++ b/c/src/lib/libbsp/arm/shared/arm-cp15-set-ttb-entries.c
>> > > @@ -88,10 +88,10 @@ uint32_t arm_cp15_set_translation_table_entries(
>> > >     rtems_interrupt_level level;
>> > >     uint32_t section_flags_of_first_entry;
>> > >
>> > > -  rtems_interrupt_disable(level);
>> > > +  rtems_interrupt_local_disable(level);
>> > >     section_flags_of_first_entry =
>> > >       set_translation_table_entries(begin, end, section_flags);
>> > > -  rtems_interrupt_enable(level);
>> > > +  rtems_interrupt_local_enable(level);
>> > >
>> > >     return section_flags_of_first_entry;
>> > >   }
>> >
>> > We should only change this if this is known to work on SMP.
>>
>> My analysis for this concrete case,
>>
>> 1) RTEMS uses same MMU table for all CPUs.
>> 2) inner set_translation_table_entries() ensures that change
>>    is visible to all CPUs
>> 3) when arm_cp15_set_translation_table_entries() is called for range
>>    which is not (yet) accessed by other CPU(s) then the function should
>>    not cause any breakage
>> 4) when change is requested for same range in parallel/twice from multiple
>>    threads or even CPUs then  it is indication of some more serious problem
>>    in upper layers
>> 5) if the update of mapping is requested for some range from cached to
>>    uncached for example then it is not enough to protect
>>    arm_cp15_set_translation_table_entries() alone. Protection
>>    of necessary cache flush and invalidation is required in addition.
>>
>>
>> So generally, there is almost no difference between case when there is
>> no protection or weak protection by rtems_interrupt_local_disable().
>> May it be that there can be some difference if operation
>>     ttb [i] = addr | section_flags;
>> is translated by multiple memory accesses by C compiler (highly
>> improbable).
>>
>> If the dynamic MMU operations/virtual space allocations are needed
>> like in multiprocess OS then MMU table changes needs to be protected
>> by spinlock or better semaphore (to not cause block of other tasks).
>>
>> So I agree that simple change of rtems_interrupt_disable(level)
>> to rtems_interrupt_local_disable(level) is not perfect but on the other
>> hand real situation is not changed. In the fact, it would worth
>> to change protection to MMU context semaphore.
>>
>> So generally, I am unsure what is preferred solution for now.
>>
>> By the way, is there some defined function/way to check if RTEMS
>> executive reached switch to multitasking mode?
>> I would like to protect VideoCore operations with mutex/semaphore
>> but these operations has to be accessible even from bsp_start_hook_0 or 1
>> before core is ready. So I would like to skip mutex/semaphore operations
>> if functions are called during early initialization.
>>
>> May it be that same concept should be used for MMU operations
>> serialization.
>>
>> Best wishes,
>>
>>               Pavel
>>
>>
>>
>> _______________________________________________
>> devel mailing list
>> devel at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
>