Re: [PATCH 1/4] KVM: x86/pmu: Force reprogramming of all counters on PMU filter change

[Like Xu <like.xu.linux@gmail.com>]

On 14/10/2022 4:53 am, Sean Christopherson wrote:
> On Thu, Oct 13, 2022, Like Xu wrote:
>> Firstly, thanks for your comments that spewed out around vpmu.
>>
>> On 23/9/2022 8:13 am, Sean Christopherson wrote:
>>> Force vCPUs to reprogram all counters on a PMU filter change to provide
>>> a sane ABI for userspace.  Use the existing KVM_REQ_PMU to do the
>>> programming, and take advantage of the fact that the reprogram_pmi bitmap
>>> fits in a u64 to set all bits in a single atomic update.  Note, setting
>>> the bitmap and making the request needs to be done _after_ the SRCU
>>> synchronization to ensure that vCPUs will reprogram using the new filter.
>>>
>>> KVM's current "lazy" approach is confusing and non-deterministic.  It's
>>
>> The resolute lazy approach was introduced in patch 03, right after this change.
> 
> This is referring to the lazy recognition of the filter, not the deferred
> reprogramming of the counters.  Regardless of whether reprogramming is handled
> via request or in-line, KVM is still lazily recognizing the new filter as vCPUs
> won't picke up the new filter until the _guest_ triggers a refresh.

It may still be too late for the pmu filter to take effect. To eliminate this 
"non-deterministic",
should we kick out all vpmu-enabled vcpus right after making KVM_REQ_PMU requests ?

> 
>>> @@ -613,9 +615,18 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
>>>    	mutex_lock(&kvm->lock);
>>>    	filter = rcu_replace_pointer(kvm->arch.pmu_event_filter, filter,
>>>    				     mutex_is_locked(&kvm->lock));
>>> -	mutex_unlock(&kvm->lock);
>>> -
>>>    	synchronize_srcu_expedited(&kvm->srcu);
>>
>> The relative order of these two operations has been reversed
>> 	mutex_unlock() and synchronize_srcu_expedited()
>> , extending the execution window of the critical area of "kvm->lock)".
>> The motivation is also not explicitly stated in the commit message.
> 
> I'll add a blurb, after I re-convince myself that the sync+request needs to be
> done under kvm->lock.
> 
>>> +	BUILD_BUG_ON(sizeof(((struct kvm_pmu *)0)->reprogram_pmi) >
>>> +		     sizeof(((struct kvm_pmu *)0)->__reprogram_pmi));
>>> +
>>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>>> +		atomic64_set(&vcpu_to_pmu(vcpu)->__reprogram_pmi, -1ull);
>>
>> How about:
>> 	bitmap_copy(pmu->reprogram_pmi, pmu->all_valid_pmc_idx, X86_PMC_IDX_MAX);
>> to avoid further cycles on calls of
>> "static_call(kvm_x86_pmu_pmc_idx_to_pmc)(pmu, bit)" ?
> 
> bitmap_copy() was my first choice too, but unfortunately it's doesn't guarantee
> atomicity and could lead to data corruption if the target vCPU is concurrently
> modifying the bitmap.

Indeed, it may help to reuse "pmu->global_ctrl_mask" instead of "-1ull":

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 4504987cbbe2..8e279f816e27 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -621,7 +621,8 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void 
__user *argp)
  		     sizeof(((struct kvm_pmu *)0)->__reprogram_pmi));

  	kvm_for_each_vcpu(i, vcpu, kvm)
-		atomic64_set(&vcpu_to_pmu(vcpu)->__reprogram_pmi, -1ull);
+		atomic64_set(&vcpu_to_pmu(vcpu)->__reprogram_pmi,
+			     pmu->global_ctrl_mask);

  	kvm_make_all_cpus_request(kvm, KVM_REQ_PMU);

diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index b68956299fa8..a946c1c57e1d 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -185,6 +185,7 @@ static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
  	pmu->nr_arch_fixed_counters = 0;
  	pmu->global_status = 0;
  	bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
+	pmu->global_ctrl_mask = ~((1ull << pmu->nr_arch_gp_counters) - 1);
  }

  static void amd_pmu_init(struct kvm_vcpu *vcpu)
-- 
2.38.0