[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] x86/nospec: Fix evaluate_nospec() code generation under Clang


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 26 Apr 2022 09:18:48 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=2VA7SvFpQMmBLN71emlYqmPV1v/s2k3j7xy4Z5BhQiY=; b=JC0GIdcZk/lpgh3XKHECTeptf9hVF5UMaKAr5gl0fYUT8M/CvWnsv+0bWjTvmonCzFLjdJeU/VLANrap8uG2ZNj7U0JEJSW7wgWdTTigkXwStCDHnJRP3nLh64hwTwdaQSE6L0ybxV6e5AXZEPcDu2nQCDWwSDfHNASeJ3Y8xvYYAMVIX6+rrdXRdoe0fnvrZes2XMlEeHlAzkdlh7kQG4gtQso0YFnHzYrYRJ2GnVr6zk7BeB3LQ2IYUHZeCOma8fgzaK3TM1lKIu09aAW439rrMdIXdtYvVcP7U4XmUw7O9sqF6IRSyv7HOC6AConsM878SmjNznWbwM7cneG9hw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ar+Nelhxf1YXlIeqkjTmja1CoxuTHljBPQFGlersq13MsCLEpcnbvj3OId+qhQ1PPmKIZ5neDg8FsmRW0gwo7brFzUkVXzkv/ZSou/pmLkxV9sBgDlXMdEUJ7zShzUttosDjHOzPQ1NJfCFpQ0Uk0rUmRKDH4pmmPqVVjkqxKz7O32NkO6zxAyq3WYCo97MEvIu4fDbqBm3A8dONP0AJGwWpsMbeGTmNvU0WxwfCuAetO11KGaBgMfanCthfTSoqzThcWMz9NCSz+GsTA+7nB2NuMZrOc30/N3EWabbMOt5rC5PWr72nPf+vjzZPw9wlxkydOGdLSpveZrWVay0ZJQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Tue, 26 Apr 2022 07:19:05 +0000
  • Ironport-data: A9a23:KBqmCaKUZKR/06OgFE+RpZQlxSXFcZb7ZxGr2PjKsXjdYENS02ZSm GcbDWzXaKvYNDbzetEjbY/lo0wAvJHTnNRmS1BlqX01Q3x08seUXt7xwmUcns+xwm8vaGo9s q3yv/GZdJhcokf0/0vrav67xZVF/fngqoDUUYYoAQgsA148IMsdoUg7wbRh3tQ22YHR7z6l4 rseneWOYDdJ5BYsWo4kw/rrRMRH5amaVJsw5zTSVNgT1LPsvyB94KE3fMldG0DQUIhMdtNWc s6YpF2PEsE1yD92Yj+tuu6TnkTn2dc+NyDW4pZdc/DKbhSvOkXee0v0XRYRQR4/ttmHozx+4 PBRrbWOQw13BbSSw8M4QTlHHQBBEKITrdcrIVDn2SCS52vvViK1ht9IXAQxN4Be/ftrC2ZT8 /BeMCoKch2Im+OxxvS8V/VogcMgasLsOevzuFk5lW2fUalgHMCFGvqRjTNb9G5YasRmB/HRa tBfcTNyRB/BfwdOKhEcD5dWcOKA2CiuL2YD8A79SawfpGjL5w0s4OTXMuXnX9jaVfpOoVScu TeTl4j+KlRAXDCF8hKV/3TpiuLRkCfTXIMJCKb+5vNsmEeUxGEYFFsRT1TTiduTh1O6WtlfA 1cJ4Sdopq83nGS0SvHtUhv+p2SL1iPwQPJVGuw+rQuLmqzd5l/AAnBeF2EaLts7qMUxWDomk EeTmM/kDiBut7vTTm+B8rCTrnW5Pi19wXI+WBLohDAtu7HLyLzfRDqWJjq/OMZZVuHIJAw=
  • Ironport-hdrordr: A9a23:/kifKq4PjsxZfLjBwwPXwVqBI+orL9Y04lQ7vn2ZFiY5TiXIra qTdaogviMc6Ax/ZJjvo6HkBEClewKlyXcT2/hrAV7CZniehILMFu1fBOTZowEIdxeOldK1kJ 0QCZSWa+eAcmSS7/yKhzVQeuxIqLfnzEnrv5a5854Ed3AXV0gK1XYcNu/0KDwVeOEQbqBJaa Z0q/A37gaISDAyVICWF3MFV+/Mq5nik4/nWwcPA1oC5BOVhT2lxbbmG1zAty1uGA9n8PMHyy zoggb57qKsv7WSzQLd7Xba69BzlMH6wtVOKcSQgow+KynqiCyveIN9Mofy9AwdkaWK0hIHgd PMqxAvM4Ba7G7QRHi8pV/X1wzpwF8Vmgvf4G7dpUGmjd3yRTo8BcYEr5leaAHl500pu8w5+L 5X3kqC3qAnQi/orWDY3ZzlRhtqnk27rT4JiugIlUFSVoMYdft4sZEfxkVIC50NdRiKpLzPKN MeTf002cwmMW9zNxvizypSKZ2XLzkO9y69MwY/Upf/6UkVoJh7p3FosfD30E1wsa7VcKM0lt gsAp4Y6o2mcfVmHZ6VJN1xNvdfWVa9Ny4lDgqpUCfaPZBCHU7xgLjKx5hwzN2WWfUzvekPcd L6IRlliVI=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Mon, Apr 25, 2022 at 06:56:03PM +0100, Andrew Cooper wrote:
