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

[Pavel Pisa]

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?

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.

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