Re: [Xen-devel] [PATCH v6.5 26/26] x86/idle: Clear SPEC_CTRL while idle

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 01:15, <andrew.cooper3@xxxxxxxxxx> wrote:
> On contemporary hardware, setting IBRS/STIBP has a performance impact on
> adjacent hyperthreads.  It is therefore recommended to clear the setting
> before becoming idle, to avoid an idle core preventing adjacent userspace
> execution from running at full performance.
> 
> Care must be taken to ensure there are no ret or indirect branch instructions
> between spec_ctrl_{enter,exit}_idle() invocations, which are forced always
> inline.  Care must also be taken to avoid using spec_ctrl_enter_idle() between
> flushing caches and becoming idle, in cases where that matters.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one remark:

> @@ -31,6 +33,38 @@ static inline void init_shadow_spec_ctrl_state(void)
>      info->shadow_spec_ctrl = info->use_shadow_spec_ctrl = 0;
>  }
>  
> +/* WARNING! `ret`, `call *`, `jmp *` not safe after this call. */
> +static always_inline void spec_ctrl_enter_idle(struct cpu_info *info)
> +{
> +    uint32_t val = 0;
> +
> +    /*
> +     * Latch the new shadow value, then enable shadowing, then update the 
> MSR.
> +     * There are no SMP issues here; only local processor ordering concerns.
> +     */
> +    info->shadow_spec_ctrl = val;
> +    barrier();
> +    info->use_shadow_spec_ctrl = true;
> +    barrier();
> +    asm volatile (ALTERNATIVE(ASM_NOP3, "wrmsr", X86_FEATURE_XEN_IBRS_SET)
> +                  :: "a" (val), "c" (MSR_SPEC_CTRL), "d" (0) : "memory");
> +}
> +
> +/* WARNING! `ret`, `call *`, `jmp *` not safe before this call. */
> +static always_inline void spec_ctrl_exit_idle(struct cpu_info *info)
> +{
> +    uint32_t val = SPEC_CTRL_IBRS;
> +
> +    /*
> +     * Disable shadowing before updating the MSR.  There are no SMP issues
> +     * here; only local processor ordering concerns.
> +     */
> +    info->use_shadow_spec_ctrl = false;
> +    barrier();
> +    asm volatile (ALTERNATIVE(ASM_NOP3, "wrmsr", X86_FEATURE_XEN_IBRS_SET)
> +                  :: "a" (val), "c" (MSR_SPEC_CTRL), "d" (0) : "memory");
> +}

The comments ahead of these new functions imply that we rely on
the compiler inlining all function calls inside the critical section - I
don't think there's any such guarantee, but I also don't see what
we can do about it. We may want to change some "inline" to
"always_inline" though (we've already worked out that this would
appear to only affect inb() and inl() in io.h; the R-b stands with
that addition).

Jan



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