linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Jann Horn <jannh@google.com>
Cc: Andy Lutomirski <luto@kernel.org>, Will Deacon <will@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	x86 <x86@kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>,
	Nicholas Piggin <npiggin@gmail.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	stable <stable@vger.kernel.org>
Subject: Re: [RFC please help] membarrier: Rewrite sync_core_before_usermode()
Date: Mon, 28 Dec 2020 19:08:52 +0000	[thread overview]
Message-ID: <20201228190852.GI1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <CAG48ez0YZ_iy6qZpdGUj38wqeg_NzLHHhU-mBCBf5hcopYGVPg@mail.gmail.com>

On Mon, Dec 28, 2020 at 07:29:34PM +0100, Jann Horn wrote:
> After chatting with rmk about this (but without claiming that any of
> this is his opinion), based on the manpage, I think membarrier()
> currently doesn't really claim to be synchronizing caches? It just
> serializes cores. So arguably if userspace wants to use membarrier()
> to synchronize code changes, userspace should first do the code
> change, then flush icache as appropriate for the architecture, and
> then do the membarrier() to ensure that the old code is unused?
> 
> For 32-bit arm, rmk pointed out that that would be the cacheflush()
> syscall. That might cause you to end up with two IPIs instead of one
> in total, but we probably don't care _that_ much about extra IPIs on
> 32-bit arm?
> 
> For arm64, I believe userspace can flush icache across the entire
> system with some instructions from userspace - "DC CVAU" followed by
> "DSB ISH", or something like that, I think? (See e.g.
> compat_arm_syscall(), the arm64 compat code that implements the 32-bit
> arm cacheflush() syscall.)

Note that the ARM cacheflush syscall calls flush_icache_user_range()
over the range of addresses that userspace has passed - it's intention
since day one is to support cases where userspace wants to change
executable code.

It will issue the appropriate write-backs to the data cache (DCCMVAU),
the invalidates to the instruction cache (ICIMVAU), invalidate the
branch target buffer (BPIALLIS or BPIALL as appropriate), and issue
the appropriate barriers (DSB ISHST, ISB).

Note that neither flush_icache_user_range() nor flush_icache_range()
result in IPIs; cache operations are broadcast across all CPUs (which
is one of the minimums we require for SMP systems.)

Now, that all said, I think the question that has to be asked is...

	What is the basic purpose of membarrier?

Is the purpose of it to provide memory barriers, or is it to provide
memory coherence?

If it's the former and not the latter, then cache flushes are out of
scope, and expecting memory written to be visible to the instruction
stream is totally out of scope of the membarrier interface, whether
or not the writes happen on the same or a different CPU to the one
executing the rewritten code.

The documentation in the kernel does not seem to describe what it's
supposed to be doing - the only thing I could find is this:
Documentation/features/sched/membarrier-sync-core/arch-support.txt
which describes it as "arch supports core serializing membarrier"
whatever that means.

Seems to be the standard and usual case of utterly poor to non-existent
documentation within the kernel tree, or even a pointer to where any
useful documentation can be found.

Reading the membarrier(2) man page, I find nothing in there that talks
about any kind of cache coherency for self-modifying code - it only
seems to be about _barriers_ and nothing more, and barriers alone do
precisely nothing to save you from non-coherent Harvard caches.

So, either Andy has a misunderstanding, or the man page is wrong, or
my rudimentary understanding of what membarrier is supposed to be
doing is wrong...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

  parent reply	other threads:[~2020-12-28 19:09 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-27 18:28 [RFC please help] membarrier: Rewrite sync_core_before_usermode() Andy Lutomirski
2020-12-27 20:18 ` Mathieu Desnoyers
2020-12-27 21:36   ` Andy Lutomirski
2020-12-28 10:25     ` Russell King - ARM Linux admin
2020-12-28 17:14       ` Andy Lutomirski
2020-12-28 17:23         ` Russell King - ARM Linux admin
2020-12-28 18:10           ` Andy Lutomirski
2020-12-28 18:29         ` Jann Horn
2020-12-28 18:50           ` Andy Lutomirski
2020-12-28 19:08           ` Russell King - ARM Linux admin [this message]
2020-12-28 19:44             ` Andy Lutomirski
2020-12-28 20:24               ` Russell King - ARM Linux admin
2020-12-28 20:40                 ` Mathieu Desnoyers
2020-12-28 20:32               ` Mathieu Desnoyers
2020-12-28 21:06                 ` Andy Lutomirski
2020-12-28 21:26                   ` Mathieu Desnoyers
2020-12-29  0:36                   ` Nicholas Piggin
2020-12-29  0:56                     ` Andy Lutomirski
2020-12-29  3:09                       ` Nicholas Piggin
2020-12-29 10:44                         ` Russell King - ARM Linux admin
2020-12-30  2:33                           ` Nicholas Piggin
2020-12-30 10:00                             ` Russell King - ARM Linux admin
2020-12-30 10:58                               ` Russell King - ARM Linux admin
2020-12-30 11:57                                 ` Nicholas Piggin
2020-12-28 21:09     ` Mathieu Desnoyers
2020-12-29  0:30       ` Andy Lutomirski
2020-12-29  0:11 ` Nicholas Piggin
2020-12-29  0:36   ` Andy Lutomirski
2020-12-29  3:31     ` Nicholas Piggin
2021-01-01 18:33     ` David Laight
2021-01-05 13:26     ` Will Deacon
2021-01-05 16:20       ` Andy Lutomirski
2021-01-05 16:37         ` Peter Zijlstra
2021-01-05 22:41         ` Will Deacon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201228190852.GI1551@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=arnd@arndb.de \
    --cc=benh@kernel.crashing.org \
    --cc=catalin.marinas@arm.com \
    --cc=jannh@google.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=luto@kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mpe@ellerman.id.au \
    --cc=npiggin@gmail.com \
    --cc=paulus@samba.org \
    --cc=stable@vger.kernel.org \
    --cc=will@kernel.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).