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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Tue, Aug 9, 2022 at 1:01 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> The documentation for eIBRS has finally been clarified to state that it is
> intended to flush the RSB on VMExit.  So in principle, we shouldn't have been
> using opt_rsb_hvm on eIBRS hardware.
>
> However, dropping the 32 entry RSB stuff makes us vulnerable to Post-Barrier
> RSB speculation on affected Intel CPUs.
>
> Introduce hvm_rsb_calculations() which selects between a 32-entry stuff, a
> PBRSB specific workaround, or nothing, based on hardware details.
>
> To mitigate PBRSB, put an LFENCE at the top of vmx_vmexit_handler().  This
> forces the necessary safety property, without having to do a 1-entry RSB stuff
> and fix up the stack(s) afterwards.
>
> Update opt_rsb_hvm to be tristate.  On eIBRS-capable CPUs not susceptible to
> PBRSB, this disables HVM RSB software protections entirely.  On eIBRS-capable
> CPUs suceptible to to PBRSB, this reduces a 32-entry RSB stuff down to just
> one LFENCE.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> ---
>  xen/arch/x86/hvm/vmx/entry.S           |   1 +
>  xen/arch/x86/hvm/vmx/vmx.c             |  20 ++++++-
>  xen/arch/x86/include/asm/cpufeatures.h |   1 +
>  xen/arch/x86/spec_ctrl.c               | 103 
> ++++++++++++++++++++++++++++++++-
>  4 files changed, 120 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
> index 5f5de45a1309..222495aed19f 100644
> --- 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() */

vulnerable

>
>          /* Hardware clears MSR_DEBUGCTL on VMExit.  Reinstate it if 
> debugging Xen. */
>          .macro restore_lbr
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 17e103188a53..8a6a5cf20525 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3934,8 +3934,24 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>  {
>      unsigned long exit_qualification, exit_reason, idtv_info, intr_info = 0;
>      unsigned int vector = 0, mode;
> -    struct vcpu *v = current;
> -    struct domain *currd = v->domain;
> +    struct vcpu *v;
> +    struct domain *currd;
> +
> +    /*
> +     * To mitigate Post-Barrier RSB speculation, we must force one CALL
> +     * instruction to retire before letting a RET instruction execute.

I think it would be clearer if this comment mentioned LFENCE like the
commit message does.  Looking at this change without the commit
message the connection is not obvious to me at least.  Maybe "we must
force one CALL instruction to retire (with LFENCE) before letting a
RET instruction execute"?

> +     *
> +     * On PBRSB-vulnerable CPUs, it is not safe for a RET to be executed
> +     * before this point.
> +     *
> +     * Defer any non-trivial variable initialisation to avoid problems if the
> +     * compiler decides to out-of-line any helpers.  This depends on
> +     * alternative() being a full compiler barrier too.
> +     */
> +    alternative("", "lfence", X86_BUG_PBRSB);
> +
> +    v = current;
> +    currd = v->domain;
>
>      __vmread(GUEST_RIP,    &regs->rip);
>      __vmread(GUEST_RSP,    &regs->rsp);


> +    /*
> +     * 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.

vulnerable

> +        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.

"then they" and "don't"

Regards,
Jason