linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute
@ 2022-01-31 22:52 Nick Desaulniers
  2022-01-31 23:53 ` Kees Cook
  2022-02-01  9:32 ` Ard Biesheuvel
  0 siblings, 2 replies; 8+ messages in thread
From: Nick Desaulniers @ 2022-01-31 22:52 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Josh Poimboeuf, Borislav Petkov, Peter Zijlstra, llvm,
	Nick Desaulniers, Kees Cook, Alan Stern, Andrea Parri,
	Will Deacon, Boqun Feng, Nicholas Piggin, David Howells,
	Jade Alglave, Luc Maranget, Paul E. McKenney, Akira Yokosawa,
	Daniel Lustig, Joel Fernandes, Gustavo A. R. Silva, Len Baker,
	linux-kernel, linux-arch, linux-doc

Add text to memory-barriers.txt and deprecated.rst to denote that
volatile-qualifying an asm statement is not a substitute for either a
compiler barrier (``barrier();``) or a clobber list.

This way we can point to this in code that strengthens existing
volatile-qualified asm statements to use a compiler barrier.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Example: https://godbolt.org/z/8PW549zz9

 Documentation/memory-barriers.txt    | 24 ++++++++++++++++++++++++
 Documentation/process/deprecated.rst | 17 +++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index b12df9137e1c..f3908c0812da 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1726,6 +1726,30 @@ of optimizations:
      respect the order in which the READ_ONCE()s and WRITE_ONCE()s occur,
      though the CPU of course need not do so.
 
+ (*) Similarly, the compiler is within its rights to reorder instructions
+     around an asm statement so long as clobbers are not violated. For example,
+
+	asm volatile ("");
+	flag = true;
+
+     May be modified by the compiler to:
+
+	flag = true;
+	asm volatile ("");
+
+     Marking an asm statement as volatile is not a substitute for barrier(),
+     and is implicit for asm goto statements and asm statements that do not
+     have outputs (like the above example). Prefer either:
+
+	asm ("":::"memory");
+	flag = true;
+
+     Or:
+
+	asm ("");
+	barrier();
+	flag = true;
+
  (*) The compiler is within its rights to invent stores to a variable,
      as in the following example:
 
diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index 388cb19f5dbb..432816e2f79e 100644
--- a/Documentation/process/deprecated.rst
+++ b/Documentation/process/deprecated.rst
@@ -329,3 +329,20 @@ struct_size() and flex_array_size() helpers::
         instance->count = count;
 
         memcpy(instance->items, source, flex_array_size(instance, items, instance->count));
