On Tue, Dec 09, 2025 at 08:51:16PM +0000, Colton Lewis wrote:
> +enum vcpu_pmu_register_access {
> + VCPU_PMU_ACCESS_UNSET,
> + VCPU_PMU_ACCESS_VIRTUAL,
> + VCPU_PMU_ACCESS_PHYSICAL,
> +};
This is confusing. Even when the guest is accessing registers directly
on the CPU I'd still call that "hardware assisted virtualization" and
not "physical".
> +#endif /* _ASM_ARM64_KVM_TYPES_H */
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 0ab89c91e19cb..c2cf6b308ec60 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -34,7 +34,7 @@ static int cpu_has_spe(u64 dfr0)
> * - Self-hosted Trace Filter controls (MDCR_EL2_TTRF)
> * - Self-hosted Trace (MDCR_EL2_TTRF/MDCR_EL2_E2TB)
> */
> -static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> +void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> {
> int hpmn = kvm_pmu_hpmn(vcpu);
>
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h
> b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index bde79ec1a1836..ea288a712bb5d 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -963,6 +963,8 @@ static bool kvm_hyp_handle_pmu_regs(struct kvm_vcpu *vcpu)
> if (ret)
> __kvm_skip_instr(vcpu);
>
> + kvm_pmu_set_physical_access(vcpu);
> +
> return ret;
> }
>
> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 8d0d6d1a0d851..c5767e2ebc651 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c
> @@ -73,6 +73,7 @@ bool kvm_vcpu_pmu_use_fgt(struct kvm_vcpu *vcpu)
> u8 hpmn = vcpu->kvm->arch.nr_pmu_counters;
>
> return kvm_vcpu_pmu_is_partitioned(vcpu) &&
> + vcpu->arch.pmu.access == VCPU_PMU_ACCESS_PHYSICAL &&
> cpus_have_final_cap(ARM64_HAS_FGT) &&
> (hpmn != 0 || cpus_have_final_cap(ARM64_HAS_HPMN0));
> }
> @@ -92,6 +93,26 @@ u64 kvm_pmu_fgt2_bits(void)
> | HDFGRTR2_EL2_nPMICNTR_EL0;
> }
>
> +/**
> + * kvm_pmu_set_physical_access()
> + * @vcpu: Pointer to vcpu struct
> + *
> + * Reconfigure the guest for physical access of PMU hardware if
> + * allowed. This means reconfiguring mdcr_el2 and loading the vCPU
> + * state onto hardware.
> + *
> + */
> +
> +void kvm_pmu_set_physical_access(struct kvm_vcpu *vcpu)
> +{
> + if (kvm_vcpu_pmu_is_partitioned(vcpu)
> + && vcpu->arch.pmu.access == VCPU_PMU_ACCESS_VIRTUAL) {
> + vcpu->arch.pmu.access = VCPU_PMU_ACCESS_PHYSICAL;
> + kvm_arm_setup_mdcr_el2(vcpu);
> + kvm_pmu_load(vcpu);
> + }
It isn't immediately obvious how this guards against preemption.
Also, the general approach for these context-loading situations is to do
a full load/put on the vCPU rather than a directed load.
> +static void kvm_pmu_register_init(struct kvm_vcpu *vcpu)
> +{
> + if (vcpu->arch.pmu.access == VCPU_PMU_ACCESS_UNSET)
> + vcpu->arch.pmu.access = VCPU_PMU_ACCESS_VIRTUAL;
This is confusing. The zero value of the enum should be consistent with
the "unloaded" state.
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index f2ae761625a66..d73218706b834 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1197,6 +1197,8 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu,
> struct sys_reg_params *p,
> p->regval = __vcpu_sys_reg(vcpu, reg);
> }
>
> + kvm_pmu_set_physical_access(vcpu);
> +
> return true;
> }
>
> @@ -1302,6 +1304,8 @@ static bool access_pmovs(struct kvm_vcpu *vcpu, struct
> sys_reg_params *p,
> p->regval = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
> }
>
> + kvm_pmu_set_physical_access(vcpu);
> +
> return true;
> }
Aren't there a ton of other registers the guest may access before
these two? Having generic PMU register accessors would allow you to
manage residence of PMU registers from a single spot.
Thanks,
Oliver
