Re: [RFC 2/3] KVM: arm64: pmu: Fix chained SW_INCR counters

[Auger Eric]

Hi Marc,

On 12/5/19 10:43 AM, Marc Zyngier wrote:
> Hi Eric,
> 
> On 2019-12-04 20:44, Eric Auger wrote:
>> At the moment a SW_INCR counter always overflows on 32-bit
>> boundary, independently on whether the n+1th counter is
>> programmed as CHAIN.
>>
>> Check whether the SW_INCR counter is a 64b counter and if so,
>> implement the 64b logic.
>>
>> Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU counters")
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>> ---
>> Âvirt/kvm/arm/pmu.c | 16 +++++++++++++++-
>> Â1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>> index c3f8b059881e..7ab477db2f75 100644
>> --- a/virt/kvm/arm/pmu.c
>> +++ b/virt/kvm/arm/pmu.c
>> @@ -491,6 +491,8 @@ void kvm_pmu_software_increment(struct kvm_vcpu
>> *vcpu, u64 val)
>>
>> ÂÂÂÂ enable = __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
>> ÂÂÂÂ for (i = 0; i < ARMV8_PMU_CYCLE_IDX; i++) {
>> +ÂÂÂÂÂÂÂ bool chained = test_bit(i >> 1, vcpu->arch.pmu.chained);
>> +
> 
> I'd rather you use kvm_pmu_pmc_is_chained() rather than open-coding
> this. But see below:
> 
>> ÂÂÂÂÂÂÂÂ if (!(val & BIT(i)))
>> ÂÂÂÂÂÂÂÂÂÂÂÂ continue;
>> ÂÂÂÂÂÂÂÂ type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i)
>> @@ -500,8 +502,20 @@ void kvm_pmu_software_increment(struct kvm_vcpu
>> *vcpu, u64 val)
>> ÂÂÂÂÂÂÂÂÂÂÂÂ reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
>> ÂÂÂÂÂÂÂÂÂÂÂÂ reg = lower_32_bits(reg);
>> ÂÂÂÂÂÂÂÂÂÂÂÂ __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
>> -ÂÂÂÂÂÂÂÂÂÂÂ if (!reg)
>> +ÂÂÂÂÂÂÂÂÂÂÂ if (reg) /* no overflow */
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
>> +ÂÂÂÂÂÂÂÂÂÂÂ if (chained) {
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) + 1;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ reg = lower_32_bits(reg);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) = reg;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (reg)
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* mark an overflow on high counter */
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i + 1);
>> +ÂÂÂÂÂÂÂÂÂÂÂ } else {
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* mark an overflow */
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
>> +ÂÂÂÂÂÂÂÂÂÂÂ }
>> ÂÂÂÂÂÂÂÂ }
>> ÂÂÂÂ }
>> Â}
> 
> I think the whole function is a bit of a mess, and could be better
> structured to treat 64bit counters as a first class citizen.
> 
> I'm suggesting something along those lines, which tries to
> streamline things a bit and keep the flow uniform between the
> two word sizes. IMHO, it helps reasonning about it and gives
> scope to the ARMv8.5 full 64bit counters... It is of course
> completely untested.

Looks OK to me as well. One remark though, don't we need to test if the
n+1th reg is enabled before incrementing it?

Thanks

Eric
> 
> Thoughts?
> 
> ÂÂÂÂÂÂÂ M.
> 
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index 8731dfeced8b..cf371f643ade 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -480,26 +480,43 @@ static void kvm_pmu_perf_overflow(struct
> perf_event *perf_event,
> Â */
> Âvoid kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val)
> Â{
> +ÂÂÂ struct kvm_pmu *pmu = &vcpu->arch.pmu;
> ÂÂÂÂ int i;
> -ÂÂÂ u64 type, enable, reg;
> 
> -ÂÂÂ if (val == 0)
> -ÂÂÂÂÂÂÂ return;
> +ÂÂÂ /* Weed out disabled counters */
> +ÂÂÂ val &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> 
> -ÂÂÂ enable = __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> ÂÂÂÂ for (i = 0; i < ARMV8_PMU_CYCLE_IDX; i++) {
> +ÂÂÂÂÂÂÂ u64 type, reg;
> +ÂÂÂÂÂÂÂ int ovs = i;
> +
> ÂÂÂÂÂÂÂÂ if (!(val & BIT(i)))
> ÂÂÂÂÂÂÂÂÂÂÂÂ continue;
> -ÂÂÂÂÂÂÂ type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i)
> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ & ARMV8_PMU_EVTYPE_EVENT;
> -ÂÂÂÂÂÂÂ if ((type == ARMV8_PMUV3_PERFCTR_SW_INCR)
> -ÂÂÂÂÂÂÂÂÂÂÂ && (enable & BIT(i))) {
> -ÂÂÂÂÂÂÂÂÂÂÂ reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
> -ÂÂÂÂÂÂÂÂÂÂÂ reg = lower_32_bits(reg);
> -ÂÂÂÂÂÂÂÂÂÂÂ __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
> -ÂÂÂÂÂÂÂÂÂÂÂ if (!reg)
> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
> +
> +ÂÂÂÂÂÂÂ /* PMSWINC only applies to ... SW_INC! */
> +ÂÂÂÂÂÂÂ type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
> +ÂÂÂÂÂÂÂ type &= ARMV8_PMU_EVTYPE_EVENT;
> +ÂÂÂÂÂÂÂ if (type != ARMV8_PMUV3_PERFCTR_SW_INCR)
> +ÂÂÂÂÂÂÂÂÂÂÂ continue;
> +
> +ÂÂÂÂÂÂÂ /* Potential 64bit value */
> +ÂÂÂÂÂÂÂ reg = kvm_pmu_get_counter_value(vcpu, i) + 1;
> +
> +ÂÂÂÂÂÂÂ /* Start by writing back the low 32bits */
> +ÂÂÂÂÂÂÂ __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = lower_32_bits(reg);
> +
> +ÂÂÂÂÂÂÂ /*
> +ÂÂÂÂÂÂÂÂ * 64bit counter? Write back the upper bits and target
> +ÂÂÂÂÂÂÂÂ * the overflow bit at the next counter
> +ÂÂÂÂÂÂÂÂ */
> +ÂÂÂÂÂÂÂ if (kvm_pmu_pmc_is_chained(&pmu->pmc[i])) {
> +ÂÂÂÂÂÂÂÂÂÂÂ reg = upper_32_bits(reg);
> +ÂÂÂÂÂÂÂÂÂÂÂ __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) = reg;
> +ÂÂÂÂÂÂÂÂÂÂÂ ovs++;
> ÂÂÂÂÂÂÂÂ }
> +
> +ÂÂÂÂÂÂÂ if (!lower_32_bits(reg))
> +ÂÂÂÂÂÂÂÂÂÂÂ __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(ovs);
> ÂÂÂÂ }
> Â}
> 
>