Re: [PATCH 10/21] KVM: arm64: Set up MDCR_EL2 to handle a Partitioned PMU

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] MDCR_EL2_HPMN update is incorrectly gated by FEAT_FGT, breaking hardware PMU isolation and allowing guest escapes on non-FGT CPUs.
- [High] Unconditional restriction of PMU counters for non-partitioned VMs based on host capabilities.
- [High] Evaluating a per-CPU variable via `this_cpu_ptr()` from a preemptible context in `kvm_arm_pmu_get_max_counters()`.
- [Medium] MDCR_EL2_HPME is set unconditionally for all VMs.

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

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

KVM: arm64: Set up MDCR_EL2 to handle a Partitioned PMU

Set up MDCR_EL2 to handle a Partitioned PMU. If partitioned, set the
HPME, HPMD, and HCCD bits. If we have the ability to use Fine Grain
Traps (FEAT_FGT) also, unset the TPM and TPMCR bits that trap all PMU
accesses and set HPMN to the correct number of guest counters so
hardware enforces the right values.

> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index f4d7b12045e8f..c84321277d893 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c

[ ... ]

> @@ -43,14 +43,35 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  	vcpu->arch.mdcr_el2 = FIELD_PREP(MDCR_EL2_HPMN, *host_data_ptr(nr_event_counters));
>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
>  				MDCR_EL2_TPMS |
>  				MDCR_EL2_TTRF |
>  				MDCR_EL2_TPMCR |
>  				MDCR_EL2_TDRA |
> -				MDCR_EL2_TDOSA);
> +				MDCR_EL2_TDOSA |
> +				MDCR_EL2_HPME);

[Severity: Medium]
The commit message states "If partitioned, set the HPME, HPMD, and HCCD bits",
but MDCR_EL2_HPME is added to the unconditional bitmask here. Does this leave
an inactive state bit that could cause unpredictable interactions in nested
virtualization environments for non-partitioned VMs?

> +
> +	if (kvm_pmu_is_partitioned(vcpu->kvm)) {
> +		u8 nr_guest_cntr = vcpu->kvm->arch.nr_pmu_counters;
> +
> +		vcpu->arch.mdcr_el2 |= (MDCR_EL2_HPMD | MDCR_EL2_HCCD);
> +
> +		/*
> +		 * Take out the coarse grain traps if we are using
> +		 * fine grain traps and enforce counter access with
> +		 * HPMN.
> +		 */
> +		if (!vcpu_on_unsupported_cpu(vcpu) &&
> +		    cpus_have_final_cap(ARM64_HAS_FGT) &&
> +		    (cpus_have_final_cap(ARM64_HAS_HPMN0) || nr_guest_cntr > 0)) {
> +			vcpu->arch.mdcr_el2 &= ~(MDCR_EL2_TPM | MDCR_EL2_TPMCR | MDCR_EL2_HPMN);
> +			vcpu->arch.mdcr_el2 |= FIELD_PREP(MDCR_EL2_HPMN, nr_guest_cntr);
> +		}

[Severity: Critical]
Is it safe to gate the MDCR_EL2_HPMN update behind the ARM64_HAS_FGT
capability check?

If FEAT_FGT is absent, HPMN remains equal to the total number of event
counters. While KVM falls back to trapping PMU accesses via TPM=1 on non-FGT
CPUs, when the PMU is guest-owned, the trap handlers can write unmasked
guest values directly to the hardware register:

arch/arm64/kvm/sys_regs.c:pmu_reg_write() {
    if (kvm_pmu_get_access(vcpu) == VCPU_PMU_ACCESS_GUEST_OWNED) {
        if (set)
            write_sysreg(val, pmcntenset_el0);
    ...
}

Since KVM executes at EL2 and HPMN equals the total number of event counters,
could this bypass hardware protections and allow a guest to overwrite host
performance counters on non-FGT CPUs?

[Severity: High]
This is a pre-existing issue, but I noticed that after MDCR_EL2 is updated
directly when has_vhe() is true in kvm_arm_setup_mdcr_el2():

arch/arm64/kvm/debug.c:kvm_arm_setup_mdcr_el2() {
    ...
    if (has_vhe())
        write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);

    preempt_enable();
}

There is no isb() immediately following the write to this control-plane
register. Because KVM immediately accesses PMU registers that depend on
MDCR_EL2 state (like writing to PMCNTENSET_EL0 after returning to
pmu_reg_write), could it observe architecturally undefined pipeline state?

> +
> +
> +	}
>  
>  	/* Is the VM being debugged by userspace? */
>  	if (vcpu->guest_debug)

[ ... ]

> diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> index 9ad3520417413..55cda8021400a 100644
> --- a/arch/arm64/kvm/pmu.c
> +++ b/arch/arm64/kvm/pmu.c

[ ... ]

> @@ -552,6 +552,13 @@ u8 kvm_arm_pmu_get_max_counters(struct kvm *kvm)
>  	if (cpus_have_final_cap(ARM64_WORKAROUND_PMUV3_IMPDEF_TRAPS))
>  		return 1;
>  
> +	/*
> +	 * If partitioned then we are limited by the max counters in
> +	 * the guest partition.
> +	 */
> +	if (pmu_is_partitioned(arm_pmu))
> +		return arm_pmu->max_guest_counters;

[Severity: High]
Does this incorrectly restrict the returned counters for all VMs on a
partitioned host?

By using pmu_is_partitioned(arm_pmu) which checks the host capability, rather
than kvm_pmu_is_partitioned(kvm) which checks if the specific VM has opted
into the feature, this limits the counters to arm_pmu->max_guest_counters
even for non-partitioned VMs that safely use perf_event multiplexing.

Could this degrade PMU capabilities for non-partitioned VMs and break live
migration?

[Severity: High]
Can evaluating pmu_is_partitioned() here trigger a BUG splat?

kvm_arm_pmu_get_max_counters() is called from preemptible contexts during VM
configuration, and pmu_is_partitioned() evaluates a per-CPU variable:

arch/arm64/kvm/pmu-direct.c:pmu_is_partitioned() {
    ...
    return pmu->max_guest_counters >= 0 &&
        pmu->max_guest_counters <= *host_data_ptr(nr_event_counters);
}

Since host_data_ptr() uses this_cpu_ptr(), could accessing this CPU-local
variable with preemption enabled result in a BUG splat for using
smp_processor_id() in preemptible contexts?

> +
>  	/*
>  	 * The arm_pmu->cntr_mask considers the fixed counter(s) as well.
>  	 * Ignore those and return only the general-purpose counters.

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