[PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Ganapatrao Kulkarni]

On 09-01-2023 07:14 pm, Marc Zyngier wrote:
> On Mon, 09 Jan 2023 12:25:13 +0000,
> Ganapatrao Kulkarni <gankulkarni at os.amperecomputing.com> wrote:
>>
>>
>> Hi Marc,
>>
>> On 29-12-2022 06:30 pm, Marc Zyngier wrote:
>>> On Wed, 24 Aug 2022 07:03:02 +0100,
>>> Ganapatrao Kulkarni <gankulkarni at os.amperecomputing.com> wrote:
>>>>
>>>> From: D Scott Phillips <scott at os.amperecomputing.com>
>>>>
>>>> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
>>>> timer is being emulated.
>>>>
>>>> While the CPU is executing in L1 context, the L2 timers are emulated using
>>>> host hrtimer. When the delta of cval and current time reaches zero, the
>>>> vtimer interrupt is fired/forwarded to L2, however the emulation function
>>>> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
>>>> set to now, triggering hrtimer to fire immediately and resulting in a
>>>> continuous trigger of hrtimer and endless looping in the timer emulation.
>>>>
>>>> Adding a fix to avoid restarting of the hrtimer if the interrupt is
>>>> already fired.
>>>>
>>>> Signed-off-by: D Scott Phillips <scott at os.amperecomputing.com>
>>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni at os.amperecomputing.com>
>>>> ---
>>>>    arch/arm64/kvm/arch_timer.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>>> index 2371796b1ab5..27a6ec46803a 100644
>>>> --- a/arch/arm64/kvm/arch_timer.c
>>>> +++ b/arch/arm64/kvm/arch_timer.c
>>>> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>>    		return;
>>>>    	}
>>>>    -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>>>> +	if (!ctx->irq.level)
>>>> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>>>>    }
>>>>      static void timer_save_state(struct arch_timer_context *ctx)
>>>
>>> I think this is a regression introduced by bee038a67487 ("KVM:
>>> arm/arm64: Rework the timer code to use a timer_map"), and you can see
>>> it because the comment in this function doesn't make much sense
>>> anymore.
>>
>> OK, check was removed while rework in bee038a67487.
>>>
>>> Does the following work for you, mostly restoring the original code?
>>
>> Below diff too works and avoids the unnecessary soft timer restarts
>> with zero delta.
>>>
>>> Thanks,
>>>
>>> 	M.
>>>
>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>> index ad2a5df88810..4945c5b96f05 100644
>>> --- a/arch/arm64/kvm/arch_timer.c
>>> +++ b/arch/arm64/kvm/arch_timer.c
>>> @@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>    	 * scheduled for the future.  If the timer cannot fire at all,
>>>    	 * then we also don't need a soft timer.
>>>    	 */
>>> -	if (!kvm_timer_irq_can_fire(ctx)) {
>>> +	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {
>>
>> Now, aligns to comment.
>>>    		soft_timer_cancel(&ctx->hrtimer);
>>>    		return;
>>>    	}
>>>
>>
>> Shall I resend this patch as regression fix of bee038a67487?
> 
> I already have a patch written for this at [1], also getting rid of
> the soft_timer_cancel() call in the process (as it doesn't make much
> sense either).

OK, thanks.
> 
> Please give it a go if you have a chance (though the whole branch
> might be of interest to you...).

Sure, I will try this branch.
> 
> Thanks,
> 
> 	M.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/nv-6.2-WIP&id=effdcfa175c374a1740f60642d221ad2e930c978
> 

Thanks,
Ganapat