[PATCH 0/2] xen/nospec: Improvements

[PATCH 0/2] xen/nospec: Improvements

[Andrew Cooper]

Andrew Cooper (2):
  xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation
  xen/nospec: Allow evaluate_nospec() to short circuit constant expressions

 xen/arch/arm/include/asm/nospec.h   |  9 --------
 xen/arch/ppc/include/asm/nospec.h   |  9 --------
 xen/arch/riscv/include/asm/nospec.h |  9 --------
 xen/arch/x86/include/asm/nospec.h   |  8 ++++----
 xen/include/xen/nospec.h            | 32 +++++++++++++++++++++++++++++
 5 files changed, 36 insertions(+), 31 deletions(-)

-- 
2.30.2

[PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Andrew Cooper]

It is daft to require all architectures to provide empty implementations of
this functionality.

Provide evaluate_nospec() and block_speculation() unconditionally in
xen/nospec.h with architectures able to opt in by providing suitable arch
variants.

Rename x86's implementation to the arch_*() variants.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
---
 xen/arch/arm/include/asm/nospec.h   |  9 ---------
 xen/arch/ppc/include/asm/nospec.h   |  9 ---------
 xen/arch/riscv/include/asm/nospec.h |  9 ---------
 xen/arch/x86/include/asm/nospec.h   |  8 ++++----
 xen/include/xen/nospec.h            | 23 +++++++++++++++++++++++
 5 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/include/asm/nospec.h b/xen/arch/arm/include/asm/nospec.h
index efac51fc03be..05df096faab0 100644
--- a/xen/arch/arm/include/asm/nospec.h
+++ b/xen/arch/arm/include/asm/nospec.h
@@ -12,15 +12,6 @@
 # error "unknown ARM variant"
 #endif
 
-static inline bool evaluate_nospec(bool condition)
-{
-    return condition;
-}
-
-static inline void block_speculation(void)
-{
-}
-
 #endif /* _ASM_ARM_NOSPEC_H */
 
 /*
diff --git a/xen/arch/ppc/include/asm/nospec.h b/xen/arch/ppc/include/asm/nospec.h
index b97322e48d32..9b57a7e4b24d 100644
--- a/xen/arch/ppc/include/asm/nospec.h
+++ b/xen/arch/ppc/include/asm/nospec.h
@@ -3,13 +3,4 @@
 #ifndef __ASM_PPC_NOSPEC_H__
 #define __ASM_PPC_NOSPEC_H__
 
-static inline bool evaluate_nospec(bool condition)
-{
-    return condition;
-}
-
-static inline void block_speculation(void)
-{
-}
-
 #endif /* __ASM_PPC_NOSPEC_H__ */
diff --git a/xen/arch/riscv/include/asm/nospec.h b/xen/arch/riscv/include/asm/nospec.h
index e30f0a781b68..b227fc61ed8b 100644
--- a/xen/arch/riscv/include/asm/nospec.h
+++ b/xen/arch/riscv/include/asm/nospec.h
@@ -4,15 +4,6 @@
 #ifndef _ASM_RISCV_NOSPEC_H
 #define _ASM_RISCV_NOSPEC_H
 
-static inline bool evaluate_nospec(bool condition)
-{
-    return condition;
-}
-
-static inline void block_speculation(void)
-{
-}
-
 #endif /* _ASM_RISCV_NOSPEC_H */
 
 /*
diff --git a/xen/arch/x86/include/asm/nospec.h b/xen/arch/x86/include/asm/nospec.h
index 07606834c4c9..defc97707f03 100644
--- a/xen/arch/x86/include/asm/nospec.h
+++ b/xen/arch/x86/include/asm/nospec.h
@@ -23,20 +23,20 @@ static always_inline bool barrier_nospec_false(void)
     return false;
 }
 
-/* Allow to protect evaluation of conditionals with respect to speculation */
-static always_inline bool evaluate_nospec(bool condition)
+static always_inline bool arch_evaluate_nospec(bool condition)
 {
     if ( condition )
         return barrier_nospec_true();
     else
         return barrier_nospec_false();
 }
+#define arch_evaluate_nospec arch_evaluate_nospec
 
-/* Allow to block speculative execution in generic code */
-static always_inline void block_speculation(void)
+static always_inline void arch_block_speculation(void)
 {
     barrier_nospec_true();
 }
+#define arch_block_speculation arch_block_speculation
 
 /**
  * array_index_mask_nospec() - generate a mask that is ~0UL when the
diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h
index 4c250ebbd663..a4155af08770 100644
--- a/xen/include/xen/nospec.h
+++ b/xen/include/xen/nospec.h
@@ -9,6 +9,29 @@
 
 #include <asm/nospec.h>
 
+/*
+ * Protect a conditional branch from bad speculation.  Architectures *must*
+ * provide arch_evaluate_nospec() for this to be effective.
+ */
+static always_inline bool evaluate_nospec(bool cond)
+{
+#ifndef arch_evaluate_nospec
+#define arch_evaluate_nospec(cond) cond
+#endif
+    return arch_evaluate_nospec(cond);
+}
+
+/*
+ * Halt speculation unconditonally.  Architectures *must* provide
+ * arch_block_speculation() for this to be effective.
+ */
+static always_inline void block_speculation(void)
+{
+#ifdef arch_block_speculation
+    arch_block_speculation();
+#endif
+}
+
 /**
  * array_index_mask_nospec() - generate a ~0 mask when index < size, 0 otherwise
  * @index: array element index
-- 
2.30.2

[PATCH 2/2] xen/nospec: Allow evaluate_nospec() to short circuit constant expressions

[Andrew Cooper]

When the compiler can reduce the condition to a constant, it can elide the
conditional and one of the basic blocks.  However, arch_evaluate_nospec() will
still insert speculation protection, despite there being nothing to protect.

Allow the speculation protection to be skipped entirely when the compiler is
removing the condition entirely.

e.g. for x86, given:

  int foo(void)
  {
      if ( evaluate_nospec(1) )
          return 2;
      else
          return 42;
  }

then before, we get:

  <foo>:
      lfence
      mov    $0x2,%eax
      retq

and afterwards, we get:

  <foo>:
      mov    $0x2,%eax
      retq

which is correct.  With no conditional branch to protect, the lfence isn't
providing any relevant safety.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/include/xen/nospec.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h
index a4155af08770..56cf67a44176 100644
--- a/xen/include/xen/nospec.h
+++ b/xen/include/xen/nospec.h
@@ -18,6 +18,15 @@ static always_inline bool evaluate_nospec(bool cond)
 #ifndef arch_evaluate_nospec
 #define arch_evaluate_nospec(cond) cond
 #endif
+
+    /*
+     * If the compiler can reduce the condition to a constant, then it won't
+     * be emitting a conditional branch, and there's nothing needing
+     * protecting.
+     */
+    if ( __builtin_constant_p(cond) )
+        return cond;
+
     return arch_evaluate_nospec(cond);
 }
 
-- 
2.30.2

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Julien Grall]

Hi Andrew,

On 04/03/2024 16:10, Andrew Cooper wrote:
> It is daft to require all architectures to provide empty implementations of
> this functionality.

Oleksii recenlty sent a similar patch [1]. This was pushed back because 
from naming, it sounds like the helpers ought to be non-empty on every 
architecture.

It would be best if asm-generic provides a safe version of the helpers. 
So my preference is to not have this patch. This can of course change if 
I see an explanation why it is empty on Arm (I believe it should contain 
csdb) and other arch would want the same.

Cheers,

[1] 
5889d7a5fa81722472f95cc1448af0be8f359a7d.1707146506.git.oleksii.kurochko@gmail.com

-- 
Julien Grall

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Jan Beulich]

On 04.03.2024 17:31, Julien Grall wrote:
> On 04/03/2024 16:10, Andrew Cooper wrote:
>> It is daft to require all architectures to provide empty implementations of
>> this functionality.
> 
> Oleksii recenlty sent a similar patch [1]. This was pushed back because 
> from naming, it sounds like the helpers ought to be non-empty on every 
> architecture.
> 
> It would be best if asm-generic provides a safe version of the helpers. 
> So my preference is to not have this patch. This can of course change if 
> I see an explanation why it is empty on Arm (I believe it should contain 
> csdb) and other arch would want the same.

Except that there's no new asm-generic/ header here (as opposed to how
Oleksii had it). Imo avoiding the need for empty stubs is okay this way,
when introducing an asm-generic/ header would not have been. Of course
if Arm wants to put something there rather sooner than later, then
perhaps the functions better wouldn't be removed from there, just to then
be put back pretty soon.

Jan

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Jan Beulich]

On 04.03.2024 17:10, Andrew Cooper wrote:
> --- a/xen/arch/x86/include/asm/nospec.h
> +++ b/xen/arch/x86/include/asm/nospec.h
> @@ -23,20 +23,20 @@ static always_inline bool barrier_nospec_false(void)
>      return false;
>  }
>  
> -/* Allow to protect evaluation of conditionals with respect to speculation */
> -static always_inline bool evaluate_nospec(bool condition)
> +static always_inline bool arch_evaluate_nospec(bool condition)
>  {
>      if ( condition )
>          return barrier_nospec_true();
>      else
>          return barrier_nospec_false();
>  }
> +#define arch_evaluate_nospec arch_evaluate_nospec
>  
> -/* Allow to block speculative execution in generic code */
> -static always_inline void block_speculation(void)
> +static always_inline void arch_block_speculation(void)
>  {
>      barrier_nospec_true();
>  }
> +#define arch_block_speculation arch_block_speculation

I'm having some difficulty seeing the need for the renaming (adding
or the arch_ prefixes): Why can't the original names be kept, and
...

