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

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

>>> On 03.12.18 at 17:18, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -442,6 +442,74 @@ static struct ssbd_ls_cfg {
>  } *ssbd_ls_cfg[4];
>  static unsigned int ssbd_max_cores;
>  
> +/*
> + * Must only be called when the LEGACY_SSBD is in used.  Called with NULL to

... when LEGACY_SSBD is in use ... ?

> + * switch back to Xen's default value.
> + */
> +void amd_ctxt_switch_legacy_ssbd(const struct vcpu *next)
> +{
> +	static DEFINE_PER_CPU(bool, ssbd);
> +	bool *this_ssbd = &this_cpu(ssbd);
> +	bool disable = opt_ssbd;
> +	struct cpuinfo_x86 *c = &current_cpu_data;

const

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

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

Jan



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