Message-ID: <9d37ab07-97c6-5245-6939-9c1090b4b3a9@redhat.com>
Date:   Wed, 31 May 2023 13:02:29 -0400
From:   Waiman Long <longman@...hat.com>
To:     Jon Kohler <jon@...anix.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Andrea Arcangeli <aarcange@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH v4] KVM: VMX: do not disable interception for
 MSR_IA32_SPEC_CTRL on eIBRS

On 5/31/23 10:41, Jon Kohler wrote:
> Avoid expensive rdmsr on every VM Exit for MSR_IA32_SPEC_CTRL on
> eIBRS enabled systems iff the guest only sets IA32_SPEC_CTRL[0] (IBRS)
> and not [1] (STIBP) or [2] (SSBD) by not disabling interception in
> the MSR bitmap. Note: this logic is only for eIBRS, as Intel's guidance
> has long been that eIBRS only needs to be set once, so most guests with
> eIBRS awareness should behave nicely. We would not want to accidentally
> regress misbehaving guests on pre-eIBRS systems, who might be spamming
> IBRS MSR without the hypervisor being able to see it today.
>
> eIBRS enabled guests using just IBRS will only write SPEC_CTRL MSR
> once or twice per vCPU on boot, so it is far better to take those
> VM exits on boot than having to read and save this msr on every
> single VM exit forever. This outcome was suggested on Andrea's commit
> 2f46993d83ff ("x86: change default to spec_store_bypass_disable=prctl spectre_v2_user=prctl")
> however, since interception is still unilaterally disabled, the rdmsr
> tax is still there even after that commit.
>
> This is a significant win for eIBRS enabled systems as this rdmsr
> accounts for roughly ~50% of time for vmx_vcpu_run() as observed
> by perf top disassembly, and is in the critical path for all
> VM-Exits, including fastpath exits.
>
> Opportunistically update comments for both MSR_IA32_SPEC_CTRL and
> MSR_IA32_PRED_CMD to make it clear how L1 vs L2 handling works.
>
> Fixes: 2f46993d83ff ("x86: change default to spec_store_bypass_disable=prctl spectre_v2_user=prctl")
> Signed-off-by: Jon Kohler <jon@...anix.com>
> Cc: Sean Christopherson <seanjc@...gle.com>
> Cc: Andrea Arcangeli <aarcange@...hat.com>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Josh Poimboeuf <jpoimboe@...hat.com>
> Cc: Waiman Long <longman@...hat.com>
> ---
> v1
>   - https://lore.kernel.org/all/20220512174427.3608-1-jon@nutanix.com/
> v1 -> v2:
>   - https://lore.kernel.org/all/20220520195303.58692-1-jon@nutanix.com/
>   - Addressed comments on approach from Sean.
> v2 -> v3:
>   - https://lore.kernel.org/kvm/20220520204115.67580-1-jon@nutanix.com/
>   - Addressed comments on approach from Sean.
> v3 -> v4:
>   - Fixed inline code comments from Sean.
>
>   arch/x86/kvm/vmx/vmx.c | 35 ++++++++++++++++++++++++-----------
>   1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 44fb619803b8..5e643ac897bc 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2260,20 +2260,33 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   			return 1;
>
>   		vmx->spec_ctrl = data;
> -		if (!data)
> +
> +		/*
> +		 * Disable interception on the first non-zero write, except if
> +		 * eIBRS is advertised to the guest and the guest is enabling
> +		 * _only_ IBRS.  On eIBRS systems, kernels typically set IBRS
> +		 * once at boot and never touch it post-boot.  All other bits,
> +		 * and IBRS on non-eIBRS systems, are often set on a per-task
> +		 * basis, i.e. change frequently, so the benefit of avoiding
> +		 * VM-exits during guest context switches outweighs the cost of
> +		 * RDMSR on every VM-Exit to save the guest's value.
> +		 */
> +		if (!data ||
> +		    (data == SPEC_CTRL_IBRS &&
> +		     (vcpu->arch.arch_capabilities & ARCH_CAP_IBRS_ALL)))
>   			break;
>
>   		/*
> -		 * For non-nested:
> -		 * When it's written (to non-zero) for the first time, pass
> -		 * it through.
> -		 *
> -		 * For nested:
> -		 * The handling of the MSR bitmap for L2 guests is done in
> -		 * nested_vmx_prepare_msr_bitmap. We should not touch the
> -		 * vmcs02.msr_bitmap here since it gets completely overwritten
> -		 * in the merging. We update the vmcs01 here for L1 as well
> -		 * since it will end up touching the MSR anyway now.
> +		 * Update vmcs01.msr_bitmap even if L2 is active, i.e. disable
> +		 * interception for the vCPU on the first write regardless of
> +		 * whether the WRMSR came from L1 or L2.  vmcs02's bitmap is a
> +		 * combination of vmcs01 and vmcs12 bitmaps, and will be
> +		 * recomputed by nested_vmx_prepare_msr_bitmap() on the next
> +		 * nested VM-Enter.  Note, this does mean that future WRMSRs
> +		 * from L2 will be intercepted until the next nested VM-Exit if
> +		 * L2 was the first to write, but L1 exposing the MSR to L2
> +		 * without first writing it is unlikely and not worth the
> +		 * extra bit of complexity.
>   		 */
>   		vmx_disable_intercept_for_msr(vcpu,
>   					      MSR_IA32_SPEC_CTRL,

I have 2 comments.

1) Besides the IBRS, STIBP & SSBD bits, the SPEC_CTRL MSR may have the 
RRSBA_DIS_S bit set in the future. I am not aware of any current Intel 
processors having this capability yet, but a future Intel processor may 
have this and the above patch will have to be modified accordingly. It 
looks like that the RRSBA_DIS_S bit will be set once.

2) AMD has their own AutoIBRS capability in Genoa which is similar to 
eIBRS but is not identified by the ARCH_CAP_IBRS_ALL bit. Instead it is 
identified by the AUTOIBRS bit in MSR_EFER. Are you planning to support 
that?

Cheers,
Longman

Powered by blists - more mailing lists