Re: [PATCH v7 08/20] KVM: arm64: Add Partitioned PMU register trap handlers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Guest PMCR_EL0 writes directly trigger hardware reset at host privilege level, clearing host PMU counters.
- [High] Unmasked reads of PMCNTENSET_EL0 and PMINTENSET_EL1 leak host PMU configuration to the guest.
- [High] Race condition and data corruption due to hardware PMU registers being accessed without disabling IRQs or preemption.
- [High] Architectural violation: Missing instruction synchronization barrier (ISB) between PMSELR_EL0 write and PMXEVCNTR_EL0 access.
- [Medium] Nested virtualization boundary bypass in `pmu_read_pmcr`.
- [Low] Dead code and brittle logic: Unused `reg` variable in `access_pmu_evtyper` relies on implicit enum layout.
--

commit 6a28050b2477f91b45c9d48b5b98d954e310fb44
Author: Colton Lewis <coltonlewis@google.com>

KVM: arm64: Add Partitioned PMU register trap handlers

We may want a partitioned PMU but not have FEAT_FGT to untrap the
specific registers that would normally be untrapped. Add handling for
those trapped register accesses that does the right thing if the PMU
is partitioned.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 0a8e8ee69cd00..cc3d1804ab200 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c

[ ... ]

> @@ -1016,6 +1032,29 @@ static bool pmu_access_event_counter_el0_disabled(struct kvm_vcpu *vcpu)
>  	return check_pmu_access_disabled(vcpu, ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_EN);
>  }
>  
> +static void pmu_write_pmcr(struct kvm_vcpu *vcpu, u64 val)
> +{
> +	if (kvm_vcpu_pmu_is_partitioned(vcpu)) {
> +		write_sysreg(val, pmcr_el0);
> +		return;
> +	}
> +
> +	kvm_pmu_handle_pmcr(vcpu, val);
> +}

Since KVM sysreg trap handlers execute at EL2 in VHE mode, does writing the
guest's value directly to pmcr_el0 allow a guest to reset all host PMU
counters?

According to the ARM Architecture Reference Manual, writing PMCR_EL0.P = 1 or
PMCR_EL0.C = 1 from EL2 unconditionally resets all event counters or the cycle
counter respectively, bypassing MDCR_EL2.HPMN isolation.

> +
> +static u64 pmu_read_pmcr(struct kvm_vcpu *vcpu)
> +{
> +	if (kvm_vcpu_pmu_is_partitioned(vcpu)) {
> +		return u64_replace_bits(
> +			read_sysreg(pmcr_el0),
> +			vcpu->kvm->arch.nr_pmu_counters,
> +			ARMV8_PMU_PMCR_N);
> +	}
> +
> +	return kvm_vcpu_read_pmcr(vcpu);
> +
> +}

Does this hardcoded replacement of the N field with
vcpu->kvm->arch.nr_pmu_counters ignore L1's nested virtualization limits?

The standard KVM read path in kvm_vcpu_read_pmcr() recalculates the N field
based on nested state to restrict counters for an L2 guest.

[ ... ]

> @@ -1128,6 +1184,44 @@ static int set_pmu_evcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>  	return 0;
>  }
>  
> +static void pmu_write_evcntr(struct kvm_vcpu *vcpu, u64 val, u64 idx)
> +{
> +	u64 pmselr;
> +
> +	if (!kvm_vcpu_pmu_is_partitioned(vcpu)) {
> +		kvm_pmu_set_counter_value(vcpu, idx, val);
> +		return;
> +	}
> +
> +	if (idx == ARMV8_PMU_CYCLE_IDX) {
> +		write_sysreg(val, pmccntr_el0);
> +		return;
> +	}
> +
> +	pmselr = read_sysreg(pmselr_el0);
> +	write_sysreg(idx, pmselr_el0);
> +	write_sysreg(val, pmxevcntr_el0);
> +	write_sysreg(pmselr, pmselr_el0);
> +}

Are preemption and local IRQs enabled here?

If a host Performance Monitoring Interrupt (PMI) fires between setting
pmselr_el0 and accessing pmxevcntr_el0, couldn't the host PMU driver modify
pmselr_el0 to service its own counter, causing KVM to access the wrong
counter when it resumes?

Also, is an instruction synchronization barrier (ISB) required between
writing pmselr_el0 and accessing pmxevcntr_el0?

The ARM Architecture Reference Manual requires a context synchronization event
after writing to a selector register before accessing multiplexed registers
like pmxevcntr_el0.

> +
> +static u64 pmu_read_evcntr(struct kvm_vcpu *vcpu, u64 idx)
> +{
> +	u64 pmselr;
> +	u64 val;
> +
> +	if (!kvm_vcpu_pmu_is_partitioned(vcpu))
> +		return kvm_pmu_get_counter_value(vcpu, idx);
> +
> +	if (idx == ARMV8_PMU_CYCLE_IDX)
> +		return read_sysreg(pmccntr_el0);
> +
> +	pmselr = read_sysreg(pmselr_el0);
> +	write_sysreg(idx, pmselr_el0);
> +	val = read_sysreg(pmxevcntr_el0);
> +	write_sysreg(pmselr, pmselr_el0);
> +	return val;
> +}

The same questions regarding preemption, local IRQs, and the missing ISB
apply here when reading pmxevcntr_el0.

