On Fri, Aug 05, 2022 at 02:58:09PM +0100, Marc Zyngier wrote:
> kvm_pmu_set_counter_value() is pretty odd, as it tries to update
> the counter value while taking into account the value that is
> currently held by the running perf counter.
>
> This is not only complicated, this is quite wrong. Nowhere in
> the architecture is it said that the counter would be offset
> by something that is pending. The counter should be updated
> with the value set by SW, and start counting from there if
> required.
>
> Remove the odd computation and just assign the provided value
> after having released the perf event (which is then restarted).
>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
Looks much better.
Reviewed-by: Oliver Upton <oliver.upton@xxxxxxxxx>
> ---
> arch/arm64/kvm/pmu-emul.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 9be485d23416..ddd79b64b38a 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -21,6 +21,7 @@ static LIST_HEAD(arm_pmus);
> static DEFINE_MUTEX(arm_pmus_lock);
>
> static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx);
> +static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc);
>
> static u32 kvm_pmu_event_mask(struct kvm *kvm)
> {
> @@ -129,8 +130,10 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> if (!kvm_vcpu_has_pmu(vcpu))
> return;
>
> + kvm_pmu_release_perf_event(&vcpu->arch.pmu.pmc[select_idx]);
> +
<bikeshed>
Not your code, but since we're here: it seems as though there is some
inconsistency in parameterization as some functions take an index and
others take a kvm_pmc pointer. For example,
kvm_pmu_{create,release}_perf_event() are inconsistent.
It might be nice to pick a scheme and apply consistently throughout.
</bikeshed>
--
Thanks,
Oliver
> reg = counter_index_to_reg(select_idx);
> - __vcpu_sys_reg(vcpu, reg) += (s64)val - kvm_pmu_get_counter_value(vcpu, select_idx);
> + __vcpu_sys_reg(vcpu, reg) = val;
>
> /* Recreate the perf event to reflect the updated sample_period */
> kvm_pmu_create_perf_event(vcpu, select_idx);
> --
> 2.34.1
>
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
