Re: [PATCH v2 2/2] libbsp/arm: Fix the local interrupt mask disable/enable calls.
Hi Pavel, I am sorry about the delay. I have been on other things and then I was out with a pesky flue virus for the past week. On 26/08/2016 03:47, Pavel Pisa wrote: Hello Gedare, On Thursday 25 of August 2016 17:32:09 Gedare Bloom wrote: On Thu, Aug 25, 2016 at 6:56 AM, Pavel Pisa wrote: Is there some defined function/way to check if RTEMS executive reached switch to multitasking mode? _System_state_Is_before_multitasking() Thanks, great that is what I have been looking for. 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. His (correct) concern has been about SMP. But I think that need of locking has been eliminated by my previous commit. There is another reason against locking by IRQ disable or spinlock. If we consider update of pagetables at runtime where some real time tasks run then if scheduler is blocked but change in large part of address space then we can consider this as almost unbound latency source. So I vote for no locking at this level at all and mutex based locking (skipped if part of initialization and _System_state_Is_before_multitasking) at higher level when concurrent use need arises - for example allocation of virtual address space for mmap or something similar. Great. I was hoping you would provide a clear decision. I agree. I will remove the interrupt mask and push the change. Thanks for posts and the detail. It is really valuable. Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2 2/2] libbsp/arm: Fix the local interrupt mask disable/enable calls.
On Thu, Aug 25, 2016 at 6:56 AM, Pavel Pisa 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). >>
Re: [PATCH v2 2/2] libbsp/arm: Fix the local interrupt mask disable/enable calls.
On Thu, Aug 25, 2016 at 1:47 PM, Pavel Pisa wrote: > Hello Gedare, > > On Thursday 25 of August 2016 17:32:09 Gedare Bloom wrote: >> On Thu, Aug 25, 2016 at 6:56 AM, Pavel Pisa wrote: >> > Is there some defined function/way to check if RTEMS executive >> > reached switch to multitasking mode? >> >> _System_state_Is_before_multitasking() > > Thanks, great that is what I have been looking for. > >> > 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. > > His (correct) concern has been about SMP. > But I think that need of locking has been eliminated by my previous commit. > There is another reason against locking by IRQ disable or spinlock. > If we consider update of pagetables at runtime where some real time tasks > run then if scheduler is blocked but change in large part of address space > then we can consider this as almost unbound latency source. > > So I vote for no locking at this level at all and mutex based locking > (skipped if part of initialization and _System_state_Is_before_multitasking) > at higher level when concurrent use need arises - for example allocation > of virtual address space for mmap or something similar. > This seems reasonable to me. I expect to eventually add some (limited) capabilities in this direction but I have not finished any (mental) design. > Best wishes, > > Pavel > > ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2 2/2] libbsp/arm: Fix the local interrupt mask disable/enable calls.
Hello Gedare, On Thursday 25 of August 2016 17:32:09 Gedare Bloom wrote: > On Thu, Aug 25, 2016 at 6:56 AM, Pavel Pisa wrote: > > Is there some defined function/way to check if RTEMS executive > > reached switch to multitasking mode? > > _System_state_Is_before_multitasking() Thanks, great that is what I have been looking for. > > 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. His (correct) concern has been about SMP. But I think that need of locking has been eliminated by my previous commit. There is another reason against locking by IRQ disable or spinlock. If we consider update of pagetables at runtime where some real time tasks run then if scheduler is blocked but change in large part of address space then we can consider this as almost unbound latency source. So I vote for no locking at this level at all and mutex based locking (skipped if part of initialization and _System_state_Is_before_multitasking) at higher level when concurrent use need arises - for example allocation of virtual address space for mmap or something similar. Best wishes, Pavel ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v2 2/2] libbsp/arm: Fix the local interrupt mask disable/enable calls.
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. I
Re: [PATCH v2 2/2] libbsp/arm: Fix the local interrupt mask disable/enable calls.
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
Re: [PATCH v2 2/2] libbsp/arm: Fix the local interrupt mask disable/enable calls.
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. -- Sebastian Huber, embedded brains GmbH Address : Dornierstr. 4, D-82178 Puchheim, Germany Phone : +49 89 189 47 41-16 Fax : +49 89 189 47 41-09 E-Mail : sebastian.hu...@embedded-brains.de PGP : Public key available on request. Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG. ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