+
+Volatile Qualified asm Statements
+=================================
+
+According to `the GCC docs on inline asm
+https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile`_:
+
+  asm statements that have no output operands and asm goto statements,
+  are implicitly volatile.
+
+For many uses of asm statements, that means adding a volatile qualifier won't
+hurt (making the implicit explicit), but it will not strengthen the semantics
+for such cases where it would have been implied. Care should be taken not to
+confuse ``volatile`` with the kernel's ``barrier()`` macro or an explicit
+clobber list. See [memory-barriers]_ for more info on ``barrier()``.
+
+.. [memory-barriers] Documentation/memory-barriers.txt
-- 
2.35.0.rc2.247.g8bbb082509-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute
  2022-01-31 22:52 [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute Nick Desaulniers
@ 2022-01-31 23:53 ` Kees Cook
  2022-02-01  7:37   ` Arnd Bergmann
  2022-02-01  9:32 ` Ard Biesheuvel
  1 sibling, 1 reply; 8+ messages in thread
From: Kees Cook @ 2022-01-31 23:53 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Jonathan Corbet, Josh Poimboeuf, Borislav Petkov, Peter Zijlstra,
	llvm, Alan Stern, Andrea Parri, Will Deacon, Boqun Feng,
	Nicholas Piggin, David Howells, Jade Alglave, Luc Maranget,
	Paul E. McKenney, Akira Yokosawa, Daniel Lustig, Joel Fernandes,
	Gustavo A. R. Silva, Len Baker, linux-kernel, linux-arch,
	linux-doc

On Mon, Jan 31, 2022 at 02:52:47PM -0800, Nick Desaulniers wrote:
> Add text to memory-barriers.txt and deprecated.rst to denote that
> volatile-qualifying an asm statement is not a substitute for either a
> compiler barrier (``barrier();``) or a clobber list.
> 
> This way we can point to this in code that strengthens existing
> volatile-qualified asm statements to use a compiler barrier.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Example: https://godbolt.org/z/8PW549zz9
> 
>  Documentation/memory-barriers.txt    | 24 ++++++++++++++++++++++++
>  Documentation/process/deprecated.rst | 17 +++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index b12df9137e1c..f3908c0812da 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1726,6 +1726,30 @@ of optimizations:
>       respect the order in which the READ_ONCE()s and WRITE_ONCE()s occur,
>       though the CPU of course need not do so.
>  
> + (*) Similarly, the compiler is within its rights to reorder instructions
> +     around an asm statement so long as clobbers are not violated. For example,
> +
> +	asm volatile ("");
> +	flag = true;
> +
> +     May be modified by the compiler to:
> +
> +	flag = true;
> +	asm volatile ("");
> +
> +     Marking an asm statement as volatile is not a substitute for barrier(),
> +     and is implicit for asm goto statements and asm statements that do not
> +     have outputs (like the above example). Prefer either:
> +
> +	asm ("":::"memory");
> +	flag = true;
> +
> +     Or:
> +
> +	asm ("");
> +	barrier();
> +	flag = true;
> +

I like this!

>   (*) The compiler is within its rights to invent stores to a variable,
>       as in the following example:
>  
> diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> index 388cb19f5dbb..432816e2f79e 100644
> --- a/Documentation/process/deprecated.rst
> +++ b/Documentation/process/deprecated.rst
> @@ -329,3 +329,20 @@ struct_size() and flex_array_size() helpers::
>          instance->count = count;
>  
>          memcpy(instance->items, source, flex_array_size(instance, items, instance->count));
> +
> +Volatile Qualified asm Statements
> +=================================

I would open with an example, like:

Instead of::

	volatile asm("...");

just use::

	asm("...");


> +
> +According to `the GCC docs on inline asm
> +https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile`_:
> +
> +  asm statements that have no output operands and asm goto statements,
> +  are implicitly volatile.

Does this mean "volatile" _is_ needed when there are operands, etc?

> +
> +For many uses of asm statements, that means adding a volatile qualifier won't
> +hurt (making the implicit explicit), but it will not strengthen the semantics
> +for such cases where it would have been implied. Care should be taken not to
> +confuse ``volatile`` with the kernel's ``barrier()`` macro or an explicit
> +clobber list. See [memory-barriers]_ for more info on ``barrier()``.
> +
> +.. [memory-barriers] Documentation/memory-barriers.txt
> -- 
> 2.35.0.rc2.247.g8bbb082509-goog
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute
  2022-01-31 23:53 ` Kees Cook
@ 2022-02-01  7:37   ` Arnd Bergmann
  0 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2022-02-01  7:37 UTC (permalink / raw)
  To: Kees Cook
  Cc: Nick Desaulniers, Jonathan Corbet, Josh Poimboeuf,
	Borislav Petkov, Peter Zijlstra, llvm, Alan Stern, Andrea Parri,
	Will Deacon, Boqun Feng, Nicholas Piggin, David Howells,
	Jade Alglave, Luc Maranget, Paul E. McKenney, Akira Yokosawa,
	Daniel Lustig, Joel Fernandes, Gustavo A. R. Silva, Len Baker,
	Linux Kernel Mailing List, linux-arch, open list:DOCUMENTATION

On Tue, Feb 1, 2022 at 12:53 AM Kees Cook <keescook@chromium.org> wrote:

> > +
> > +According to `the GCC docs on inline asm
> > +https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile`_:
> > +
> > +  asm statements that have no output operands and asm goto statements,
> > +  are implicitly volatile.
>
> Does this mean "volatile" _is_ needed when there are operands, etc?

It depends on what you want to express. The idea here is to give a way to
gcc for optimizing out anything with an output, like x86 rdtsc() when the
result is not used, which is sensible. If there is no output, such as in
writel(), you don't need 'volatile' because gcc can assume that an
inline asm without outputs has side-effects already.

A case where you need to add volatile is for (void)readl(ADDR),
which is an operation that has an output as well as a side-effect.

          Arnd

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute
  2022-01-31 22:52 [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute Nick Desaulniers
  2022-01-31 23:53 ` Kees Cook
@ 2022-02-01  9:32 ` Ard Biesheuvel
  2022-02-01 19:40   ` Nick Desaulniers
  1 sibling, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2022-02-01  9:32 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Jonathan Corbet, Josh Poimboeuf, Borislav Petkov, Peter Zijlstra,
	llvm, Kees Cook, Alan Stern, Andrea Parri, Will Deacon,
	Boqun Feng, Nicholas Piggin, David Howells, Jade Alglave,
	Luc Maranget, Paul E. McKenney, Akira Yokosawa, Daniel Lustig,
	Joel Fernandes, Gustavo A. R. Silva, Len Baker,
	Linux Kernel Mailing List, linux-arch, Linux Doc Mailing List

On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Add text to memory-barriers.txt and deprecated.rst to denote that
> volatile-qualifying an asm statement is not a substitute for either a
> compiler barrier (``barrier();``) or a clobber list.
>
> This way we can point to this in code that strengthens existing
> volatile-qualified asm statements to use a compiler barrier.
>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Example: https://godbolt.org/z/8PW549zz9
>
>  Documentation/memory-barriers.txt    | 24 ++++++++++++++++++++++++
>  Documentation/process/deprecated.rst | 17 +++++++++++++++++
>  2 files changed, 41 insertions(+)
>
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index b12df9137e1c..f3908c0812da 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1726,6 +1726,30 @@ of optimizations:
>       respect the order in which the READ_ONCE()s and WRITE_ONCE()s occur,
>       though the CPU of course need not do so.
>
> + (*) Similarly, the compiler is within its rights to reorder instructions

Similar to what? Was this intended to be the second bullet point
rather than the first?

> +     around an asm statement so long as clobbers are not violated. For example,
> +
> +       asm volatile ("");
> +       flag = true;
> +
> +     May be modified by the compiler to:
> +
> +       flag = true;
> +       asm volatile ("");
> +
> +     Marking an asm statement as volatile is not a substitute for barrier(),
> +     and is implicit for asm goto statements and asm statements that do not
> +     have outputs (like the above example). Prefer either:
> +
> +       asm ("":::"memory");
> +       flag = true;
> +
> +     Or:
> +
> +       asm ("");
> +       barrier();
> +       flag = true;
> +

I would expect the memory clobber to only hazard against the
assignment of flag if it results in a store, but looking at your
Godbolt example, this appears to apply even if flag is kept in a
register.

Is that behavior documented/codified anywhere? Or are we relying on
compiler implementation details here?


>   (*) The compiler is within its rights to invent stores to a variable,
>       as in the following example:
>
> diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> index 388cb19f5dbb..432816e2f79e 100644
> --- a/Documentation/process/deprecated.rst
> +++ b/Documentation/process/deprecated.rst
> @@ -329,3 +329,20 @@ struct_size() and flex_array_size() helpers::
>          instance->count = count;
>
>          memcpy(instance->items, source, flex_array_size(instance, items, instance->count));
> +
> +Volatile Qualified asm Statements
> +=================================
> +
> +According to `the GCC docs on inline asm
> +https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile`_:
> +
> +  asm statements that have no output operands and asm goto statements,
> +  are implicitly volatile.
> +
> +For many uses of asm statements, that means adding a volatile qualifier won't
> +hurt (making the implicit explicit), but it will not strengthen the semantics
> +for such cases where it would have been implied. Care should be taken not to
> +confuse ``volatile`` with the kernel's ``barrier()`` macro or an explicit
> +clobber list. See [memory-barriers]_ for more info on ``barrier()``.
> +
> +.. [memory-barriers] Documentation/memory-barriers.txt
> --
> 2.35.0.rc2.247.g8bbb082509-goog
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute
  2022-02-01  9:32 ` Ard Biesheuvel
@ 2022-02-01 19:40   ` Nick Desaulniers
  2022-02-01 22:15     ` Ard Biesheuvel
  0 siblings, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2022-02-01 19:40 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Jonathan Corbet, Josh Poimboeuf, Borislav Petkov, Peter Zijlstra,
	llvm, Kees Cook, Alan Stern, Andrea Parri, Will Deacon,
	Boqun Feng, Nicholas Piggin, David Howells, Jade Alglave,
	Luc Maranget, Paul E. McKenney, Akira Yokosawa, Daniel Lustig,
	Joel Fernandes, Gustavo A. R. Silva, Len Baker,
	Linux Kernel Mailing List, linux-arch, Linux Doc Mailing List

On Tue, Feb 1, 2022 at 1:32 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > Add text to memory-barriers.txt and deprecated.rst to denote that
> > volatile-qualifying an asm statement is not a substitute for either a
> > compiler barrier (``barrier();``) or a clobber list.
> >
> > This way we can point to this in code that strengthens existing
> > volatile-qualified asm statements to use a compiler barrier.
> >
> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > Example: https://godbolt.org/z/8PW549zz9
> >
> >  Documentation/memory-barriers.txt    | 24 ++++++++++++++++++++++++
> >  Documentation/process/deprecated.rst | 17 +++++++++++++++++
> >  2 files changed, 41 insertions(+)
> >
> > diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> > index b12df9137e1c..f3908c0812da 100644
> > --- a/Documentation/memory-barriers.txt
> > +++ b/Documentation/memory-barriers.txt
> > @@ -1726,6 +1726,30 @@ of optimizations:
> >       respect the order in which the READ_ONCE()s and WRITE_ONCE()s occur,
> >       though the CPU of course need not do so.
> >
> > + (*) Similarly, the compiler is within its rights to reorder instructions
>
> Similar to what? Was this intended to be the second bullet point
> rather than the first?

Similar to the previous bullet point. This isn't the first use of
`Similarly, ` in this document.

>
> > +     around an asm statement so long as clobbers are not violated. For example,
> > +
> > +       asm volatile ("");
> > +       flag = true;
> > +
> > +     May be modified by the compiler to:
> > +
> > +       flag = true;
> > +       asm volatile ("");
> > +
> > +     Marking an asm statement as volatile is not a substitute for barrier(),
> > +     and is implicit for asm goto statements and asm statements that do not
> > +     have outputs (like the above example). Prefer either:
> > +
> > +       asm ("":::"memory");
> > +       flag = true;
> > +
> > +     Or:
> > +
> > +       asm ("");
> > +       barrier();
> > +       flag = true;
> > +
>
> I would expect the memory clobber to only hazard against the
> assignment of flag if it results in a store, but looking at your
> Godbolt example, this appears to apply even if flag is kept in a
> register.
>
> Is that behavior documented/codified anywhere? Or are we relying on
> compiler implementation details here?

https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
"Note that the compiler can move even volatile asm instructions
relative to other code, including across jump instructions."

>
>
> >   (*) The compiler is within its rights to invent stores to a variable,
> >       as in the following example:
> >
> > diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> > index 388cb19f5dbb..432816e2f79e 100644
> > --- a/Documentation/process/deprecated.rst
> > +++ b/Documentation/process/deprecated.rst
> > @@ -329,3 +329,20 @@ struct_size() and flex_array_size() helpers::
> >          instance->count = count;
> >
> >          memcpy(instance->items, source, flex_array_size(instance, items, instance->count));
> > +
> > +Volatile Qualified asm Statements
> > +=================================
> > +
> > +According to `the GCC docs on inline asm
> > +https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile`_:
> > +
> > +  asm statements that have no output operands and asm goto statements,
> > +  are implicitly volatile.
> > +
> > +For many uses of asm statements, that means adding a volatile qualifier won't
> > +hurt (making the implicit explicit), but it will not strengthen the semantics
> > +for such cases where it would have been implied. Care should be taken not to
> > +confuse ``volatile`` with the kernel's ``barrier()`` macro or an explicit
> > +clobber list. See [memory-barriers]_ for more info on ``barrier()``.
> > +
> > +.. [memory-barriers] Documentation/memory-barriers.txt
> > --
> > 2.35.0.rc2.247.g8bbb082509-goog
> >



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute
  2022-02-01 19:40   ` Nick Desaulniers
@ 2022-02-01 22:15     ` Ard Biesheuvel
  2022-02-02  3:26       ` Nick Desaulniers
  0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2022-02-01 22:15 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Jonathan Corbet, Josh Poimboeuf, Borislav Petkov, Peter Zijlstra,
	llvm, Kees Cook, Alan Stern, Andrea Parri, Will Deacon,
	Boqun Feng, Nicholas Piggin, David Howells, Jade Alglave,
	Luc Maranget, Paul E. McKenney, Akira Yokosawa, Daniel Lustig,
	Joel Fernandes, Gustavo A. R. Silva, Len Baker,
	Linux Kernel Mailing List, linux-arch, Linux Doc Mailing List

On Tue, 1 Feb 2022 at 20:40, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Tue, Feb 1, 2022 at 1:32 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > Add text to memory-barriers.txt and deprecated.rst to denote that
> > > volatile-qualifying an asm statement is not a substitute for either a
> > > compiler barrier (``barrier();``) or a clobber list.
> > >
> > > This way we can point to this in code that strengthens existing
> > > volatile-qualified asm statements to use a compiler barrier.
> > >
> > > Suggested-by: Kees Cook <keescook@chromium.org>
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > ---
> > > Example: https://godbolt.org/z/8PW549zz9
> > >
> > >  Documentation/memory-barriers.txt    | 24 ++++++++++++++++++++++++
> > >  Documentation/process/deprecated.rst | 17 +++++++++++++++++
> > >  2 files changed, 41 insertions(+)
> > >
> > > diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> > > index b12df9137e1c..f3908c0812da 100644
> > > --- a/Documentation/memory-barriers.txt
> > > +++ b/Documentation/memory-barriers.txt
> > > @@ -1726,6 +1726,30 @@ of optimizations:
> > >       respect the order in which the READ_ONCE()s and WRITE_ONCE()s occur,
> > >       though the CPU of course need not do so.
> > >
> > > + (*) Similarly, the compiler is within its rights to reorder instructions
> >
> > Similar to what? Was this intended to be the second bullet point
> > rather than the first?
>
> Similar to the previous bullet point. This isn't the first use of
> `Similarly, ` in this document.
>

Ah right, I misread the context and thought you were inserting this
bullet point at the start. Sorry for the noise.

> >
> > > +     around an asm statement so long as clobbers are not violated. For example,
> > > +
> > > +       asm volatile ("");
> > > +       flag = true;
> > > +
> > > +     May be modified by the compiler to:
> > > +
> > > +       flag = true;
> > > +       asm volatile ("");
> > > +
> > > +     Marking an asm statement as volatile is not a substitute for barrier(),
> > > +     and is implicit for asm goto statements and asm statements that do not
> > > +     have outputs (like the above example). Prefer either:
> > > +
> > > +       asm ("":::"memory");
> > > +       flag = true;
> > > +
> > > +     Or:
> > > +
> > > +       asm ("");
> > > +       barrier();
> > > +       flag = true;
> > > +
> >
> > I would expect the memory clobber to only hazard against the
> > assignment of flag if it results in a store, but looking at your
> > Godbolt example, this appears to apply even if flag is kept in a
> > register.
> >
> > Is that behavior documented/codified anywhere? Or are we relying on
> > compiler implementation details here?
>
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
> "Note that the compiler can move even volatile asm instructions
> relative to other code, including across jump instructions."
>

That doesn't really answer my question. We are documenting here that
asm volatile does not prevent reordering but non-volatile asm with a
"memory" clobber does, and even prevents reordering of instructions
that do not modify memory to begin with.

Why is it justified to rely on this undocumented behavior?


> >
> >
> > >   (*) The compiler is within its rights to invent stores to a variable,
> > >       as in the following example:
> > >
> > > diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> > > index 388cb19f5dbb..432816e2f79e 100644
> > > --- a/Documentation/process/deprecated.rst
> > > +++ b/Documentation/process/deprecated.rst
> > > @@ -329,3 +329,20 @@ struct_size() and flex_array_size() helpers::
> > >          instance->count = count;
> > >
> > >          memcpy(instance->items, source, flex_array_size(instance, items, instance->count));
> > > +
> > > +Volatile Qualified asm Statements
> > > +=================================
> > > +
> > > +According to `the GCC docs on inline asm
> > > +https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile`_:
> > > +
> > > +  asm statements that have no output operands and asm goto statements,
> > > +  are implicitly volatile.
> > > +
> > > +For many uses of asm statements, that means adding a volatile qualifier won't
> > > +hurt (making the implicit explicit), but it will not strengthen the semantics
> > > +for such cases where it would have been implied. Care should be taken not to
> > > +confuse ``volatile`` with the kernel's ``barrier()`` macro or an explicit
> > > +clobber list. See [memory-barriers]_ for more info on ``barrier()``.
> > > +
> > > +.. [memory-barriers] Documentation/memory-barriers.txt
> > > --
> > > 2.35.0.rc2.247.g8bbb082509-goog
> > >
>
>
>
> --
> Thanks,
> ~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute
  2022-02-01 22:15     ` Ard Biesheuvel
@ 2022-02-02  3:26       ` Nick Desaulniers
  2022-02-02 10:54         ` Ard Biesheuvel
  0 siblings, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2022-02-02  3:26 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Jonathan Corbet, Josh Poimboeuf, Borislav Petkov, Peter Zijlstra,
	llvm, Kees Cook, Alan Stern, Andrea Parri, Will Deacon,
	Boqun Feng, Nicholas Piggin, David Howells, Jade Alglave,
	Luc Maranget, Paul E. McKenney, Akira Yokosawa, Daniel Lustig,
	Joel Fernandes, Gustavo A. R. Silva, Len Baker,
	Linux Kernel Mailing List, linux-arch, Linux Doc Mailing List

On Tue, Feb 1, 2022 at 2:15 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 1 Feb 2022 at 20:40, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > On Tue, Feb 1, 2022 at 1:32 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > > >
> > > > +     around an asm statement so long as clobbers are not violated. For example,
> > > > +
> > > > +       asm volatile ("");
> > > > +       flag = true;
> > > > +
> > > > +     May be modified by the compiler to:
> > > > +
> > > > +       flag = true;
> > > > +       asm volatile ("");
> > > > +
> > > > +     Marking an asm statement as volatile is not a substitute for barrier(),
> > > > +     and is implicit for asm goto statements and asm statements that do not
> > > > +     have outputs (like the above example). Prefer either:
> > > > +
> > > > +       asm ("":::"memory");
> > > > +       flag = true;
> > > > +
> > > > +     Or:
> > > > +
> > > > +       asm ("");
> > > > +       barrier();
> > > > +       flag = true;
> > > > +
> > >
> > > I would expect the memory clobber to only hazard against the
> > > assignment of flag if it results in a store, but looking at your
> > > Godbolt example, this appears to apply even if flag is kept in a
> > > register.
> > >
> > > Is that behavior documented/codified anywhere? Or are we relying on
> > > compiler implementation details here?
> >
> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
> > "Note that the compiler can move even volatile asm instructions
> > relative to other code, including across jump instructions."
> >
>
> That doesn't really answer my question. We are documenting here that
> asm volatile does not prevent reordering but non-volatile asm with a
> "memory" clobber does, and even prevents reordering of instructions
> that do not modify memory to begin with.
>
> Why is it justified to rely on this undocumented behavior?

I see your point.  You're right, I couldn't find anywhere where such
behavior was specified.  So the suggestion to use barrier() would rely
on unspecified behavior and should not be suggested.

Probably worth still mentioning that `volatile` qualifying an asm
statement doesn't prevent such reordering in this document somehow,
and perhaps that it's (currently) unspecified whether a barrier() can
prevent re-ordering with regards to non-memory-modifying instructions.
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute
  2022-02-02  3:26       ` Nick Desaulniers
@ 2022-02-02 10:54         ` Ard Biesheuvel
  0 siblings, 0 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2022-02-02 10:54 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Jonathan Corbet, Josh Poimboeuf, Borislav Petkov, Peter Zijlstra,
	llvm, Kees Cook, Alan Stern, Andrea Parri, Will Deacon,
	Boqun Feng, Nicholas Piggin, David Howells, Jade Alglave,
	Luc Maranget, Paul E. McKenney, Akira Yokosawa, Daniel Lustig,
	Joel Fernandes, Gustavo A. R. Silva, Len Baker,
	Linux Kernel Mailing List, linux-arch, Linux Doc Mailing List

On Wed, 2 Feb 2022 at 04:26, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Tue, Feb 1, 2022 at 2:15 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Tue, 1 Feb 2022 at 20:40, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > On Tue, Feb 1, 2022 at 1:32 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > > > >
> > > > > +     around an asm statement so long as clobbers are not violated. For example,
> > > > > +
> > > > > +       asm volatile ("");
> > > > > +       flag = true;
> > > > > +
> > > > > +     May be modified by the compiler to:
> > > > > +
> > > > > +       flag = true;
> > > > > +       asm volatile ("");
> > > > > +
> > > > > +     Marking an asm statement as volatile is not a substitute for barrier(),
> > > > > +     and is implicit for asm goto statements and asm statements that do not
> > > > > +     have outputs (like the above example). Prefer either:
> > > > > +
> > > > > +       asm ("":::"memory");
> > > > > +       flag = true;
> > > > > +
> > > > > +     Or:
> > > > > +
> > > > > +       asm ("");
> > > > > +       barrier();
> > > > > +       flag = true;
> > > > > +
> > > >
> > > > I would expect the memory clobber to only hazard against the
> > > > assignment of flag if it results in a store, but looking at your
> > > > Godbolt example, this appears to apply even if flag is kept in a
> > > > register.
> > > >
> > > > Is that behavior documented/codified anywhere? Or are we relying on
> > > > compiler implementation details here?
> > >
> > > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
> > > "Note that the compiler can move even volatile asm instructions
> > > relative to other code, including across jump instructions."
> > >
> >
> > That doesn't really answer my question. We are documenting here that
> > asm volatile does not prevent reordering but non-volatile asm with a
> > "memory" clobber does, and even prevents reordering of instructions
> > that do not modify memory to begin with.
> >
> > Why is it justified to rely on this undocumented behavior?
>
> I see your point.  You're right, I couldn't find anywhere where such
> behavior was specified.  So the suggestion to use barrier() would rely
> on unspecified behavior and should not be suggested.
>
> Probably worth still mentioning that `volatile` qualifying an asm
> statement doesn't prevent such reordering in this document somehow,
> and perhaps that it's (currently) unspecified whether a barrier() can
> prevent re-ordering with regards to non-memory-modifying instructions.

Yes, and to correct myself here, I meant 'instructions that do not
/reference/ memory to begin with.

I suppose the problem here is that the distinction only matters for
things like objtool obsessing about whether or not instructions look
unreachable, and relying on asm volatile() to insert markers into the
object code.

So it seems we need a stronger clobber, one which avoids any kind of
reordering at the instruction level.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-02-02 10:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-31 22:52 [PATCH] docs/memory-barriers.txt: volatile is not a barrier() substitute Nick Desaulniers
2022-01-31 23:53 ` Kees Cook
2022-02-01  7:37   ` Arnd Bergmann
2022-02-01  9:32 ` Ard Biesheuvel
2022-02-01 19:40   ` Nick Desaulniers
2022-02-01 22:15     ` Ard Biesheuvel
2022-02-02  3:26       ` Nick Desaulniers
2022-02-02 10:54         ` Ard Biesheuvel

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).