On Thu, Aug 25, 2016 at 6:56 AM, Pavel Pisa <ppisa4li...@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@rtems.org >> http://lists.rtems.org/mailman/listinfo/devel > _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel