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 <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> 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
On Tue, Feb 1, 2022 at 12:53 AM Kees Cook <[email protected]> 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
On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <[email protected]> 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 <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> 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
>
On Tue, Feb 1, 2022 at 2:15 PM Ard Biesheuvel <[email protected]> wrote:
>
> On Tue, 1 Feb 2022 at 20:40, Nick Desaulniers <[email protected]> wrote:
> >
> > On Tue, Feb 1, 2022 at 1:32 AM Ard Biesheuvel <[email protected]> wrote:
> > >
> > > On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <[email protected]> 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
On Tue, 1 Feb 2022 at 20:40, Nick Desaulniers <[email protected]> wrote:
>
> On Tue, Feb 1, 2022 at 1:32 AM Ard Biesheuvel <[email protected]> wrote:
> >
> > On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Nick Desaulniers <[email protected]>
> > > ---
> > > 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
On Wed, 2 Feb 2022 at 04:26, Nick Desaulniers <[email protected]> wrote:
>
> On Tue, Feb 1, 2022 at 2:15 PM Ard Biesheuvel <[email protected]> wrote:
> >
> > On Tue, 1 Feb 2022 at 20:40, Nick Desaulniers <[email protected]> wrote:
> > >
> > > On Tue, Feb 1, 2022 at 1:32 AM Ard Biesheuvel <[email protected]> wrote:
> > > >
> > > > On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <[email protected]> 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.
On Tue, Feb 1, 2022 at 1:32 AM Ard Biesheuvel <[email protected]> wrote:
>
> On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <[email protected]> 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 <[email protected]>
> > Signed-off-by: Nick Desaulniers <[email protected]>
> > ---
> > 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