Date: Wed, 16 May 2018 21:28:57 -0400
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: [MODERATED] Re: [patch 06/15] SSB updates V17 6
Message-ID: <20180517012856.GE10272@char.us.oracle.com>
References: <20180516135132.687640705@linutronix.de>
 <20180516135209.810720423@linutronix.de>
MIME-Version: 1.0
In-Reply-To: <20180516135209.810720423@linutronix.de>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
To: speck@linutronix.de

On Wed, May 16, 2018 at 03:51:38PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch 06/15] x86/speculation: Handle HT correctly on AMD
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The AMD64_LS_CFG MSR is a per core MSR on Family 17H CPUs. That means when
> hyperthreading is enabled the SSBD bit toggle needs to take both cores into
> account. Otherwise the following situation can happen:
> 
> CPU0		CPU1
> 
> disable SSB
> 		disable SSB
> 		enable  SSB <- Enables it for the Core, i.e. for CPU0 as well
> 
> So after the SSB enable on CPU1 the task on CPU0 runs with SSB enabled
> again.
> 
> On Intel the SSBD control is per core as well, but the synchronization
> logic is implemented behind the per thread SPEC_CTRL MSR. It works like
> this:
> 
>   CORE_SPEC_CTRL = THREAD0_SPEC_CTRL | THREAD1_SPEC_CTRL
> 
> i.e. if one of the threads enables a mitigation then this affects both and
> the mitigation is only disabled in the core when both threads disabled it.
> 
> Add the necessary synchronization logic for AMD family 17H. Unfortunately
> that requires a spinlock to serialize the access to the MSR, but the locks
> are only shared between siblings.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Borislav Petkov <bp@suse.de>
> ---
> V2: Amended changelog and clarified code comments.
> ---
>  arch/x86/include/asm/spec-ctrl.h |    6 +
>  arch/x86/kernel/process.c        |  125 +++++++++++++++++++++++++++++++++++++--
>  arch/x86/kernel/smpboot.c        |    5 +
>  3 files changed, 130 insertions(+), 6 deletions(-)
> 
> --- a/arch/x86/include/asm/spec-ctrl.h
> +++ b/arch/x86/include/asm/spec-ctrl.h
> @@ -33,6 +33,12 @@ static inline u64 ssbd_tif_to_amd_ls_cfg
>  	return (tifn & _TIF_SSBD) ? x86_amd_ls_cfg_ssbd_mask : 0ULL;
>  }
>  
> +#ifdef CONFIG_SMP
> +extern void speculative_store_bypass_ht_init(void);
> +#else
> +static inline void speculative_store_bypass_ht_init(void) { }
> +#endif
> +
>  extern void speculative_store_bypass_update(void);
>  
>  #endif
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -279,22 +279,135 @@ static inline void switch_to_bitmap(stru
>  	}
>  }
>  
> -static __always_inline void __speculative_store_bypass_update(unsigned long tifn)
> +#ifdef CONFIG_SMP
> +
> +struct ssb_state {
> +	struct ssb_state	*shared_state;
> +	raw_spinlock_t		lock;
> +	unsigned int		disable_state;
> +	unsigned long		local_state;
> +};
> +
> +#define LSTATE_SSB	0
                     ^^^
I don't know why, but this tab is throwing me off.

Feel free to ignore it, but in case you want to change it would
it be better if it was a space?

Either way:

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!