> --- a/xen/include/xen/nospec.h
> +++ b/xen/include/xen/nospec.h
> @@ -9,6 +9,29 @@
>  
>  #include <asm/nospec.h>
>  
> +/*
> + * Protect a conditional branch from bad speculation.  Architectures *must*
> + * provide arch_evaluate_nospec() for this to be effective.
> + */
> +static always_inline bool evaluate_nospec(bool cond)
> +{
> +#ifndef arch_evaluate_nospec
> +#define arch_evaluate_nospec(cond) cond
> +#endif
> +    return arch_evaluate_nospec(cond);
> +}
> +
> +/*
> + * Halt speculation unconditonally.  Architectures *must* provide
> + * arch_block_speculation() for this to be effective.
> + */
> +static always_inline void block_speculation(void)
> +{
> +#ifdef arch_block_speculation
> +    arch_block_speculation();
> +#endif
> +}

... stubs be introduced here if the original names aren't #define-d?
IOW what does the extra layer gain us?

Jan

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Andrew Cooper]

On 04/03/2024 4:45 pm, Jan Beulich wrote:
> On 04.03.2024 17:10, Andrew Cooper wrote:
>> --- a/xen/arch/x86/include/asm/nospec.h
>> +++ b/xen/arch/x86/include/asm/nospec.h
>> @@ -23,20 +23,20 @@ static always_inline bool barrier_nospec_false(void)
>>      return false;
>>  }
>>  
>> -/* Allow to protect evaluation of conditionals with respect to speculation */
>> -static always_inline bool evaluate_nospec(bool condition)
>> +static always_inline bool arch_evaluate_nospec(bool condition)
>>  {
>>      if ( condition )
>>          return barrier_nospec_true();
>>      else
>>          return barrier_nospec_false();
>>  }
>> +#define arch_evaluate_nospec arch_evaluate_nospec
>>  
>> -/* Allow to block speculative execution in generic code */
>> -static always_inline void block_speculation(void)
>> +static always_inline void arch_block_speculation(void)
>>  {
>>      barrier_nospec_true();
>>  }
>> +#define arch_block_speculation arch_block_speculation
> I'm having some difficulty seeing the need for the renaming (adding
> or the arch_ prefixes): Why can't the original names be kept, and
> ...
>
>> --- a/xen/include/xen/nospec.h
>> +++ b/xen/include/xen/nospec.h
>> @@ -9,6 +9,29 @@
>>  
>>  #include <asm/nospec.h>
>>  
>> +/*
>> + * Protect a conditional branch from bad speculation.  Architectures *must*
>> + * provide arch_evaluate_nospec() for this to be effective.
>> + */
>> +static always_inline bool evaluate_nospec(bool cond)
>> +{
>> +#ifndef arch_evaluate_nospec
>> +#define arch_evaluate_nospec(cond) cond
>> +#endif
>> +    return arch_evaluate_nospec(cond);
>> +}
>> +
>> +/*
>> + * Halt speculation unconditonally.  Architectures *must* provide
>> + * arch_block_speculation() for this to be effective.
>> + */
>> +static always_inline void block_speculation(void)
>> +{
>> +#ifdef arch_block_speculation
>> +    arch_block_speculation();
>> +#endif
>> +}
> ... stubs be introduced here if the original names aren't #define-d?
> IOW what does the extra layer gain us?

See the next patch.

~Andrew

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Julien Grall]

Hi Jan,

On 04/03/2024 16:41, Jan Beulich wrote:
> On 04.03.2024 17:31, Julien Grall wrote:
>> On 04/03/2024 16:10, Andrew Cooper wrote:
>>> It is daft to require all architectures to provide empty implementations of
>>> this functionality.
>>
>> Oleksii recenlty sent a similar patch [1]. This was pushed back because
>> from naming, it sounds like the helpers ought to be non-empty on every
>> architecture.
>>
>> It would be best if asm-generic provides a safe version of the helpers.
>> So my preference is to not have this patch. This can of course change if
>> I see an explanation why it is empty on Arm (I believe it should contain
>> csdb) and other arch would want the same.
> 
> Except that there's no new asm-generic/ header here (as opposed to how
> Oleksii had it). Imo avoiding the need for empty stubs is okay this way,
> when introducing an asm-generic/ header would not have been. Of course
> if Arm wants to put something there rather sooner than later, then
> perhaps the functions better wouldn't be removed from there, just to then
> be put back pretty soon.

I am confused. I agree the patch is slightly different, but I thought 
the fundamental problem was the block_speculation() implementation may 
not be safe everywhere. And it was best to let each architecture decide 
how they want to implement (vs Xen decide for us the default).

Reading the original thread, I thought you had agreed with that 
statement. Did I misinterpret?

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Andrew Cooper]

