Re: [PATCH v5 07/13] KVM: x86/pmu: Disable counters based on Host-Only/Guest-Only bits in SVM

[Yosry Ahmed <yosry@kernel.org>]

> > Did you see my other replies and code snippet tracking disabling
> > reasons? I think the code snippet would still work, I just need to
> > move the pmc_is_nested_disabled() check into pmc_is_locally_enabled().
> 
> I did.  IMO, all of what you proposed is an optimization to avoid the "costly"
> checks at the time of pmc_is_locally_enabled().  In quotes because I don't think
> the _overall_ cost is actually all that high.  pmc_is_locally_enabled() is only
> called in relatively slow paths, and my guess is the CALL+RET (or untrained RET,
> ugh) is probably more expensive than the logic itself.
> 
> The very nice side effect of incorporating the logic into pmc_is_locally_enabled()
> is that I _think_ we can drop kvm_pmu_ops.reprogram_counters(), because
> amd_mediated_pmu_handle_host_guest_bits() will instead be:
> 
>   static bool amd_pmc_is_locally_disabled(struct kvm_pmc *pmc)
>   {
> 	struct kvm_pmu *pmu = pmc_to_pmu(pmc);
> 	struct kvm_vcpu *vcpu = pmu_to_vcpu(pmu);
> 	u64 host_guest_bits;
> 
> 	/* Common code is supposed to check the common enable bit. */
> 	if (WARN_ON_ONCE(!(pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE)))
> 		return false;
> 
> 	/*
> 	 * If both bits are cleared, always keep the counter enabled. Otherwise,
> 	 * counter enablement needs to be re-evaluated on every nested
> 	 * transition (and EFER.SVME change).
> 	 */
> 	host_guest_bits = pmc->eventsel & AMD64_EVENTSEL_HOST_GUEST_MASK;
> 	if (!host_guest_bits)
> 		return true;
> 
> 	/* If either bit is set and EFER.SVME=0, the counter is disabled. */
> 	if (!(vcpu->arch.efer & EFER_SVME))
> 		return false;
> 
> 	if (host_guest_bits == AMD64_EVENTSEL_HOST_GUEST_MASK)
> 		return true;
> 
> 	return !!(host_guest_bits & AMD64_EVENTSEL_GUESTONLY) == is_guest_mode(vcpu);
>   }

If we do this and drop kvm_pmu_ops.reprogram_counters(), we still need
somewhere to actually clear ARCH_PERFMON_EVENTSEL_ENABLE in eventsel_hw.

What if we call kvm_pmu_ops.pmc_is_locally_disabled() at the top of
reprogram_counter(), cache the result, and use that for eventsel_hw
modification and in pmc_is_locally_enabled()?

We'd also probably want to rename it. I would honeslty just use 'nested'
instead of 'locally_disabled' and 'mode_specific_enables' as that's the
only current user.

Something like this with your proposed amd_pmc_is_locally_disabled()
above, which is similar to the kvm_pmu_ops.mediated_reprogram_counter()
implementation in v4 except that the vendor-specific callback is more
targeted:

static void pmc_check_nested_disabled(struct kvm_pmc *pmc)
{
	if (!(pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE))
		return;
	
	if (!test_bit(pmc->idx, &pmu->pmc_has_nested_enables))
		return;

	pmc->is_nested_disabled = kvm_pmu_call(pmc_is_nested_disabled)(pmc);
	if (!pmc->is_nested_disabled)
		pmc->eventsel_hw &= ~ARCH_PERFMON_EVENTSEL_ENABLE;
}

static int reprogram_counter(struct kvm_pmc *pmc)
{
	...

	if (kvm_vcpu_has_mediated_pmu(pmu_to_vcpu(pmu))) {
		kvm_mediated_pmu_refresh_event_filter(pmc);
		pmc_check_nested_disabled(pmc);
		return 0;
	}
	...
}

Then pmc_is_locally_enabled() also checks pmc->is_nested_disabled. This
completely avoids the extra calls in pmc_is_locally_enabled() and
provides a place for us to actually clear ARCH_PERFMON_EVENTSEL_ENABLE.

> 
> reprogram_pmcs_on_nested_transitions would need to be handled somewhere else, but
> (a) that's probably the correct approach anyways (hook writes to the eventsel)
> and (b) is _also_ an optimization, because KVM can start with the naive approach
> of always reprogramming counters on nested transitions (if guest/host bits are
> supported).
> 
> And if we're clever, we can optimize pmc_is_locally_enabled() by putting
> reprogram_pmcs_on_nested_transitions in kvm_pmu, e.g. as something like
> pmc_has_mode_specific_enables, and then doing:

Yeah I more-or-less incorporated this above. pmu->pmc_has_nested_enables
would be set for a counter in amd_pmu_set_msr() if either bits is set.
Then, svm_pmu_handle_nested_transition() (in the next patch) will use
pmu->pmc_has_nested_enables instead of
svm->nested.reprogram_pmcs_on_nested_transitions.

WDYT? This is essentially the v4 approach except that the per-counter
callback now returns a boolean that we cache and reuse in
pmc_is_locally_enabled(). We have come full circle with the per-counter
callback vs. single callback for all :)

Also, would you rather I send a new version with everything, or do you
want to pick up some of the patches in this version independently?

> 
> 	if (!(pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE))
> 		return false;
> 
> 	if (!test_bit(pmc->idx, &pmu->pmc_has_mode_specific_enables))
> 		return true;
> 
> 	return !kvm_pmu_call(pmc_is_locally_disabled(pmc));
>