Re: [PATCH v3 1/2] KVM: arm64: PMU: Introduce FIXED_COUNTERS_ONLY

[Oliver Upton <oupton@kernel.org>]

Hi Akihiko,

On Wed, Feb 25, 2026 at 01:31:15PM +0900, Akihiko Odaki wrote:
> @@ -629,6 +629,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  		kvm_vcpu_load_vhe(vcpu);
>  	kvm_arch_vcpu_load_fp(vcpu);
>  	kvm_vcpu_pmu_restore_guest(vcpu);
> +	if (test_bit(KVM_ARCH_FLAG_PMU_V3_FIXED_COUNTERS_ONLY, &vcpu->kvm->arch.flags))
> +		kvm_make_request(KVM_REQ_CREATE_PMU, vcpu);

We only need to set the request if the vCPU has migrated to a different
PMU implementation, no?

>  	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
>  		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
>  
> @@ -1056,6 +1058,9 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu)
>  		if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
>  			kvm_vcpu_reload_pmu(vcpu);
>  
> +		if (kvm_check_request(KVM_REQ_CREATE_PMU, vcpu))
> +			kvm_vcpu_create_pmu(vcpu);
> +

My strong preference would be to squash the migration handling into
kvm_vcpu_reload_pmu(). It is already reprogramming PMU events in
response to other things.

>  		if (kvm_check_request(KVM_REQ_RESYNC_PMU_EL0, vcpu))
>  			kvm_vcpu_pmu_restore_guest(vcpu);
>  
> @@ -1516,7 +1521,8 @@ static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)
>  	 * When the vCPU has a PMU, but no PMU is set for the guest
>  	 * yet, set the default one.
>  	 */
> -	if (kvm_vcpu_has_pmu(vcpu) && !kvm->arch.arm_pmu)
> +	if (kvm_vcpu_has_pmu(vcpu) && !kvm->arch.arm_pmu &&
> +	    !test_bit(KVM_ARCH_FLAG_PMU_V3_FIXED_COUNTERS_ONLY, &kvm->arch.flags))
>  		ret = kvm_arm_set_default_pmu(kvm);

I'd rather just initialize it to a default than have to deal with the
field being sometimes null.

> -static bool kvm_pmu_counter_is_enabled(struct kvm_pmc *pmc)
> +static u64 kvm_pmu_enabled_counter_mask(struct kvm_vcpu *vcpu)
>  {
> -	struct kvm_vcpu *vcpu = kvm_pmc_to_vcpu(pmc);
> -	unsigned int mdcr = __vcpu_sys_reg(vcpu, MDCR_EL2);
> +	u64 mask = 0;
>  
> -	if (!(__vcpu_sys_reg(vcpu, PMCNTENSET_EL0) & BIT(pmc->idx)))
> -		return false;
> +	if (__vcpu_sys_reg(vcpu, MDCR_EL2) & MDCR_EL2_HPME)
> +		mask |= kvm_pmu_hyp_counter_mask(vcpu);
>  
> -	if (kvm_pmu_counter_is_hyp(vcpu, pmc->idx))
> -		return mdcr & MDCR_EL2_HPME;
> +	if (kvm_vcpu_read_pmcr(vcpu) & ARMV8_PMU_PMCR_E)
> +		mask |= ~kvm_pmu_hyp_counter_mask(vcpu);
>  
> -	return kvm_vcpu_read_pmcr(vcpu) & ARMV8_PMU_PMCR_E;
> +	return __vcpu_sys_reg(vcpu, PMCNTENSET_EL0) & mask;
> +}
> +
> +static bool kvm_pmu_counter_is_enabled(struct kvm_pmc *pmc)
> +{
> +	struct kvm_vcpu *vcpu = kvm_pmc_to_vcpu(pmc);
> +
> +	return kvm_pmu_enabled_counter_mask(vcpu) & BIT(pmc->idx);
>  }

You're churning a good bit of code, this needs to happen in a separate
patch (if at all).

> @@ -689,6 +710,14 @@ static void kvm_pmu_create_perf_event(struct kvm_pmc *pmc)
>  	int eventsel;
>  	u64 evtreg;
>  
> +	if (!arm_pmu) {
> +		arm_pmu = kvm_pmu_probe_armpmu(vcpu->cpu);

kvm_pmu_probe_armpmu() takes a global mutex, I'm not sure that's what we
want.

What prevents us from opening a PERF_TYPE_RAW event and allowing perf to
work out the right PMU for this CPU?

> +		if (!arm_pmu) {
> +			vcpu_set_on_unsupported_cpu(vcpu);

At this point it seems pretty late to flag the CPU as unsupported. Maybe
instead we can compute the union cpumask for all the PMU implemetations
the VM may schedule on.

> @@ -1249,6 +1299,10 @@ int kvm_arm_pmu_v3_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		irq = vcpu->arch.pmu.irq_num;
>  		return put_user(irq, uaddr);
>  	}
> +	case KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY:
> +		lockdep_assert_held(&vcpu->kvm->arch.config_lock);
> +		if (test_bit(KVM_ARCH_FLAG_PMU_V3_FIXED_COUNTERS_ONLY, &vcpu->kvm->arch.flags))
> +			return 0;

We don't need a getter for this, userspace should remember how it
provisioned the VM.

Thanks,
Oliver