On 04/03/2024 4:41 pm, Jan Beulich wrote:
> On 04.03.2024 17:31, Julien Grall wrote:
>> On 04/03/2024 16:10, Andrew Cooper wrote:
>>> It is daft to require all architectures to provide empty implementations of
>>> this functionality.
>> Oleksii recenlty sent a similar patch [1]. This was pushed back because 
>> from naming, it sounds like the helpers ought to be non-empty on every 
>> architecture.
>>
>> It would be best if asm-generic provides a safe version of the helpers. 
>> So my preference is to not have this patch. This can of course change if 
>> I see an explanation why it is empty on Arm (I believe it should contain 
>> csdb) and other arch would want the same.
> Except that there's no new asm-generic/ header here (as opposed to how
> Oleksii had it). Imo avoiding the need for empty stubs is okay this way,
> when introducing an asm-generic/ header would not have been. Of course
> if Arm wants to put something there rather sooner than later, then
> perhaps the functions better wouldn't be removed from there, just to then
> be put back pretty soon.

If the ARM maintainers want Spectre-v1 safety then they can do the work
themselves, after this patch has gone in.

~Andrew

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Jan Beulich]

On 04.03.2024 17:46, Julien Grall wrote:
> On 04/03/2024 16:41, Jan Beulich wrote:
>> On 04.03.2024 17:31, Julien Grall wrote:
>>> On 04/03/2024 16:10, Andrew Cooper wrote:
>>>> It is daft to require all architectures to provide empty implementations of
>>>> this functionality.
>>>
>>> Oleksii recenlty sent a similar patch [1]. This was pushed back because
>>> from naming, it sounds like the helpers ought to be non-empty on every
>>> architecture.
>>>
>>> It would be best if asm-generic provides a safe version of the helpers.
>>> So my preference is to not have this patch. This can of course change if
>>> I see an explanation why it is empty on Arm (I believe it should contain
>>> csdb) and other arch would want the same.
>>
>> Except that there's no new asm-generic/ header here (as opposed to how
>> Oleksii had it). Imo avoiding the need for empty stubs is okay this way,
>> when introducing an asm-generic/ header would not have been. Of course
>> if Arm wants to put something there rather sooner than later, then
>> perhaps the functions better wouldn't be removed from there, just to then
>> be put back pretty soon.
> 
> I am confused. I agree the patch is slightly different, but I thought 
> the fundamental problem was the block_speculation() implementation may 
> not be safe everywhere. And it was best to let each architecture decide 
> how they want to implement (vs Xen decide for us the default).
> 
> Reading the original thread, I thought you had agreed with that 
> statement. Did I misinterpret?
Yes and no. Whatever is put in asm-generic/ ought to be correct and safe
by default, imo. The same doesn't apply to fallbacks put in place in
headers in xen/: If an arch doesn't provide its own implementation, it
indicates that the default (fallback) is good enough. Still I can easily
see that other views are possible here ...

Jan

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Andrew Cooper]

On 04/03/2024 4:55 pm, Jan Beulich wrote:
> On 04.03.2024 17:46, Julien Grall wrote:
>> On 04/03/2024 16:41, Jan Beulich wrote:
>>> On 04.03.2024 17:31, Julien Grall wrote:
>>>> On 04/03/2024 16:10, Andrew Cooper wrote:
>>>>> It is daft to require all architectures to provide empty implementations of
>>>>> this functionality.
>>>> Oleksii recenlty sent a similar patch [1]. This was pushed back because
>>>> from naming, it sounds like the helpers ought to be non-empty on every
>>>> architecture.
>>>>
>>>> It would be best if asm-generic provides a safe version of the helpers.
>>>> So my preference is to not have this patch. This can of course change if
>>>> I see an explanation why it is empty on Arm (I believe it should contain
>>>> csdb) and other arch would want the same.
>>> Except that there's no new asm-generic/ header here (as opposed to how
>>> Oleksii had it). Imo avoiding the need for empty stubs is okay this way,
>>> when introducing an asm-generic/ header would not have been. Of course
>>> if Arm wants to put something there rather sooner than later, then
>>> perhaps the functions better wouldn't be removed from there, just to then
>>> be put back pretty soon.
>> I am confused. I agree the patch is slightly different, but I thought 
>> the fundamental problem was the block_speculation() implementation may 
>> not be safe everywhere. And it was best to let each architecture decide 
>> how they want to implement (vs Xen decide for us the default).
>>
>> Reading the original thread, I thought you had agreed with that 
>> statement. Did I misinterpret?
> Yes and no. Whatever is put in asm-generic/ ought to be correct and safe
> by default, imo. The same doesn't apply to fallbacks put in place in
> headers in xen/: If an arch doesn't provide its own implementation, it
> indicates that the default (fallback) is good enough. Still I can easily
> see that other views are possible here ...

With speculation, there's absolutely nothing we can possibly do in any
common code which will be safe generally.

But we can make it less invasive until an architecture wants to
implement the primitives.

~Andrew

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Julien Grall]

Hi Andrew,

