Re: [PATCH 2/2] KVM: arm64: timers: Adjust CVAL of a ptimer across guest entry and exits

[Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>]

On 22-08-2023 05:46 pm, Marc Zyngier wrote:
> On Thu, 17 Aug 2023 10:27:55 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>>
>> Hi Marc,
>>
>> On 17-08-2023 01:57 pm, Marc Zyngier wrote:
>>> [Fixing Christoffer's email address]
>>
>> Thanks.
>>>
>>> On Thu, 17 Aug 2023 07:03:14 +0100,
>>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>>
>>>> As per FEAT_ECV, when HCR_EL2.{E2H, TGE} == {1, 1}, Enhanced Counter
>>>> Virtualization functionality is disabled and CNTPOFF_EL2 value is treated
>>>> as zero. On VHE host, E2H and TGE are set, hence it is required
>>>> to adjust CVAL by incrementing it by CNTPOFF_EL2 after guest
>>>> exit to avoid false physical timer interrupts and also
>>>> decrement/restore CVAL before the guest entry.
>>>
>>> No, this is wrong. Neither E2H nor TGE have any impact on writing to
>>> CNTPOFF_EL2, nor does it have an impact on CNTP_CVAL_EL0. Just read
>>> the pseudocode to convince yourself.
>>>
>>> CNTPOFF_EL2 is applied at exactly two points: when SW is reading
>>> CNTPCT_EL0 from EL1 while {E2H,TGE}=={1, 0} and when the HW is
>>> comparing CNTPCT_EL0 with the CNTP_CVAL_EL0. In both cases the offset
>>> is subtracted from the counter. And that's the point where the running
>>> EL matters. Which means that CNTPOFF_EL2 behaves exactly like
>>> CNTVOFF_EL2. No ifs, no buts.
>>
>> As per ARM ARM (ARM DDI 0487J.a page D11-5989)
>> "When FEAT_ECV is implemented, the CNTPOFF_EL2 register allows an
>> offset to be applied to the physical counter, as viewed from EL1 and
>> EL0, and to the EL1 physical timer. The functionality of this 64-bit
>> register is affected by CNTHCTL_EL2.ECV."
>>
>> As per ARM ARM (ARM DDI 0487J.a page D19-7857)
>> "When HCR_EL2.{E2H, TGE} == {1, 1} or SCR_EL3.{NS, EEL2} == {0, 0}, then
>> Enhanced Counter Virtualization functionality is disabled."
>>
>> "The EL1 physical timer interrupt is triggered when ((PCount<63:0> -
>> CNTPOFF_EL2<63:0>) - PCVal<63:0>) is greater than or equal to 0."
>>
>> As per ARM ARM (ARM DDI 0487J.a page D19-7938)
>> "When EL2 is implemented and enabled in the current Security state,
>> the physical counter uses a fixed physical offset of *zero* if any of
>> the following are true:
>> • CNTHCTL_EL2.ECV is 0.
>> • SCR_EL3.ECVEn is 0.
>> • HCR_EL2.{E2H, TGE} is {1, 1}."
>>
>> In VHE host hypervisor, E2H=TGE=1 hence ECV is disabled and Ptimer
>> interrupt is triggered based on PCount<63:0> - PCVal<63:0>
>>
>> Since cval is set by Guest as per offsetted PCounter value and pCount
>> is not subtracted by CNTPOFF when in VHE-L0, results in cval becoming
>> much lesser than physical counter(bumped up since CNTPOFF is zero) and
>> timer interrupt trigger condition is met falsely.
>>
>> There is no issue/impact on cval due to ECV, however it can be/is
>> manipulated to handle this on and off of CNTPOFF/ECV.
>>
>> IIUC, CNTPOFF and CNTVOFF are not same as per specification.
> 
> I owe you an apology. You are correct, and I was totally wrong.  I'm
> truly amazed how wrong we got this part of the architecture, but it is
> way too late for any change, and we'll have to live with it.
> 
Thanks Marc.

> Now, to the actual patch: I think the way you offset CVAL isn't
> great. You should never have to change it on entry, and you should
> instead read the correct value from memory. Then, save/restore of CVAL
> must be amended to always apply the offset. Can you give the hack
> below a go on your HW?

Sure, I will try this and update.

