Re: [PATCH v4 3/6] KVM: x86/pmu: Disable counters based on Host-Only/Guest-Only bits in SVM

[Yosry Ahmed]

> > I think leave this callback as-is and handle everything through
> > reprogram_counter(). Export reprogram_counter() and rename it to
> > kvm_pmu_reprogram_counter(), and end up with something like this:
> >
> > void __kvm_pmu_handle_nested_transition(struct kvm_vcpu *vcpu, bool defer)
> > {
> >       struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> >       DECLARE_BITMAP(bitmap, X86_PMC_IDX_MAX);
> >
> >       if (bitmap_empty(pmu->reprogram_on_nested_transition, X86_PMC_IDX_MAX))
> >               return;
> >
> >       bitmap_copy(bitmap, pmu->reprogram_pmi, X86_PMC_IDX_MAX);
> >       bitmap_zero(pmu->reprogram_on_nested_transition, X86_PMC_IDX_MAX);
> >
> >       BUILD_BUG_ON(sizeof(pmu->reprogram_on_nested_transition) != sizeof(atomic64_t));
> >       if (defer) {
> >               atomic64_or(*(s64 *)pmu->reprogram_on_nested_transition,
> >                           &vcpu_to_pmu(vcpu)->__reprogram_pmi);
> >               kvm_make_request(KVM_REQ_PMU, vcpu);
> >               return;
> >       }
> >
> >       kvm_for_each_pmc(pmu, pmc, bit, bitmap)
> >               kvm_pmu_reprogram_counter(pmc);
> > }
> >
> > void kvm_pmu_handle_nested_transition(struct kvm_vcpu *vcpu)
> > {
> >       __kvm_pmu_handle_nested_transition(vcpu, false);
> > }
> >
> > Actually, if that's desired, we can move this logic into SVM code now.
> > We won't be calling kvm_pmu_handle_nested_transition() from inside
> > enter_guest_mode() and leave_guest_mode() anyway so that we can only
> > defer for the svm_leave_nested() path.
> >
> > So we can move:
> > - kvm_pmu_handle_nested_transition() to
> > svm_pmu_handle_nested_transition()
> > - pmu->reprogram_on_nested_transition to
> > svm->nested.reprogram_on_nested_transition
> >
> > Not sure if we want to keep SVM-specific logic in SVM code, or if we
> > want to keep code generic as much as possible. I can see good arguments
> > for both stances.
>
> We can have our cake and eat it too.  Add svm_pmu_handle_nested_transition(),
> but then also rename and rework reprogram_counters() to support both deferred and
> synchronous operation, e.g. something like so:
>
> ---
> static inline void __kvm_pmu_reprogram_counters(struct kvm_pmu *pmu, u64 diff,
>                                                 bool defer)
> {
>         struct kvm_vcpu *vcpu = pmu_to_vcpu(pmu);
>
>         lockdep_assert_once(defer || kvm_get_running_vcpu() == vcpu);
>
>         if (!diff)
>                 return;
>
>         atomic64_or(diff, &pmu->__reprogram_pmi);
>
>         if (defer)
>                 kvm_make_request(KVM_REQ_PMU, vcpu);
>         else
>                 kvm_pmu_handle_event(pmu_to_vcpu(pmu));
> }

I like that the KVM PMU code is now presenting a generic API to
reprogram counters rather than handling nested transitions, even
though reprogram_on_nested_transition fits better semantically in
kvm_pmu (than svm_nested_state).

I do have a few questions:

1. Do we want to do all of the work in kvm_pmu_handle_event() on every
nested transition (rather than just reprogram counters)? Genuinely
asking as I am not sure if the rest of it is significant.

2. This approach will reprogram all counters that need it on nested
transitions. In my proposed approach above, I only iterate over
counters in reprogram_on_nested_transition and reprogram them. Do you
think it matters? I guess if other counters need reprogramming we'll
probably do it in kvm_pmu_handle_event() before running the vCPU
anyway, but then we're repeating the work here?

3. In this world we still keep the mediated_reprogram_counter() callback, right?

>
> static inline void kvm_pmu_reprogram_counters(struct kvm_pmu *pmu, u64 diff)
> {
>         __kvm_pmu_reprogram_counters(pmu, diff, true);
> }
> ---
>
> and then have SVM code pass in the reprogram_on_nested_transition or whatever.