Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Wed, 29 Nov 2023 10:25:29 -0800
In-Reply-To: <bcbc9d0f-8b52-468b-8c69-0e09ec168a39@gmail.com>
Mime-Version: 1.0
References: <20231123075818.12521-1-likexu@tencent.com> <ZWVCwvoETD_NVOFG@google.com>
 <bcbc9d0f-8b52-468b-8c69-0e09ec168a39@gmail.com>
Message-ID: <ZWeB_nfLPmyRnbAs@google.com>
Subject: Re: [PATCH] KVM: x86: Use get_cpl directly in case of vcpu_load to
 improve accuracy
From:   Sean Christopherson <seanjc@google.com>
To:     Like Xu <like.xu.linux@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Wed, Nov 29, 2023, Like Xu wrote:
> > Rather than fudge around that ugliness with a kvm_get_running_vcpu() check, what
> > if we instead repurpose kvm_arch_dy_has_pending_interrupt(), which is effectively
> > x86 specific, to deal with not being able to read the current CPL for a vCPU that
> > is (possibly) not "loaded", which AFAICT is also x86 specific (or rather, Intel/VMX
> > specific).
> 
> I'd break it into two parts, the first step applying this simpler, more
> straightforward fix (which is backport friendly compared to the diff below),
> and the second step applying your insight for more decoupling and cleanup.
> 
> You'd prefer one move to fix it, right ?

Yeah, I'll apply your patch first, though if you don't object I'd like to reword
the shortlog+changelog to make it explicitly clear that this is a correctness fix,
that the preemption case really needs to have a separate API, and that checking
for vcpu->preempted isn't safe.

I've applied it to kvm-x86/fixes with the below changelog, holler if you want to
change anything.

[1/1] KVM: x86: Get CPL directly when checking if loaded vCPU is in kernel mode
      https://github.com/kvm-x86/linux/commit/8eedf4177184

    KVM: x86: Get CPL directly when checking if loaded vCPU is in kernel mode
    
    When querying whether or not a vCPU "is" running in kernel mode, directly
    get the CPL if the vCPU is the currently loaded vCPU.  In scenarios where
    a guest is profiled via perf-kvm, querying vcpu->arch.preempted_in_kernel
    from kvm_guest_state() is wrong the vCPU is actively running, i.e. hasn't
    been preempted and so preempted_in_kernel is stale.
    
    This affects perf/core's ability to accurately tag guest RIP with
    PERF_RECORD_MISC_GUEST_{KERNEL|USER} and record it in the sample.  This
    causes perf/tool to fail to connect the vCPU RIPs to the guest kernel
    space symbols when parsing these samples due to incorrect PERF_RECORD_MISC
    flags:
    
       Before (perf-report of a cpu-cycles sample):
          1.23%  :58945   [unknown]         [u] 0xffffffff818012e0
    
       After:
          1.35%  :60703   [kernel.vmlinux]  [g] asm_exc_page_fault
    
    Note, checking preempted_in_kernel in kvm_arch_vcpu_in_kernel() is awful
    as nothing in the API's suggests that it's safe to use if and only if the
    vCPU was preempted.  That can be cleaned up in the future, for now just
    fix the glaring correctness bug.
    
    Note #2, checking vcpu->preempted is NOT safe, as getting the CPL on VMX
    requires VMREAD, i.e. is correct if and only if the vCPU is loaded.  If
    the target vCPU *was* preempted, then it can be scheduled back in after
    the check on vcpu->preempted in kvm_vcpu_on_spin(), i.e. KVM could end up
    trying to do VMREAD on a VMCS that isn't loaded on the current pCPU.
    
    Signed-off-by: Like Xu <likexu@tencent.com>
    Fixes: e1bfc24577cc ("KVM: Move x86's perf guest info callbacks to generic KVM")
    Link: https://lore.kernel.org/r/20231123075818.12521-1-likexu@tencent.com
    [sean: massage changelong, add Fixes]
    Signed-off-by: Sean Christopherson <seanjc@google.com>

> > And if getting the CPL for a vCPU that may not be loaded is problematic for other
> > architectures, then I think the correct fix is to move preempted_in_kernel into
> > common code and check it directly in kvm_vcpu_on_spin().
> 
> Not sure which tests would cover this part of the change.

It'd likely require a human to look at results, i.e. as you did.

> > +bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
> > +{
> > +	/*
> > +	 * Treat the vCPU as being in-kernel if it has a pending interrupt, as
> > +	 * the vCPU trying to yield may be spinning on IPI delivery, i.e. the
> > +	 * the target vCPU is in-kernel for the purposes of directed yield.
> 
> How about the case "vcpu->arch.guest_state_protected == true" ?

Ah, right, the existing code considers vCPUs to always be in-kernel for preemption
checks.

> > +	return vcpu->arch.preempted_in_kernel ||
> > +	       kvm_dy_has_pending_interrupt(vcpu);
> >   }
> >   bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu)
> > @@ -13043,7 +13051,7 @@ bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu)
> >   		 kvm_test_request(KVM_REQ_EVENT, vcpu))
> >   		return true;
> > -	return kvm_arch_dy_has_pending_interrupt(vcpu);
> > +	return kvm_dy_has_pending_interrupt(vcpu);
> >   }
> >   bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
> > @@ -13051,7 +13059,7 @@ bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
> >   	if (vcpu->arch.guest_state_protected)
> >   		return true;
> > -	return vcpu->arch.preempted_in_kernel;
> > +	return static_call(kvm_x86_get_cpl)(vcpu);
> 
> We need "return static_call(kvm_x86_get_cpl)(vcpu) == 0;" here.

Doh, I had fixed this locally but forgot to refresh the copy+paste with the updated
diff.

> > -bool __weak kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
> > +bool __weak kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
> >   {
> > -	return false;
> > +	return kvm_arch_vcpu_in_kernel(vcpu);
> >   }
> >   void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
> > @@ -4086,8 +4086,7 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
> >   			if (kvm_vcpu_is_blocking(vcpu) && !vcpu_dy_runnable(vcpu))
> >   				continue;
> >   			if (READ_ONCE(vcpu->preempted) && yield_to_kernel_mode &&
> > -			    !kvm_arch_dy_has_pending_interrupt(vcpu) &&
> > -			    !kvm_arch_vcpu_in_kernel(vcpu))
> > +			    kvm_arch_vcpu_preempted_in_kernel(vcpu))
> 
> Use !kvm_arch_vcpu_preempted_in_kernel(vcpu) ?

Double doh.  Yeah, this is inverted.