Re: [RFC PATCH v7 20/28] KVM: arm64: Allow guest SPE physical timestamps only if kernel allows it

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Sep 03, 2026 at 05:06:15PM +0100, Alexandru Elisei wrote:
> The SPE driver allows userspace to use physical timestamps for records only
> if the kernel allows it, do the same for a virtual machine.

This is really bare bones again. You are making a broader change of restoring
PMSCR_EL2 and gating on perf_allow_kernel() etc. - you should explain what
you're doing here.

Also you definitely need a 'why' here :>)

>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/include/asm/kvm_spe.h | 1 +
>  arch/arm64/kvm/hyp/vhe/spe-sr.c  | 5 +++--
>  arch/arm64/kvm/spe.c             | 8 ++++++++
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_spe.h b/arch/arm64/include/asm/kvm_spe.h
> index ad0ad6ace93c..8a55ea0de4e5 100644
> --- a/arch/arm64/include/asm/kvm_spe.h
> +++ b/arch/arm64/include/asm/kvm_spe.h
> @@ -15,6 +15,7 @@ struct arm_spe_pmu;
>  struct kvm_spe {
>  	struct arm_spe_pmu *spe_pmu;
>  	u64 pmblimitr_el1_res0_mask;
> +	u64 guest_pmscr_el2;
>  };
>
>  struct kvm_vcpu_spe {
> diff --git a/arch/arm64/kvm/hyp/vhe/spe-sr.c b/arch/arm64/kvm/hyp/vhe/spe-sr.c
> index 978620ad99fa..3c32a1338cc8 100644
> --- a/arch/arm64/kvm/hyp/vhe/spe-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/spe-sr.c
> @@ -107,6 +107,7 @@ static bool __kvm_spe_profiling_buffer_enabled(u64 pmblimitr, u64 pmbsr)
>   */
>  void __kvm_spe_restore_guest_state_vhe(struct kvm_vcpu *vcpu, struct kvm_cpu_context *guest_ctxt)
>  {
> +	struct kvm *kvm = vcpu->kvm;
>  	u64 pmblimitr, pmbsr;
>
>  	pmbsr = ctxt_sys_reg(guest_ctxt, PMBSR_EL1);
> @@ -120,10 +121,10 @@ void __kvm_spe_restore_guest_state_vhe(struct kvm_vcpu *vcpu, struct kvm_cpu_con
>  		write_sysreg_s(pmbsr, SYS_PMBSR_EL1);
>  	}
>
> -	__kvm_spe_restore_sampling_regs(vcpu->kvm, guest_ctxt);
> +	__kvm_spe_restore_sampling_regs(kvm, guest_ctxt);
>
>  	write_sysreg_el1(ctxt_sys_reg(guest_ctxt, PMSCR_EL1), SYS_PMSCR);
> -	write_sysreg_el2(0, SYS_PMSCR);
> +	write_sysreg_el2(kvm->arch.kvm_spe.guest_pmscr_el2, SYS_PMSCR);
>  }
>  NOKPROBE_SYMBOL(__kvm_spe_restore_guest_state_vhe);
>
> diff --git a/arch/arm64/kvm/spe.c b/arch/arm64/kvm/spe.c
> index af1e19cb8e9a..68b751bdb384 100644
> --- a/arch/arm64/kvm/spe.c
> +++ b/arch/arm64/kvm/spe.c
> @@ -3,9 +3,11 @@
>   * Copyright (C) 2021 - ARM Ltd
>   */
>
> +#include <linux/capability.h>
>  #include <linux/cpumask.h>
>  #include <linux/kvm_host.h>
>  #include <linux/perf/arm_spe_pmu.h>
> +#include <linux/perf_event.h>
>
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_spe.h>
> @@ -97,6 +99,12 @@ int kvm_spe_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  	if (!kvm_spe->pmblimitr_el1_res0_mask)
>  		kvm_spe_compute_pmblimitr_el1_res0_mask(kvm);
>
> +	if (kvm_vm_has_ran_once(kvm))
> +		return 0;
> +
> +	if (perf_allow_kernel())
> +		kvm_spe->guest_pmscr_el2 = PMSCR_EL2_PCT_PHYS;

Ouch, I see the bot raised that perf_allow_kernel() does the 'return 0 if
success' pattern which is just AWFUL naming.

Though I'm confused as to why you're gating on this at all? The driver needs to,
I get that, as you don't want host userspace to gain access.

But physical timestamps in the SPE buffer seems fine? What's the threat model
there? This should be explained with a comment/commit msg if there if it's
needed.

Does it expose this data to userspace in a way that is not correctly gated? Or
does the driver already handle this?

Looking at [0], I can see that there are multiple options here in PMSCR_EL2.PCT:

* PMSCR_EL2_PCT_VIRT = 0b00 - Virtual, offset by CNTVOFF_EL2 (unless EL2-owned
  then 0, but patch 19 already made it EL1-owned when guest sampling).

* PMSCR_EL2_PCT_PHYS = 0b01 - Defer to PMSCR_EL1.PCT if EL1-owned, otherwise
  phys. Define phys as the RAW physical counter (no offset).

* PMSCR_EL2_PCT_GUEST = 0b11 - Defer to PMSCR_EL1.PCT if EL1-owned.
  Virtual means raw phys minus CNTVOFF_EL2. Phys means raw phys minus
  CNTPOFF_EL2 when ECV is enabled, otherwise no offset.

And [1], [2] tells me that there are offsets to take into account (ugh)
which I see can be set via KVM_ARM_SET_COUNTER_OFFSET, and these counters will
be what is used to correlate the data with other perf counters.

As far as I can make out from the timer code, if there's no CNTPOFF_EL2
but userspace has set a physical offset, KVM traps the guest's CNTPCT
reads and applies the offset in software, i.e. in timer_set_traps():

    /*
     * We have two possibility to deal with a physical offset:
     *
     * - Either we have CNTPOFF (yay!) or the offset is 0:
     *	 we let the guest freely access the HW
     *
     * - or neither of these condition apply:
     *	 we trap accesses to the HW, but still use it
     *	 after correcting the physical offset
     */
    if (!has_cntpoff() && timer_get_offset(map->direct_ptimer))
	tpt = tpc = true;

So just accepting physical timestamps in SPE would mean they'd be offset
from the physical counter.

Where CNTPOFF_EL2 exists, PMSCR_EL2_PCT_GUEST does the right thing in
both directions - virtual minus CNTVOFF_EL2, physical minus
CNTPOFF_EL2).

But you're kinda screwed if it doesn't, so the only way for the timestamps
to be consistent with what's expected is to force a virtual offset.

So the code should look like:

    u64 pct;

    ...

    /* Without CNTPOFF_EL2 physical timestamps can't be offset, so deny them. */
    pct = has_cntpoff() ? PMSCR_EL2_PCT_GUEST : PMSCR_EL2_PCT_VIRT;
    kvm_spe->guest_pmscr_el2 = FIELD_PREP(PMSCR_EL2_PCT, pct);

I think.

> +
>  	return 0;
>  }
>
> --
> 2.43.0
>
>

--
Cheers, Lorenzo

[0]:https://support.arm.com/documentation/ddi0601/2026-09/AArch64-Registers/PMSCR-EL2--Statistical-Profiling-Control-Register--EL2-?lang=en
[1]:https://support.arm.com/documentation/ddi0601/2026-09/AArch64-Registers/CNTVOFF-EL2--Counter-timer-Virtual-Offset-Register?lang=en
[2]:https://support.arm.com/documentation/ddi0601/2026-09/AArch64-Registers/CNTPOFF-EL2--Counter-timer-Physical-Offset-Register?lang=en