Date:   Thu, 1 Jun 2023 19:28:47 +0000
From:   Jon Kohler <jon@...anix.com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     Jim Mattson <jmattson@...gle.com>,
        Waiman Long <longman@...hat.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" <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" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH v4] KVM: VMX: do not disable interception for
 MSR_IA32_SPEC_CTRL on eIBRS



> On Jun 1, 2023, at 1:43 PM, Sean Christopherson <seanjc@...gle.com> wrote:
> 
> On Wed, May 31, 2023, Jon Kohler wrote:
>> 
>>> On May 31, 2023, at 2:30 PM, Jim Mattson <jmattson@...gle.com> wrote:
>>> 
>>> On Wed, May 31, 2023 at 11:17 AM Jon Kohler <jon@...anix.com> wrote:
>>>> Yea, I thought about it. One one hand, simplicity is kingand on the other
>>>> hand, not having to think about this again is nice too.
>>>> 
>>>> The challenge in my mind is that on setups where this truly is static, we’d
>>>> be taking some incremental amount of memory to keep the counter around,
> 
> Not really.  The vCPU structures are already order-2 allocations, increasing the
> size by 8-16 bytes doesn't affect the actual memory usage in practice.  Death by
> a thousand cuts is a potential problem, but we're a ways away from crossing back
> over into order-3 allocations.
> 
>>>> just to have the same outcome each time. Doesn’t feel right (to me) unless that is
>>>> also used for “other” stuff as some sort of general purpose/common counter.
> 
> ...
> 
>> Yes, there is places this could be stuffed I’m sure. Still feels a bit heavy
>> handed for the same-outcome-every-time situations though.
> 
> There's no guarantee the outcome will be the same.  You're assuming that (a) the
> guest is eIBRS aware, (b) SPEC_CTRL doesn't get extended for future mitigations,
> and (c) that if L1 is running VMs of its own, that L1 is advertising eIBRS to L2
> and that the L2 kernel is also aware of eIBRS.
> 
>>>> RE Cost: I can’t put my finger on it, but I swear that RDMSR for *this*
>>>> specific MSR is more expensive than any other RDMSR I’ve come across
>>>> for run-of-the-mill random MSRs. I flipped thru the SDM and the mitigations
>>>> documentation, and it only ever mentions that there is a notable cost to
>>>> do WRMSR IA32_SPEC_CTRL, but nothing about the RDMSR side.
>>>> 
>>>> If anyone happens to know from an Intel-internals perspective, I’d be quite
>>>> interested to know why it just “feels” so darn costly. i.e. is the proc also doing
>>>> special things under the covers, similar to what the processor does on
>>>> writes to this one?
>>> 
>>> What do you mean by "feels"? Have you measured it?
>> 
>> There are plenty of rdmsr’s scattered around the entry and exit paths that get
>> hit every time, but this is far and away always the most expensive one when
>> profiling with perf top. I haven’t measured it separately from the existing code,
>> But rather noted during profiling that it appears to be nastier than others. 
>> 
>> I’m more curious than anything else, but it doesn’t matter all that much going
>> forward since this commit will nuke it from orbit for the run of the mill 
>> eIBRS-only use cases.
> 
> As above, you're making multiple assumptions that may or may not hold true.  I
> agree with Jim, reacting to what the guest is actually doing is more robust than
> assuming the guest will do XYZ based on the vCPU model or some other heuristic.
> 
> The code isn't that complex, and KVM can even reuse the number of exits snapshot
> to periodically re-enable the intercept, e.g. to avoid unnecessary RDMSRs if the
> vCPU stops writing MSR_IA32_SPEC_CTRL for whatever reason.
> 
> Needs actual testing and informed magic numbers, but I think this captures the
> gist of what Jim is suggesting.

Thanks, Sean, Jim. I agree that having something robust and lightweight would be
real nice here. Thanks, Sean for the suggested code. I’ll take that, do some
testing, and report back.

> 
> ---
> arch/x86/include/asm/kvm_host.h |  3 +++
> arch/x86/kvm/svm/svm.c          | 22 ++++++++--------------
> arch/x86/kvm/vmx/vmx.c          | 28 ++++++++++------------------
> arch/x86/kvm/x86.h              | 24 ++++++++++++++++++++++++
> 4 files changed, 45 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index fb9d1f2d6136..3fdb6048cd58 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -966,6 +966,9 @@ struct kvm_vcpu_arch {
> 	/* Host CPU on which VM-entry was most recently attempted */
> 	int last_vmentry_cpu;
> 
> +	u32 nr_quick_spec_ctrl_writes;
> +	u64 spec_ctrl_nr_exits_snapshot;
> +
> 	/* AMD MSRC001_0015 Hardware Configuration */
> 	u64 msr_hwcr;
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index ca32389f3c36..f749613204d3 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2959,21 +2959,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> 			svm->vmcb->save.spec_ctrl = data;
> 		else
> 			svm->spec_ctrl = data;
> -		if (!data)
> -			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_svm_vmrun_msrpm.
> -		 * We update the L1 MSR bit as well since it will end up
> -		 * touching the MSR anyway now.
> -		 */
> -		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SPEC_CTRL, 1, 1);
> +		if (!msr->host_initiated &&
> +		    kvm_account_msr_spec_ctrl_write(vcpu))
> +			set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SPEC_CTRL, 1, 1);
> 		break;
> 	case MSR_AMD64_VIRT_SPEC_CTRL:
> 		if (!msr->host_initiated &&
> @@ -4158,6 +4147,11 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
> 
> 	svm_complete_interrupts(vcpu);
> 
> +	if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL) &&
> +	    !spec_ctrl_intercepted &&
> +	    kvm_account_msr_spec_ctrl_passthrough(vcpu))
> +		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SPEC_CTRL, 0, 0);
> +
> 	if (is_guest_mode(vcpu))
> 		return EXIT_FASTPATH_NONE;
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 44fb619803b8..4f4a2c3507bc 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2260,24 +2260,11 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> 			return 1;
> 
> 		vmx->spec_ctrl = data;
> -		if (!data)
> -			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.
> -		 */
> -		vmx_disable_intercept_for_msr(vcpu,
> -					      MSR_IA32_SPEC_CTRL,
> -					      MSR_TYPE_RW);
> +		if (msr_info->host_initiated &&
> +		    kvm_account_msr_spec_ctrl_write(vcpu))
> +			vmx_disable_intercept_for_msr(vcpu, MSR_IA32_SPEC_CTRL,
> +						      MSR_TYPE_RW);
> 		break;
> 	case MSR_IA32_TSX_CTRL:
> 		if (!msr_info->host_initiated &&
> @@ -7192,6 +7179,7 @@ static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
> static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
> {
> 	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	unsigned int run_flags = __vmx_vcpu_run_flags(vmx);
> 	unsigned long cr3, cr4;
> 
> 	/* Record the guest's net vcpu time for enforced NMI injections. */
> @@ -7280,7 +7268,7 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
> 	kvm_wait_lapic_expire(vcpu);
> 
> 	/* The actual VMENTER/EXIT is in the .noinstr.text section. */
> -	vmx_vcpu_enter_exit(vcpu, __vmx_vcpu_run_flags(vmx));
> +	vmx_vcpu_enter_exit(vcpu, run_flags);
> 
> 	/* All fields are clean at this point */
> 	if (kvm_is_using_evmcs()) {
> @@ -7346,6 +7334,10 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
> 	vmx_recover_nmi_blocking(vmx);
> 	vmx_complete_interrupts(vmx);
> 
> +	if ((run_flags & VMX_RUN_SAVE_SPEC_CTRL) &&
> +	    kvm_account_msr_spec_ctrl_passthrough(vcpu))
> +		vmx_enable_intercept_for_msr(vcpu, MSR_IA32_SPEC_CTRL, MSR_TYPE_RW);
> +
> 	if (is_guest_mode(vcpu))
> 		return EXIT_FASTPATH_NONE;
> 
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index c544602d07a3..454bcbf5b543 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -492,7 +492,31 @@ static inline void kvm_machine_check(void)
> 
> void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu);
> void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu);
> +
> int kvm_spec_ctrl_test_value(u64 value);
> +
> +static inline bool kvm_account_msr_spec_ctrl_write(struct kvm_vcpu *vcpu)
> +{
> +	if ((vcpu->stat.exits - vcpu->arch.spec_ctrl_nr_exits_snapshot) < 20)
> +		vcpu->arch.nr_quick_spec_ctrl_writes++;
> +	else
> +		vcpu->arch.nr_quick_spec_ctrl_writes = 0;
> +
> +	vcpu->arch.spec_ctrl_nr_exits_snapshot = vcpu->stat.exits;
> +
> +	return vcpu->arch.nr_quick_spec_ctrl_writes >= 10;
> +}
> +
> +static inline bool kvm_account_msr_spec_ctrl_passthrough(struct kvm_vcpu *vcpu)
> +{
> +	if ((vcpu->stat.exits - vcpu->arch.spec_ctrl_nr_exits_snapshot) < 100000)
> +		return false;
> +
> +	vcpu->arch.spec_ctrl_nr_exits_snapshot = vcpu->stat.exits;
> +	vcpu->arch.nr_quick_spec_ctrl_writes = 0;
> +	return true;
> +}
> +
> bool __kvm_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
> int kvm_handle_memory_failure(struct kvm_vcpu *vcpu, int r,
> 			      struct x86_exception *e);
> 
> base-commit: 39428f6ea9eace95011681628717062ff7f5eb5f
> -- 

Powered by blists - more mailing lists