> 
> Thanks,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index ea46b4e1e7a8..bb80fdd84676 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -55,11 +55,6 @@ static struct irq_ops arch_timer_irq_ops = {
>   	.get_input_level = kvm_arch_timer_get_input_level,
>   };
>   
> -static bool has_cntpoff(void)
> -{
> -	return (has_vhe() && cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF));
> -}
> -
>   static int nr_timers(struct kvm_vcpu *vcpu)
>   {
>   	if (!vcpu_has_nv2(vcpu))
> @@ -180,7 +175,7 @@ u64 kvm_phys_timer_read(void)
>   	return timecounter->cc->read(timecounter->cc);
>   }
>   
> -static void get_timer_map(struct kvm_vcpu *vcpu, struct timer_map *map)
> +void get_timer_map(struct kvm_vcpu *vcpu, struct timer_map *map)
>   {
>   	if (vcpu_has_nv2(vcpu)) {
>   		if (is_hyp_ctxt(vcpu)) {
> @@ -569,8 +564,7 @@ static void timer_save_state(struct arch_timer_context *ctx)
>   		timer_set_ctl(ctx, read_sysreg_el0(SYS_CNTP_CTL));
>   		cval = read_sysreg_el0(SYS_CNTP_CVAL);
>   
> -		if (!has_cntpoff())
> -			cval -= timer_get_offset(ctx);
> +		cval -= timer_get_offset(ctx);
>   
>   		timer_set_cval(ctx, cval);
>   
> @@ -657,8 +651,7 @@ static void timer_restore_state(struct arch_timer_context *ctx)
>   		cval = timer_get_cval(ctx);
>   		offset = timer_get_offset(ctx);
>   		set_cntpoff(offset);
> -		if (!has_cntpoff())
> -			cval += offset;
> +		cval += offset;
>   		write_sysreg_el0(cval, SYS_CNTP_CVAL);
>   		isb();
>   		write_sysreg_el0(timer_get_ctl(ctx), SYS_CNTP_CTL);
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index 9611b4eaf661..6e3d3e16563f 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -90,6 +90,20 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>   
>   	___activate_traps(vcpu, hcr);
>   
> +	if (has_cntpoff()) {
> +		struct timer_map map;
> +
> +		get_timer_map(vcpu, &map);
> +
> +		if (map.direct_ptimer == vcpu_ptimer(vcpu))
> +			val = __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0);
> +		else
> +			val = __vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2);
> +
> +		isb();
> +		write_sysreg_s(val, SYS_CNTP_CVAL_EL0);
> +	}
> +
>   	val = read_sysreg(cpacr_el1);
>   	val |= CPACR_ELx_TTA;
>   	val &= ~(CPACR_EL1_ZEN_EL0EN | CPACR_EL1_ZEN_EL1EN |
> @@ -131,6 +145,23 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
>   
>   	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
>   
> +	if (has_cntpoff()) {
> +		struct timer_map map;
> +		u64 val, offset;
> +
> +		get_timer_map(vcpu, &map);
> +
> +		val = read_sysreg_s(SYS_CNTP_CVAL_EL0);
> +		if (map.direct_ptimer == vcpu_ptimer(vcpu))
> +			__vcpu_sys_reg(vcpu, CNTP_CVAL_EL0) = val;
> +		else
> +			__vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2) = val;
> +
> +		offset = read_sysreg_s(SYS_CNTPOFF_EL2);
> +		write_sysreg_s(val + offset, SYS_CNTP_CVAL_EL0);
> +		isb();
> +	}
> +
>   	/*
>   	 * ARM errata 1165522 and 1530923 require the actual execution of the
>   	 * above before we can switch to the EL2/EL0 translation regime used by
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index ea77a569a907..86a73ad1446a 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -82,6 +82,8 @@ struct timer_map {
>   	struct arch_timer_context *emul_ptimer;
>   };
>   
> +void get_timer_map(struct kvm_vcpu *vcpu, struct timer_map *map);
> +
>   struct arch_timer_cpu {
>   	struct arch_timer_context timers[NR_KVM_TIMERS];
>   
> @@ -149,4 +151,9 @@ void kvm_timer_cpu_down(void);
>   /* CNTKCTL_EL1 valid bits as of DDI0476J.a */
>   #define CNTKCTL_VALID_BITS	(BIT(17) | GENMASK_ULL(9, 0))
>   
> +static inline bool has_cntpoff(void)
> +{
> +	return (has_vhe() && cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF));
> +}
> +
>   #endif
> 

Thanks,
Ganapat