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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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