Re: [PATCH v7 09/20] 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:
- [High] Unconditionally setting MDCR_EL2.HPME forces host PMU counters on, breaking the host's ability to globally disable them.
- [High] Setting MDCR_EL2.HPMD and MDCR_EL2.HCCD unconditionally disables host profiling at EL2, blinding `perf` to the VHE host kernel.
- [High] kvm_pmu_hpmn() fails to fall back to max_guest_counters when HPMN0 is unsupported, breaking host partitioning.
- [High] Unsafe per-CPU data access with preemption enabled causes TOCTOU races and UNPREDICTABLE state on heterogeneous CPUs.
- [High] Desynchronized vcpu_on_unsupported_cpu() checks grant the guest untrapped access to the physical PMU of unsupported CPUs.
--

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

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

This commit configures MDCR_EL2 to support PMU partitioning by calculating the
appropriate HPMN value and configuring coarse and fine-grained traps. It also
enables host PMU counting and sets filtering bits for the PMU hardware.

> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 3ad6b7c6e4ba7..0ab89c91e19cb 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -36,20 +36,43 @@ static int cpu_has_spe(u64 dfr0)
>   */
>  static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  {
> +	int hpmn = kvm_pmu_hpmn(vcpu);
> +
>  	preempt_disable();

Does calling kvm_pmu_hpmn() before preempt_disable() cause a Time-of-Check to
Time-of-Use race here?

kvm_pmu_hpmn() uses host_data_ptr(nr_event_counters), which accesses
per-CPU data. If the thread migrates, could KVM program MDCR_EL2.HPMN
using the counter value from the wrong physical CPU?

[ ... ]

> -	vcpu->arch.mdcr_el2 = FIELD_PREP(MDCR_EL2_HPMN,
> -					 *host_data_ptr(nr_event_counters));
> +	vcpu->arch.mdcr_el2 = FIELD_PREP(MDCR_EL2_HPMN, hpmn);
>  	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);

Does unconditionally setting MDCR_EL2.HPME force the host PMU counters on?

When PMU partitioning is active, MDCR_EL2.HPME becomes the global enable for
the host partition. If KVM forces this to 1 while the vCPU is loaded, does
the host PMU driver lose the ability to globally stop its counters, for
example during counter reprogramming or overflow IRQ handling?

> +	if (kvm_vcpu_pmu_is_partitioned(vcpu)) {
> +		/*
> +		 * Filtering these should be redundant because we trap
> +		 * all the TYPER and FILTR registers anyway and ensure
> +		 * they filter EL2, but set the bits if they are here.
> +		 */
> +		if (is_pmuv3p1(read_pmuver()))
> +			vcpu->arch.mdcr_el2 |= MDCR_EL2_HPMD;
> +		if (is_pmuv3p5(read_pmuver()))
> +			vcpu->arch.mdcr_el2 |= MDCR_EL2_HCCD;

The commit message states these bits are set to ensure nothing in the guest
counts at EL2. Do these bits actually apply to the host partition rather than
the guest?

According to the ARM Architecture Reference Manual, when partitioned, the
guest counters are already prohibited from counting at EL2, while HPMD and
HCCD apply to the host partition counters. Does setting these bits permanently
disable the host's own counters from profiling at EL2?

> +		/*
> +		 * Take out the coarse grain traps if we are using
> +		 * fine grain traps.
> +		 */
> +		if (kvm_vcpu_pmu_use_fgt(vcpu))
> +			vcpu->arch.mdcr_el2 &= ~(MDCR_EL2_TPM | MDCR_EL2_TPMCR);
> +
> +	}

Can this grant the guest untrapped access to the physical PMU on unsupported
CPUs?

If a vCPU is on an unsupported CPU, kvm_pmu_hpmn() returns the host's maximum
counters, effectively disabling the partition. However, kvm_vcpu_pmu_use_fgt()
does not check vcpu_on_unsupported_cpu(). Will it return true and clear the
trap bits here, allowing direct hardware access to the PMU?

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 2148bc46079c4..63ac72910e4b5 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c

[ ... ]

> +u8 kvm_pmu_hpmn(struct kvm_vcpu *vcpu)
> +{
> +	u8 nr_guest_cntr = vcpu->kvm->arch.nr_pmu_counters;
> +
> +	if (kvm_vcpu_pmu_is_partitioned(vcpu)
> +	    && !vcpu_on_unsupported_cpu(vcpu)
> +	    && (cpus_have_final_cap(ARM64_HAS_HPMN0) || nr_guest_cntr > 0))
> +		return nr_guest_cntr;
> +
> +	return *host_data_ptr(nr_event_counters);
> +}

The commit message states this will fall back to max_guest_counters if needed
when partitioned but unable to set the implied HPMN value. Does the
implementation miss this fallback?

If the conditions fail (for instance, nr_guest_cntr is 0 on hardware without
HPMN0), it returns the total number of host counters instead of
max_guest_counters. Will setting HPMN to the maximum counters destroy the host
partition and cause the host's reserved counters to silently stop?

> diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> index ee2f0f7e61bcf..8c10ad05661bc 100644
> --- a/arch/arm64/kvm/pmu.c
> +++ b/arch/arm64/kvm/pmu.c
> @@ -542,6 +542,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 (kvm_pmu_is_partitioned(arm_pmu))
> +		return arm_pmu->max_guest_counters;
> +

Can this be called with preemption enabled?

kvm_arm_pmu_get_max_counters() can be called from userspace ioctls. Since
kvm_pmu_is_partitioned() evaluates host_data_ptr(), does this create a race
condition with per-CPU data access similar to the one in
kvm_arm_setup_mdcr_el2()?

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