On 04/03/2024 17:07, Andrew Cooper wrote:
> On 04/03/2024 4:55 pm, Jan Beulich wrote:
>> On 04.03.2024 17:46, Julien Grall wrote:
>>> On 04/03/2024 16:41, Jan Beulich wrote:
>>>> On 04.03.2024 17:31, Julien Grall wrote:
>>>>> On 04/03/2024 16:10, Andrew Cooper wrote:
>>>>>> It is daft to require all architectures to provide empty implementations of
>>>>>> this functionality.
>>>>> Oleksii recenlty sent a similar patch [1]. This was pushed back because
>>>>> from naming, it sounds like the helpers ought to be non-empty on every
>>>>> architecture.
>>>>>
>>>>> It would be best if asm-generic provides a safe version of the helpers.
>>>>> So my preference is to not have this patch. This can of course change if
>>>>> I see an explanation why it is empty on Arm (I believe it should contain
>>>>> csdb) and other arch would want the same.
>>>> Except that there's no new asm-generic/ header here (as opposed to how
>>>> Oleksii had it). Imo avoiding the need for empty stubs is okay this way,
>>>> when introducing an asm-generic/ header would not have been. Of course
>>>> if Arm wants to put something there rather sooner than later, then
>>>> perhaps the functions better wouldn't be removed from there, just to then
>>>> be put back pretty soon.
>>> I am confused. I agree the patch is slightly different, but I thought
>>> the fundamental problem was the block_speculation() implementation may
>>> not be safe everywhere. And it was best to let each architecture decide
>>> how they want to implement (vs Xen decide for us the default).
>>>
>>> Reading the original thread, I thought you had agreed with that
>>> statement. Did I misinterpret?
>> Yes and no. Whatever is put in asm-generic/ ought to be correct and safe
>> by default, imo. The same doesn't apply to fallbacks put in place in
>> headers in xen/: If an arch doesn't provide its own implementation, it
>> indicates that the default (fallback) is good enough. Still I can easily
>> see that other views are possible here ...
> 
> With speculation, there's absolutely nothing we can possibly do in any
> common code which will be safe generally.
> 
> But we can make it less invasive until an architecture wants to
> implement the primitives.

I understand the goal. However, I am unsure it is a good idea to provide 
unsafe just to reduce the arch specific header by a few lines. My 
concern is new ports may not realize that block_speculation() needs to 
be implemented. This could end to a preventable XSA in the future.

I guess the risk could be reduced if we had some documentation 
explaining how to port Xen to a new architecture (I am not asking you to 
write the doc).

Anyway, I understand this is a matter of perspective. I will not oppose 
this change if you can get others to agree with the risk.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Andrew Cooper]

On 04/03/2024 5:40 pm, Julien Grall wrote:
> Hi Andrew,
>
> On 04/03/2024 17:07, Andrew Cooper wrote:
>> On 04/03/2024 4:55 pm, Jan Beulich wrote:
>>> On 04.03.2024 17:46, Julien Grall wrote:
>>>> On 04/03/2024 16:41, Jan Beulich wrote:
>>>>> On 04.03.2024 17:31, Julien Grall wrote:
>>>>>> On 04/03/2024 16:10, Andrew Cooper wrote:
>>>>>>> It is daft to require all architectures to provide empty
>>>>>>> implementations of
>>>>>>> this functionality.
>>>>>> Oleksii recenlty sent a similar patch [1]. This was pushed back
>>>>>> because
>>>>>> from naming, it sounds like the helpers ought to be non-empty on
>>>>>> every
>>>>>> architecture.
>>>>>>
>>>>>> It would be best if asm-generic provides a safe version of the
>>>>>> helpers.
>>>>>> So my preference is to not have this patch. This can of course
>>>>>> change if
>>>>>> I see an explanation why it is empty on Arm (I believe it should
>>>>>> contain
>>>>>> csdb) and other arch would want the same.
>>>>> Except that there's no new asm-generic/ header here (as opposed to
>>>>> how
>>>>> Oleksii had it). Imo avoiding the need for empty stubs is okay
>>>>> this way,
>>>>> when introducing an asm-generic/ header would not have been. Of
>>>>> course
>>>>> if Arm wants to put something there rather sooner than later, then
>>>>> perhaps the functions better wouldn't be removed from there, just
>>>>> to then
>>>>> be put back pretty soon.
>>>> I am confused. I agree the patch is slightly different, but I thought
>>>> the fundamental problem was the block_speculation() implementation may
>>>> not be safe everywhere. And it was best to let each architecture
>>>> decide
>>>> how they want to implement (vs Xen decide for us the default).
>>>>
>>>> Reading the original thread, I thought you had agreed with that
>>>> statement. Did I misinterpret?
>>> Yes and no. Whatever is put in asm-generic/ ought to be correct and
>>> safe
>>> by default, imo. The same doesn't apply to fallbacks put in place in
>>> headers in xen/: If an arch doesn't provide its own implementation, it
>>> indicates that the default (fallback) is good enough. Still I can
>>> easily
>>> see that other views are possible here ...
>>
>> With speculation, there's absolutely nothing we can possibly do in any
>> common code which will be safe generally.
>>
>> But we can make it less invasive until an architecture wants to
>> implement the primitives.
>
> I understand the goal. However, I am unsure it is a good idea to
> provide unsafe just to reduce the arch specific header by a few lines.