> It turns out that evaluate_nospec() code generation is not safe under Clang.
> Given:
> 
>   void eval_nospec_test(int x)
>   {
>       if ( evaluate_nospec(x) )
>           asm volatile ("nop #true" ::: "memory");
>       else
>           asm volatile ("nop #false" ::: "memory");
>   }
> 
> Clang emits:
> 
>   <eval_nospec_test>:
>          0f ae e8                lfence
>          85 ff                   test   %edi,%edi
>          74 02                   je     <eval_nospec_test+0x9>
>          90                      nop
>          c3                      ret
>          90                      nop
>          c3                      ret
> 
> which is not safe because the lfence has been hoisted above the conditional
> jump.  Clang concludes that both barrier_nospec_true()'s have identical side
> effects and can safely be merged.
> 
> Clang can be persuaded that the side effects are different if there are
> different comments in the asm blocks.  This is fragile, but no more fragile
> that other aspects of this construct.
> 
> Introduce barrier_nospec_false() with a separate internal comment to prevent
> Clang merging it with barrier_nospec_true() despite the otherwise-identical
> content.  The generated code now becomes:
> 
>   <eval_nospec_test>:
>          85 ff                   test   %edi,%edi
>          74 05                   je     <eval_nospec_test+0x9>
>          0f ae e8                lfence
>          90                      nop
>          c3                      ret
>          0f ae e8                lfence
>          90                      nop
>          c3                      ret
> 
> which has the correct number of lfence's, and in the correct place.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Like Jan I wonder what the clang devs think of this solution.  Is
there any test in clang to assert that comments won't be stripped from
asm blocks before optimization?

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> ---
>  xen/arch/x86/include/asm/nospec.h | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/nospec.h 
> b/xen/arch/x86/include/asm/nospec.h
> index 5312ae4c6f31..7150e76b87fb 100644
> --- a/xen/arch/x86/include/asm/nospec.h
> +++ b/xen/arch/x86/include/asm/nospec.h
> @@ -10,15 +10,26 @@
>  static always_inline bool barrier_nospec_true(void)
>  {
>  #ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH
> -    alternative("lfence", "", X86_FEATURE_SC_NO_BRANCH_HARDEN);
> +    alternative("lfence #nospec-true", "", X86_FEATURE_SC_NO_BRANCH_HARDEN);
>  #endif
>      return true;
>  }
>  
> +static always_inline bool barrier_nospec_false(void)
> +{
> +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH
> +    alternative("lfence #nospec-false", "", X86_FEATURE_SC_NO_BRANCH_HARDEN);
> +#endif
> +    return false;
> +}
> +
>  /* Allow to protect evaluation of conditionals with respect to speculation */
>  static always_inline bool evaluate_nospec(bool condition)
>  {
> -    return condition ? barrier_nospec_true() : !barrier_nospec_true();
> +    if ( condition )
> +        return barrier_nospec_true();
> +    else
> +        return barrier_nospec_false();
>  }

Is the switch from using a ternary operator also a requirement for
clang not optimizing this? (I would assume not, but better ask)

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.