On Thu, Oct 03, 2019 at 06:24:00PM +0100, Marc Zyngier wrote:
> When a counter is disabled, its value is sampled before the event
> is being disabled, and the value written back in the shadow register.
>
> In that process, the value gets truncated to 32bit, which is adequate
Doh, that shouldn't have happened.
> for any counter but the cycle counter, which can be configured to
> hold a 64bit value. This obviously results in a corrupted counter,
> and things like "perf record -e cycles" not working at all when
> run in a guest...
>
> Make the truncation conditional on the counter not being 64bit.
>
> Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU counters")
> Cc: Andrew Murray <andrew.murray@xxxxxxx>
> Reported-by: Julien Thierry Julien Thierry <julien.thierry.kdev@xxxxxxxxx>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> ---
> virt/kvm/arm/pmu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index 362a01886bab..d716aef2bae9 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -206,9 +206,11 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
> __vcpu_sys_reg(vcpu, reg) = lower_32_bits(counter);
> __vcpu_sys_reg(vcpu, reg + 1) = upper_32_bits(counter);
> } else {
> + if (!kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
> + counter = lower_32_bits(counter);
> reg = (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
> - __vcpu_sys_reg(vcpu, reg) = lower_32_bits(counter);
> + __vcpu_sys_reg(vcpu, reg) = counter;
The other uses of lower_32_bits look OK to me.
Reviewed-by: Andrew Murray <andrew.murray@xxxxxxx>
As a side note, I'm not convinced that the implementation (or perhaps the
use of) kvm_pmu_idx_is_64bit is correct:
static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
{
return (select_idx == ARMV8_PMU_CYCLE_IDX &&
__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
}
We shouldn't truncate the value of a cycle counter to 32 bits just because
_PMCR_LC is unset. We should only be interested in _PMCR_LC when setting
the sample_period.
If you agree this is wrong, I'll spin a change.
Though unsetting _PMCR_LC is deprecated so I can't imagine this causes any
issue.
Thanks,
Andrew Murray
> }
>
> kvm_pmu_release_perf_event(pmc);
> --
> 2.20.1
>
