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

[Pavel Pisa]

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