It doesn't matter if it's unsafe in the arch header or unsafe in the
common code.  It's still unsafe.

There is no change in risk here.  There's simply less blind copy/pasting
of the unsafe form into new architectures in order to get Xen to compile.

~Andrew

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Jan Beulich]

On 04.03.2024 18:40, Julien Grall wrote:
> Hi Andrew,
> 
> On 04/03/2024 17:07, Andrew Cooper wrote:
>> On 04/03/2024 4:55 pm, Jan Beulich wrote:
>>> On 04.03.2024 17:46, Julien Grall wrote:
>>>> On 04/03/2024 16:41, Jan Beulich wrote:
>>>>> On 04.03.2024 17:31, Julien Grall wrote:
>>>>>> On 04/03/2024 16:10, Andrew Cooper wrote:
>>>>>>> It is daft to require all architectures to provide empty implementations of
>>>>>>> this functionality.
>>>>>> Oleksii recenlty sent a similar patch [1]. This was pushed back because
>>>>>> from naming, it sounds like the helpers ought to be non-empty on every
>>>>>> architecture.
>>>>>>
>>>>>> It would be best if asm-generic provides a safe version of the helpers.
>>>>>> So my preference is to not have this patch. This can of course change if
>>>>>> I see an explanation why it is empty on Arm (I believe it should contain
>>>>>> csdb) and other arch would want the same.
>>>>> Except that there's no new asm-generic/ header here (as opposed to how
>>>>> Oleksii had it). Imo avoiding the need for empty stubs is okay this way,
>>>>> when introducing an asm-generic/ header would not have been. Of course
>>>>> if Arm wants to put something there rather sooner than later, then
>>>>> perhaps the functions better wouldn't be removed from there, just to then
>>>>> be put back pretty soon.
>>>> I am confused. I agree the patch is slightly different, but I thought
>>>> the fundamental problem was the block_speculation() implementation may
>>>> not be safe everywhere. And it was best to let each architecture decide
>>>> how they want to implement (vs Xen decide for us the default).
>>>>
>>>> Reading the original thread, I thought you had agreed with that
>>>> statement. Did I misinterpret?
>>> Yes and no. Whatever is put in asm-generic/ ought to be correct and safe
>>> by default, imo. The same doesn't apply to fallbacks put in place in
>>> headers in xen/: If an arch doesn't provide its own implementation, it
>>> indicates that the default (fallback) is good enough. Still I can easily
>>> see that other views are possible here ...
>>
>> With speculation, there's absolutely nothing we can possibly do in any
>> common code which will be safe generally.
>>
>> But we can make it less invasive until an architecture wants to
>> implement the primitives.
> 
> I understand the goal. However, I am unsure it is a good idea to provide 
> unsafe just to reduce the arch specific header by a few lines. My 
> concern is new ports may not realize that block_speculation() needs to 
> be implemented. This could end to a preventable XSA in the future.
> 
> I guess the risk could be reduced if we had some documentation 
> explaining how to port Xen to a new architecture (I am not asking you to 
> write the doc).

But that's precisely the difference I'm trying to point out between having
a stub header in asm-generic/ vs having the fallback in xen/nospec.h: This
way an arch still has to supply asm/nospec.h, and hence they can be
expected to consider what needs putting there and what can be left to the
fallbacks (whether just "for the time being" is a separate question).
Whereas allowing to simply point at the asm-generic/ header is (imo) far
more likely to have only little thought applied ("oh, there is that
generic header, let's just use it").

Yet as said, the line between the two can certainly be viewed as blurred.

Jan

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Jan Beulich]

On 04.03.2024 17:10, Andrew Cooper wrote:
> It is daft to require all architectures to provide empty implementations of
> this functionality.
> 
> Provide evaluate_nospec() and block_speculation() unconditionally in
> xen/nospec.h with architectures able to opt in by providing suitable arch
> variants.
> 
> Rename x86's implementation to the arch_*() variants.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Upon further thinking and with Julien's recent reply in mind:
Acked-by: Jan Beulich <jbeulich@suse.com>

Still I'd prefer if the arch_* were left out. They look to me to go this
half step too far (despite now having looked at patch 2 as well; I'll
reply there separately). And it is this which was why I decided that
going the other half step (moving these handful lines of code) wasn't
really worth it, when considering whether to make such a patch myself.

Jan

Re: [PATCH 2/2] xen/nospec: Allow evaluate_nospec() to short circuit constant expressions

[Jan Beulich]

