[MODERATED] Re: [patch 06/15] Hidden 6

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Sun, May 13, 2018 at 04:00:54PM +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.

I am missing something here. You speak of hardware synchronization which
would mean you would get the same exact behavior as AMD? That is whacking
the MSR would synchronize the state on both siblings? That is if you
enable memory disambiguation on one sibling it would enable it on the other?


has sibling level granularity? So if one sibling is running with SSBD
ON and the other with OFF it has the brains to figure this out? Or would
the brains be to keep it OFF for both siblings?

In which case why the 'On Intel the SSBD control is per core as well' ?

Or.. oh, you are saying it keeps the state latched - so if one has memory
disambiguation disabled then _both_ siblings have it so - even if the other
tries to enable it back on. Gosh, I hope this is spelled out in the SDM
when that comes out.

Perhaps then:
"s/synchronization/synchronization (keep it disabled even if another
sibling enables - only enable it if both siblings set this)/" ?

> 
> 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>
> ---
>  arch/x86/include/asm/spec-ctrl.h |    6 ++
>  arch/x86/kernel/process.c        |  108 ++++++++++++++++++++++++++++++++++++---
>  arch/x86/kernel/smpboot.c        |    5 +
>  3 files changed, 113 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,118 @@ 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
> +
> +static DEFINE_PER_CPU(struct ssb_state, ssb_state);
> +
> +void speculative_store_bypass_ht_init(void)
>  {
> -	u64 msr;
> +	struct ssb_state *st = this_cpu_ptr(&ssb_state);
> +	unsigned int this_cpu = smp_processor_id();
> +	unsigned int cpu;
> +
> +	st->local_state = 0;
> +	if (st->shared_state)
> +		return;
> +
> +	raw_spin_lock_init(&st->lock);

Should we also hold this lock in the CPU hotplug code? That is when
you power off an CPU?
> +
> +	/* Go over HT siblings: */
> +	for_each_cpu(cpu, topology_sibling_cpumask(this_cpu)) {

.. As could you (on a bad of course), access the sibling here - right
when the sibling is powered-off?

> +		if (cpu == this_cpu)
> +			continue;
>  
> -	if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD)) {
> -		msr = x86_amd_ls_cfg_base | ssbd_tif_to_amd_ls_cfg(tifn);
> +		if (!per_cpu(ssb_state, cpu).shared_state)
> +			continue;
> +
> +		/* Link it to the state of the sibling: */
> +		st->shared_state = per_cpu(ssb_state, cpu).shared_state;

And then this would refer to a dead per-cpu area. Do we clear the per-cpu
area when offlining? Aka is the per_cpu(.., cpu) where CPU is offline
end up with a NULL pointer?


> +		return;
> +	}
> +	/* Link shared state of the first HT sibling to itself. */
> +	st->shared_state = st;
> +}
> +
> +/*
> + * Logic is: first HT sibling enables SSBD for both siblings in the core and
> + * last sibling to disable it, disables it for the whole core.

Would it make sense to say this follows how the Intel CPU has it implemented?

> + */