Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 25 May 2022 17:04:25 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Jon Kohler <jon@nutanix.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Waiman Long <longman@redhat.com>,
        Kees Cook <keescook@chromium.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] KVM: VMX: do not disable interception for
 MSR_IA32_SPEC_CTRL on eIBRS
Message-ID: <Yo5hmcdRvE1UrI4y@google.com>
References: <20220520204115.67580-1-jon@nutanix.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220520204115.67580-1-jon@nutanix.com>
Precedence: bulk

On Fri, May 20, 2022, Jon Kohler wrote:
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 610355b9ccce..1c725d17d984 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2057,20 +2057,32 @@ 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, unless the
> +		 * guest is hosted on an eIBRS system and setting only

The "unless guest is hosted on an eIBRS system" blurb is wrong and doesn't match
the code.  Again, it's all about whether eIBRS is advertised to the guest.  With
some other minor tweaking to wrangle the comment to 80 chars...

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

> +		 * SPEC_CTRL_IBRS, which is typically set once at boot and never

Uber nit, when it makes sense, avoid regurgitating the code verbatim, e.g. refer
to "setting SPEC_CTRL_IBRS" as "enabling IBRS".  That little bit of abstraction
can sometimes help unfamiliar readers understand the effect of the code, whereas
copy+pasting bits of the code doesn't provide any additional context.

> +		 * touched again.  All other bits are often set on a per-task
> +		 * basis, i.e. may 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)))

Align the two halves of the logical-OR, i.e.

		if (!data ||
		    (data == SPEC_CTRL_IBRS &&
		     (vcpu->arch.arch_capabilities & ARCH_CAP_IBRS_ALL)))
			break;