Re: [PATCH 5/9] KVM: arm64: PMU: Simplify setting a counter to a specific value

[Oliver Upton <oliver.upton@linux.dev>]

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@kernel.org>

Looks much better.

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

> ---
>  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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm