[PATCH v6 12/21] KVM: ARM64: Add reset and access handlers for PMCNTENSET and PMCNTENCLR register

[zhaoshenglong@huawei.com (Shannon Zhao)]

On 2015/12/9 0:42, Marc Zyngier wrote:
>> +void kvm_pmu_enable_counter(struct kvm_vcpu *vcpu, u32 val, bool all_enable)
>> > +{
>> > +	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.

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

Thanks,
-- 
Shannon