On Sat, Feb 08, 2025 at 02:01:11AM +0000, Colton Lewis wrote:
> The ARM architecture specifies that when MDCR_EL2.HPMN is set, EL1 and
> EL0, which includes KVM guests, should read that value for PMCR.N.
>
> Signed-off-by: Colton Lewis <coltonlewis@xxxxxxxxxx>
> ---
> arch/arm64/kvm/debug.c | 3 +--
> arch/arm64/kvm/pmu-emul.c | 8 +++++++-
> tools/testing/selftests/kvm/arm64/vpmu_counter_access.c | 2 +-
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 0e4c805e7e89..7c04db00bf6c 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -36,8 +36,7 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> * This also clears MDCR_EL2_E2PB_MASK and MDCR_EL2_E2TB_MASK
> * to disable guest access to the profiling and trace buffers
> */
> - vcpu->arch.mdcr_el2 = FIELD_PREP(MDCR_EL2_HPMN,
> - *host_data_ptr(nr_event_counters));
> + vcpu->arch.mdcr_el2 = FIELD_PREP(MDCR_EL2_HPMN, read_mdcr());
Please avoid unnecessary accesses to MDCR_EL2 at all costs. This is a
guaranteed trap to EL2 with nested virt.
> vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
> MDCR_EL2_TPMS |
> MDCR_EL2_TTRF |
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 6c5950b9ceac..052ce8c721fe 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -993,12 +993,18 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
> u8 kvm_arm_pmu_get_max_counters(struct kvm *kvm)
> {
> struct arm_pmu *arm_pmu = kvm->arch.arm_pmu;
> + u8 limit;
> +
> + if (arm_pmu->partitioned)
> + limit = arm_pmu->hpmn - 1;
> + else
> + limit = ARMV8_PMU_MAX_GENERAL_COUNTERS;
>
> /*
> * The arm_pmu->cntr_mask considers the fixed counter(s) as well.
> * Ignore those and return only the general-purpose counters.
> */
> - return bitmap_weight(arm_pmu->cntr_mask, ARMV8_PMU_MAX_GENERAL_COUNTERS);
> + return bitmap_weight(arm_pmu->cntr_mask, limit);
> }
This isn't necessary and is likely to regress the existing behavior.
When the architecture says the lower ELs should see PMCR_EL0.N have the
same value as MDCR_EL2.HPMN, what it really means is direct reads from
hardware will return the value.
So my expectation would be that a VM using the partitioned PMU
implementation would never reach any of the *emulated* PMU handlers, as
we would've disabled the associated traps.
The partitioned PMU will not replace the emulated vPMU implementation in
KVM, so it'd be good to refactor what we have today to make room for
your work. I think that'd be along the lines of:
- Shared code for event filter enforcement and handling of the vPMU
overflow IRQ.
- Emulated PMU implementation that provides trap handlers for all PMUv3
registers and backs into host perf
- Partitioned PMU implementation that doesn't rely on traps and instead
saves / restores a portion of the PMU that contains the guest
context.
These should be done in separate files, i.e. I do not want to see a
bunch of inline handling to cope with emulated v. partitioned PMUs.
> static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> diff --git a/tools/testing/selftests/kvm/arm64/vpmu_counter_access.c b/tools/testing/selftests/kvm/arm64/vpmu_counter_access.c
> index f16b3b27e32e..b5bc18b7528d 100644
> --- a/tools/testing/selftests/kvm/arm64/vpmu_counter_access.c
> +++ b/tools/testing/selftests/kvm/arm64/vpmu_counter_access.c
> @@ -609,7 +609,7 @@ static void run_pmregs_validity_test(uint64_t pmcr_n)
> */
> static void run_error_test(uint64_t pmcr_n)
> {
> - pr_debug("Error test with pmcr_n %lu (larger than the host)\n", pmcr_n);
> + pr_debug("Error test with pmcr_n %lu (larger than the host allows)\n", pmcr_n);
NBD for an RFC, but in the future please do selftests changes in a
separate patch.
--
Thanks,
Oliver
