* [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE [not found] <cover.1623813516.git.luto@kernel.org> @ 2021-06-16 3:21 ` Andy Lutomirski 2021-06-16 9:28 ` Russell King (Oracle) ` (3 more replies) 2021-06-16 3:21 ` [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation Andy Lutomirski 1 sibling, 4 replies; 45+ messages in thread From: Andy Lutomirski @ 2021-06-16 3:21 UTC (permalink / raw) To: x86 Cc: Dave Hansen, LKML, linux-mm, Andrew Morton, Andy Lutomirski, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra, Russell King, linux-arm-kernel On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it. Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Russell King <linux@armlinux.org.uk> Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Andy Lutomirski <luto@kernel.org> --- arch/arm/Kconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 24804f11302d..89a885fba724 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -10,7 +10,6 @@ config ARM select ARCH_HAS_FORTIFY_SOURCE select ARCH_HAS_KEEPINITRD select ARCH_HAS_KCOV - select ARCH_HAS_MEMBARRIER_SYNC_CORE select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE select ARCH_HAS_PTE_SPECIAL if ARM_LPAE select ARCH_HAS_PHYS_TO_DMA -- 2.31.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 3:21 ` [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE Andy Lutomirski @ 2021-06-16 9:28 ` Russell King (Oracle) 2021-06-16 10:16 ` Peter Zijlstra ` (2 subsequent siblings) 3 siblings, 0 replies; 45+ messages in thread From: Russell King (Oracle) @ 2021-06-16 9:28 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra, linux-arm-kernel On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > On arm32, the only way to safely flush icache from usermode is to call > cacheflush(2). This also handles any required pipeline flushes, so > membarrier's SYNC_CORE feature is useless on arm. Remove it. Yay! About time too. Acked-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 3:21 ` [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE Andy Lutomirski 2021-06-16 9:28 ` Russell King (Oracle) @ 2021-06-16 10:16 ` Peter Zijlstra 2021-06-16 10:20 ` Peter Zijlstra 2021-06-17 10:40 ` Mark Rutland 2021-06-18 0:07 ` Andy Lutomirski 3 siblings, 1 reply; 45+ messages in thread From: Peter Zijlstra @ 2021-06-16 10:16 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Russell King, linux-arm-kernel, Will Deacon On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > On arm32, the only way to safely flush icache from usermode is to call > cacheflush(2). This also handles any required pipeline flushes, so > membarrier's SYNC_CORE feature is useless on arm. Remove it. So SYNC_CORE is there to help an architecture that needs to do something per CPU. If I$ invalidation is broadcast and I$ invalidation also triggers the flush of any uarch caches derived from it (if there are any). Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), which, if I read things right, end up in arch/arm/mm/*.S, but that doesn't consider cache_ops_need_broadcast(). Will suggests that perhaps ARM 11MPCore might need this due to their I$ flush maybe not being broadcast _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 10:16 ` Peter Zijlstra @ 2021-06-16 10:20 ` Peter Zijlstra 2021-06-16 10:34 ` Russell King (Oracle) 0 siblings, 1 reply; 45+ messages in thread From: Peter Zijlstra @ 2021-06-16 10:20 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Russell King, linux-arm-kernel, Will Deacon On Wed, Jun 16, 2021 at 12:16:27PM +0200, Peter Zijlstra wrote: > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > On arm32, the only way to safely flush icache from usermode is to call > > cacheflush(2). This also handles any required pipeline flushes, so > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > So SYNC_CORE is there to help an architecture that needs to do something > per CPU. If I$ invalidation is broadcast and I$ invalidation also > triggers the flush of any uarch caches derived from it (if there are > any). Incomplete sentence there: + then we don't need SYNC_CORE. > Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), > which, if I read things right, end up in arch/arm/mm/*.S, but that > doesn't consider cache_ops_need_broadcast(). > > Will suggests that perhaps ARM 11MPCore might need this due to their I$ > flush maybe not being broadcast _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 10:20 ` Peter Zijlstra @ 2021-06-16 10:34 ` Russell King (Oracle) 2021-06-16 11:10 ` Peter Zijlstra 0 siblings, 1 reply; 45+ messages in thread From: Russell King (Oracle) @ 2021-06-16 10:34 UTC (permalink / raw) To: Peter Zijlstra Cc: Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel, Will Deacon On Wed, Jun 16, 2021 at 12:20:06PM +0200, Peter Zijlstra wrote: > On Wed, Jun 16, 2021 at 12:16:27PM +0200, Peter Zijlstra wrote: > > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > > On arm32, the only way to safely flush icache from usermode is to call > > > cacheflush(2). This also handles any required pipeline flushes, so > > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > > > So SYNC_CORE is there to help an architecture that needs to do something > > per CPU. If I$ invalidation is broadcast and I$ invalidation also > > triggers the flush of any uarch caches derived from it (if there are > > any). > > Incomplete sentence there: + then we don't need SYNC_CORE. > > > Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), > > which, if I read things right, end up in arch/arm/mm/*.S, but that > > doesn't consider cache_ops_need_broadcast(). > > > > Will suggests that perhaps ARM 11MPCore might need this due to their I$ > > flush maybe not being broadcast If it leaves other cores with incoherent I cache, then that's already a problem for SMP cores, since there could be no guarantee that the modifications made by one core will be visible to some other core that ends up running that code - and there is little option for userspace to work around that except by pinning the thread making the modifications and subsequently executing the code to a core. The same is also true of flush_icache_range() - which is used when loading a kernel module. In the case Will is referring to, these alias to the same code. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 10:34 ` Russell King (Oracle) @ 2021-06-16 11:10 ` Peter Zijlstra 2021-06-16 13:22 ` Russell King (Oracle) 0 siblings, 1 reply; 45+ messages in thread From: Peter Zijlstra @ 2021-06-16 11:10 UTC (permalink / raw) To: Russell King (Oracle) Cc: Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel, Will Deacon On Wed, Jun 16, 2021 at 11:34:46AM +0100, Russell King (Oracle) wrote: > On Wed, Jun 16, 2021 at 12:20:06PM +0200, Peter Zijlstra wrote: > > On Wed, Jun 16, 2021 at 12:16:27PM +0200, Peter Zijlstra wrote: > > > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > > > On arm32, the only way to safely flush icache from usermode is to call > > > > cacheflush(2). This also handles any required pipeline flushes, so > > > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > > > > > So SYNC_CORE is there to help an architecture that needs to do something > > > per CPU. If I$ invalidation is broadcast and I$ invalidation also > > > triggers the flush of any uarch caches derived from it (if there are > > > any). > > > > Incomplete sentence there: + then we don't need SYNC_CORE. > > > > > Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), > > > which, if I read things right, end up in arch/arm/mm/*.S, but that > > > doesn't consider cache_ops_need_broadcast(). > > > > > > Will suggests that perhaps ARM 11MPCore might need this due to their I$ > > > flush maybe not being broadcast > > If it leaves other cores with incoherent I cache, then that's already > a problem for SMP cores, since there could be no guarantee that the > modifications made by one core will be visible to some other core that > ends up running that code - and there is little option for userspace to > work around that except by pinning the thread making the modifications > and subsequently executing the code to a core. That's where SYNC_CORE can help. Or you make sys_cacheflush() do a system wide IPI. > The same is also true of flush_icache_range() - which is used when > loading a kernel module. In the case Will is referring to, these alias > to the same code. Yes, cache_ops_need_broadcast() seems to be missing in more places. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 11:10 ` Peter Zijlstra @ 2021-06-16 13:22 ` Russell King (Oracle) 2021-06-16 15:04 ` Catalin Marinas 0 siblings, 1 reply; 45+ messages in thread From: Russell King (Oracle) @ 2021-06-16 13:22 UTC (permalink / raw) To: Peter Zijlstra Cc: Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel, Will Deacon On Wed, Jun 16, 2021 at 01:10:58PM +0200, Peter Zijlstra wrote: > On Wed, Jun 16, 2021 at 11:34:46AM +0100, Russell King (Oracle) wrote: > > On Wed, Jun 16, 2021 at 12:20:06PM +0200, Peter Zijlstra wrote: > > > On Wed, Jun 16, 2021 at 12:16:27PM +0200, Peter Zijlstra wrote: > > > > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > > > > On arm32, the only way to safely flush icache from usermode is to call > > > > > cacheflush(2). This also handles any required pipeline flushes, so > > > > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > > > > > > > So SYNC_CORE is there to help an architecture that needs to do something > > > > per CPU. If I$ invalidation is broadcast and I$ invalidation also > > > > triggers the flush of any uarch caches derived from it (if there are > > > > any). > > > > > > Incomplete sentence there: + then we don't need SYNC_CORE. > > > > > > > Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), > > > > which, if I read things right, end up in arch/arm/mm/*.S, but that > > > > doesn't consider cache_ops_need_broadcast(). > > > > > > > > Will suggests that perhaps ARM 11MPCore might need this due to their I$ > > > > flush maybe not being broadcast > > > > If it leaves other cores with incoherent I cache, then that's already > > a problem for SMP cores, since there could be no guarantee that the > > modifications made by one core will be visible to some other core that > > ends up running that code - and there is little option for userspace to > > work around that except by pinning the thread making the modifications > > and subsequently executing the code to a core. > > That's where SYNC_CORE can help. Or you make sys_cacheflush() do a > system wide IPI. If it's a problem, then it needs fixing. sys_cacheflush() is used to implement GCC's __builtin___clear_cache(). I'm not sure who added this to gcc. > > The same is also true of flush_icache_range() - which is used when > > loading a kernel module. In the case Will is referring to, these alias > > to the same code. > > Yes, cache_ops_need_broadcast() seems to be missing in more places. Likely only in places where we care about I/D coherency - as the data cache is required to be PIPT on these SMP platforms. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 13:22 ` Russell King (Oracle) @ 2021-06-16 15:04 ` Catalin Marinas 2021-06-16 15:23 ` Russell King (Oracle) 0 siblings, 1 reply; 45+ messages in thread From: Catalin Marinas @ 2021-06-16 15:04 UTC (permalink / raw) To: Russell King (Oracle) Cc: Peter Zijlstra, Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel, Will Deacon On Wed, Jun 16, 2021 at 02:22:27PM +0100, Russell King wrote: > On Wed, Jun 16, 2021 at 01:10:58PM +0200, Peter Zijlstra wrote: > > On Wed, Jun 16, 2021 at 11:34:46AM +0100, Russell King (Oracle) wrote: > > > On Wed, Jun 16, 2021 at 12:20:06PM +0200, Peter Zijlstra wrote: > > > > On Wed, Jun 16, 2021 at 12:16:27PM +0200, Peter Zijlstra wrote: > > > > > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > > > > > On arm32, the only way to safely flush icache from usermode is to call > > > > > > cacheflush(2). This also handles any required pipeline flushes, so > > > > > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > > > > > > > > > So SYNC_CORE is there to help an architecture that needs to do something > > > > > per CPU. If I$ invalidation is broadcast and I$ invalidation also > > > > > triggers the flush of any uarch caches derived from it (if there are > > > > > any). > > > > > > > > Incomplete sentence there: + then we don't need SYNC_CORE. > > > > > > > > > Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), > > > > > which, if I read things right, end up in arch/arm/mm/*.S, but that > > > > > doesn't consider cache_ops_need_broadcast(). > > > > > > > > > > Will suggests that perhaps ARM 11MPCore might need this due to their I$ > > > > > flush maybe not being broadcast > > > > > > If it leaves other cores with incoherent I cache, then that's already > > > a problem for SMP cores, since there could be no guarantee that the > > > modifications made by one core will be visible to some other core that > > > ends up running that code - and there is little option for userspace to > > > work around that except by pinning the thread making the modifications > > > and subsequently executing the code to a core. > > > > That's where SYNC_CORE can help. Or you make sys_cacheflush() do a > > system wide IPI. > > If it's a problem, then it needs fixing. sys_cacheflush() is used to > implement GCC's __builtin___clear_cache(). I'm not sure who added this > to gcc. I'm surprised that it works. I guess it's just luck that the thread doing the code writing doesn't migrate before the sys_cacheflush() call. > > > The same is also true of flush_icache_range() - which is used when > > > loading a kernel module. In the case Will is referring to, these alias > > > to the same code. > > > > Yes, cache_ops_need_broadcast() seems to be missing in more places. > > Likely only in places where we care about I/D coherency - as the data > cache is required to be PIPT on these SMP platforms. We had similar issue with the cache maintenance for DMA. The hack we employed (in cache.S) is relying on the MESI protocol internals and forcing a read/write for ownership before the D-cache maintenance. Luckily ARM11MPCore doesn't do speculative data loads to trigger some migration back. The simpler fix for flush_icache_range() is to disable preemption, read a word in a cacheline to force any dirty lines on another CPU to be evicted and then issue the D-cache maintenance (for those cache lines which are still dirty on the current CPU). It's a hack that only works on ARM11MPCore. Newer MP cores are saner. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 15:04 ` Catalin Marinas @ 2021-06-16 15:23 ` Russell King (Oracle) 2021-06-16 15:45 ` Catalin Marinas 0 siblings, 1 reply; 45+ messages in thread From: Russell King (Oracle) @ 2021-06-16 15:23 UTC (permalink / raw) To: Catalin Marinas Cc: Peter Zijlstra, Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel, Will Deacon On Wed, Jun 16, 2021 at 04:04:56PM +0100, Catalin Marinas wrote: > On Wed, Jun 16, 2021 at 02:22:27PM +0100, Russell King wrote: > > If it's a problem, then it needs fixing. sys_cacheflush() is used to > > implement GCC's __builtin___clear_cache(). I'm not sure who added this > > to gcc. > > I'm surprised that it works. I guess it's just luck that the thread > doing the code writing doesn't migrate before the sys_cacheflush() call. Maybe the platforms that use ARM MPCore avoid the issue somehow (maybe by not using self-modifying code?) > > Likely only in places where we care about I/D coherency - as the data > > cache is required to be PIPT on these SMP platforms. > > We had similar issue with the cache maintenance for DMA. The hack we > employed (in cache.S) is relying on the MESI protocol internals and > forcing a read/write for ownership before the D-cache maintenance. > Luckily ARM11MPCore doesn't do speculative data loads to trigger some > migration back. That's very similar to the hack that was originally implemented for MPCore DMA - see the DMA_CACHE_RWFO configuration option. An interesting point here is that cache_ops_need_broadcast() reads MMFR3 bits 12..15, which in the MPCore TRM has nothing to with cache operation broadcasting - but luckily is documented as containing zero. So, cache_ops_need_broadcast() returns correctly (true) here. > The simpler fix for flush_icache_range() is to disable preemption, read > a word in a cacheline to force any dirty lines on another CPU to be > evicted and then issue the D-cache maintenance (for those cache lines > which are still dirty on the current CPU). Is just reading sufficient? If so, why do we do a read-then-write in the MPCore DMA cache ops? Don't we need the write to force exclusive ownership? If we don't have exclusive ownership of the dirty line, how can we be sure to write it out of the caches? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 15:23 ` Russell King (Oracle) @ 2021-06-16 15:45 ` Catalin Marinas 2021-06-16 16:00 ` Catalin Marinas 0 siblings, 1 reply; 45+ messages in thread From: Catalin Marinas @ 2021-06-16 15:45 UTC (permalink / raw) To: Russell King (Oracle) Cc: Peter Zijlstra, Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel, Will Deacon On Wed, Jun 16, 2021 at 04:23:26PM +0100, Russell King wrote: > On Wed, Jun 16, 2021 at 04:04:56PM +0100, Catalin Marinas wrote: > > On Wed, Jun 16, 2021 at 02:22:27PM +0100, Russell King wrote: > > > If it's a problem, then it needs fixing. sys_cacheflush() is used to > > > implement GCC's __builtin___clear_cache(). I'm not sure who added this > > > to gcc. > > > > I'm surprised that it works. I guess it's just luck that the thread > > doing the code writing doesn't migrate before the sys_cacheflush() call. > > Maybe the platforms that use ARM MPCore avoid the issue somehow (maybe > by not using self-modifying code?) Not sure how widely it is/was used with JITs. In general, I think the systems at the time were quite tolerant to missing I-cache maintenance (maybe small caches?). We ran Linux for a while without 826cbdaff297 ("[ARM] 5092/1: Fix the I-cache invalidation on ARMv6 and later CPUs"). > > > Likely only in places where we care about I/D coherency - as the data > > > cache is required to be PIPT on these SMP platforms. > > > > We had similar issue with the cache maintenance for DMA. The hack we > > employed (in cache.S) is relying on the MESI protocol internals and > > forcing a read/write for ownership before the D-cache maintenance. > > Luckily ARM11MPCore doesn't do speculative data loads to trigger some > > migration back. > > That's very similar to the hack that was originally implemented for > MPCore DMA - see the DMA_CACHE_RWFO configuration option. Well, yes, that's what I wrote above ;) (I added the hack and config option IIRC). > An interesting point here is that cache_ops_need_broadcast() reads > MMFR3 bits 12..15, which in the MPCore TRM has nothing to with cache > operation broadcasting - but luckily is documented as containing zero. > So, cache_ops_need_broadcast() returns correctly (true) here. That's typical with any new feature. The 12..15 field was added in ARMv7 stating that cache maintenance is broadcast in hardware. Prior to this, the field was read-as-zero. So it's not luck but we could have avoided negating the meaning here, i.e. call it cache_ops_are_broadcast(). > > The simpler fix for flush_icache_range() is to disable preemption, read > > a word in a cacheline to force any dirty lines on another CPU to be > > evicted and then issue the D-cache maintenance (for those cache lines > > which are still dirty on the current CPU). > > Is just reading sufficient? If so, why do we do a read-then-write in > the MPCore DMA cache ops? Don't we need the write to force exclusive > ownership? If we don't have exclusive ownership of the dirty line, > how can we be sure to write it out of the caches? For cleaning (which is the case for I/D coherency), we only need reading since we are fine with clean lines being left in the D-cache on other CPUs. For invalidation, we indeed need to force the exclusive ownership, hence the write. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 15:45 ` Catalin Marinas @ 2021-06-16 16:00 ` Catalin Marinas 2021-06-16 16:27 ` Russell King (Oracle) 0 siblings, 1 reply; 45+ messages in thread From: Catalin Marinas @ 2021-06-16 16:00 UTC (permalink / raw) To: Russell King (Oracle) Cc: Peter Zijlstra, Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel, Will Deacon On Wed, Jun 16, 2021 at 04:45:29PM +0100, Catalin Marinas wrote: > On Wed, Jun 16, 2021 at 04:23:26PM +0100, Russell King wrote: > > On Wed, Jun 16, 2021 at 04:04:56PM +0100, Catalin Marinas wrote: > > > The simpler fix for flush_icache_range() is to disable preemption, read > > > a word in a cacheline to force any dirty lines on another CPU to be > > > evicted and then issue the D-cache maintenance (for those cache lines > > > which are still dirty on the current CPU). > > > > Is just reading sufficient? If so, why do we do a read-then-write in > > the MPCore DMA cache ops? Don't we need the write to force exclusive > > ownership? If we don't have exclusive ownership of the dirty line, > > how can we be sure to write it out of the caches? > > For cleaning (which is the case for I/D coherency), we only need reading > since we are fine with clean lines being left in the D-cache on other > CPUs. For invalidation, we indeed need to force the exclusive ownership, > hence the write. Ah, I'm not sure the I-cache is broadcast in hardware on ARM11MPCore either. So fixing the D side won't be sufficient. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 16:00 ` Catalin Marinas @ 2021-06-16 16:27 ` Russell King (Oracle) 2021-06-17 8:55 ` Krzysztof Hałasa 2021-06-18 12:54 ` Linus Walleij 0 siblings, 2 replies; 45+ messages in thread From: Russell King (Oracle) @ 2021-06-16 16:27 UTC (permalink / raw) To: Catalin Marinas, Linus Walleij, Krzysztof Halasa, Neil Armstrong Cc: Peter Zijlstra, Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel, Will Deacon On Wed, Jun 16, 2021 at 05:00:51PM +0100, Catalin Marinas wrote: > On Wed, Jun 16, 2021 at 04:45:29PM +0100, Catalin Marinas wrote: > > On Wed, Jun 16, 2021 at 04:23:26PM +0100, Russell King wrote: > > > On Wed, Jun 16, 2021 at 04:04:56PM +0100, Catalin Marinas wrote: > > > > The simpler fix for flush_icache_range() is to disable preemption, read > > > > a word in a cacheline to force any dirty lines on another CPU to be > > > > evicted and then issue the D-cache maintenance (for those cache lines > > > > which are still dirty on the current CPU). > > > > > > Is just reading sufficient? If so, why do we do a read-then-write in > > > the MPCore DMA cache ops? Don't we need the write to force exclusive > > > ownership? If we don't have exclusive ownership of the dirty line, > > > how can we be sure to write it out of the caches? > > > > For cleaning (which is the case for I/D coherency), we only need reading > > since we are fine with clean lines being left in the D-cache on other > > CPUs. For invalidation, we indeed need to force the exclusive ownership, > > hence the write. > > Ah, I'm not sure the I-cache is broadcast in hardware on ARM11MPCore > either. So fixing the D side won't be sufficient. The other question is... do we bother to fix this. Arnd tells me that the current remaining ARM11MPCore users are: - CNS3xxx (where there is some martinal interest in the Gateworks Laguna platform) - Similar for OXNAS - There used to be the Realview MPCore tile - I haven't turned that on in ages, and it may be that the 3V cell that backs up the encryption keys is dead so it may not even boot. - Not sure about the story with QEMU - Arnd doesn't think there would be a problem there as it may not model caches. So it seems to come down to a question about CNS3xxx and OXNAS. If these aren't being used, maybe we can drop ARM11MPCore support and the associated platforms? Linus, Krzysztof, Neil, any input? Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 16:27 ` Russell King (Oracle) @ 2021-06-17 8:55 ` Krzysztof Hałasa 2021-06-18 12:54 ` Linus Walleij 1 sibling, 0 replies; 45+ messages in thread From: Krzysztof Hałasa @ 2021-06-17 8:55 UTC (permalink / raw) To: Russell King (Oracle) Cc: Catalin Marinas, Linus Walleij, Neil Armstrong, Peter Zijlstra, Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel, Will Deacon "Russell King (Oracle)" <linux@armlinux.org.uk> writes: > So it seems to come down to a question about CNS3xxx and OXNAS. If > these aren't being used, maybe we can drop ARM11MPCore support and > the associated platforms? Well, it appears we haven't updated software on our Gateworks Lagunas (CNS3xxx dual core) for 4 years. This is old stuff, pre-DTB and all. We have replacement setups (i.MX6 + mPCIe to mPCI bridge) which we don't use either (due to lack of interest in mPCI - the old parallel, not the express). I don't have a problem with the CNS3xxx being dropped. In fact, we don't use anything (ARM) older than v7 here. Chris. -- Krzysztof Hałasa Sieć Badawcza Łukasiewicz Przemysłowy Instytut Automatyki i Pomiarów PIAP Al. Jerozolimskie 202, 02-486 Warszawa _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 16:27 ` Russell King (Oracle) 2021-06-17 8:55 ` Krzysztof Hałasa @ 2021-06-18 12:54 ` Linus Walleij 2021-06-18 13:19 ` Russell King (Oracle) 2021-06-18 13:36 ` Arnd Bergmann 1 sibling, 2 replies; 45+ messages in thread From: Linus Walleij @ 2021-06-18 12:54 UTC (permalink / raw) To: Russell King (Oracle) Cc: Catalin Marinas, Krzysztof Halasa, Neil Armstrong, Peter Zijlstra, Andy Lutomirski, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Dave Hansen, LKML, Linux Memory Management List, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Linux ARM, Will Deacon On Wed, Jun 16, 2021 at 6:27 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > Arnd tells me that the current remaining ARM11MPCore users are: > - CNS3xxx (where there is some martinal interest in the Gateworks > Laguna platform) > - Similar for OXNAS > - There used to be the Realview MPCore tile - I haven't turned that on > in ages, and it may be that the 3V cell that backs up the encryption > keys is dead so it may not even boot. I have this machine with 4 x ARM11 MPCore, it works like a charm. I use it to test exactly this kind of stuff, I know if a kernel works on ARM11MPCore it works on anything because of how fragile it is. > So it seems to come down to a question about CNS3xxx and OXNAS. If > these aren't being used, maybe we can drop ARM11MPCore support and > the associated platforms? > > Linus, Krzysztof, Neil, any input? I don't especially need to keep the ARM11MPCore machine alive, it is just a testchip after all. The Oxnas is another story, that has wide deployment and was contributed recently (2016) and has excellent support in OpenWrt so I wouldn't really want to axe that. Yours, Linus Walleij _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-18 12:54 ` Linus Walleij @ 2021-06-18 13:19 ` Russell King (Oracle) 2021-06-18 13:36 ` Arnd Bergmann 1 sibling, 0 replies; 45+ messages in thread From: Russell King (Oracle) @ 2021-06-18 13:19 UTC (permalink / raw) To: Linus Walleij Cc: Catalin Marinas, Krzysztof Halasa, Neil Armstrong, Peter Zijlstra, Andy Lutomirski, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Dave Hansen, LKML, Linux Memory Management List, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Linux ARM, Will Deacon On Fri, Jun 18, 2021 at 02:54:05PM +0200, Linus Walleij wrote: > On Wed, Jun 16, 2021 at 6:27 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > Arnd tells me that the current remaining ARM11MPCore users are: > > - CNS3xxx (where there is some martinal interest in the Gateworks > > Laguna platform) > > - Similar for OXNAS > > - There used to be the Realview MPCore tile - I haven't turned that on > > in ages, and it may be that the 3V cell that backs up the encryption > > keys is dead so it may not even boot. > > I have this machine with 4 x ARM11 MPCore, it works like a charm. > I use it to test exactly this kind of stuff, I know if a kernel works > on ARM11MPCore it works on anything because of how fragile > it is. > > > So it seems to come down to a question about CNS3xxx and OXNAS. If > > these aren't being used, maybe we can drop ARM11MPCore support and > > the associated platforms? > > > > Linus, Krzysztof, Neil, any input? > > I don't especially need to keep the ARM11MPCore machine alive, > it is just a testchip after all. The Oxnas is another story, that has wide > deployment and was contributed recently (2016) and has excellent > support in OpenWrt so I wouldn't really want > to axe that. So I suppose the next question is... are these issues (with userland self-modifying code and kernel module loading) entirely theoretical or can they be produced on real hardware? If they can't be produced on real hardware, and we attempt to fix them how do we know that the fix has worked... -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-18 12:54 ` Linus Walleij 2021-06-18 13:19 ` Russell King (Oracle) @ 2021-06-18 13:36 ` Arnd Bergmann 1 sibling, 0 replies; 45+ messages in thread From: Arnd Bergmann @ 2021-06-18 13:36 UTC (permalink / raw) To: Linus Walleij Cc: Russell King (Oracle), Catalin Marinas, Krzysztof Halasa, Neil Armstrong, Peter Zijlstra, Andy Lutomirski, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Dave Hansen, LKML, Linux Memory Management List, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Linux ARM, Will Deacon, Daniel Golle On Fri, Jun 18, 2021 at 2:54 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Jun 16, 2021 at 6:27 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > So it seems to come down to a question about CNS3xxx and OXNAS. If > > these aren't being used, maybe we can drop ARM11MPCore support and > > the associated platforms? > > > > Linus, Krzysztof, Neil, any input? > > I don't especially need to keep the ARM11MPCore machine alive, > it is just a testchip after all. The Oxnas is another story, that has wide > deployment and was contributed recently (2016) and has excellent > support in OpenWrt so I wouldn't really want to axe that. Agreed, as long as oxnas and/or cns3xxx are around, we should just keep the realview 11mpcore support, but if both of the commercial platforms are gone, then the realview can be retired as far as I'm concerned. Regarding oxnas, I see that OpenWRT has a number of essential device drivers (sata, pcie, usb and reset) that look like they could just be merged upstream, but that effort appears to have stalled: no device support was added to the dts files since the original 2016 merge. While the support in OpenWRT may be excellent, the platform support in the mainline kernel is limited to ethernet, nand, uart and gpio. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 3:21 ` [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE Andy Lutomirski 2021-06-16 9:28 ` Russell King (Oracle) 2021-06-16 10:16 ` Peter Zijlstra @ 2021-06-17 10:40 ` Mark Rutland 2021-06-17 11:23 ` Russell King (Oracle) 2021-06-18 0:07 ` Andy Lutomirski 3 siblings, 1 reply; 45+ messages in thread From: Mark Rutland @ 2021-06-17 10:40 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra, Russell King, linux-arm-kernel On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > On arm32, the only way to safely flush icache from usermode is to call > cacheflush(2). This also handles any required pipeline flushes, so > membarrier's SYNC_CORE feature is useless on arm. Remove it. Unfortunately, it's a bit more complicated than that, and these days SYNC_CORE is equally necessary on arm as on arm64. This is something that changed in the architecture over time, but since ARMv7 we generally need both the cache maintenance *and* a context synchronization event (the latter must occur on the CPU which will execute the instructions). If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section A3.5.4 "Concurrent modification and execution of instructions" covers this. That manual can be found at: https://developer.arm.com/documentation/ddi0406/latest/ Likewise for ARMv8-A; the latest manual (ARM DDI 0487G.a) covers this in sections B2.2.5 and E2.3.5. That manual can be found at: https://developer.arm.com/documentation/ddi0487/ga I am not sure about exactly what's required 11MPcore, since that's somewhat a special case as the only SMP design prior to ARMv7-A mandating broadcast maintenance. For intuition's sake, one reason for this is that once a CPU has fetched an instruction from an instruction cache into its pipeline and that instruction is "in-flight", changes to that instruction cache are not guaranteed to affect the "in-flight" copy (which e.g. could be decomposed into micro-ops and so on). While these parts of a CPU aren't necessarily designed as caches, they effectively transiently cache a stale copy of the instruction while it is being executed. This is more pronounced on newer designs with more complex execution pipelines (e.g. with bigger windows for out-of-order execution and speculation), and generally it's unlikely for this to be noticed on smaller/simpler designs. As above, modifying instructions requires two things: 1) Making sure that *subsequent* instruction fetches will see the new instructions. This is what cacheflush(2) does, and this is similar to what SW does on arm64 with DC CVAU + IC IVAU instructions and associated memory barriers. 2) Making sure that a CPU fetches the instructions *after* the cache maintenance is complete. There are a few ways to do this: * A context synchronization event (e.g. an ISB or exception return) on the CPU that will execute the instructions. This is what membarrier(SYNC_CORE) does. * In ARMv8-A there are some restrictions on the order in which modified instructions are guaranteed to be observed (e.g. if you publish a function, then subsequently install a branch to that new function), where an ISB may not be necessary. In the latest ARMv8-A manual as linked above, those are described in sections: - B2.3.8 "Ordering of instruction fetches" (for 64-bit) - E2.3.8 "Ordering of instruction fetches" (for 32-bit) * Where we can guarantee that a CPU cannot possibly have an instruction in-flight (e.g. due to a lack of a mapping to fetch instructions from), nothing is necessary. This is what we rely on when faulting in code pages. In these cases, the CPU is liable to take fault on the missing translation anyway. Thanks, Mark. > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: Nicholas Piggin <npiggin@gmail.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > arch/arm/Kconfig | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index 24804f11302d..89a885fba724 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -10,7 +10,6 @@ config ARM > select ARCH_HAS_FORTIFY_SOURCE > select ARCH_HAS_KEEPINITRD > select ARCH_HAS_KCOV > - select ARCH_HAS_MEMBARRIER_SYNC_CORE > select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE > select ARCH_HAS_PTE_SPECIAL if ARM_LPAE > select ARCH_HAS_PHYS_TO_DMA > -- > 2.31.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 10:40 ` Mark Rutland @ 2021-06-17 11:23 ` Russell King (Oracle) 2021-06-17 11:33 ` Mark Rutland 0 siblings, 1 reply; 45+ messages in thread From: Russell King (Oracle) @ 2021-06-17 11:23 UTC (permalink / raw) To: Mark Rutland Cc: Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra, linux-arm-kernel On Thu, Jun 17, 2021 at 11:40:46AM +0100, Mark Rutland wrote: > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > On arm32, the only way to safely flush icache from usermode is to call > > cacheflush(2). This also handles any required pipeline flushes, so > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > Unfortunately, it's a bit more complicated than that, and these days > SYNC_CORE is equally necessary on arm as on arm64. This is something > that changed in the architecture over time, but since ARMv7 we generally > need both the cache maintenance *and* a context synchronization event > (the latter must occur on the CPU which will execute the instructions). > > If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section > A3.5.4 "Concurrent modification and execution of instructions" covers > this. That manual can be found at: > > https://developer.arm.com/documentation/ddi0406/latest/ Looking at that, sys_cacheflush() meets this. The manual details a series of cache maintenance calls in "step 1" that the modifying thread must issue - this is exactly what sys_cacheflush() does. The same is true for ARMv6, except the "ISB" terminology is replaced by a "PrefetchFlush" terminology. (I checked DDI0100I). "step 2" requires an ISB on the "other CPU" prior to executing that code. As I understand it, in ARMv7, userspace can issue an ISB itself. For ARMv6K, it doesn't have ISB, but instead has a CP15 instruction for this that isn't availble to userspace. This is where we come to the situation about ARM 11MPCore, and whether we continue to support it or not. So, I think we're completely fine with ARMv7 under 32-bit ARM kernels as userspace has everything that's required. ARMv6K is a different matter as we've already identified for several reasons. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 11:23 ` Russell King (Oracle) @ 2021-06-17 11:33 ` Mark Rutland 2021-06-17 13:41 ` Andy Lutomirski 0 siblings, 1 reply; 45+ messages in thread From: Mark Rutland @ 2021-06-17 11:33 UTC (permalink / raw) To: Russell King (Oracle) Cc: Andy Lutomirski, x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra, linux-arm-kernel On Thu, Jun 17, 2021 at 12:23:05PM +0100, Russell King (Oracle) wrote: > On Thu, Jun 17, 2021 at 11:40:46AM +0100, Mark Rutland wrote: > > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > > On arm32, the only way to safely flush icache from usermode is to call > > > cacheflush(2). This also handles any required pipeline flushes, so > > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > > > Unfortunately, it's a bit more complicated than that, and these days > > SYNC_CORE is equally necessary on arm as on arm64. This is something > > that changed in the architecture over time, but since ARMv7 we generally > > need both the cache maintenance *and* a context synchronization event > > (the latter must occur on the CPU which will execute the instructions). > > > > If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section > > A3.5.4 "Concurrent modification and execution of instructions" covers > > this. That manual can be found at: > > > > https://developer.arm.com/documentation/ddi0406/latest/ > > Looking at that, sys_cacheflush() meets this. The manual details a > series of cache maintenance calls in "step 1" that the modifying thread > must issue - this is exactly what sys_cacheflush() does. The same is > true for ARMv6, except the "ISB" terminology is replaced by a > "PrefetchFlush" terminology. (I checked DDI0100I). > > "step 2" requires an ISB on the "other CPU" prior to executing that > code. As I understand it, in ARMv7, userspace can issue an ISB itself. > > For ARMv6K, it doesn't have ISB, but instead has a CP15 instruction > for this that isn't availble to userspace. This is where we come to > the situation about ARM 11MPCore, and whether we continue to support > it or not. > > So, I think we're completely fine with ARMv7 under 32-bit ARM kernels > as userspace has everything that's required. ARMv6K is a different > matter as we've already identified for several reasons. Sure, and I agree we should not change cacheflush(). The point of membarrier(SYNC_CORE) is that you can move the cost of that ISB out of the fast-path in the executing thread(s) and into the slow-path on the thread which generated the code. So e.g. rather than an executing thread always having to do: LDR <reg>, [<funcptr>] ISB // in case funcptr was just updated BLR <reg> ... you have the thread generating the code use membarrier(SYNC_CORE) prior to plublishing the funcptr, and the fast-path on all the executing threads can be: LDR <reg> [<funcptr>] BLR <reg> ... and thus I think we still want membarrier(SYNC_CORE) so that people can do this, even if there are other means to achieve the same functionality. Thanks, Mark. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 11:33 ` Mark Rutland @ 2021-06-17 13:41 ` Andy Lutomirski 2021-06-17 13:51 ` Mark Rutland 2021-06-17 14:05 ` Peter Zijlstra 0 siblings, 2 replies; 45+ messages in thread From: Andy Lutomirski @ 2021-06-17 13:41 UTC (permalink / raw) To: Mark Rutland, Russell King (Oracle) Cc: the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra (Intel), linux-arm-kernel On Thu, Jun 17, 2021, at 4:33 AM, Mark Rutland wrote: > On Thu, Jun 17, 2021 at 12:23:05PM +0100, Russell King (Oracle) wrote: > > On Thu, Jun 17, 2021 at 11:40:46AM +0100, Mark Rutland wrote: > > > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > > > On arm32, the only way to safely flush icache from usermode is to call > > > > cacheflush(2). This also handles any required pipeline flushes, so > > > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > > > > > Unfortunately, it's a bit more complicated than that, and these days > > > SYNC_CORE is equally necessary on arm as on arm64. This is something > > > that changed in the architecture over time, but since ARMv7 we generally > > > need both the cache maintenance *and* a context synchronization event > > > (the latter must occur on the CPU which will execute the instructions). > > > > > > If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section > > > A3.5.4 "Concurrent modification and execution of instructions" covers > > > this. That manual can be found at: > > > > > > https://developer.arm.com/documentation/ddi0406/latest/ > > > > Looking at that, sys_cacheflush() meets this. The manual details a > > series of cache maintenance calls in "step 1" that the modifying thread > > must issue - this is exactly what sys_cacheflush() does. The same is > > true for ARMv6, except the "ISB" terminology is replaced by a > > "PrefetchFlush" terminology. (I checked DDI0100I). > > > > "step 2" requires an ISB on the "other CPU" prior to executing that > > code. As I understand it, in ARMv7, userspace can issue an ISB itself. > > > > For ARMv6K, it doesn't have ISB, but instead has a CP15 instruction > > for this that isn't availble to userspace. This is where we come to > > the situation about ARM 11MPCore, and whether we continue to support > > it or not. > > > > So, I think we're completely fine with ARMv7 under 32-bit ARM kernels > > as userspace has everything that's required. ARMv6K is a different > > matter as we've already identified for several reasons. > > Sure, and I agree we should not change cacheflush(). > > The point of membarrier(SYNC_CORE) is that you can move the cost of that > ISB out of the fast-path in the executing thread(s) and into the > slow-path on the thread which generated the code. > > So e.g. rather than an executing thread always having to do: > > LDR <reg>, [<funcptr>] > ISB // in case funcptr was just updated > BLR <reg> > > ... you have the thread generating the code use membarrier(SYNC_CORE) > prior to plublishing the funcptr, and the fast-path on all the executing > threads can be: > > LDR <reg> [<funcptr>] > BLR <reg> > > ... and thus I think we still want membarrier(SYNC_CORE) so that people > can do this, even if there are other means to achieve the same > functionality. I had the impression that sys_cacheflush() did that. Am I wrong? In any event, I’m even more convinced that no new SYNC_CORE arches should be added. We need a new API that just does the right thing. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 13:41 ` Andy Lutomirski @ 2021-06-17 13:51 ` Mark Rutland 2021-06-17 14:00 ` Andy Lutomirski 2021-06-17 14:16 ` Mathieu Desnoyers 2021-06-17 14:05 ` Peter Zijlstra 1 sibling, 2 replies; 45+ messages in thread From: Mark Rutland @ 2021-06-17 13:51 UTC (permalink / raw) To: Andy Lutomirski Cc: Russell King (Oracle), the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra (Intel), linux-arm-kernel On Thu, Jun 17, 2021 at 06:41:41AM -0700, Andy Lutomirski wrote: > > > On Thu, Jun 17, 2021, at 4:33 AM, Mark Rutland wrote: > > On Thu, Jun 17, 2021 at 12:23:05PM +0100, Russell King (Oracle) wrote: > > > On Thu, Jun 17, 2021 at 11:40:46AM +0100, Mark Rutland wrote: > > > > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > > > > On arm32, the only way to safely flush icache from usermode is to call > > > > > cacheflush(2). This also handles any required pipeline flushes, so > > > > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > > > > > > > Unfortunately, it's a bit more complicated than that, and these days > > > > SYNC_CORE is equally necessary on arm as on arm64. This is something > > > > that changed in the architecture over time, but since ARMv7 we generally > > > > need both the cache maintenance *and* a context synchronization event > > > > (the latter must occur on the CPU which will execute the instructions). > > > > > > > > If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section > > > > A3.5.4 "Concurrent modification and execution of instructions" covers > > > > this. That manual can be found at: > > > > > > > > https://developer.arm.com/documentation/ddi0406/latest/ > > > > > > Looking at that, sys_cacheflush() meets this. The manual details a > > > series of cache maintenance calls in "step 1" that the modifying thread > > > must issue - this is exactly what sys_cacheflush() does. The same is > > > true for ARMv6, except the "ISB" terminology is replaced by a > > > "PrefetchFlush" terminology. (I checked DDI0100I). > > > > > > "step 2" requires an ISB on the "other CPU" prior to executing that > > > code. As I understand it, in ARMv7, userspace can issue an ISB itself. > > > > > > For ARMv6K, it doesn't have ISB, but instead has a CP15 instruction > > > for this that isn't availble to userspace. This is where we come to > > > the situation about ARM 11MPCore, and whether we continue to support > > > it or not. > > > > > > So, I think we're completely fine with ARMv7 under 32-bit ARM kernels > > > as userspace has everything that's required. ARMv6K is a different > > > matter as we've already identified for several reasons. > > > > Sure, and I agree we should not change cacheflush(). > > > > The point of membarrier(SYNC_CORE) is that you can move the cost of that > > ISB out of the fast-path in the executing thread(s) and into the > > slow-path on the thread which generated the code. > > > > So e.g. rather than an executing thread always having to do: > > > > LDR <reg>, [<funcptr>] > > ISB // in case funcptr was just updated > > BLR <reg> > > > > ... you have the thread generating the code use membarrier(SYNC_CORE) > > prior to plublishing the funcptr, and the fast-path on all the executing > > threads can be: > > > > LDR <reg> [<funcptr>] > > BLR <reg> > > > > ... and thus I think we still want membarrier(SYNC_CORE) so that people > > can do this, even if there are other means to achieve the same > > functionality. > > I had the impression that sys_cacheflush() did that. Am I wrong? Currently sys_cacheflush() doesn't do this, and IIUC it has never done remote context synchronization even for architectures that need that (e.g. x86 requiring a serializing instruction). > In any event, I’m even more convinced that no new SYNC_CORE arches > should be added. We need a new API that just does the right thing. My intuition is the other way around, and that this is a gnereally useful thing for architectures that require context synchronization. It's not clear to me what "the right thing" would mean specifically, and on architectures with userspace cache maintenance JITs can usually do the most optimal maintenance, and only need help for the context synchronization. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 13:51 ` Mark Rutland @ 2021-06-17 14:00 ` Andy Lutomirski 2021-06-17 14:20 ` Mark Rutland 2021-06-17 15:01 ` Peter Zijlstra 2021-06-17 14:16 ` Mathieu Desnoyers 1 sibling, 2 replies; 45+ messages in thread From: Andy Lutomirski @ 2021-06-17 14:00 UTC (permalink / raw) To: Mark Rutland Cc: Russell King (Oracle), the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra (Intel), linux-arm-kernel On Thu, Jun 17, 2021, at 6:51 AM, Mark Rutland wrote: > On Thu, Jun 17, 2021 at 06:41:41AM -0700, Andy Lutomirski wrote: > > In any event, I’m even more convinced that no new SYNC_CORE arches > > should be added. We need a new API that just does the right thing. > > My intuition is the other way around, and that this is a gnereally > useful thing for architectures that require context synchronization. Except that you can't use it in a generic way. You have to know the specific rules for your arch. > > It's not clear to me what "the right thing" would mean specifically, and > on architectures with userspace cache maintenance JITs can usually do > the most optimal maintenance, and only need help for the context > synchronization. > This I simply don't believe -- I doubt that any sane architecture really works like this. I wrote an email about it to Intel that apparently generated internal discussion but no results. Consider: mmap(some shared library, some previously unmapped address); this does no heavyweight synchronization, at least on x86. There is no "serializing" instruction in the fast path, and it *works* despite anything the SDM may or may not say. We can and, IMO, should develop a sane way for user programs to install instructions into VMAs, for security-conscious software to verify them (by splitting the read and write sides?), and for their consumers to execute them, without knowing any arch details. And I think this can be done with no IPIs except for possible TLB flushing when needed, at least on most architectures. It would require a nontrivial amount of design work, and it would not resemble sys_cacheflush() or SYNC_CORE. --Andy _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 14:00 ` Andy Lutomirski @ 2021-06-17 14:20 ` Mark Rutland 2021-06-17 15:01 ` Peter Zijlstra 1 sibling, 0 replies; 45+ messages in thread From: Mark Rutland @ 2021-06-17 14:20 UTC (permalink / raw) To: Andy Lutomirski Cc: Russell King (Oracle), the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra (Intel), linux-arm-kernel On Thu, Jun 17, 2021 at 07:00:26AM -0700, Andy Lutomirski wrote: > > > On Thu, Jun 17, 2021, at 6:51 AM, Mark Rutland wrote: > > On Thu, Jun 17, 2021 at 06:41:41AM -0700, Andy Lutomirski wrote: > > > > In any event, I’m even more convinced that no new SYNC_CORE arches > > > should be added. We need a new API that just does the right thing. > > > > My intuition is the other way around, and that this is a gnereally > > useful thing for architectures that require context synchronization. > > Except that you can't use it in a generic way. You have to know the > specific rules for your arch. That's generally true for modifying instruction streams though? The man page for cacheflush(2) calls out that it is not portable. I think what's necessary here is some mandatory per-arch documentation? > > It's not clear to me what "the right thing" would mean specifically, and > > on architectures with userspace cache maintenance JITs can usually do > > the most optimal maintenance, and only need help for the context > > synchronization. > > > > This I simply don't believe -- I doubt that any sane architecture > really works like this. I wrote an email about it to Intel that > apparently generated internal discussion but no results. Consider: > > mmap(some shared library, some previously unmapped address); > > this does no heavyweight synchronization, at least on x86. There is > no "serializing" instruction in the fast path, and it *works* despite > anything the SDM may or may not say. Sure, and I called this case out specifically when I said: | * Where we can guarantee that a CPU cannot possibly have an | instruction in-flight (e.g. due to a lack of a mapping to fetch | instructions from), nothing is necessary. This is what we rely on | when faulting in code pages. In these cases, the CPU is liable to | take fault on the missing translation anyway. .. what really matters is whether the CPU had the oppoprtunity to fetch something stale; the context synchronization is necessary to discard that. Bear in mind that in many cases where this could occur in theory, we don't hit in practice because CPUs don't happen to predict/speculate as aggressively as they are permitted to. On arm/arm64 it's obvious that this is a problem because the documentation clearly defines the boundaries of what a CPU is permitted to do, whereas on other architectures docuentation is not necessarily as clear whether this is permited or whether the architecture mandates additional guarantees. > We can and, IMO, should develop a sane way for user programs to > install instructions into VMAs, for security-conscious software to > verify them (by splitting the read and write sides?), and for their > consumers to execute them, without knowing any arch details. And I > think this can be done with no IPIs except for possible TLB flushing > when needed, at least on most architectures. It would require a > nontrivial amount of design work, and it would not resemble > sys_cacheflush() or SYNC_CORE. I'm not opposed to adding new interfaces for stuff like that, but I don't think that membarrier(SYNC_CORE) or cacheflush(2) are necessarily wrong as-is. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 14:00 ` Andy Lutomirski 2021-06-17 14:20 ` Mark Rutland @ 2021-06-17 15:01 ` Peter Zijlstra 2021-06-17 15:13 ` Peter Zijlstra 1 sibling, 1 reply; 45+ messages in thread From: Peter Zijlstra @ 2021-06-17 15:01 UTC (permalink / raw) To: Andy Lutomirski Cc: Mark Rutland, Russell King (Oracle), the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel On Thu, Jun 17, 2021 at 07:00:26AM -0700, Andy Lutomirski wrote: > On Thu, Jun 17, 2021, at 6:51 AM, Mark Rutland wrote: > > It's not clear to me what "the right thing" would mean specifically, and > > on architectures with userspace cache maintenance JITs can usually do > > the most optimal maintenance, and only need help for the context > > synchronization. > > > > This I simply don't believe -- I doubt that any sane architecture > really works like this. I wrote an email about it to Intel that > apparently generated internal discussion but no results. Consider: > > mmap(some shared library, some previously unmapped address); > > this does no heavyweight synchronization, at least on x86. There is > no "serializing" instruction in the fast path, and it *works* despite > anything the SDM may or may not say. I'm confused; why do you think that is relevant? The only way to get into a memory address space is CR3 write, which is serializing and will flush everything. Since there wasn't anything mapped, nothing could be 'cached' from that location. So that has to work... > We can and, IMO, should develop a sane way for user programs to > install instructions into VMAs, for security-conscious software to > verify them (by splitting the read and write sides?), and for their > consumers to execute them, without knowing any arch details. And I > think this can be done with no IPIs except for possible TLB flushing > when needed, at least on most architectures. It would require a > nontrivial amount of design work, and it would not resemble > sys_cacheflush() or SYNC_CORE. The interesting use-case is where we modify code that is under active execution in a multi-threaded process; where CPU0 runs code and doesn't make any syscalls at all, while CPU1 modifies code that is visible to CPU0. In that case CPU0 can have various internal state still reflecting the old instructions that no longer exist in memory -- presumably. We also need to inject at least a full memory barrier and pipeline flush, to create a proper before and after modified. To reason about when the *other* threads will be able to observe the new code. Now, the SDM documents that prefetch and trace buffers are not flushed on i$ invalidate (actual implementations might of course differ) and doing this requires the SERIALIZE instruction or one of the many instructions that implies this, one of which is IRET. For the cross-modifying case, I really don't see how you can not send an IPI and expect behavour one can reason about, irrespective of any non-coherent behaviour. Now, the SDM documents non-coherent behaviour and requires SERIALIZE, while at the same time any IPI already implies IRET which implies SERIALIZE -- except some Luto guy was having plans to optimize the IRET paths so we couldn't rely on that. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 15:01 ` Peter Zijlstra @ 2021-06-17 15:13 ` Peter Zijlstra 0 siblings, 0 replies; 45+ messages in thread From: Peter Zijlstra @ 2021-06-17 15:13 UTC (permalink / raw) To: Andy Lutomirski Cc: Mark Rutland, Russell King (Oracle), the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel On Thu, Jun 17, 2021 at 05:01:53PM +0200, Peter Zijlstra wrote: > On Thu, Jun 17, 2021 at 07:00:26AM -0700, Andy Lutomirski wrote: > > On Thu, Jun 17, 2021, at 6:51 AM, Mark Rutland wrote: > > > > It's not clear to me what "the right thing" would mean specifically, and > > > on architectures with userspace cache maintenance JITs can usually do > > > the most optimal maintenance, and only need help for the context > > > synchronization. > > > > > > > This I simply don't believe -- I doubt that any sane architecture > > really works like this. I wrote an email about it to Intel that > > apparently generated internal discussion but no results. Consider: > > > > mmap(some shared library, some previously unmapped address); > > > > this does no heavyweight synchronization, at least on x86. There is > > no "serializing" instruction in the fast path, and it *works* despite > > anything the SDM may or may not say. > > I'm confused; why do you think that is relevant? > > The only way to get into a memory address space is CR3 write, which is > serializing and will flush everything. Since there wasn't anything > mapped, nothing could be 'cached' from that location. > > So that has to work... Ooh, you mean mmap where there was something mmap'ed before. Not virgin space so to say. But in that case, the unmap() would've caused a TLB invalidate, which on x86 is IPIs, which is IRET. Other architectures include I/D cache flushes in their TLB invalidations -- but as elsewhere in the thread, that might not be suffient on its own. But yes, I think TLBI has to imply flushing micro-arch instruction related buffers for any of that to work. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 13:51 ` Mark Rutland 2021-06-17 14:00 ` Andy Lutomirski @ 2021-06-17 14:16 ` Mathieu Desnoyers 1 sibling, 0 replies; 45+ messages in thread From: Mathieu Desnoyers @ 2021-06-17 14:16 UTC (permalink / raw) To: Mark Rutland Cc: Andy Lutomirski, Russell King (Oracle), the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Nicholas Piggin, Peter Zijlstra (Intel), linux-arm-kernel On 17-Jun-2021 02:51:33 PM, Mark Rutland wrote: > On Thu, Jun 17, 2021 at 06:41:41AM -0700, Andy Lutomirski wrote: > > > > > > On Thu, Jun 17, 2021, at 4:33 AM, Mark Rutland wrote: > > > On Thu, Jun 17, 2021 at 12:23:05PM +0100, Russell King (Oracle) wrote: > > > > On Thu, Jun 17, 2021 at 11:40:46AM +0100, Mark Rutland wrote: > > > > > On Tue, Jun 15, 2021 at 08:21:12PM -0700, Andy Lutomirski wrote: > > > > > > On arm32, the only way to safely flush icache from usermode is to call > > > > > > cacheflush(2). This also handles any required pipeline flushes, so > > > > > > membarrier's SYNC_CORE feature is useless on arm. Remove it. > > > > > > > > > > Unfortunately, it's a bit more complicated than that, and these days > > > > > SYNC_CORE is equally necessary on arm as on arm64. This is something > > > > > that changed in the architecture over time, but since ARMv7 we generally > > > > > need both the cache maintenance *and* a context synchronization event > > > > > (the latter must occur on the CPU which will execute the instructions). > > > > > > > > > > If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section > > > > > A3.5.4 "Concurrent modification and execution of instructions" covers > > > > > this. That manual can be found at: > > > > > > > > > > https://developer.arm.com/documentation/ddi0406/latest/ > > > > > > > > Looking at that, sys_cacheflush() meets this. The manual details a > > > > series of cache maintenance calls in "step 1" that the modifying thread > > > > must issue - this is exactly what sys_cacheflush() does. The same is > > > > true for ARMv6, except the "ISB" terminology is replaced by a > > > > "PrefetchFlush" terminology. (I checked DDI0100I). > > > > > > > > "step 2" requires an ISB on the "other CPU" prior to executing that > > > > code. As I understand it, in ARMv7, userspace can issue an ISB itself. > > > > > > > > For ARMv6K, it doesn't have ISB, but instead has a CP15 instruction > > > > for this that isn't availble to userspace. This is where we come to > > > > the situation about ARM 11MPCore, and whether we continue to support > > > > it or not. > > > > > > > > So, I think we're completely fine with ARMv7 under 32-bit ARM kernels > > > > as userspace has everything that's required. ARMv6K is a different > > > > matter as we've already identified for several reasons. > > > > > > Sure, and I agree we should not change cacheflush(). > > > > > > The point of membarrier(SYNC_CORE) is that you can move the cost of that > > > ISB out of the fast-path in the executing thread(s) and into the > > > slow-path on the thread which generated the code. > > > > > > So e.g. rather than an executing thread always having to do: > > > > > > LDR <reg>, [<funcptr>] > > > ISB // in case funcptr was just updated > > > BLR <reg> > > > > > > ... you have the thread generating the code use membarrier(SYNC_CORE) > > > prior to plublishing the funcptr, and the fast-path on all the executing > > > threads can be: > > > > > > LDR <reg> [<funcptr>] > > > BLR <reg> > > > > > > ... and thus I think we still want membarrier(SYNC_CORE) so that people > > > can do this, even if there are other means to achieve the same > > > functionality. > > > > I had the impression that sys_cacheflush() did that. Am I wrong? > > Currently sys_cacheflush() doesn't do this, and IIUC it has never done > remote context synchronization even for architectures that need that > (e.g. x86 requiring a serializing instruction). > > > In any event, I’m even more convinced that no new SYNC_CORE arches > > should be added. We need a new API that just does the right thing. > > My intuition is the other way around, and that this is a gnereally > useful thing for architectures that require context synchronization. > > It's not clear to me what "the right thing" would mean specifically, and > on architectures with userspace cache maintenance JITs can usually do > the most optimal maintenance, and only need help for the context > synchronization. If I can attempt to summarize the current situation for ARMv7: - In addition to the cache flushing on the core doing the code update, the architecture requires every core to perform a context synchronizing instruction before executing the updated code. - sys_cacheflush() don't do this core sync on every core. It also takes a single address range as parameter. - ARM, ARM64, powerpc, powerpc64, x86, x86-64 all currently handle the context synchronization requirement for updating user-space code on SMP with sys_membarrier SYNC_CORE. It's not, however, meant to replace explicit cache flushing operations if those are needed. So removing membarrier SYNC_CORE from ARM would be a step backward here. On ARMv7, the SYNC_CORE is needed _in addition_ to sys_cacheflush. Adding a sync-core operation at the end of sys_cacheflush would be inefficient for common GC use-cases where a rather large set of address ranges are invalidated in one go: for this, we either want the GC to: - Invoke sys_cacheflush for each targeted range, and then issue a single sys_membarrier SYNC_CORE, or - Implement a new "sys_cacheflush_iov" which takes an iovec input. There I see that it could indeed invalidate all relevant cache lines *and* issue the SYNC_CORE at the end. But shoehorning the SYNC_CORE in the pre-existing sys_cacheflush after the fact seems like a bad idea. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-17 13:41 ` Andy Lutomirski 2021-06-17 13:51 ` Mark Rutland @ 2021-06-17 14:05 ` Peter Zijlstra 1 sibling, 0 replies; 45+ messages in thread From: Peter Zijlstra @ 2021-06-17 14:05 UTC (permalink / raw) To: Andy Lutomirski Cc: Mark Rutland, Russell King (Oracle), the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, linux-arm-kernel On Thu, Jun 17, 2021 at 06:41:41AM -0700, Andy Lutomirski wrote: > On Thu, Jun 17, 2021, at 4:33 AM, Mark Rutland wrote: > > Sure, and I agree we should not change cacheflush(). > > > > The point of membarrier(SYNC_CORE) is that you can move the cost of that > > ISB out of the fast-path in the executing thread(s) and into the > > slow-path on the thread which generated the code. > > > > So e.g. rather than an executing thread always having to do: > > > > LDR <reg>, [<funcptr>] > > ISB // in case funcptr was just updated > > BLR <reg> > > > > ... you have the thread generating the code use membarrier(SYNC_CORE) > > prior to plublishing the funcptr, and the fast-path on all the executing > > threads can be: > > > > LDR <reg> [<funcptr>] > > BLR <reg> > > > > ... and thus I think we still want membarrier(SYNC_CORE) so that people > > can do this, even if there are other means to achieve the same > > functionality. > > I had the impression that sys_cacheflush() did that. Am I wrong? Yes, sys_cacheflush() only does what it says on the tin (and only correctly for hardware broadcast -- everything except 11mpcore). It only invalidates the caches, but not the per CPU derived state like prefetch buffers and micro-op buffers, and certainly not instructions already in flight. So anything OoO needs at the very least a complete pipeline stall injected, but probably something stronger to make it flush the buffers. > In any event, I’m even more convinced that no new SYNC_CORE arches > should be added. We need a new API that just does the right thing. I really don't understand why you hate the thing so much; SYNC_CORE is a means of injecting whatever instruction is required to flush all uarch state related to instructions on all theads (not all CPUs) of a process as efficient as possible. The alternative is sending signals to all threads (including the non-running ones) which is known to scale very poorly indeed, or, as Mark suggests above, have very expensive instructions unconditinoally in the instruction stream, which is also undesired. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE 2021-06-16 3:21 ` [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE Andy Lutomirski ` (2 preceding siblings ...) 2021-06-17 10:40 ` Mark Rutland @ 2021-06-18 0:07 ` Andy Lutomirski 3 siblings, 0 replies; 45+ messages in thread From: Andy Lutomirski @ 2021-06-18 0:07 UTC (permalink / raw) To: x86 Cc: Dave Hansen, LKML, linux-mm, Andrew Morton, Mathieu Desnoyers, Nicholas Piggin, Peter Zijlstra, Russell King, linux-arm-kernel On 6/15/21 8:21 PM, Andy Lutomirski wrote: > On arm32, the only way to safely flush icache from usermode is to call > cacheflush(2). This also handles any required pipeline flushes, so > membarrier's SYNC_CORE feature is useless on arm. Remove it. After all the discussion, I'm dropping this patch. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation [not found] <cover.1623813516.git.luto@kernel.org> 2021-06-16 3:21 ` [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE Andy Lutomirski @ 2021-06-16 3:21 ` Andy Lutomirski 2021-06-16 4:45 ` Nicholas Piggin ` (3 more replies) 1 sibling, 4 replies; 45+ messages in thread From: Andy Lutomirski @ 2021-06-16 3:21 UTC (permalink / raw) To: x86 Cc: Dave Hansen, LKML, linux-mm, Andrew Morton, Andy Lutomirski, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, Will Deacon, linux-arm-kernel, Mathieu Desnoyers, Peter Zijlstra, stable The old sync_core_before_usermode() comments suggested that a non-icache-syncing return-to-usermode instruction is x86-specific and that all other architectures automatically notice cross-modified code on return to userspace. This is misleading. The incantation needed to modify code from one CPU and execute it on another CPU is highly architecture dependent. On x86, according to the SDM, one must modify the code, issue SFENCE if the modification was WC or nontemporal, and then issue a "serializing instruction" on the CPU that will execute the code. membarrier() can do the latter. On arm64 and powerpc, one must flush the icache and then flush the pipeline on the target CPU, although the CPU manuals don't necessarily use this language. So let's drop any pretense that we can have a generic way to define or implement membarrier's SYNC_CORE operation and instead require all architectures to define the helper and supply their own documentation as to how to use it. This means x86, arm64, and powerpc for now. Let's also rename the function from sync_core_before_usermode() to membarrier_sync_core_before_usermode() because the precise flushing details may very well be specific to membarrier, and even the concept of "sync_core" in the kernel is mostly an x86-ism. (It may well be the case that, on real x86 processors, synchronizing the icache (which requires no action at all) and "flushing the pipeline" is sufficient, but trying to use this language would be confusing at best. LFENCE does something awfully like "flushing the pipeline", but the SDM does not permit LFENCE as an alternative to a "serializing instruction" for this purpose.) Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: linuxppc-dev@lists.ozlabs.org Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: linux-arm-kernel@lists.infradead.org Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: x86@kernel.org Cc: stable@vger.kernel.org Fixes: 70216e18e519 ("membarrier: Provide core serializing command, *_SYNC_CORE") Signed-off-by: Andy Lutomirski <luto@kernel.org> --- .../membarrier-sync-core/arch-support.txt | 68 ++++++------------- arch/arm64/include/asm/sync_core.h | 19 ++++++ arch/powerpc/include/asm/sync_core.h | 14 ++++ arch/x86/Kconfig | 1 - arch/x86/include/asm/sync_core.h | 7 +- arch/x86/kernel/alternative.c | 2 +- arch/x86/kernel/cpu/mce/core.c | 2 +- arch/x86/mm/tlb.c | 3 +- drivers/misc/sgi-gru/grufault.c | 2 +- drivers/misc/sgi-gru/gruhandles.c | 2 +- drivers/misc/sgi-gru/grukservices.c | 2 +- include/linux/sched/mm.h | 1 - include/linux/sync_core.h | 21 ------ init/Kconfig | 3 - kernel/sched/membarrier.c | 15 ++-- 15 files changed, 75 insertions(+), 87 deletions(-) create mode 100644 arch/arm64/include/asm/sync_core.h create mode 100644 arch/powerpc/include/asm/sync_core.h delete mode 100644 include/linux/sync_core.h diff --git a/Documentation/features/sched/membarrier-sync-core/arch-support.txt b/Documentation/features/sched/membarrier-sync-core/arch-support.txt index 883d33b265d6..41c9ebcb275f 100644 --- a/Documentation/features/sched/membarrier-sync-core/arch-support.txt +++ b/Documentation/features/sched/membarrier-sync-core/arch-support.txt @@ -5,51 +5,25 @@ # # Architecture requirements # -# * arm/arm64/powerpc # -# Rely on implicit context synchronization as a result of exception return -# when returning from IPI handler, and when returning to user-space. -# -# * x86 -# -# x86-32 uses IRET as return from interrupt, which takes care of the IPI. -# However, it uses both IRET and SYSEXIT to go back to user-space. The IRET -# instruction is core serializing, but not SYSEXIT. -# -# x86-64 uses IRET as return from interrupt, which takes care of the IPI. -# However, it can return to user-space through either SYSRETL (compat code), -# SYSRETQ, or IRET. -# -# Given that neither SYSRET{L,Q}, nor SYSEXIT, are core serializing, we rely -# instead on write_cr3() performed by switch_mm() to provide core serialization -# after changing the current mm, and deal with the special case of kthread -> -# uthread (temporarily keeping current mm into active_mm) by issuing a -# sync_core_before_usermode() in that specific case. -# - ----------------------- - | arch |status| - ----------------------- - | alpha: | TODO | - | arc: | TODO | - | arm: | ok | - | arm64: | ok | - | csky: | TODO | - | h8300: | TODO | - | hexagon: | TODO | - | ia64: | TODO | - | m68k: | TODO | - | microblaze: | TODO | - | mips: | TODO | - | nds32: | TODO | - | nios2: | TODO | - | openrisc: | TODO | - | parisc: | TODO | - | powerpc: | ok | - | riscv: | TODO | - | s390: | TODO | - | sh: | TODO | - | sparc: | TODO | - | um: | TODO | - | x86: | ok | - | xtensa: | TODO | - ----------------------- +# An architecture that wants to support +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE needs to define precisely what it +# is supposed to do and implement membarrier_sync_core_before_usermode() to +# make it do that. Then it can select ARCH_HAS_MEMBARRIER_SYNC_CORE via +# Kconfig.Unfortunately, MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE is not a +# fantastic API and may not make sense on all architectures. Once an +# architecture meets these requirements, +# +# On x86, a program can safely modify code, issue +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, and then execute that code, via +# the modified address or an alias, from any thread in the calling process. +# +# On arm64, a program can modify code, flush the icache as needed, and issue +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE to force a "context synchronizing +# event", aka pipeline flush on all CPUs that might run the calling process. +# Then the program can execute the modified code as long as it is executed +# from an address consistent with the icache flush and the CPU's cache type. +# +# On powerpc, a program can use MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE +# similarly to arm64. It would be nice if the powerpc maintainers could +# add a more clear explanantion. diff --git a/arch/arm64/include/asm/sync_core.h b/arch/arm64/include/asm/sync_core.h new file mode 100644 index 000000000000..74996bf533bb --- /dev/null +++ b/arch/arm64/include/asm/sync_core.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_ARM64_SYNC_CORE_H +#define _ASM_ARM64_SYNC_CORE_H + +#include <asm/barrier.h> + +/* + * On arm64, anyone trying to use membarrier() to handle JIT code is + * required to first flush the icache and then do SYNC_CORE. All that's + * needed after the icache flush is to execute a "context synchronization + * event". Right now, ERET does this, and we are guaranteed to ERET before + * any user code runs. If Linux ever programs the CPU to make ERET stop + * being a context synchronizing event, then this will need to be adjusted. + */ +static inline void membarrier_sync_core_before_usermode(void) +{ +} + +#endif /* _ASM_ARM64_SYNC_CORE_H */ diff --git a/arch/powerpc/include/asm/sync_core.h b/arch/powerpc/include/asm/sync_core.h new file mode 100644 index 000000000000..589fdb34beab --- /dev/null +++ b/arch/powerpc/include/asm/sync_core.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_POWERPC_SYNC_CORE_H +#define _ASM_POWERPC_SYNC_CORE_H + +#include <asm/barrier.h> + +/* + * XXX: can a powerpc person put an appropriate comment here? + */ +static inline void membarrier_sync_core_before_usermode(void) +{ +} + +#endif /* _ASM_POWERPC_SYNC_CORE_H */ diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 0045e1b44190..f010897a1e8a 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -89,7 +89,6 @@ config X86 select ARCH_HAS_SET_DIRECT_MAP select ARCH_HAS_STRICT_KERNEL_RWX select ARCH_HAS_STRICT_MODULE_RWX - select ARCH_HAS_SYNC_CORE_BEFORE_USERMODE select ARCH_HAS_SYSCALL_WRAPPER select ARCH_HAS_UBSAN_SANITIZE_ALL select ARCH_HAS_DEBUG_WX diff --git a/arch/x86/include/asm/sync_core.h b/arch/x86/include/asm/sync_core.h index ab7382f92aff..c665b453969a 100644 --- a/arch/x86/include/asm/sync_core.h +++ b/arch/x86/include/asm/sync_core.h @@ -89,11 +89,10 @@ static inline void sync_core(void) } /* - * Ensure that a core serializing instruction is issued before returning - * to user-mode. x86 implements return to user-space through sysexit, - * sysrel, and sysretq, which are not core serializing. + * Ensure that the CPU notices any instruction changes before the next time + * it returns to usermode. */ -static inline void sync_core_before_usermode(void) +static inline void membarrier_sync_core_before_usermode(void) { /* With PTI, we unconditionally serialize before running user code. */ if (static_cpu_has(X86_FEATURE_PTI)) diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index 6974b5174495..52ead5f4fcdc 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -17,7 +17,7 @@ #include <linux/kprobes.h> #include <linux/mmu_context.h> #include <linux/bsearch.h> -#include <linux/sync_core.h> +#include <asm/sync_core.h> #include <asm/text-patching.h> #include <asm/alternative.h> #include <asm/sections.h> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index bf7fe87a7e88..4a577980d4d1 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -41,12 +41,12 @@ #include <linux/irq_work.h> #include <linux/export.h> #include <linux/set_memory.h> -#include <linux/sync_core.h> #include <linux/task_work.h> #include <linux/hardirq.h> #include <asm/intel-family.h> #include <asm/processor.h> +#include <asm/sync_core.h> #include <asm/traps.h> #include <asm/tlbflush.h> #include <asm/mce.h> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c index 59488d663e68..35b622fd2ed1 100644 --- a/arch/x86/mm/tlb.c +++ b/arch/x86/mm/tlb.c @@ -11,6 +11,7 @@ #include <linux/sched/mm.h> #include <asm/tlbflush.h> +#include <asm/sync_core.h> #include <asm/mmu_context.h> #include <asm/nospec-branch.h> #include <asm/cache.h> @@ -538,7 +539,7 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next, */ if (unlikely(atomic_read(&next->membarrier_state) & MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE)) - sync_core_before_usermode(); + membarrier_sync_core_before_usermode(); #endif return; diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c index 723825524ea0..48fd5b101de1 100644 --- a/drivers/misc/sgi-gru/grufault.c +++ b/drivers/misc/sgi-gru/grufault.c @@ -20,8 +20,8 @@ #include <linux/io.h> #include <linux/uaccess.h> #include <linux/security.h> -#include <linux/sync_core.h> #include <linux/prefetch.h> +#include <asm/sync_core.h> #include "gru.h" #include "grutables.h" #include "grulib.h" diff --git a/drivers/misc/sgi-gru/gruhandles.c b/drivers/misc/sgi-gru/gruhandles.c index 1d75d5e540bc..c8cba1c1b00f 100644 --- a/drivers/misc/sgi-gru/gruhandles.c +++ b/drivers/misc/sgi-gru/gruhandles.c @@ -16,7 +16,7 @@ #define GRU_OPERATION_TIMEOUT (((cycles_t) local_cpu_data->itc_freq)*10) #define CLKS2NSEC(c) ((c) *1000000000 / local_cpu_data->itc_freq) #else -#include <linux/sync_core.h> +#include <asm/sync_core.h> #include <asm/tsc.h> #define GRU_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000) #define CLKS2NSEC(c) ((c) * 1000000 / tsc_khz) diff --git a/drivers/misc/sgi-gru/grukservices.c b/drivers/misc/sgi-gru/grukservices.c index 0ea923fe6371..ce03ff3f7c3a 100644 --- a/drivers/misc/sgi-gru/grukservices.c +++ b/drivers/misc/sgi-gru/grukservices.c @@ -16,10 +16,10 @@ #include <linux/miscdevice.h> #include <linux/proc_fs.h> #include <linux/interrupt.h> -#include <linux/sync_core.h> #include <linux/uaccess.h> #include <linux/delay.h> #include <linux/export.h> +#include <asm/sync_core.h> #include <asm/io_apic.h> #include "gru.h" #include "grulib.h" diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h index c6eebbafadb0..845db11190cd 100644 --- a/include/linux/sched/mm.h +++ b/include/linux/sched/mm.h @@ -7,7 +7,6 @@ #include <linux/sched.h> #include <linux/mm_types.h> #include <linux/gfp.h> -#include <linux/sync_core.h> /* * Routines for handling mm_structs diff --git a/include/linux/sync_core.h b/include/linux/sync_core.h deleted file mode 100644 index 013da4b8b327..000000000000 --- a/include/linux/sync_core.h +++ /dev/null @@ -1,21 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -#ifndef _LINUX_SYNC_CORE_H -#define _LINUX_SYNC_CORE_H - -#ifdef CONFIG_ARCH_HAS_SYNC_CORE_BEFORE_USERMODE -#include <asm/sync_core.h> -#else -/* - * This is a dummy sync_core_before_usermode() implementation that can be used - * on all architectures which return to user-space through core serializing - * instructions. - * If your architecture returns to user-space through non-core-serializing - * instructions, you need to write your own functions. - */ -static inline void sync_core_before_usermode(void) -{ -} -#endif - -#endif /* _LINUX_SYNC_CORE_H */ - diff --git a/init/Kconfig b/init/Kconfig index 1ea12c64e4c9..e5d552b0823e 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -2377,9 +2377,6 @@ source "kernel/Kconfig.locks" config ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE bool -config ARCH_HAS_SYNC_CORE_BEFORE_USERMODE - bool - # It may be useful for an architecture to override the definitions of the # SYSCALL_DEFINE() and __SYSCALL_DEFINEx() macros in <linux/syscalls.h> # and the COMPAT_ variants in <linux/compat.h>, in particular to use a diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c index c32c32a2441e..f72a6ab3fac2 100644 --- a/kernel/sched/membarrier.c +++ b/kernel/sched/membarrier.c @@ -5,6 +5,9 @@ * membarrier system call */ #include "sched.h" +#ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE +#include <asm/sync_core.h> +#endif /* * The basic principle behind the regular memory barrier mode of membarrier() @@ -221,6 +224,7 @@ static void ipi_mb(void *info) smp_mb(); /* IPIs should be serializing but paranoid. */ } +#ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE static void ipi_sync_core(void *info) { /* @@ -230,13 +234,14 @@ static void ipi_sync_core(void *info) * the big comment at the top of this file. * * A sync_core() would provide this guarantee, but - * sync_core_before_usermode() might end up being deferred until - * after membarrier()'s smp_mb(). + * membarrier_sync_core_before_usermode() might end up being deferred + * until after membarrier()'s smp_mb(). */ smp_mb(); /* IPIs should be serializing but paranoid. */ - sync_core_before_usermode(); + membarrier_sync_core_before_usermode(); } +#endif static void ipi_rseq(void *info) { @@ -368,12 +373,14 @@ static int membarrier_private_expedited(int flags, int cpu_id) smp_call_func_t ipi_func = ipi_mb; if (flags == MEMBARRIER_FLAG_SYNC_CORE) { - if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) +#ifndef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE return -EINVAL; +#else if (!(atomic_read(&mm->membarrier_state) & MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) return -EPERM; ipi_func = ipi_sync_core; +#endif } else if (flags == MEMBARRIER_FLAG_RSEQ) { if (!IS_ENABLED(CONFIG_RSEQ)) return -EINVAL; -- 2.31.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-16 3:21 ` [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation Andy Lutomirski @ 2021-06-16 4:45 ` Nicholas Piggin 2021-06-16 18:52 ` Andy Lutomirski 2021-06-16 10:20 ` Will Deacon ` (2 subsequent siblings) 3 siblings, 1 reply; 45+ messages in thread From: Nicholas Piggin @ 2021-06-16 4:45 UTC (permalink / raw) To: Andy Lutomirski, x86 Cc: Andrew Morton, Benjamin Herrenschmidt, Catalin Marinas, Dave Hansen, linux-arm-kernel, LKML, linux-mm, linuxppc-dev, Mathieu Desnoyers, Michael Ellerman, Paul Mackerras, Peter Zijlstra, stable, Will Deacon Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm: > The old sync_core_before_usermode() comments suggested that a non-icache-syncing > return-to-usermode instruction is x86-specific and that all other > architectures automatically notice cross-modified code on return to > userspace. > > This is misleading. The incantation needed to modify code from one > CPU and execute it on another CPU is highly architecture dependent. > On x86, according to the SDM, one must modify the code, issue SFENCE > if the modification was WC or nontemporal, and then issue a "serializing > instruction" on the CPU that will execute the code. membarrier() can do > the latter. > > On arm64 and powerpc, one must flush the icache and then flush the pipeline > on the target CPU, although the CPU manuals don't necessarily use this > language. > > So let's drop any pretense that we can have a generic way to define or > implement membarrier's SYNC_CORE operation and instead require all > architectures to define the helper and supply their own documentation as to > how to use it. This means x86, arm64, and powerpc for now. Let's also > rename the function from sync_core_before_usermode() to > membarrier_sync_core_before_usermode() because the precise flushing details > may very well be specific to membarrier, and even the concept of > "sync_core" in the kernel is mostly an x86-ism. > > (It may well be the case that, on real x86 processors, synchronizing the > icache (which requires no action at all) and "flushing the pipeline" is > sufficient, but trying to use this language would be confusing at best. > LFENCE does something awfully like "flushing the pipeline", but the SDM > does not permit LFENCE as an alternative to a "serializing instruction" > for this purpose.) > > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: linuxppc-dev@lists.ozlabs.org > Cc: Nicholas Piggin <npiggin@gmail.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: Nicholas Piggin <npiggin@gmail.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: x86@kernel.org > Cc: stable@vger.kernel.org > Fixes: 70216e18e519 ("membarrier: Provide core serializing command, *_SYNC_CORE") > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > .../membarrier-sync-core/arch-support.txt | 68 ++++++------------- > arch/arm64/include/asm/sync_core.h | 19 ++++++ > arch/powerpc/include/asm/sync_core.h | 14 ++++ > arch/x86/Kconfig | 1 - > arch/x86/include/asm/sync_core.h | 7 +- > arch/x86/kernel/alternative.c | 2 +- > arch/x86/kernel/cpu/mce/core.c | 2 +- > arch/x86/mm/tlb.c | 3 +- > drivers/misc/sgi-gru/grufault.c | 2 +- > drivers/misc/sgi-gru/gruhandles.c | 2 +- > drivers/misc/sgi-gru/grukservices.c | 2 +- > include/linux/sched/mm.h | 1 - > include/linux/sync_core.h | 21 ------ > init/Kconfig | 3 - > kernel/sched/membarrier.c | 15 ++-- > 15 files changed, 75 insertions(+), 87 deletions(-) > create mode 100644 arch/arm64/include/asm/sync_core.h > create mode 100644 arch/powerpc/include/asm/sync_core.h > delete mode 100644 include/linux/sync_core.h > > diff --git a/Documentation/features/sched/membarrier-sync-core/arch-support.txt b/Documentation/features/sched/membarrier-sync-core/arch-support.txt > index 883d33b265d6..41c9ebcb275f 100644 > --- a/Documentation/features/sched/membarrier-sync-core/arch-support.txt > +++ b/Documentation/features/sched/membarrier-sync-core/arch-support.txt > @@ -5,51 +5,25 @@ > # > # Architecture requirements > # > -# * arm/arm64/powerpc > # > -# Rely on implicit context synchronization as a result of exception return > -# when returning from IPI handler, and when returning to user-space. > -# > -# * x86 > -# > -# x86-32 uses IRET as return from interrupt, which takes care of the IPI. > -# However, it uses both IRET and SYSEXIT to go back to user-space. The IRET > -# instruction is core serializing, but not SYSEXIT. > -# > -# x86-64 uses IRET as return from interrupt, which takes care of the IPI. > -# However, it can return to user-space through either SYSRETL (compat code), > -# SYSRETQ, or IRET. > -# > -# Given that neither SYSRET{L,Q}, nor SYSEXIT, are core serializing, we rely > -# instead on write_cr3() performed by switch_mm() to provide core serialization > -# after changing the current mm, and deal with the special case of kthread -> > -# uthread (temporarily keeping current mm into active_mm) by issuing a > -# sync_core_before_usermode() in that specific case. > -# > - ----------------------- > - | arch |status| > - ----------------------- > - | alpha: | TODO | > - | arc: | TODO | > - | arm: | ok | > - | arm64: | ok | > - | csky: | TODO | > - | h8300: | TODO | > - | hexagon: | TODO | > - | ia64: | TODO | > - | m68k: | TODO | > - | microblaze: | TODO | > - | mips: | TODO | > - | nds32: | TODO | > - | nios2: | TODO | > - | openrisc: | TODO | > - | parisc: | TODO | > - | powerpc: | ok | > - | riscv: | TODO | > - | s390: | TODO | > - | sh: | TODO | > - | sparc: | TODO | > - | um: | TODO | > - | x86: | ok | > - | xtensa: | TODO | > - ----------------------- > +# An architecture that wants to support > +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE needs to define precisely what it > +# is supposed to do and implement membarrier_sync_core_before_usermode() to > +# make it do that. Then it can select ARCH_HAS_MEMBARRIER_SYNC_CORE via > +# Kconfig.Unfortunately, MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE is not a > +# fantastic API and may not make sense on all architectures. Once an > +# architecture meets these requirements, > +# > +# On x86, a program can safely modify code, issue > +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, and then execute that code, via > +# the modified address or an alias, from any thread in the calling process. > +# > +# On arm64, a program can modify code, flush the icache as needed, and issue > +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE to force a "context synchronizing > +# event", aka pipeline flush on all CPUs that might run the calling process. > +# Then the program can execute the modified code as long as it is executed > +# from an address consistent with the icache flush and the CPU's cache type. > +# > +# On powerpc, a program can use MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE > +# similarly to arm64. It would be nice if the powerpc maintainers could > +# add a more clear explanantion. > diff --git a/arch/arm64/include/asm/sync_core.h b/arch/arm64/include/asm/sync_core.h > new file mode 100644 > index 000000000000..74996bf533bb > --- /dev/null > +++ b/arch/arm64/include/asm/sync_core.h > @@ -0,0 +1,19 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_ARM64_SYNC_CORE_H > +#define _ASM_ARM64_SYNC_CORE_H > + > +#include <asm/barrier.h> > + > +/* > + * On arm64, anyone trying to use membarrier() to handle JIT code is > + * required to first flush the icache and then do SYNC_CORE. All that's > + * needed after the icache flush is to execute a "context synchronization > + * event". Right now, ERET does this, and we are guaranteed to ERET before > + * any user code runs. If Linux ever programs the CPU to make ERET stop > + * being a context synchronizing event, then this will need to be adjusted. > + */ > +static inline void membarrier_sync_core_before_usermode(void) > +{ > +} > + > +#endif /* _ASM_ARM64_SYNC_CORE_H */ > diff --git a/arch/powerpc/include/asm/sync_core.h b/arch/powerpc/include/asm/sync_core.h > new file mode 100644 > index 000000000000..589fdb34beab > --- /dev/null > +++ b/arch/powerpc/include/asm/sync_core.h > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_POWERPC_SYNC_CORE_H > +#define _ASM_POWERPC_SYNC_CORE_H > + > +#include <asm/barrier.h> > + > +/* > + * XXX: can a powerpc person put an appropriate comment here? > + */ > +static inline void membarrier_sync_core_before_usermode(void) > +{ > +} > + > +#endif /* _ASM_POWERPC_SYNC_CORE_H */ powerpc's can just go in asm/membarrier.h /* * The RFI family of instructions are context synchronising, and * that is how we return to userspace, so nothing is required here. */ > diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c > index c32c32a2441e..f72a6ab3fac2 100644 > --- a/kernel/sched/membarrier.c > +++ b/kernel/sched/membarrier.c > @@ -5,6 +5,9 @@ > * membarrier system call > */ > #include "sched.h" > +#ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE > +#include <asm/sync_core.h> > +#endif Can you #else static inline void membarrier_sync_core_before_usermode(void) { /* this gets constant folded out */ } #endif And avoid adding the ifdefs in the following code? Otherwise I think this is good. Acked-by: Nicholas Piggin <npiggin@gmail.com> Thanks, Nick > > /* > * The basic principle behind the regular memory barrier mode of membarrier() > @@ -221,6 +224,7 @@ static void ipi_mb(void *info) > smp_mb(); /* IPIs should be serializing but paranoid. */ > } > > +#ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE > static void ipi_sync_core(void *info) > { > /* > @@ -230,13 +234,14 @@ static void ipi_sync_core(void *info) > * the big comment at the top of this file. > * > * A sync_core() would provide this guarantee, but > - * sync_core_before_usermode() might end up being deferred until > - * after membarrier()'s smp_mb(). > + * membarrier_sync_core_before_usermode() might end up being deferred > + * until after membarrier()'s smp_mb(). > */ > smp_mb(); /* IPIs should be serializing but paranoid. */ > > - sync_core_before_usermode(); > + membarrier_sync_core_before_usermode(); > } > +#endif > > static void ipi_rseq(void *info) > { > @@ -368,12 +373,14 @@ static int membarrier_private_expedited(int flags, int cpu_id) > smp_call_func_t ipi_func = ipi_mb; > > if (flags == MEMBARRIER_FLAG_SYNC_CORE) { > - if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) > +#ifndef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE > return -EINVAL; > +#else > if (!(atomic_read(&mm->membarrier_state) & > MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) > return -EPERM; > ipi_func = ipi_sync_core; > +#endif > } else if (flags == MEMBARRIER_FLAG_RSEQ) { > if (!IS_ENABLED(CONFIG_RSEQ)) > return -EINVAL; > -- > 2.31.1 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-16 4:45 ` Nicholas Piggin @ 2021-06-16 18:52 ` Andy Lutomirski 2021-06-16 23:48 ` Andy Lutomirski 2021-06-18 15:27 ` Christophe Leroy 0 siblings, 2 replies; 45+ messages in thread From: Andy Lutomirski @ 2021-06-16 18:52 UTC (permalink / raw) To: Nicholas Piggin, x86 Cc: Andrew Morton, Benjamin Herrenschmidt, Catalin Marinas, Dave Hansen, linux-arm-kernel, LKML, linux-mm, linuxppc-dev, Mathieu Desnoyers, Michael Ellerman, Paul Mackerras, Peter Zijlstra, stable, Will Deacon On 6/15/21 9:45 PM, Nicholas Piggin wrote: > Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm: >> The old sync_core_before_usermode() comments suggested that a non-icache-syncing >> return-to-usermode instruction is x86-specific and that all other >> architectures automatically notice cross-modified code on return to >> userspace. >> +/* >> + * XXX: can a powerpc person put an appropriate comment here? >> + */ >> +static inline void membarrier_sync_core_before_usermode(void) >> +{ >> +} >> + >> +#endif /* _ASM_POWERPC_SYNC_CORE_H */ > > powerpc's can just go in asm/membarrier.h $ ls arch/powerpc/include/asm/membarrier.h ls: cannot access 'arch/powerpc/include/asm/membarrier.h': No such file or directory > > /* > * The RFI family of instructions are context synchronising, and > * that is how we return to userspace, so nothing is required here. > */ Thanks! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-16 18:52 ` Andy Lutomirski @ 2021-06-16 23:48 ` Andy Lutomirski 2021-06-18 15:27 ` Christophe Leroy 1 sibling, 0 replies; 45+ messages in thread From: Andy Lutomirski @ 2021-06-16 23:48 UTC (permalink / raw) To: Nicholas Piggin, the arch/x86 maintainers Cc: Andrew Morton, Benjamin Herrenschmidt, Catalin Marinas, Dave Hansen, linux-arm-kernel, Linux Kernel Mailing List, linux-mm, linuxppc-dev, Mathieu Desnoyers, Michael Ellerman, Paul Mackerras, Peter Zijlstra (Intel), stable, Will Deacon On Wed, Jun 16, 2021, at 11:52 AM, Andy Lutomirski wrote: > On 6/15/21 9:45 PM, Nicholas Piggin wrote: > > Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm: > >> The old sync_core_before_usermode() comments suggested that a non-icache-syncing > >> return-to-usermode instruction is x86-specific and that all other > >> architectures automatically notice cross-modified code on return to > >> userspace. > > >> +/* > >> + * XXX: can a powerpc person put an appropriate comment here? > >> + */ > >> +static inline void membarrier_sync_core_before_usermode(void) > >> +{ > >> +} > >> + > >> +#endif /* _ASM_POWERPC_SYNC_CORE_H */ > > > > powerpc's can just go in asm/membarrier.h > > $ ls arch/powerpc/include/asm/membarrier.h > ls: cannot access 'arch/powerpc/include/asm/membarrier.h': No such file > or directory Which is because I deleted it. Duh. I'll clean this up. > > > > > > /* > > * The RFI family of instructions are context synchronising, and > > * that is how we return to userspace, so nothing is required here. > > */ > > Thanks! > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-16 18:52 ` Andy Lutomirski 2021-06-16 23:48 ` Andy Lutomirski @ 2021-06-18 15:27 ` Christophe Leroy 1 sibling, 0 replies; 45+ messages in thread From: Christophe Leroy @ 2021-06-18 15:27 UTC (permalink / raw) To: Andy Lutomirski, Nicholas Piggin, x86 Cc: Will Deacon, linux-mm, Peter Zijlstra, LKML, stable, Dave Hansen, Mathieu Desnoyers, Catalin Marinas, Paul Mackerras, Andrew Morton, linuxppc-dev, linux-arm-kernel Le 16/06/2021 à 20:52, Andy Lutomirski a écrit : > On 6/15/21 9:45 PM, Nicholas Piggin wrote: >> Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm: >>> The old sync_core_before_usermode() comments suggested that a non-icache-syncing >>> return-to-usermode instruction is x86-specific and that all other >>> architectures automatically notice cross-modified code on return to >>> userspace. > >>> +/* >>> + * XXX: can a powerpc person put an appropriate comment here? >>> + */ >>> +static inline void membarrier_sync_core_before_usermode(void) >>> +{ >>> +} >>> + >>> +#endif /* _ASM_POWERPC_SYNC_CORE_H */ >> >> powerpc's can just go in asm/membarrier.h > > $ ls arch/powerpc/include/asm/membarrier.h > ls: cannot access 'arch/powerpc/include/asm/membarrier.h': No such file > or directory https://github.com/torvalds/linux/blob/master/arch/powerpc/include/asm/membarrier.h Was added by https://github.com/torvalds/linux/commit/3ccfebedd8cf54e291c809c838d8ad5cc00f5688 > > >> >> /* >> * The RFI family of instructions are context synchronising, and >> * that is how we return to userspace, so nothing is required here. >> */ > > Thanks! > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-16 3:21 ` [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation Andy Lutomirski 2021-06-16 4:45 ` Nicholas Piggin @ 2021-06-16 10:20 ` Will Deacon 2021-06-16 23:58 ` Andy Lutomirski 2021-06-17 14:47 ` Mathieu Desnoyers 2021-06-17 15:16 ` Mathieu Desnoyers 3 siblings, 1 reply; 45+ messages in thread From: Will Deacon @ 2021-06-16 10:20 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, Dave Hansen, LKML, linux-mm, Andrew Morton, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, linux-arm-kernel, Mathieu Desnoyers, Peter Zijlstra, stable On Tue, Jun 15, 2021 at 08:21:13PM -0700, Andy Lutomirski wrote: > The old sync_core_before_usermode() comments suggested that a non-icache-syncing > return-to-usermode instruction is x86-specific and that all other > architectures automatically notice cross-modified code on return to > userspace. > > This is misleading. The incantation needed to modify code from one > CPU and execute it on another CPU is highly architecture dependent. > On x86, according to the SDM, one must modify the code, issue SFENCE > if the modification was WC or nontemporal, and then issue a "serializing > instruction" on the CPU that will execute the code. membarrier() can do > the latter. > > On arm64 and powerpc, one must flush the icache and then flush the pipeline > on the target CPU, although the CPU manuals don't necessarily use this > language. > > So let's drop any pretense that we can have a generic way to define or > implement membarrier's SYNC_CORE operation and instead require all > architectures to define the helper and supply their own documentation as to > how to use it. This means x86, arm64, and powerpc for now. Let's also > rename the function from sync_core_before_usermode() to > membarrier_sync_core_before_usermode() because the precise flushing details > may very well be specific to membarrier, and even the concept of > "sync_core" in the kernel is mostly an x86-ism. > > (It may well be the case that, on real x86 processors, synchronizing the > icache (which requires no action at all) and "flushing the pipeline" is > sufficient, but trying to use this language would be confusing at best. > LFENCE does something awfully like "flushing the pipeline", but the SDM > does not permit LFENCE as an alternative to a "serializing instruction" > for this purpose.) > > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: linuxppc-dev@lists.ozlabs.org > Cc: Nicholas Piggin <npiggin@gmail.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: Nicholas Piggin <npiggin@gmail.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: x86@kernel.org > Cc: stable@vger.kernel.org > Fixes: 70216e18e519 ("membarrier: Provide core serializing command, *_SYNC_CORE") > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > .../membarrier-sync-core/arch-support.txt | 68 ++++++------------- > arch/arm64/include/asm/sync_core.h | 19 ++++++ > arch/powerpc/include/asm/sync_core.h | 14 ++++ > arch/x86/Kconfig | 1 - > arch/x86/include/asm/sync_core.h | 7 +- > arch/x86/kernel/alternative.c | 2 +- > arch/x86/kernel/cpu/mce/core.c | 2 +- > arch/x86/mm/tlb.c | 3 +- > drivers/misc/sgi-gru/grufault.c | 2 +- > drivers/misc/sgi-gru/gruhandles.c | 2 +- > drivers/misc/sgi-gru/grukservices.c | 2 +- > include/linux/sched/mm.h | 1 - > include/linux/sync_core.h | 21 ------ > init/Kconfig | 3 - > kernel/sched/membarrier.c | 15 ++-- > 15 files changed, 75 insertions(+), 87 deletions(-) > create mode 100644 arch/arm64/include/asm/sync_core.h > create mode 100644 arch/powerpc/include/asm/sync_core.h > delete mode 100644 include/linux/sync_core.h For the arm64 bits (docs and asm/sync_core.h): Acked-by: Will Deacon <will@kernel.org> Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-16 10:20 ` Will Deacon @ 2021-06-16 23:58 ` Andy Lutomirski 0 siblings, 0 replies; 45+ messages in thread From: Andy Lutomirski @ 2021-06-16 23:58 UTC (permalink / raw) To: Will Deacon Cc: the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, linux-arm-kernel, Mathieu Desnoyers, Peter Zijlstra (Intel), stable On Wed, Jun 16, 2021, at 3:20 AM, Will Deacon wrote: > > For the arm64 bits (docs and asm/sync_core.h): > > Acked-by: Will Deacon <will@kernel.org> > Thanks. Per Nick's suggestion, I renamed the header to membarrier.h. Unless I hear otherwise, I'll keep the ack. > Will > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-16 3:21 ` [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation Andy Lutomirski 2021-06-16 4:45 ` Nicholas Piggin 2021-06-16 10:20 ` Will Deacon @ 2021-06-17 14:47 ` Mathieu Desnoyers 2021-06-18 0:12 ` Andy Lutomirski 2021-06-17 15:16 ` Mathieu Desnoyers 3 siblings, 1 reply; 45+ messages in thread From: Mathieu Desnoyers @ 2021-06-17 14:47 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, Dave Hansen, linux-kernel, linux-mm, Andrew Morton, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, Will Deacon, linux-arm-kernel, Peter Zijlstra, stable ----- On Jun 15, 2021, at 11:21 PM, Andy Lutomirski luto@kernel.org wrote: > The old sync_core_before_usermode() comments suggested that a non-icache-syncing > return-to-usermode instruction is x86-specific and that all other > architectures automatically notice cross-modified code on return to > userspace. > > This is misleading. The incantation needed to modify code from one > CPU and execute it on another CPU is highly architecture dependent. > On x86, according to the SDM, one must modify the code, issue SFENCE > if the modification was WC or nontemporal, and then issue a "serializing > instruction" on the CPU that will execute the code. membarrier() can do > the latter. > > On arm64 and powerpc, one must flush the icache and then flush the pipeline > on the target CPU, although the CPU manuals don't necessarily use this > language. > > So let's drop any pretense that we can have a generic way to define or > implement membarrier's SYNC_CORE operation and instead require all > architectures to define the helper and supply their own documentation as to > how to use it. Agreed. Documentation of the sequence of operations that need to be performed when cross-modifying code on SMP should be per-architecture. The documentation of the architectural effects of membarrier sync-core should be per-arch as well. > This means x86, arm64, and powerpc for now. And also arm32, as discussed in the other leg of the patchset's email thread. > Let's also > rename the function from sync_core_before_usermode() to > membarrier_sync_core_before_usermode() because the precise flushing details > may very well be specific to membarrier, and even the concept of > "sync_core" in the kernel is mostly an x86-ism. OK > [...] > > static void ipi_rseq(void *info) > { > @@ -368,12 +373,14 @@ static int membarrier_private_expedited(int flags, int > cpu_id) > smp_call_func_t ipi_func = ipi_mb; > > if (flags == MEMBARRIER_FLAG_SYNC_CORE) { > - if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) > +#ifndef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE > return -EINVAL; > +#else > if (!(atomic_read(&mm->membarrier_state) & > MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) > return -EPERM; > ipi_func = ipi_sync_core; > +#endif Please change back this #ifndef / #else / #endif within function for if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { ... } else { ... } I don't think mixing up preprocessor and code logic makes it more readable. Thanks, Mathieu > } else if (flags == MEMBARRIER_FLAG_RSEQ) { > if (!IS_ENABLED(CONFIG_RSEQ)) > return -EINVAL; > -- > 2.31.1 -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-17 14:47 ` Mathieu Desnoyers @ 2021-06-18 0:12 ` Andy Lutomirski 2021-06-18 16:31 ` Mathieu Desnoyers 0 siblings, 1 reply; 45+ messages in thread From: Andy Lutomirski @ 2021-06-18 0:12 UTC (permalink / raw) To: Mathieu Desnoyers Cc: x86, Dave Hansen, linux-kernel, linux-mm, Andrew Morton, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, Will Deacon, linux-arm-kernel, Peter Zijlstra, stable On 6/17/21 7:47 AM, Mathieu Desnoyers wrote: > Please change back this #ifndef / #else / #endif within function for > > if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { > ... > } else { > ... > } > > I don't think mixing up preprocessor and code logic makes it more readable. I agree, but I don't know how to make the result work well. membarrier_sync_core_before_usermode() isn't defined in the !IS_ENABLED case, so either I need to fake up a definition or use #ifdef. If I faked up a definition, I would want to assert, at build time, that it isn't called. I don't think we can do: static void membarrier_sync_core_before_usermode() { BUILD_BUG_IF_REACHABLE(); } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-18 0:12 ` Andy Lutomirski @ 2021-06-18 16:31 ` Mathieu Desnoyers 2021-06-18 19:58 ` Andy Lutomirski 0 siblings, 1 reply; 45+ messages in thread From: Mathieu Desnoyers @ 2021-06-18 16:31 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, Dave Hansen, linux-kernel, linux-mm, Andrew Morton, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, Will Deacon, linux-arm-kernel, Peter Zijlstra, stable ----- On Jun 17, 2021, at 8:12 PM, Andy Lutomirski luto@kernel.org wrote: > On 6/17/21 7:47 AM, Mathieu Desnoyers wrote: > >> Please change back this #ifndef / #else / #endif within function for >> >> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { >> ... >> } else { >> ... >> } >> >> I don't think mixing up preprocessor and code logic makes it more readable. > > I agree, but I don't know how to make the result work well. > membarrier_sync_core_before_usermode() isn't defined in the !IS_ENABLED > case, so either I need to fake up a definition or use #ifdef. > > If I faked up a definition, I would want to assert, at build time, that > it isn't called. I don't think we can do: > > static void membarrier_sync_core_before_usermode() > { > BUILD_BUG_IF_REACHABLE(); > } Let's look at the context here: static void ipi_sync_core(void *info) { [....] membarrier_sync_core_before_usermode() } ^ this can be within #ifdef / #endif static int membarrier_private_expedited(int flags, int cpu_id) [...] if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) return -EINVAL; if (!(atomic_read(&mm->membarrier_state) & MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) return -EPERM; ipi_func = ipi_sync_core; All we need to make the line above work is to define an empty ipi_sync_core function in the #else case after the ipi_sync_core() function definition. Or am I missing your point ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-18 16:31 ` Mathieu Desnoyers @ 2021-06-18 19:58 ` Andy Lutomirski 2021-06-18 20:09 ` Mathieu Desnoyers 0 siblings, 1 reply; 45+ messages in thread From: Andy Lutomirski @ 2021-06-18 19:58 UTC (permalink / raw) To: Mathieu Desnoyers Cc: the arch/x86 maintainers, Dave Hansen, Linux Kernel Mailing List, linux-mm, Andrew Morton, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, Will Deacon, linux-arm-kernel, Peter Zijlstra (Intel), stable On Fri, Jun 18, 2021, at 9:31 AM, Mathieu Desnoyers wrote: > ----- On Jun 17, 2021, at 8:12 PM, Andy Lutomirski luto@kernel.org wrote: > > > On 6/17/21 7:47 AM, Mathieu Desnoyers wrote: > > > >> Please change back this #ifndef / #else / #endif within function for > >> > >> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { > >> ... > >> } else { > >> ... > >> } > >> > >> I don't think mixing up preprocessor and code logic makes it more readable. > > > > I agree, but I don't know how to make the result work well. > > membarrier_sync_core_before_usermode() isn't defined in the !IS_ENABLED > > case, so either I need to fake up a definition or use #ifdef. > > > > If I faked up a definition, I would want to assert, at build time, that > > it isn't called. I don't think we can do: > > > > static void membarrier_sync_core_before_usermode() > > { > > BUILD_BUG_IF_REACHABLE(); > > } > > Let's look at the context here: > > static void ipi_sync_core(void *info) > { > [....] > membarrier_sync_core_before_usermode() > } > > ^ this can be within #ifdef / #endif > > static int membarrier_private_expedited(int flags, int cpu_id) > [...] > if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) > return -EINVAL; > if (!(atomic_read(&mm->membarrier_state) & > MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) > return -EPERM; > ipi_func = ipi_sync_core; > > All we need to make the line above work is to define an empty ipi_sync_core > function in the #else case after the ipi_sync_core() function definition. > > Or am I missing your point ? Maybe? My objection is that an empty ipi_sync_core is a lie — it doesn’t sync the core. I would be fine with that if I could have the compiler statically verify that it’s not called, but I’m uncomfortable having it there if the implementation is actively incorrect. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-18 19:58 ` Andy Lutomirski @ 2021-06-18 20:09 ` Mathieu Desnoyers 2021-06-19 6:02 ` Nicholas Piggin 0 siblings, 1 reply; 45+ messages in thread From: Mathieu Desnoyers @ 2021-06-18 20:09 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, Dave Hansen, linux-kernel, linux-mm, Andrew Morton, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, Will Deacon, linux-arm-kernel, Peter Zijlstra, stable ----- On Jun 18, 2021, at 3:58 PM, Andy Lutomirski luto@kernel.org wrote: > On Fri, Jun 18, 2021, at 9:31 AM, Mathieu Desnoyers wrote: >> ----- On Jun 17, 2021, at 8:12 PM, Andy Lutomirski luto@kernel.org wrote: >> >> > On 6/17/21 7:47 AM, Mathieu Desnoyers wrote: >> > >> >> Please change back this #ifndef / #else / #endif within function for >> >> >> >> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { >> >> ... >> >> } else { >> >> ... >> >> } >> >> >> >> I don't think mixing up preprocessor and code logic makes it more readable. >> > >> > I agree, but I don't know how to make the result work well. >> > membarrier_sync_core_before_usermode() isn't defined in the !IS_ENABLED >> > case, so either I need to fake up a definition or use #ifdef. >> > >> > If I faked up a definition, I would want to assert, at build time, that >> > it isn't called. I don't think we can do: >> > >> > static void membarrier_sync_core_before_usermode() >> > { >> > BUILD_BUG_IF_REACHABLE(); >> > } >> >> Let's look at the context here: >> >> static void ipi_sync_core(void *info) >> { >> [....] >> membarrier_sync_core_before_usermode() >> } >> >> ^ this can be within #ifdef / #endif >> >> static int membarrier_private_expedited(int flags, int cpu_id) >> [...] >> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) >> return -EINVAL; >> if (!(atomic_read(&mm->membarrier_state) & >> MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) >> return -EPERM; >> ipi_func = ipi_sync_core; >> >> All we need to make the line above work is to define an empty ipi_sync_core >> function in the #else case after the ipi_sync_core() function definition. >> >> Or am I missing your point ? > > Maybe? > > My objection is that an empty ipi_sync_core is a lie — it doesn’t sync the core. > I would be fine with that if I could have the compiler statically verify that > it’s not called, but I’m uncomfortable having it there if the implementation is > actively incorrect. I see. Another approach would be to implement a "setter" function to populate "ipi_func". That setter function would return -EINVAL in its #ifndef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE implementation. Would that be better ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-18 20:09 ` Mathieu Desnoyers @ 2021-06-19 6:02 ` Nicholas Piggin 2021-06-19 15:50 ` Andy Lutomirski 0 siblings, 1 reply; 45+ messages in thread From: Nicholas Piggin @ 2021-06-19 6:02 UTC (permalink / raw) To: Andy Lutomirski, Mathieu Desnoyers Cc: Andrew Morton, Benjamin Herrenschmidt, Catalin Marinas, Dave Hansen, linux-arm-kernel, linux-kernel, linux-mm, linuxppc-dev, Michael Ellerman, Paul Mackerras, Peter Zijlstra, stable, Will Deacon, x86 Excerpts from Mathieu Desnoyers's message of June 19, 2021 6:09 am: > ----- On Jun 18, 2021, at 3:58 PM, Andy Lutomirski luto@kernel.org wrote: > >> On Fri, Jun 18, 2021, at 9:31 AM, Mathieu Desnoyers wrote: >>> ----- On Jun 17, 2021, at 8:12 PM, Andy Lutomirski luto@kernel.org wrote: >>> >>> > On 6/17/21 7:47 AM, Mathieu Desnoyers wrote: >>> > >>> >> Please change back this #ifndef / #else / #endif within function for >>> >> >>> >> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { >>> >> ... >>> >> } else { >>> >> ... >>> >> } >>> >> >>> >> I don't think mixing up preprocessor and code logic makes it more readable. >>> > >>> > I agree, but I don't know how to make the result work well. >>> > membarrier_sync_core_before_usermode() isn't defined in the !IS_ENABLED >>> > case, so either I need to fake up a definition or use #ifdef. >>> > >>> > If I faked up a definition, I would want to assert, at build time, that >>> > it isn't called. I don't think we can do: >>> > >>> > static void membarrier_sync_core_before_usermode() >>> > { >>> > BUILD_BUG_IF_REACHABLE(); >>> > } >>> >>> Let's look at the context here: >>> >>> static void ipi_sync_core(void *info) >>> { >>> [....] >>> membarrier_sync_core_before_usermode() >>> } >>> >>> ^ this can be within #ifdef / #endif >>> >>> static int membarrier_private_expedited(int flags, int cpu_id) >>> [...] >>> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) >>> return -EINVAL; >>> if (!(atomic_read(&mm->membarrier_state) & >>> MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) >>> return -EPERM; >>> ipi_func = ipi_sync_core; >>> >>> All we need to make the line above work is to define an empty ipi_sync_core >>> function in the #else case after the ipi_sync_core() function definition. >>> >>> Or am I missing your point ? >> >> Maybe? >> >> My objection is that an empty ipi_sync_core is a lie — it doesn’t sync the core. >> I would be fine with that if I could have the compiler statically verify that >> it’s not called, but I’m uncomfortable having it there if the implementation is >> actively incorrect. > > I see. Another approach would be to implement a "setter" function to populate > "ipi_func". That setter function would return -EINVAL in its #ifndef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE > implementation. I still don't get the problem with my suggestion. Sure the ipi is a "lie", but it doesn't get used. That's how a lot of ifdef folding works out. E.g., diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c index b5add64d9698..54cb32d064af 100644 --- a/kernel/sched/membarrier.c +++ b/kernel/sched/membarrier.c @@ -5,6 +5,15 @@ * membarrier system call */ #include "sched.h" +#ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE +#include <asm/sync_core.h> +#else +static inline void membarrier_sync_core_before_usermode(void) +{ + compiletime_assert(0, "architecture does not implement membarrier_sync_core_before_usermode"); +} + +#endif /* * For documentation purposes, here are some membarrier ordering _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-19 6:02 ` Nicholas Piggin @ 2021-06-19 15:50 ` Andy Lutomirski 2021-06-20 2:10 ` Nicholas Piggin 0 siblings, 1 reply; 45+ messages in thread From: Andy Lutomirski @ 2021-06-19 15:50 UTC (permalink / raw) To: Nicholas Piggin, Mathieu Desnoyers Cc: Andrew Morton, Benjamin Herrenschmidt, Catalin Marinas, Dave Hansen, linux-arm-kernel, Linux Kernel Mailing List, linux-mm, linuxppc-dev, Michael Ellerman, Paul Mackerras, Peter Zijlstra (Intel), stable, Will Deacon, the arch/x86 maintainers On Fri, Jun 18, 2021, at 11:02 PM, Nicholas Piggin wrote: > Excerpts from Mathieu Desnoyers's message of June 19, 2021 6:09 am: > > ----- On Jun 18, 2021, at 3:58 PM, Andy Lutomirski luto@kernel.org wrote: > > > >> On Fri, Jun 18, 2021, at 9:31 AM, Mathieu Desnoyers wrote: > >>> ----- On Jun 17, 2021, at 8:12 PM, Andy Lutomirski luto@kernel.org wrote: > >>> > >>> > On 6/17/21 7:47 AM, Mathieu Desnoyers wrote: > >>> > > >>> >> Please change back this #ifndef / #else / #endif within function for > >>> >> > >>> >> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { > >>> >> ... > >>> >> } else { > >>> >> ... > >>> >> } > >>> >> > >>> >> I don't think mixing up preprocessor and code logic makes it more readable. > >>> > > >>> > I agree, but I don't know how to make the result work well. > >>> > membarrier_sync_core_before_usermode() isn't defined in the !IS_ENABLED > >>> > case, so either I need to fake up a definition or use #ifdef. > >>> > > >>> > If I faked up a definition, I would want to assert, at build time, that > >>> > it isn't called. I don't think we can do: > >>> > > >>> > static void membarrier_sync_core_before_usermode() > >>> > { > >>> > BUILD_BUG_IF_REACHABLE(); > >>> > } > >>> > >>> Let's look at the context here: > >>> > >>> static void ipi_sync_core(void *info) > >>> { > >>> [....] > >>> membarrier_sync_core_before_usermode() > >>> } > >>> > >>> ^ this can be within #ifdef / #endif > >>> > >>> static int membarrier_private_expedited(int flags, int cpu_id) > >>> [...] > >>> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) > >>> return -EINVAL; > >>> if (!(atomic_read(&mm->membarrier_state) & > >>> MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) > >>> return -EPERM; > >>> ipi_func = ipi_sync_core; > >>> > >>> All we need to make the line above work is to define an empty ipi_sync_core > >>> function in the #else case after the ipi_sync_core() function definition. > >>> > >>> Or am I missing your point ? > >> > >> Maybe? > >> > >> My objection is that an empty ipi_sync_core is a lie — it doesn’t sync the core. > >> I would be fine with that if I could have the compiler statically verify that > >> it’s not called, but I’m uncomfortable having it there if the implementation is > >> actively incorrect. > > > > I see. Another approach would be to implement a "setter" function to populate > > "ipi_func". That setter function would return -EINVAL in its #ifndef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE > > implementation. > > I still don't get the problem with my suggestion. Sure the > ipi is a "lie", but it doesn't get used. That's how a lot of > ifdef folding works out. E.g., > > diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c > index b5add64d9698..54cb32d064af 100644 > --- a/kernel/sched/membarrier.c > +++ b/kernel/sched/membarrier.c > @@ -5,6 +5,15 @@ > * membarrier system call > */ > #include "sched.h" > +#ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE > +#include <asm/sync_core.h> > +#else > +static inline void membarrier_sync_core_before_usermode(void) > +{ > + compiletime_assert(0, "architecture does not implement > membarrier_sync_core_before_usermode"); > +} > + With the assert there, I’m fine with this. Let me see if the result builds. > +#endif > > /* > * For documentation purposes, here are some membarrier ordering > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-19 15:50 ` Andy Lutomirski @ 2021-06-20 2:10 ` Nicholas Piggin 0 siblings, 0 replies; 45+ messages in thread From: Nicholas Piggin @ 2021-06-20 2:10 UTC (permalink / raw) To: Andy Lutomirski, Mathieu Desnoyers Cc: Andrew Morton, Benjamin Herrenschmidt, Catalin Marinas, Dave Hansen, linux-arm-kernel, Linux Kernel Mailing List, linux-mm, linuxppc-dev, Michael Ellerman, Paul Mackerras, Peter Zijlstra (Intel), stable, Will Deacon, the arch/x86 maintainers Excerpts from Andy Lutomirski's message of June 20, 2021 1:50 am: > > > On Fri, Jun 18, 2021, at 11:02 PM, Nicholas Piggin wrote: >> Excerpts from Mathieu Desnoyers's message of June 19, 2021 6:09 am: >> > ----- On Jun 18, 2021, at 3:58 PM, Andy Lutomirski luto@kernel.org wrote: >> > >> >> On Fri, Jun 18, 2021, at 9:31 AM, Mathieu Desnoyers wrote: >> >>> ----- On Jun 17, 2021, at 8:12 PM, Andy Lutomirski luto@kernel.org wrote: >> >>> >> >>> > On 6/17/21 7:47 AM, Mathieu Desnoyers wrote: >> >>> > >> >>> >> Please change back this #ifndef / #else / #endif within function for >> >>> >> >> >>> >> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { >> >>> >> ... >> >>> >> } else { >> >>> >> ... >> >>> >> } >> >>> >> >> >>> >> I don't think mixing up preprocessor and code logic makes it more readable. >> >>> > >> >>> > I agree, but I don't know how to make the result work well. >> >>> > membarrier_sync_core_before_usermode() isn't defined in the !IS_ENABLED >> >>> > case, so either I need to fake up a definition or use #ifdef. >> >>> > >> >>> > If I faked up a definition, I would want to assert, at build time, that >> >>> > it isn't called. I don't think we can do: >> >>> > >> >>> > static void membarrier_sync_core_before_usermode() >> >>> > { >> >>> > BUILD_BUG_IF_REACHABLE(); >> >>> > } >> >>> >> >>> Let's look at the context here: >> >>> >> >>> static void ipi_sync_core(void *info) >> >>> { >> >>> [....] >> >>> membarrier_sync_core_before_usermode() >> >>> } >> >>> >> >>> ^ this can be within #ifdef / #endif >> >>> >> >>> static int membarrier_private_expedited(int flags, int cpu_id) >> >>> [...] >> >>> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) >> >>> return -EINVAL; >> >>> if (!(atomic_read(&mm->membarrier_state) & >> >>> MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) >> >>> return -EPERM; >> >>> ipi_func = ipi_sync_core; >> >>> >> >>> All we need to make the line above work is to define an empty ipi_sync_core >> >>> function in the #else case after the ipi_sync_core() function definition. >> >>> >> >>> Or am I missing your point ? >> >> >> >> Maybe? >> >> >> >> My objection is that an empty ipi_sync_core is a lie — it doesn’t sync the core. >> >> I would be fine with that if I could have the compiler statically verify that >> >> it’s not called, but I’m uncomfortable having it there if the implementation is >> >> actively incorrect. >> > >> > I see. Another approach would be to implement a "setter" function to populate >> > "ipi_func". That setter function would return -EINVAL in its #ifndef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE >> > implementation. >> >> I still don't get the problem with my suggestion. Sure the >> ipi is a "lie", but it doesn't get used. That's how a lot of >> ifdef folding works out. E.g., >> >> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c >> index b5add64d9698..54cb32d064af 100644 >> --- a/kernel/sched/membarrier.c >> +++ b/kernel/sched/membarrier.c >> @@ -5,6 +5,15 @@ >> * membarrier system call >> */ >> #include "sched.h" >> +#ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE >> +#include <asm/sync_core.h> >> +#else >> +static inline void membarrier_sync_core_before_usermode(void) >> +{ >> + compiletime_assert(0, "architecture does not implement >> membarrier_sync_core_before_usermode"); >> +} >> + > > With the assert there, I’m fine with this. Let me see if the result builds. It had better, because compiletime_assert already relies on a similar level of code elimination to work. I think it's fine to use for now, but it may not be quite the the logically correct primitive if we want to be really clean, because a valid compiletime_assert implementation should be able to fire even for code that is never linked. We would want something like to be clean IMO: #ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE #include <asm/sync_core.h> #else extern void membarrier_sync_core_before_usermode(void) __compiletime_error("architecture does not implement membarrier_sync_core_before_usermode"); #endif However that does not have the ifdef for optimising compile so AFAIKS it could break with a false positive in some cases. Something like compiletime_assert_not_called("msg") that either compiles to a noop or a __compiletime_error depending on __OPTIMIZE__ would be the way to go IMO. I don't know if anything exists that fits, but it's certainly not a unique thing in the kernel so I may not be looking hard enough. Thanks, Nick _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-16 3:21 ` [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation Andy Lutomirski ` (2 preceding siblings ...) 2021-06-17 14:47 ` Mathieu Desnoyers @ 2021-06-17 15:16 ` Mathieu Desnoyers 2021-06-18 0:13 ` Andy Lutomirski 3 siblings, 1 reply; 45+ messages in thread From: Mathieu Desnoyers @ 2021-06-17 15:16 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, Dave Hansen, linux-kernel, linux-mm, Andrew Morton, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, Will Deacon, linux-arm-kernel, Peter Zijlstra, stable ----- On Jun 15, 2021, at 11:21 PM, Andy Lutomirski luto@kernel.org wrote: [...] > +# An architecture that wants to support > +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE needs to define precisely what it > +# is supposed to do and implement membarrier_sync_core_before_usermode() to > +# make it do that. Then it can select ARCH_HAS_MEMBARRIER_SYNC_CORE via > +# Kconfig.Unfortunately, MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE is not a > +# fantastic API and may not make sense on all architectures. Once an > +# architecture meets these requirements, Can we please remove the editorial comment about the quality of the membarrier sync-core's API ? At least it's better than having all userspace rely on mprotect() undocumented side-effects to perform something which typically works, until it won't, or until this prevents mprotect's implementation to be improved because it will start breaking JITs all over the place. We can simply state that the definition of what membarrier sync-core does is defined per-architecture, and document the sequence of operations to perform when doing cross-modifying code specifically for each architecture. Now if there are weird architectures where membarrier is an odd fit (I've heard that riscv might need address ranges to which the core sync needs to apply), then those might need to implement their own arch-specific system call, which is all fine. > +# > +# On x86, a program can safely modify code, issue > +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, and then execute that code, via > +# the modified address or an alias, from any thread in the calling process. > +# > +# On arm64, a program can modify code, flush the icache as needed, and issue > +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE to force a "context synchronizing > +# event", aka pipeline flush on all CPUs that might run the calling process. > +# Then the program can execute the modified code as long as it is executed > +# from an address consistent with the icache flush and the CPU's cache type. > +# > +# On powerpc, a program can use MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE > +# similarly to arm64. It would be nice if the powerpc maintainers could > +# add a more clear explanantion. We should document the requirements on ARMv7 as well. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation 2021-06-17 15:16 ` Mathieu Desnoyers @ 2021-06-18 0:13 ` Andy Lutomirski 0 siblings, 0 replies; 45+ messages in thread From: Andy Lutomirski @ 2021-06-18 0:13 UTC (permalink / raw) To: Mathieu Desnoyers Cc: x86, Dave Hansen, linux-kernel, linux-mm, Andrew Morton, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, Nicholas Piggin, Catalin Marinas, Will Deacon, linux-arm-kernel, Peter Zijlstra, stable On 6/17/21 8:16 AM, Mathieu Desnoyers wrote: > ----- On Jun 15, 2021, at 11:21 PM, Andy Lutomirski luto@kernel.org wrote: > > [...] > >> +# An architecture that wants to support >> +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE needs to define precisely what it >> +# is supposed to do and implement membarrier_sync_core_before_usermode() to >> +# make it do that. Then it can select ARCH_HAS_MEMBARRIER_SYNC_CORE via >> +# Kconfig.Unfortunately, MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE is not a >> +# fantastic API and may not make sense on all architectures. Once an >> +# architecture meets these requirements, > > Can we please remove the editorial comment about the quality of the membarrier > sync-core's API ? Done >> +# >> +# On x86, a program can safely modify code, issue >> +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, and then execute that code, via >> +# the modified address or an alias, from any thread in the calling process. >> +# >> +# On arm64, a program can modify code, flush the icache as needed, and issue >> +# MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE to force a "context synchronizing >> +# event", aka pipeline flush on all CPUs that might run the calling process. >> +# Then the program can execute the modified code as long as it is executed >> +# from an address consistent with the icache flush and the CPU's cache type. >> +# >> +# On powerpc, a program can use MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE >> +# similarly to arm64. It would be nice if the powerpc maintainers could >> +# add a more clear explanantion. > > We should document the requirements on ARMv7 as well. Done. > > Thanks, > > Mathieu > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2021-06-20 2:12 UTC | newest] Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <cover.1623813516.git.luto@kernel.org> 2021-06-16 3:21 ` [PATCH 7/8] membarrier: Remove arm (32) support for SYNC_CORE Andy Lutomirski 2021-06-16 9:28 ` Russell King (Oracle) 2021-06-16 10:16 ` Peter Zijlstra 2021-06-16 10:20 ` Peter Zijlstra 2021-06-16 10:34 ` Russell King (Oracle) 2021-06-16 11:10 ` Peter Zijlstra 2021-06-16 13:22 ` Russell King (Oracle) 2021-06-16 15:04 ` Catalin Marinas 2021-06-16 15:23 ` Russell King (Oracle) 2021-06-16 15:45 ` Catalin Marinas 2021-06-16 16:00 ` Catalin Marinas 2021-06-16 16:27 ` Russell King (Oracle) 2021-06-17 8:55 ` Krzysztof Hałasa 2021-06-18 12:54 ` Linus Walleij 2021-06-18 13:19 ` Russell King (Oracle) 2021-06-18 13:36 ` Arnd Bergmann 2021-06-17 10:40 ` Mark Rutland 2021-06-17 11:23 ` Russell King (Oracle) 2021-06-17 11:33 ` Mark Rutland 2021-06-17 13:41 ` Andy Lutomirski 2021-06-17 13:51 ` Mark Rutland 2021-06-17 14:00 ` Andy Lutomirski 2021-06-17 14:20 ` Mark Rutland 2021-06-17 15:01 ` Peter Zijlstra 2021-06-17 15:13 ` Peter Zijlstra 2021-06-17 14:16 ` Mathieu Desnoyers 2021-06-17 14:05 ` Peter Zijlstra 2021-06-18 0:07 ` Andy Lutomirski 2021-06-16 3:21 ` [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation Andy Lutomirski 2021-06-16 4:45 ` Nicholas Piggin 2021-06-16 18:52 ` Andy Lutomirski 2021-06-16 23:48 ` Andy Lutomirski 2021-06-18 15:27 ` Christophe Leroy 2021-06-16 10:20 ` Will Deacon 2021-06-16 23:58 ` Andy Lutomirski 2021-06-17 14:47 ` Mathieu Desnoyers 2021-06-18 0:12 ` Andy Lutomirski 2021-06-18 16:31 ` Mathieu Desnoyers 2021-06-18 19:58 ` Andy Lutomirski 2021-06-18 20:09 ` Mathieu Desnoyers 2021-06-19 6:02 ` Nicholas Piggin 2021-06-19 15:50 ` Andy Lutomirski 2021-06-20 2:10 ` Nicholas Piggin 2021-06-17 15:16 ` Mathieu Desnoyers 2021-06-18 0:13 ` Andy Lutomirski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).