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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.12.18 at 17:18, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel