Re: [PATCH 17/18] KVM: x86: Push acquisition of SRCU in fastpath into kvm_pmu_trigger_event()

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 8/7/2025 1:33 AM, Sean Christopherson wrote:
> 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@google.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()).

Not sure if I understand correctly, but KVM_SET_PMU_EVENT_FILTER ioctl is a
VM-level ioctl and it can be set when vCPUs are running. So assume
KVM_SET_PMU_EVENT_FILTER ioctl is called at vCPU0 and vCPU1 is running
reprogram_counter(). Is it safe without srcu_read_lock()/srcu_read_unlock()
protection?


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

Yeah, indeed. If there is no need to
add srcu_read_lock()/srcu_read_unlock() protection in reprogram_counter(),
I'm good with this. Thanks.


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