[ ... ]

> +static void pmu_write_evtyper(struct kvm_vcpu *vcpu, u64 val, u64 idx)
> +{
> +	u64 mask;
> +
> +	if (kvm_vcpu_pmu_is_partitioned(vcpu)) {
> +		mask = kvm_pmu_evtyper_mask(vcpu->kvm);
> +		__vcpu_assign_sys_reg(vcpu, PMEVTYPER0_EL0 + idx, val & mask);
> +		return;
> +	}
> +
> +	kvm_pmu_set_counter_event_type(vcpu, val, idx);
> +	kvm_vcpu_pmu_restore_guest(vcpu);
> +}

Does PMEVTYPER0_EL0 + idx assume a specific enum layout to reach PMCCFILTR_EL0
when idx is ARMV8_PMU_CYCLE_IDX (31)?

[ ... ]

> @@ -1191,7 +1305,7 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  
>  	if (r->CRn == 9 && r->CRm == 13 && r->Op2 == 1) {
>  		/* PMXEVTYPER_EL0 */
> -		idx = SYS_FIELD_GET(PMSELR_EL0, SEL, __vcpu_sys_reg(vcpu, PMSELR_EL0));
> +		idx = SYS_FIELD_GET(PMSELR_EL0, SEL, pmu_read_pmselr(vcpu));
>  		reg = PMEVTYPER0_EL0 + idx;
>  	} else if (r->CRn == 14 && (r->CRm & 12) == 12) {
>  		idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);
> @@ -1207,12 +1321,10 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	if (!pmu_counter_idx_valid(vcpu, idx))
>  		return false;
>  
> -	if (p->is_write) {
> -		kvm_pmu_set_counter_event_type(vcpu, p->regval, idx);
> -		kvm_vcpu_pmu_restore_guest(vcpu);
> -	} else {
> -		p->regval = __vcpu_sys_reg(vcpu, reg);
> -	}
> +	if (p->is_write)
> +		pmu_write_evtyper(vcpu, p->regval, idx);
> +	else
> +		p->regval = pmu_read_evtyper(vcpu, idx);

Is the reg variable computed above now completely unused in this function?

[ ... ]

> +static u64 pmu_read_pmcnten(struct kvm_vcpu *vcpu)
> +{
> +	if (kvm_vcpu_pmu_is_partitioned(vcpu))
> +		return read_sysreg(pmcntenset_el0);
> +
> +	return __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> +}
> +
>  static bool access_pmcnten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  			   const struct sys_reg_desc *r)
>  {
> @@ -1246,40 +1387,58 @@ static bool access_pmcnten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	mask = kvm_pmu_accessible_counter_mask(vcpu);
>  	if (p->is_write) {
>  		val = p->regval & mask;
> -		if (r->Op2 & 0x1)
> -			/* accessing PMCNTENSET_EL0 */
> -			__vcpu_rmw_sys_reg(vcpu, PMCNTENSET_EL0, |=, val);
> -		else
> -			/* accessing PMCNTENCLR_EL0 */
> -			__vcpu_rmw_sys_reg(vcpu, PMCNTENSET_EL0, &=, ~val);
> -
> -		kvm_pmu_reprogram_counter_mask(vcpu, val);
> +		pmu_write_pmcnten(vcpu, val, r->Op2 & 0x1);
>  	} else {
> -		p->regval = __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> +		p->regval = pmu_read_pmcnten(vcpu);
>  	}
>  
>  	return true;
>  }

Since KVM executes at EL2, does reading pmcntenset_el0 directly return the
global PMU state, including enable bits for host-owned counters (indices >=
HPMN)?

Without applying kvm_pmu_accessible_counter_mask(vcpu) to the result in the
read path, it appears to expose host PMU activity to the guest.

[ ... ]

> +static u64 pmu_read_pminten(struct kvm_vcpu *vcpu)
> +{
> +	if (kvm_vcpu_pmu_is_partitioned(vcpu))
> +		return read_sysreg(pmintenset_el1);
> +
> +	return __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
> +}
> +
>  static bool access_pminten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  			   const struct sys_reg_desc *r)
>  {
> -	u64 mask = kvm_pmu_accessible_counter_mask(vcpu);
> +	u64 val, mask;
>  
>  	if (check_pmu_access_disabled(vcpu, 0))
>  		return false;
>  
> +	mask = kvm_pmu_accessible_counter_mask(vcpu);
>  	if (p->is_write) {
> -		u64 val = p->regval & mask;
> +		val = p->regval & mask;
>  
> -		if (r->Op2 & 0x1)
> -			/* accessing PMINTENSET_EL1 */
> -			__vcpu_rmw_sys_reg(vcpu, PMINTENSET_EL1, |=, val);
> -		else
> -			/* accessing PMINTENCLR_EL1 */
> -			__vcpu_rmw_sys_reg(vcpu, PMINTENSET_EL1, &=, ~val);
> +		pmu_write_pminten(vcpu, val, r->Op2 & 0x1);
>  	} else {
> -		p->regval = __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
> +		p->regval = pmu_read_pminten(vcpu);
>  	}
>  
>  	return true;
>  }

Similarly, does reading pmintenset_el1 directly leak host PMU interrupt
configuration to the guest without applying the mask?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504211813.1804997-1-coltonlewis@google.com?part=8