Re: [PATCH v5 19/24] KVM: arm64: Implement lazy PMU context swaps

[Colton Lewis]

Oliver Upton <oup...@kernel.org> writes:

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".

It was what I thought described the access pattern. Do you have another
naming suggestion?

+#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.

Understood. Will fix.

+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.

That's the way I initially wrote it but it had a problem on a different
kernel. I forget the exact issue, but I never saw the problem on
upstream so I'm happy to go back to it.

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.

Yes but these are the only two that use the old trap handlers. I also
call set_physical_access from my fast path handler that handles all the
other registers when partitioned.

Agree on having some generic accessors which you mention in an earlier
patch.