Message-ID: <aJOR4Bk3DwKSVdQV@google.com>
Date: Wed, 6 Aug 2025 10:33:20 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Dapeng Mi <dapeng1.mi@...ux.intel.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Xin Li <xin@...or.com>, Sandipan Das <sandipan.das@....com>
Subject: Re: [PATCH 17/18] KVM: x86: Push acquisition of SRCU in fastpath into kvm_pmu_trigger_event()

On Wed, Aug 06, 2025, Dapeng Mi wrote:
> 
> On 8/6/2025 3:05 AM, Sean Christopherson wrote:
> > Acquire SRCU in the VM-Exit fastpath if and only if KVM needs to check the
> > PMU event filter, to further trim the amount of code that is executed with
> > SRCU protection in the fastpath.  Counter-intuitively, holding SRCU can do
> > more harm than good due to masking potential bugs, and introducing a new
> > SRCU-protected asset to code reachable via kvm_skip_emulated_instruction()
> > would be quite notable, i.e. definitely worth auditing.
> >
> > E.g. the primary user of kvm->srcu is KVM's memslots, accessing memslots
> > all but guarantees guest memory may be accessed, accessing guest memory
> > can fault, and page faults might sleep, which isn't allowed while IRQs are
> > disabled.  Not acquiring SRCU means the (hypothetical) illegal sleep would
> > be flagged when running with PROVE_RCU=y, even if DEBUG_ATOMIC_SLEEP=n.
> >
> > Note, performance is NOT a motivating factor, as SRCU lock/unlock only
> > adds ~15 cycles of latency to fastpath VM-Exits.  I.e. overhead isn't a
> > concern _if_ SRCU protection needs to be extended beyond PMU events, e.g.
> > to honor userspace MSR filters.
> >
> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > ---

...

> > @@ -968,12 +968,14 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu,
> >  			     (unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX))
> >  		return;
> >  
> > +	idx = srcu_read_lock(&vcpu->kvm->srcu);
> 
> It looks the asset what "kvm->srcu" protects here is
> kvm->arch.pmu_event_filter which is only read by pmc_is_event_allowed().
> Besides here, pmc_is_event_allowed() is called by reprogram_counter() but
> without srcu_read_lock()/srcu_read_unlock() protection.

No, reprogram_counter() is only called called in the context of KVM_RUN, i.e. with
the vCPU loaded and thus with kvm->srcu already head for read (acquired by
kvm_arch_vcpu_ioctl_run()).
 
> So should we shrink the protection range further and move the
> srcu_read_lock()/srcu_read_unlock() pair into pmc_is_event_allowed()
> helper? The side effect is it would bring some extra overhead since
> srcu_read_lock()/srcu_read_unlock() could be called multiple times.

No, I don't think it's worth getting that precise.  As you note, there will be
extra overhead, and it could actually become non-trivial amount of overhead,
albeit in a somewhat pathological scenario.  And cpl_is_matched() is easy to
audit, i.e. is very low risk with respect to having "bad" behavior that's hidden
by virtue of holding SRCU.

E.g. if the guest is using all general purpose PMCs to count instructions
retired, then KVM would acquire/release SRCU 8+ times.  On Intel, the fastpath
can run in <800 cycles.  Adding 8 * 2 full memory barriers (difficult to measure,
but somewhere in the neighborhood of ~10 cycles per barrier) would increase the
latency by 10-20%.

Again, that's an extreme scenario, but since there's almost nothing to gain from
pushing SRCU acquisition into the filter checks, I don't see any reason to go
with an approach that we *know* is sub-optimal.

> An alternative could be to add srcu_read_lock()/srcu_read_unlock() around
> pmc_is_event_allowed() in reprogram_counter() helper as well.

As above, there's no need to modify reprogram_counter().  I don't see any future
where reprogram_counter() would be safe to call in the fastpath, there's simply
too much going on, i.e. I think reprogram_counter() will always be a non-issue.

Powered by blists - more mailing lists