On 04.03.2024 17:10, Andrew Cooper wrote:
> --- a/xen/include/xen/nospec.h
> +++ b/xen/include/xen/nospec.h
> @@ -18,6 +18,15 @@ static always_inline bool evaluate_nospec(bool cond)
>  #ifndef arch_evaluate_nospec
>  #define arch_evaluate_nospec(cond) cond
>  #endif
> +
> +    /*
> +     * If the compiler can reduce the condition to a constant, then it won't
> +     * be emitting a conditional branch, and there's nothing needing
> +     * protecting.
> +     */
> +    if ( __builtin_constant_p(cond) )
> +        return cond;
> +
>      return arch_evaluate_nospec(cond);
>  }

While for now, even after having some hours for considering, I can't point
out anything concrete that could potentially become a problem here, I
still have the gut feeling that this would better be left in the arch
logic. (There's the oddity of what the function actually expands to if the
#define in context actually takes effect, but that's merely cosmetic.)

The one thing I'm firmly unhappy with is "won't" in the comment: We can't
know what the compiler will do. I've certainly known of compilers which
didn't as you indicate here. That was nothing remotely recent, but
ancient DOS/Windows ones. Still, unlike with e.g. __{get,put}_user_bad()
the compiler doing something unexpected would go entirely silently here.

The other (minor) aspect I'm not entirely happy with is that you insert
between the fallback #define and its use. I think (if we need such a
#define in the first place) the two would better stay close together.

As to the need for the #define: To me

static always_inline bool evaluate_nospec(bool cond)
{
#ifdef arch_evaluate_nospec
    return arch_evaluate_nospec(cond);
#else
    return cond;
#endif
}

or even

static always_inline bool evaluate_nospec(bool cond)
{
#ifdef arch_evaluate_nospec
    return arch_evaluate_nospec(cond);
#endif
    return cond;
}

reads no worse, but perhaps slightly better, and is then consistent with
block_speculation(). At which point the question about "insertion point"
here would hopefully also disappear, as this addition is meaningful only
ahead of the #else.

Jan

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Jan Beulich]

On 04.03.2024 17:10, Andrew Cooper wrote:
> --- a/xen/include/xen/nospec.h
> +++ b/xen/include/xen/nospec.h
> @@ -9,6 +9,29 @@
>  
>  #include <asm/nospec.h>
>  
> +/*
> + * Protect a conditional branch from bad speculation.  Architectures *must*
> + * provide arch_evaluate_nospec() for this to be effective.
> + */
> +static always_inline bool evaluate_nospec(bool cond)
> +{
> +#ifndef arch_evaluate_nospec
> +#define arch_evaluate_nospec(cond) cond

Hmm, noticed only while replying to patch 2: If the #define is to be kept
(see my reply there) it needs to be one of

#define arch_evaluate_nospec(cond) (cond)

or

#define arch_evaluate_nospec

Or it ought to be #undef-ed after use (thus preventing use in a context
where "cond" may expand to other than "cond").

Jan

