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 08:27 pm, Ganapatrao Kulkarni wrote:
> 
> 
> 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?

I tried this and seems not working, this is due to timer save/restore 
are not called for some of the kvm_exit and entry paths(lighter switches).

I tried changing this patch like, Removed cval adjust from the kvm_entry 
and still cval is adjusted on kvm_exit and in timer_restore_state 
function, reduced cval by offset.

Please let me know, if this is not you intended to try?
If possible, please share the steps or pseudo code.

Thanks,
Ganapat

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