Re: [PATCH 2/2] x86/spec-ctrl: Reduce HVM RSB overhead where possible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2022 19:00, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -44,6 +44,7 @@ ENTRY(vmx_asm_vmexit_handler)
>          .endm
>          ALTERNATIVE "", restore_spec_ctrl, X86_FEATURE_SC_MSR_HVM
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
> +        /* On PBRSB-vulenrable hardware, `ret` not safe before the start of 
> vmx_vmexit_handler() */

Besides the spelling issue mentioned by Jason I think this line also
wants wrapping. Maybe the two comments also want combining to just
one, such that "WARNING!" clearly applies to both parts.

> @@ -515,7 +515,8 @@ static void __init print_details(enum ind_thunk thunk, 
> uint64_t caps)
>              boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM) ||
>              opt_eager_fpu || opt_md_clear_hvm)       ? ""               : " 
> None",
>             boot_cpu_has(X86_FEATURE_SC_MSR_HVM)      ? " MSR_SPEC_CTRL" : "",
> -           boot_cpu_has(X86_FEATURE_SC_RSB_HVM)      ? " RSB"           : "",
> +           boot_cpu_has(X86_FEATURE_SC_RSB_HVM)      ? " RSB"           :
> +           boot_cpu_has(X86_BUG_PBRSB)               ? " PBRSB"         : "",
>             opt_eager_fpu                             ? " EAGER_FPU"     : "",
>             opt_md_clear_hvm                          ? " MD_CLEAR"      : "",
>             boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM)  ? " IBPB-entry"    : 
> "");

Along the lines of half of what fdbf8bdfebc2 ("x86/spec-ctrl:
correct per-guest-type reporting of MD_CLEAR") did, I think you also want
to extend the other (earlier) conditional in this function invocation.

I also wonder whether it wouldn't be helpful to parenthesize the new
construct, such that it'll be more obvious that this is a double
conditional operator determining a single function argument.

> @@ -718,6 +719,77 @@ static bool __init rsb_is_full_width(void)
>      return true;
>  }
>  
> +/*
> + * HVM guests can create arbitrary RSB entries, including ones which point at
> + * Xen supervisor mappings.
> + *
> + * Traditionally, the RSB is not isolated on vmexit, so Xen needs to take
> + * safety precautions to prevent RSB speculation from consuming guest values.
> + *
> + * Intel eIBRS specifies that the RSB is flushed:
> + *   1) on VMExit when IBRS=1, or
> + *   2) shortly thereafter when Xen restores the host IBRS=1 setting.
> + * However, a subset of eIBRS-capable parts also suffer PBRSB and need
> + * software assistance to maintain RSB safety.
> + */
> +static __init enum hvm_rsb {
> +    hvm_rsb_none,
> +    hvm_rsb_pbrsb,
> +    hvm_rsb_stuff32,
> +} hvm_rsb_calculations(uint64_t caps)
> +{
> +    if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
> +         boot_cpu_data.x86 != 6 )
> +        return hvm_rsb_stuff32;
> +
> +    if ( !(caps & ARCH_CAPS_IBRS_ALL) )
> +        return hvm_rsb_stuff32;
> +
> +    if ( caps & ARCH_CAPS_PBRSB_NO )
> +        return hvm_rsb_none;
> +
> +    /*
> +     * We're choosing between the eIBRS-capable models which don't enumerate
> +     * PBRSB_NO.  Earlier steppings of some models don't enumerate eIBRS and
> +     * are excluded above.
> +     */
> +    switch ( boot_cpu_data.x86_model )
> +    {
> +        /*
> +         * Core (inc Hybrid) CPUs to date (August 2022) are vulenrable.
> +         */
> +    case 0x55: /* Skylake X */
> +    case 0x6a: /* Ice Lake SP */
> +    case 0x6c: /* Ice Lake D */
> +    case 0x7e: /* Ice Lake client */
> +    case 0x8a: /* Lakefield (SNC/TMT) */
> +    case 0x8c: /* Tiger Lake U */
> +    case 0x8d: /* Tiger Lake H */
> +    case 0x8e: /* Skylake-L */

Hmm, is SDM Vol 4's initial table wrong then in stating Kaby Lake /
Coffee Lake for this and ...

> +    case 0x97: /* Alder Lake S */
> +    case 0x9a: /* Alder Lake H/P/U */
> +    case 0x9e: /* Skylake */

... this? Otoh I notice that intel-family.h also says Skylake in
respective comments, despite the constants themselves being named
differently. Yet again ...

> +    case 0xa5: /* Comet Lake */
> +    case 0xa6: /* Comet Lake U62 */

... you call these Comet Lake when intel-family.h says Skylake also for
these (and names the latter's variable COMETLAKE_L).

What is in the comments here is of course not of primary concern for
getting this patch in, but the named anomalies will stand out when all
of this is switched over to use intel-family.h's constants.

> @@ -1327,9 +1400,33 @@ void __init init_speculation_mitigations(void)
>       * HVM guests can always poison the RSB to point at Xen supervisor
>       * mappings.
>       */
> +    hvm_rsb = hvm_rsb_calculations(caps);
> +    if ( opt_rsb_hvm == -1 )
> +        opt_rsb_hvm = hvm_rsb != hvm_rsb_none;
> +
>      if ( opt_rsb_hvm )
>      {
> -        setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);
> +        switch ( hvm_rsb )
> +        {
> +        case hvm_rsb_pbrsb:
> +            setup_force_cpu_cap(X86_BUG_PBRSB);
> +            break;
> +
> +        case hvm_rsb_none:
> +            /*
> +             * Somewhat arbitrary.  If something is wrong and the user has
> +             * forced HVM RSB protections on a system where we think nothing
> +             * is necessary, they they possibly know something we dont.
> +             *
> +             * Use stuff32 in this case, which is the most protection we can
> +             * muster.
> +             */
> +            fallthrough;
> +
> +        case hvm_rsb_stuff32:
> +            setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);
> +            break;
> +        }
>  
>          /*
>           * For SVM, Xen's RSB safety actions are performed before STGI, so

For people using e.g. "spec-ctrl=no-ibrs" but leaving RSB stuffing enabled
(or force-enabling it) we'd need to have an LFENCE somewhere as well. Since
putting one in the RSB stuffing macro would require a runtime conditional
(for its use for alternatives patching), can't we leverage the one
controlled by this logic? That'll be a slight abuse of the name of
X86_BUG_PBRSB, but probably acceptable with a suitable comment. In the end
it'll simply be another fall-through, I guess:

        switch ( hvm_rsb )
        {
        case hvm_rsb_none:
            /* ... */
            fallthrough;

        case hvm_rsb_stuff32:
            setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);

            /* ... */
            if ( boot_cpu_has(X86_FEATURE_SC_MSR_HVM) )
                break;
            fallthrough;
        case hvm_rsb_pbrsb:
            setup_force_cpu_cap(X86_BUG_PBRSB);
            break;
        }

That way, aiui, it also wouldn't get in the way of print_details(), which
checks X86_FEATURE_SC_RSB_HVM first.

Otoh I can see reasons why for the stuffing the LFENCE would really need
to live inside the macro, and in particular ahead of the RET there. Since
we don't want to have a runtime conditional there, I guess we should then,
as a fallback, extend the text in the command line doc to warn about the
inter-dependency. After all people "knowing what they are doing" doesn't
imply them knowing implementation details of Xen. But then I'd still be
a little concerned of the "they possibly know something we don't" case.

Jan