> +#endif
> +    return arch_evaluate_nospec(cond);
> +}
> +
> +/*
> + * Halt speculation unconditonally.  Architectures *must* provide
> + * arch_block_speculation() for this to be effective.
> + */
> +static always_inline void block_speculation(void)
> +{
> +#ifdef arch_block_speculation
> +    arch_block_speculation();
> +#endif
> +}
> +
>  /**
>   * array_index_mask_nospec() - generate a ~0 mask when index < size, 0 otherwise
>   * @index: array element index

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Oleksii]

On Mon, 2024-03-04 at 17:50 +0000, Andrew Cooper wrote:
> On 04/03/2024 5:40 pm, Julien Grall wrote:
> > Hi Andrew,
> > 
> > On 04/03/2024 17:07, Andrew Cooper wrote:
> > > On 04/03/2024 4:55 pm, Jan Beulich wrote:
> > > > On 04.03.2024 17:46, Julien Grall wrote:
> > > > > On 04/03/2024 16:41, Jan Beulich wrote:
> > > > > > On 04.03.2024 17:31, Julien Grall wrote:
> > > > > > > On 04/03/2024 16:10, Andrew Cooper wrote:
> > > > > > > > It is daft to require all architectures to provide
> > > > > > > > empty
> > > > > > > > implementations of
> > > > > > > > this functionality.
> > > > > > > Oleksii recenlty sent a similar patch [1]. This was
> > > > > > > pushed back
> > > > > > > because
> > > > > > > from naming, it sounds like the helpers ought to be non-
> > > > > > > empty on
> > > > > > > every
> > > > > > > architecture.
> > > > > > > 
> > > > > > > It would be best if asm-generic provides a safe version
> > > > > > > of the
> > > > > > > helpers.
> > > > > > > So my preference is to not have this patch. This can of
> > > > > > > course
> > > > > > > change if
> > > > > > > I see an explanation why it is empty on Arm (I believe it
> > > > > > > should
> > > > > > > contain
> > > > > > > csdb) and other arch would want the same.
> > > > > > Except that there's no new asm-generic/ header here (as
> > > > > > opposed to
> > > > > > how
> > > > > > Oleksii had it). Imo avoiding the need for empty stubs is
> > > > > > okay
> > > > > > this way,
> > > > > > when introducing an asm-generic/ header would not have
> > > > > > been. Of
> > > > > > course
> > > > > > if Arm wants to put something there rather sooner than
> > > > > > later, then
> > > > > > perhaps the functions better wouldn't be removed from
> > > > > > there, just
> > > > > > to then
> > > > > > be put back pretty soon.
> > > > > I am confused. I agree the patch is slightly different, but I
> > > > > thought
> > > > > the fundamental problem was the block_speculation()
> > > > > implementation may
> > > > > not be safe everywhere. And it was best to let each
> > > > > architecture
> > > > > decide
> > > > > how they want to implement (vs Xen decide for us the
> > > > > default).
> > > > > 
> > > > > Reading the original thread, I thought you had agreed with
> > > > > that
> > > > > statement. Did I misinterpret?
> > > > Yes and no. Whatever is put in asm-generic/ ought to be correct
> > > > and
> > > > safe
> > > > by default, imo. The same doesn't apply to fallbacks put in
> > > > place in
> > > > headers in xen/: If an arch doesn't provide its own
> > > > implementation, it
> > > > indicates that the default (fallback) is good enough. Still I
> > > > can
> > > > easily
> > > > see that other views are possible here ...
> > > 
> > > With speculation, there's absolutely nothing we can possibly do
> > > in any
> > > common code which will be safe generally.
> > > 
> > > But we can make it less invasive until an architecture wants to
> > > implement the primitives.
> > 
> > I understand the goal. However, I am unsure it is a good idea to
> > provide unsafe just to reduce the arch specific header by a few
> > lines.
> 
> It doesn't matter if it's unsafe in the arch header or unsafe in the
> common code.  It's still unsafe.
> 
> There is no change in risk here.  There's simply less blind
> copy/pasting
> of the unsafe form into new architectures in order to get Xen to
> compile.
So, we're revisiting this topic again.

I agree that upon examining the current state of the code around these
functions, it appears safe to provide stubs. However, the reason my
patch was rejected is that it may not be entirely safe, as Julien
pointed out that even with Arm, some functions shouldn't be empty.

What I would like to propose is that it might be beneficial, at least
in CONFIG_DEBUG=y, to have a warning message. Does that make sense?

~ Oleksii

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Jan Beulich]

On 05.03.2024 16:15, Oleksii wrote:
> I agree that upon examining the current state of the code around these
> functions, it appears safe to provide stubs. However, the reason my
> patch was rejected is that it may not be entirely safe, as Julien
> pointed out that even with Arm, some functions shouldn't be empty.
> 
> What I would like to propose is that it might be beneficial, at least
> in CONFIG_DEBUG=y, to have a warning message. Does that make sense?

A warning message to what effect? And are you thinking of a build-time
warning, or a runtime one? Plus wouldn't different aspects quickly lead
to proliferation of warnings?

Jan

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Andrew Cooper]

On 05/03/2024 3:43 pm, Jan Beulich wrote:
> On 05.03.2024 16:15, Oleksii wrote:
>> I agree that upon examining the current state of the code around these
>> functions, it appears safe to provide stubs. However, the reason my
>> patch was rejected is that it may not be entirely safe, as Julien
>> pointed out that even with Arm, some functions shouldn't be empty.
>>
>> What I would like to propose is that it might be beneficial, at least
>> in CONFIG_DEBUG=y, to have a warning message. Does that make sense?
> A warning message to what effect? And are you thinking of a build-time
> warning, or a runtime one? Plus wouldn't different aspects quickly lead
> to proliferation of warnings?

Putting in a warning for something like this is specifically a bad idea.

All it will do is cause people to ignore warnings, and that's the very
worst thing Xen could do.

~Andrew

Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation

[Oleksii]

On Tue, 2024-03-05 at 16:43 +0100, Jan Beulich wrote:
> On 05.03.2024 16:15, Oleksii wrote:
> > I agree that upon examining the current state of the code around
> > these
> > functions, it appears safe to provide stubs. However, the reason my
> > patch was rejected is that it may not be entirely safe, as Julien
> > pointed out that even with Arm, some functions shouldn't be empty.
> > 
> > What I would like to propose is that it might be beneficial, at
> > least
> > in CONFIG_DEBUG=y, to have a warning message. Does that make sense?
> 
> A warning message to what effect? And are you thinking of a build-
> time
> warning, or a runtime one? Plus wouldn't different aspects quickly
> lead
> to proliferation of warnings?
A warning message about that an empty definition is provided for
evaluate_nospec/block_speculation functions, so a developer will know
that it can be an issue.

Personally, I am OK with having this function empty by default as it is
done in the patch with opportunity to being redefined in arch specific
code if it is necessary, but I remembered that I had the similar
questions in my patch series which probably should be covered in this
patch series.

Runtime message can also be an option, but I thought about build-time,
but I agree that it can lead to proliferation of warning, so not the
best one idea.

~ Oleksii