Re: [patch 06/15] Hidden 6

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, 15 May 2018, speck for Konrad Rzeszutek Wilk wrote:
> 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.

Yes, the logic here is:

  CORE_SPEC_CTRL = THREAD0_SPEC_CRTL | THREAD1_SPEC_CTRL

The documentation for IBRS is a bit vague on that:

  Enabling IBRS on one logical processor of a core with Intel
  Hyper-Threading Technology may affect branch prediction on other logical
  processors of the same core.

but as Intel folks confirmed on several occasions the above logic is
reality.

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

Will rephrase.

> > +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?

No. It's set up once and stays so.

> > +		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?

Sure. Will add.

Thanks,

	tglx