Re: [PATCH v4 04/20] KVM: arm64: timers: Use CNTPOFF_EL2 to offset the physical timer

[Reiji Watanabe <reijiw@google.com>]

Hi Marc,

On Thu, Mar 30, 2023 at 06:47:44PM +0100, Marc Zyngier wrote:
> With ECV and CNTPOFF_EL2, it is very easy to offer an offset for
> the physical timer. So let's do just that.
> 
> Nothing can set the offset yet, so this should have no effect
> whatsoever (famous last words...).
> 
> Reviewed-by: Colton Lewis <coltonlewis@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/arch_timer.c          | 18 +++++++++++++++++-
>  arch/arm64/kvm/hypercalls.c          |  2 +-
>  include/clocksource/arm_arch_timer.h |  1 +
>  include/kvm/arm_arch_timer.h         |  2 ++
>  4 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 9515c645f03d..3118ea0a1b41 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -52,6 +52,11 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>  			      struct arch_timer_context *timer,
>  			      enum kvm_arch_timer_regs treg);
>  
> +static bool has_cntpoff(void)
> +{
> +	return (has_vhe() && cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF));
> +}
> +
>  u32 timer_get_ctl(struct arch_timer_context *ctxt)
>  {
>  	struct kvm_vcpu *vcpu = ctxt->vcpu;
> @@ -84,7 +89,7 @@ u64 timer_get_cval(struct arch_timer_context *ctxt)
>  
>  static u64 timer_get_offset(struct arch_timer_context *ctxt)
>  {
> -	if (ctxt->offset.vm_offset)
> +	if (ctxt && ctxt->offset.vm_offset)
>  		return *ctxt->offset.vm_offset;

Reviewed-by: Reiji Watanabe <reijiw@google.com>

Nit: This particular change appears to be unnecessary in this patch.
(needed in the following patches ?)

Thank you,
Reiji

>  
>  	return 0;
> @@ -432,6 +437,12 @@ static void set_cntvoff(u64 cntvoff)
>  	kvm_call_hyp(__kvm_timer_set_cntvoff, cntvoff);
>  }
>  
> +static void set_cntpoff(u64 cntpoff)
> +{
> +	if (has_cntpoff())
> +		write_sysreg_s(cntpoff, SYS_CNTPOFF_EL2);
> +}
> +
>  static void timer_save_state(struct arch_timer_context *ctx)
>  {
>  	struct arch_timer_cpu *timer = vcpu_timer(ctx->vcpu);
> @@ -480,6 +491,7 @@ static void timer_save_state(struct arch_timer_context *ctx)
>  		write_sysreg_el0(0, SYS_CNTP_CTL);
>  		isb();
>  
> +		set_cntpoff(0);
>  		break;
>  	case NR_KVM_TIMERS:
>  		BUG();
> @@ -550,6 +562,7 @@ static void timer_restore_state(struct arch_timer_context *ctx)
>  		write_sysreg_el0(timer_get_ctl(ctx), SYS_CNTV_CTL);
>  		break;
>  	case TIMER_PTIMER:
> +		set_cntpoff(timer_get_offset(ctx));
>  		write_sysreg_el0(timer_get_cval(ctx), SYS_CNTP_CVAL);
>  		isb();
>  		write_sysreg_el0(timer_get_ctl(ctx), SYS_CNTP_CTL);
> @@ -767,6 +780,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
>  	vtimer->vcpu = vcpu;
>  	vtimer->offset.vm_offset = &vcpu->kvm->arch.timer_data.voffset;
>  	ptimer->vcpu = vcpu;
> +	ptimer->offset.vm_offset = &vcpu->kvm->arch.timer_data.poffset;
>  
>  	/* Synchronize cntvoff across all vtimers of a VM. */
>  	timer_set_offset(vtimer, kvm_phys_timer_read());
> @@ -1297,6 +1311,8 @@ void kvm_timer_init_vhe(void)
>  	val = read_sysreg(cnthctl_el2);
>  	val |= (CNTHCTL_EL1PCEN << cnthctl_shift);
>  	val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
> +	if (cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF))
> +		val |= CNTHCTL_ECV;
>  	write_sysreg(val, cnthctl_el2);
>  }
>  
> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 5da884e11337..39a4707e081d 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -47,7 +47,7 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
>  		cycles = systime_snapshot.cycles - vcpu->kvm->arch.timer_data.voffset;
>  		break;
>  	case KVM_PTP_PHYS_COUNTER:
> -		cycles = systime_snapshot.cycles;
> +		cycles = systime_snapshot.cycles - vcpu->kvm->arch.timer_data.poffset;
>  		break;
>  	default:
>  		return;
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 057c8964aefb..cbbc9a6dc571 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -21,6 +21,7 @@
>  #define CNTHCTL_EVNTEN			(1 << 2)
>  #define CNTHCTL_EVNTDIR			(1 << 3)
>  #define CNTHCTL_EVNTI			(0xF << 4)
> +#define CNTHCTL_ECV			(1 << 12)
>  
>  enum arch_timer_reg {
>  	ARCH_TIMER_REG_CTRL,
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index 70d47c4adc6a..2dd0fd2406fb 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -34,6 +34,8 @@ struct arch_timer_offset {
>  struct arch_timer_vm_data {
>  	/* Offset applied to the virtual timer/counter */
>  	u64	voffset;
> +	/* Offset applied to the physical timer/counter */
> +	u64	poffset;
>  };
>  
>  struct arch_timer_context {
> -- 
> 2.34.1
>