Re: [PATCH 09/21] KVM: arm64: Add Partitioned PMU register trap handlers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Writes to PMU control-plane system registers lack the architecturally mandated immediate `isb()` synchronization barrier.
- [High] Trapped accesses to `PMEVTYPER`, `PMCCFILTR_EL0`, and `PMOVSSET_EL0` fail to synchronize with the active hardware PMU state while the vCPU is running.
- [High] `kvm_pmu_direct_pmcr_read` bypasses Nested Virtualization (NV) counter limits, exposing incorrect `PMCR_EL0.N` to L2 guests.
- [High] Guest `PMSELR_EL0` and `PMUSERENR_EL0` state is irrevocably lost across vCPU context switches in partitioned PMU mode.
--

commit 62a57b5fbe8c480374618085132cc6e2e40657b1
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/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 0135989667564..43f04c2f33d44 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c
> @@ -56,3 +57,32 @@ bool kvm_pmu_is_partitioned(struct kvm *kvm)
>  	return pmu_is_partitioned(kvm->arch.arm_pmu) &&
>  		test_bit(KVM_ARCH_FLAG_PARTITION_PMU_ENABLED, &kvm->arch.flags);
>  }
> +
> +void kvm_pmu_direct_pmcr_write(struct kvm_vcpu *vcpu, u64 val)
> +{
> +	bool reset_p = val & ARMV8_PMU_PMCR_P;
> +	unsigned long mask;
> +	int i;
> +
> +	val &= ~ARMV8_PMU_PMCR_P;
> +
> +	write_sysreg(val, pmcr_el0);
> +
> +	if (reset_p) {

[Severity: High]
Does this write to pmcr_el0 need an isb() barrier before we evaluate reset_p?

The ARM64 architecture requires updates to control-plane system registers
like pmcr_el0 to be followed immediately by an isb() instruction. Without
it, the pipeline might execute the subsequent instructions under an undefined
state.

> +		mask = kvm_pmu_implemented_counter_mask(vcpu) & ~BIT(ARMV8_PMU_CYCLE_IDX);
> +
> +		if (!vcpu_is_el2(vcpu))
> +			mask &= ~kvm_pmu_hyp_counter_mask(vcpu);
> +
> +		for_each_set_bit(i, &mask, ARMV8_PMU_MAX_GENERAL_COUNTERS)
> +			write_pmevcntrn(i, 0);
> +	}
> +}
> +
> +u64 kvm_pmu_direct_pmcr_read(struct kvm_vcpu *vcpu)
> +{
> +	return u64_replace_bits(
> +		read_sysreg(pmcr_el0),
> +		vcpu->kvm->arch.nr_pmu_counters,
> +		ARMV8_PMU_PMCR_N);

[Severity: High]
Can this bypass Nested Virtualization counter limits and expose an incorrect
PMCR_EL0.N value to L2 guests?

The existing kvm_vcpu_read_pmcr() function properly accounts for nested
virtualization by checking for nested guests and restricting PMCR_EL0.N based
on the L1 hypervisor's MDCR_EL2.HPMN limit. This direct read inserts the
full vcpu->kvm->arch.nr_pmu_counters, which means L2 guests under a
partitioned PMU might escape the L1 limit and observe the full count
provisioned to L1.

> +}
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index c52873a6f91ed..94572bc52c32a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
[ ... ]
> +static void pmu_reg_write(struct kvm_vcpu *vcpu, enum vcpu_sysreg reg, u64 val, bool set)
> +{
> +	unsigned long flags;
> +	u64 mask;
> +	int idx;
> +
> +	switch (reg) {
[ ... ]
> +	case PMSELR_EL0:
> +		if (kvm_pmu_is_partitioned(vcpu->kvm))
> +			write_sysreg(val, pmselr_el0);

[Severity: High]
Does this need an isb() after writing pmselr_el0? Like pmcr_el0, writes to
control-plane registers require an immediate synchronization barrier before
returning or executing further instructions.

Also, if we are untrapping or writing directly to the physical pmselr_el0
and pmuserenr_el0 registers, won't this state be irrevocably lost across
vCPU context switches?

When entering a guest, __activate_traps_common() in
arch/arm64/kvm/hyp/include/hyp/switch.h unconditionally clobbers the
physical pmselr_el0 to 0 and pmuserenr_el0 to ARMV8_PMU_USERENR_MASK.
Any state restored by kvm_pmu_load() or set here would be destroyed
before the guest actually executes.

[ ... ]
> +	case PMEVTYPER0_EL0 ... PMCCFILTR_EL0:
> +		idx = reg - PMEVTYPER0_EL0;
> +
> +		if (kvm_pmu_is_partitioned(vcpu->kvm)) {
> +			mask = kvm_pmu_evtyper_mask(vcpu->kvm);
> +			__vcpu_assign_sys_reg(vcpu, reg, val & mask);

[Severity: High]
Since traps are processed synchronously while the vCPU is loaded, doesn't
updating only the virtual sysreg array leave the active hardware with stale
configurations?

Writing to the virtual array here without requesting a PMU reload
(KVM_REQ_RELOAD_PMU) or updating the hardware means that upon ERET back to
the guest, the physical PMU will continue executing with the old state.

[ ... ]
> +	case PMCNTENSET_EL0:
> +		if (kvm_pmu_is_partitioned(vcpu->kvm)) {
> +			if (set)
> +				write_sysreg(val, pmcntenset_el0);
> +			else
> +				write_sysreg(val, pmcntenclr_el0);

[Severity: High]
Does returning after this write without an isb() violate the architectural
requirements? 

Writing to pmcntenset_el0 or pmcntenclr_el0 needs an immediate isb() to
ensure the state change is resolved before returning to the caller or
executing subsequent pipeline instructions.

[ ... ]
> +	case PMUSERENR_EL0:
> +		if (kvm_pmu_is_partitioned(vcpu->kvm))
> +			write_sysreg(val, pmuserenr_el0);

[Severity: High]
Is an isb() barrier required here after writing pmuserenr_el0, similar to the
other control-plane registers?

[ ... ]
> +static u64 pmu_reg_read(struct kvm_vcpu *vcpu, enum vcpu_sysreg reg)
> +{
> +	u64 val = 0;
> +	int idx;
> +
> +	switch (reg) {
[ ... ]
> +	case PMOVSSET_EL0:
> +		val = __vcpu_sys_reg(vcpu, reg);

[Severity: High]
Will this return an accurate value if there are unhandled physical hardware
overflows?

Because we only read the virtual array here, we could mask unhandled physical
hardware overflows from polling guests if the host hasn't yet processed the
IRQ and updated the virtual state.

> +		break;

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