Re: [PATCH 15/21] KVM: arm64: Implement lazy PMU context swaps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Lazy PMU context swap fails to load the guest's PMU context into hardware when transitioning to GUEST_OWNED.

Pre-existing issues:
- [High] Missing Context Synchronization Event (isb) after writing to the control-plane register mdcr_el2.
--

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

KVM: arm64: Implement lazy PMU context swaps

Since many guests will never touch the PMU, they need not pay the cost
of context swapping those registers.

Use an enum to implement a simple state machine for PMU register
access. The PMU is either free or guest owned. We only need to context
swap if the PMU registers are guest owned. The PMU initially starts as
free and only transitions to guest owned if a guest has touched the
PMU registers.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 044f011c9c84b..bb1f3dca03869 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c
[ ... ]
> @@ -388,3 +388,20 @@ void kvm_pmu_put(struct kvm_vcpu *vcpu)
>  	kvm_pmu_set_guest_counters(pmu, 0);
>  	preempt_enable();
>  }
> +
> +/**
> + * kvm_pmu_set_guest_owned() - Give PMU ownership to guest
> + * @vcpu: Pointer to vcpu struct
> + *
> + * Reconfigure the guest for physical access of PMU hardware if
> + * allowed. This means reconfiguring mdcr_el2.
> + *
> + */
> +void kvm_pmu_set_guest_owned(struct kvm_vcpu *vcpu)
> +{
> +	if (kvm_pmu_is_partitioned(vcpu->kvm) &&
> +	    vcpu->arch.pmu.access == VCPU_PMU_ACCESS_FREE) {
> +		vcpu->arch.pmu.access = VCPU_PMU_ACCESS_GUEST_OWNED;
> +		kvm_arm_setup_mdcr_el2(vcpu);

[Severity: High]
This is a pre-existing issue, but does kvm_arm_setup_mdcr_el2() need an isb()
after writing to mdcr_el2?

When has_vhe() is true, it writes directly to mdcr_el2 but does not issue an
isb() before returning. Now that kvm_pmu_set_guest_owned() is called from
the active trap handling path, won't the immediate subsequent PMU hardware
accesses run against an architecturally undefined pipeline state?

[Severity: High]
Does this transition the PMU ownership without loading the guest's PMU state?

If we don't call kvm_pmu_load(vcpu) here to synchronize the in-memory guest
PMU state to the hardware PMU registers, won't the trap handler immediately
access uninitialized physical hardware registers containing the host's state?

> +	}
> +}

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 94572bc52c32a..f0eebeeb5ed96 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
[ ... ]
> @@ -1192,15 +1193,17 @@ static u64 pmu_reg_read(struct kvm_vcpu *vcpu, enum vcpu_sysreg reg)
>  	u64 val = 0;
>  	int idx;
>  
> +	kvm_pmu_set_guest_owned(vcpu);
> +
>  	switch (reg) {
>  	case PMCR_EL0:
> -		if (kvm_pmu_is_partitioned(vcpu->kvm))
> +		if (kvm_pmu_get_access(vcpu) == VCPU_PMU_ACCESS_GUEST_OWNED)
>  			val = kvm_pmu_direct_pmcr_read(vcpu);

[Severity: High]
Could this leak host state to the guest and later corrupt the guest's state?

Because kvm_pmu_set_guest_owned() didn't load the guest's PMU state, this
read (and subsequent ones in this handler) will access the physical PMU
registers that still hold the host's state.

Later, when the guest exits, kvm_pmu_put() will read these physical hardware
registers and save them back into vcpu->arch, permanently corrupting the
guest's saved PMU context.

Doesn't this also silently break live migration if restored PMU state is
wiped out upon the guest's first trapped access?

>  		else
>  			val = kvm_vcpu_read_pmcr(vcpu);
>  		break;

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