Re: [PATCH v7 11/20] KVM: arm64: Enforce PMU event filter at vcpu_load()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] kvm_pmu_apply_event_filter() incorrectly writes the cycle counter's event filter configuration to the cycle counter value register (pmccntr_el0) instead of the filter register (pmccfiltr_el0).
- [High] Hardware event filters are only updated on VCPU context switches, leaving hardware state stale when the guest dynamically reconfigures events.
- [High] When running a guest in nested EL2 context, kvm_pmu_apply_event_filter() fails to disable hardware EL1 counting if the guest did not request counting in EL2.
--

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

KVM: arm64: Enforce PMU event filter at vcpu_load()

The KVM API for event filtering says that counters do not count when
blocked by the event filter. To enforce that, the event filter must be
rechecked on every load since it might have changed since the last
time the guest wrote a value. If the event is filtered, exclude
counting at all exception levels before writing the hardware.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 360d022d918d5..2252d3b905db9 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c

[ ... ]

> +		guest_include_el2 = (val & ARMV8_PMU_INCLUDE_EL2);
> +		val &= ~evtyper_clr;
> +
> +		if (unlikely(is_hyp_ctxt(vcpu)) && guest_include_el2)
> +			val &= ~ARMV8_PMU_EXCLUDE_EL1;

When the guest is running in nested EL2 context (which executes in hardware
EL1), what happens if the guest configured the counter to count in guest EL1
but not in guest EL2?

If guest_include_el2 is false, do we need to explicitly set
ARMV8_PMU_EXCLUDE_EL1 in the hardware register here to ensure the counter
pauses when the guest enters EL2?

> +
> +		if (vcpu->kvm->arch.pmu_filter &&
> +		    !test_bit(evsel, vcpu->kvm->arch.pmu_filter))
> +			val |= evtyper_set;
> +
> +		if (i == ARMV8_PMU_CYCLE_IDX) {
> +			write_sysreg(val, pmccntr_el0);

Does this write the filter configuration into the cycle counter value register
instead of the filter register?

If we use pmccfiltr_el0 here instead, will it correctly apply the exclusion
mask?

> +		} else {
> +			write_sysreg(i, pmselr_el0);
> +			write_sysreg(val, pmxevtyper_el0);
> +		}
> +	}
> +}
> +
>  /**
>   * kvm_pmu_load() - Load untrapped PMU registers
>   * @vcpu: Pointer to struct kvm_vcpu
> @@ -165,6 +218,7 @@ void kvm_pmu_load(struct kvm_vcpu *vcpu)
>  
>  	pmu = vcpu->kvm->arch.arm_pmu;
>  	guest_counters = kvm_pmu_guest_counter_mask(pmu);
> +	kvm_pmu_apply_event_filter(vcpu);

If the guest dynamically reconfigures events by writing to PMEVTYPERn_EL0 or
PMCCFILTR_EL0, does the physical hardware PMU continue counting the old event
until the VCPU happens to be scheduled out and back in?

Is there another place where we push the new value to the physical hardware
during a standard sysreg trap return to avoid leaving the hardware state stale?

>  
>  	for_each_set_bit(i, &guest_counters, ARMPMU_MAX_HWEVENTS) {
>  		val = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i);

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