On Wed, 9 Dec 2015 16:35:58 +0800
Shannon Zhao [off-list ref] wrote:

On 2015/12/9 0:42, Marc Zyngier wrote:

quoted

quoted

+void kvm_pmu_enable_counter(struct kvm_vcpu *vcpu, u32 val, bool all_enable)

quoted

+{
+	int i;
+	struct kvm_pmu *pmu = &vcpu->arch.pmu;
+	struct kvm_pmc *pmc;
+
+	if (!all_enable)
+		return;

You have the vcpu. Can you move the check for PMCR_EL0.E here instead of
having it in both of the callers?

But it still needs to check PMCR_EL0.E in kvm_pmu_handle_pmcr(). When
PMCR_EL0.E == 1, it calls kvm_pmu_enable_counter(), otherwise it calls
kvm_pmu_disable_counter(). So as it checks already, just pass the result
as a parameter.

I've seen that, but it makes the code look ugly. At any rate, you might
as well not call enable_counter if PMCR.E==0. But splitting the lookup
of the bit and the test like you do is not nice at all. Making it
self-contained looks a lot better, and you don't have to think about
the caller.

quoted

quoted

quoted

+
+	for_each_set_bit(i, (const unsigned long *)&val, ARMV8_MAX_COUNTERS) {

Nonononono... If you must have to use a long, use a long. Don't cast it
to a different type (hint: big endian).

quoted

quoted

+		pmc = &pmu->pmc[i];
+		if (pmc->perf_event) {
+			perf_event_enable(pmc->perf_event);
+			if (pmc->perf_event->state != PERF_EVENT_STATE_ACTIVE)
+				kvm_debug("fail to enable perf event\n");
+		}
+	}
+}
+
+/**
+ * kvm_pmu_disable_counter - disable selected PMU counter
+ * @vcpu: The vcpu pointer
+ * @val: the value guest writes to PMCNTENCLR register
+ *
+ * Call perf_event_disable to stop counting the perf event
+ */
+void kvm_pmu_disable_counter(struct kvm_vcpu *vcpu, u32 val)
+{
+	int i;
+	struct kvm_pmu *pmu = &vcpu->arch.pmu;
+	struct kvm_pmc *pmc;
+

Why are enable and disable asymmetric (handling of PMCR.E)?

To enable a counter, it needs both the PMCR_EL0.E and the corresponding
bit of PMCNTENSET_EL0 set to 1. But to disable a counter, it only needs
one of them and when PMCR_EL0.E == 0? it disables all the counters.

OK.

	M.
-- 
Jazz is not dead. It just smells funny.