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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
