Re: [Xen-devel] [PATCH 7/9] x86/amd: Support context switching legacy SSBD interface
>>> On 06.12.18 at 19:55, 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 = _ls_cfg[socket][core];
>>> +
>>> + if (disable) {
>>> + spin_lock(>lock);
>>> +
>>> + /* First sibling to disable updates hardware. */
>>> + if (!cfg->disable_count)
>>> + wrmsrl(MSR_AMD64_LS_CFG, val);
>>> + cfg->disable_count++;
>>> +
>>> + spin_unlock(>lock);
>>> + } else {
>>> + spin_lock(>lock);
>>> +
>>> + /* Last sibling to enable updates hardware. */
>>> + cfg->disable_count--;
>>> + if (!cfg->disable_count)
>>> + wrmsrl(MSR_AMD64_LS_CFG, val);
>>> +
>>> + spin_unlock(>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(>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(>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
Re: [Xen-devel] [PATCH 7/9] x86/amd: Support context switching legacy SSBD interface
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.
These are here because I don't trust really trust the topology logic
(which turned out to be very wise, in retrospect), not because I
actually expect them to trigger from now on.
>
>> +cfg = _ls_cfg[socket][core];
>> +
>> +if (disable) {
>> +spin_lock(>lock);
>> +
>> +/* First sibling to disable updates hardware. */
>> +if (!cfg->disable_count)
>> +wrmsrl(MSR_AMD64_LS_CFG, val);
>> +cfg->disable_count++;
>> +
>> +spin_unlock(>lock);
>> +} else {
>> +spin_lock(>lock);
>> +
>> +/* Last sibling to enable updates hardware. */
>> +cfg->disable_count--;
>> +if (!cfg->disable_count)
>> +wrmsrl(MSR_AMD64_LS_CFG, val);
>> +
>> +spin_unlock(>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.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 7/9] x86/amd: Support context switching legacy SSBD interface
>>> On 03.12.18 at 17:18, 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 = _cpu(ssbd);
> + bool disable = opt_ssbd;
> + struct cpuinfo_x86 *c = _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 = _ls_cfg[socket][core];
> +
> + if (disable) {
> + spin_lock(>lock);
> +
> + /* First sibling to disable updates hardware. */
> + if (!cfg->disable_count)
> + wrmsrl(MSR_AMD64_LS_CFG, val);
> + cfg->disable_count++;
> +
> + spin_unlock(>lock);
> + } else {
> + spin_lock(>lock);
> +
> + /* Last sibling to enable updates hardware. */
> + cfg->disable_count--;
> + if (!cfg->disable_count)
> + wrmsrl(MSR_AMD64_LS_CFG, val);
> +
> + spin_unlock(>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
Re: [Xen-devel] [PATCH 7/9] x86/amd: Support context switching legacy SSBD interface
On Mon, Dec 03, 2018 at 04:18:20PM +, Andy Cooper wrote: > It is critical that MSR_AMD64_LS_CFG is never modified outside of this > function, to avoid trampling on sibling settings. > > For now, pass in NULL from the boot paths and just set Xen's default. Later > patches will plumb in guest choices. This now supercedes the older code which > wrote to MSR_AMD64_LS_CFG once during boot. > > Signed-off-by: Andrew Cooper Reviewed-by: Brian Woods -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
