Re: [PATCH 7/9] x86/amd: Support context switching legacy SSBD interface

["Jan Beulich" <JBeulich@suse.com>]

>>> On 06.12.18 at 19:55, <andrew.cooper3@citrix.com> wrote:
> On 06/12/2018 10:51, Jan Beulich wrote:
>>
>>> +	unsigned int socket = c->phys_proc_id, core = c->cpu_core_id;
>>> +	struct ssbd_ls_cfg *cfg;
>>> +	uint64_t val;
>>> +
>>> +	ASSERT(cpu_has_legacy_ssbd);
>>> +
>>> +	/*
>>> +	 * Update hardware lazily, as these MSRs are expensive.  However, on
>>> +	 * the boot paths which pass NULL, force a write to set a consistent
>>> +	 * initial state.
>>> +	 */
>>> +	if (*this_ssbd == disable && next)
>>> +		return;
>>> +
>>> +	if (cpu_has_virt_sc_ssbd) {
>>> +		wrmsrl(MSR_VIRT_SPEC_CTRL,
>>> +		       disable ? SPEC_CTRL_SSBD : 0);
>>> +		goto done;
>>> +	}
>>> +
>>> +	val = ls_cfg_base | (disable ? ls_cfg_ssbd_mask : 0);
>>> +
>>> +	if (c->x86 < 0x17 || c->x86_num_siblings == 1) {
>>> +		/* No threads to be concerned with. */
>>> +		wrmsrl(MSR_AMD64_LS_CFG, val);
>>> +		goto done;
>>> +	}
>>> +
>>> +	/* Check that we won't overflow the worse-case allocation. */
>>> +	BUG_ON(socket >= ARRAY_SIZE(ssbd_ls_cfg));
>>> +	BUG_ON(core   >= ssbd_max_cores);
>> Wouldn't it be better to fail onlining of such CPUs?
> 
> How?  We've not currently got an ability to fail in the middle of
> start_secondary(), which is why the previous patch really does go an
> allocate the worst case.

smp_callin() very clearly has failure paths, and that's being
called out of start_secondary(). If you look there you'll notice
that it wasn't all that long ago that we've added a second
failure path here besides the HVM enabling one (which has been
there virtually forever).

>>> +	cfg = &ssbd_ls_cfg[socket][core];
>>> +
>>> +	if (disable) {
>>> +		spin_lock(&cfg->lock);
>>> +
>>> +		/* First sibling to disable updates hardware. */
>>> +		if (!cfg->disable_count)
>>> +			wrmsrl(MSR_AMD64_LS_CFG, val);
>>> +		cfg->disable_count++;
>>> +
>>> +		spin_unlock(&cfg->lock);
>>> +	} else {
>>> +		spin_lock(&cfg->lock);
>>> +
>>> +		/* Last sibling to enable updates hardware. */
>>> +		cfg->disable_count--;
>>> +		if (!cfg->disable_count)
>>> +			wrmsrl(MSR_AMD64_LS_CFG, val);
>>> +
>>> +		spin_unlock(&cfg->lock);
>>> +	}
>> Any reason for duplicating the spin_{,un}lock() calls?
> 
> To avoid having a context-dependent jump in the critical region.  Then
> again, I suppose that is completely dwarfed by the WRMSR.

If you're afraid of extra branches, how about

	spin_lock(&cfg->lock);

	cfg->disable_count -= !disable;

	/* First sibling to disable and last sibling to enable updates hardware. */
	if (!cfg->disable_count)
		wrmsrl(MSR_AMD64_LS_CFG, val);

	cfg->disable_count += disable;

	spin_unlock(&cfg->lock);

(which I'd very much hope the compiler carries out with just
the single unavoidable branch in the middle)?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel