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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Michal Orzel" <michal.orzel@amd.com>,
	"Oleksii Kurochko" <oleksii.kurochko@gmail.com>,
	"Shawn Anastasio" <sanastasio@raptorengineering.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 1/2] xen/*/nospec: Provide common versions of evaluate_nospec/block_speculation
Date: Mon, 4 Mar 2024 16:46:18 +0000	[thread overview]
Message-ID: <0c8cfbcf-d69b-499d-a915-df44713093ef@citrix.com> (raw)
In-Reply-To: <5220c8cf-bbc2-4f18-94ca-f1f30dd828c9@suse.com>

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