On 2015/12/8 23:43, Marc Zyngier wrote:
> On 08/12/15 12:47, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.z...@linaro.org>
>> +/**
>> + * kvm_pmu_get_counter_value - get PMU counter value
>> + * @vcpu: The vcpu pointer
>> + * @select_idx: The counter index
>> + */
>> +u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u32 select_idx)
>> +{
>> + u64 counter, enabled, running;
>> + struct kvm_pmu *pmu = &vcpu->arch.pmu;
>> + struct kvm_pmc *pmc = &pmu->pmc[select_idx];
>> +
>> + if (!vcpu_mode_is_32bit(vcpu))
>> + counter = vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + select_idx);
>> + else
>> + counter = vcpu_cp15(vcpu, c14_PMEVCNTR0 + select_idx);
>> +
>> + if (pmc->perf_event)
>> + counter += perf_event_read_value(pmc->perf_event, &enabled,
>> + &running);
>> +
>> + return counter & pmc->bitmask;
>
> This one confused me for a while. Is it the case that you return
> whatever is in the vcpu view of the counter, plus anything that perf
> itself has counted? If so, I'd appreciate a comment here...
>
Yes, the real counter value is the current counter value plus the value
perf event counts. I'll add a comment.
>> +}
>> +
>> +static bool kvm_pmu_counter_is_enabled(struct kvm_vcpu *vcpu, u32
>> select_idx)
>> +{
>> + if (!vcpu_mode_is_32bit(vcpu))
>> + return (vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMCR_E) &
>> + (vcpu_sys_reg(vcpu, PMCNTENSET_EL0) >> select_idx);
>
> This looks wrong. Shouldn't it be:
>
> return ((vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMCR_E) &&
> (vcpu_sys_reg(vcpu, PMCNTENSET_EL0) & (1 << select_idx)));
>
>> + else
>> + return (vcpu_sys_reg(vcpu, c9_PMCR) & ARMV8_PMCR_E) &
>> + (vcpu_sys_reg(vcpu, c9_PMCNTENSET) >> select_idx);
>> +}
>
> Also, I don't really see why we need to check the 32bit version, which
> has the exact same content.
>
>> +
>> +static inline struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
>> +{
>> + struct kvm_pmu *pmu;
>> + struct kvm_vcpu_arch *vcpu_arch;
>> +
>> + pmc -= pmc->idx;
>> + pmu = container_of(pmc, struct kvm_pmu, pmc[0]);
>> + vcpu_arch = container_of(pmu, struct kvm_vcpu_arch, pmu);
>> + return container_of(vcpu_arch, struct kvm_vcpu, arch);
>> +}
>> +
>> +/**
>> + * kvm_pmu_stop_counter - stop PMU counter
>> + * @pmc: The PMU counter pointer
>> + *
>> + * If this counter has been configured to monitor some event, release it
>> here.
>> + */
>> +static void kvm_pmu_stop_counter(struct kvm_pmc *pmc)
>> +{
>> + struct kvm_vcpu *vcpu = kvm_pmc_to_vcpu(pmc);
>> + u64 counter;
>> +
>> + if (pmc->perf_event) {
>> + counter = kvm_pmu_get_counter_value(vcpu, pmc->idx);
>> + if (!vcpu_mode_is_32bit(vcpu))
>> + vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + pmc->idx) = counter;
>> + else
>> + vcpu_cp15(vcpu, c14_PMEVCNTR0 + pmc->idx) = counter;
>
> Same thing - we don't need to make a difference between 32 and 64bit.
>
So it's fine to drop all the vcpu_mode_is_32bit(vcpu) check of this
series? The only one we should take care is the PMCCNTR, right?
>> +
>> + perf_event_release_kernel(pmc->perf_event);
>> + pmc->perf_event = NULL;
>> + }
>> +}
>> +
>> +/**
>> + * kvm_pmu_set_counter_event_type - set selected counter to monitor some
>> event
>> + * @vcpu: The vcpu pointer
>> + * @data: The data guest writes to PMXEVTYPER_EL0
>> + * @select_idx: The number of selected counter
>> + *
>> + * When OS accesses PMXEVTYPER_EL0, that means it wants to set a PMC to
>> count an
>> + * event with given hardware event number. Here we call perf_event API to
>> + * emulate this action and create a kernel perf event for it.
>> + */
>> +void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u32 data,
>> + u32 select_idx)
>> +{
>> + struct kvm_pmu *pmu = &vcpu->arch.pmu;
>> + struct kvm_pmc *pmc = &pmu->pmc[select_idx];
>> + struct perf_event *event;
>> + struct perf_event_attr attr;
>> + u32 eventsel;
>> + u64 counter;
>> +
>> + kvm_pmu_stop_counter(pmc);
>
> Wait. I didn't realize this before, but you have the vcpu right here.
> Why don't you pass it as a parameter to kvm_pmu_stop_counter and avoid
> the kvm_pmc_to_vcpu thing altogether?
>
Yeah, we could pass vcpu as a parameter for this function. But the
kvm_pmc_to_vcpu helper is also used in kvm_pmu_perf_overflow() and
within kvm_pmu_perf_overflow it needs the pmc->idx, we couldn't pass
vcpu as a parameter, so this helper is necessary for kvm_pmu_perf_overflow.
Thanks,
--
Shannon
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
