diff mbox series

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

Message ID 20220824060304.21128-2-gankulkarni@os.amperecomputing.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: nv: Fixes for Nested Virtualization issues | expand

Commit Message

Ganapatrao Kulkarni Aug. 24, 2022, 6:03 a.m. UTC 
From: D Scott Phillips <scott@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@os.amperecomputing.com>
Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/arch_timer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Marc Zyngier Dec. 29, 2022, 1 p.m. UTC | #1 
On Wed, 24 Aug 2022 07:03:02 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> From: D Scott Phillips <scott@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@os.amperecomputing.com>
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@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.

Does the following work for you, mostly restoring the original code?

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)) {
 		soft_timer_cancel(&ctx->hrtimer);
 		return;
 	}
Ganapatrao Kulkarni Jan. 9, 2023, 12:25 p.m. UTC | #2 
Hi Marc,

On 29-12-2022 06:30 pm, Marc Zyngier wrote:
> On Wed, 24 Aug 2022 07:03:02 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> From: D Scott Phillips <scott@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@os.amperecomputing.com>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@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?

Thanks,
Ganapat
Marc Zyngier Jan. 9, 2023, 1:44 p.m. UTC | #3 
On Mon, 09 Jan 2023 12:25:13 +0000,
Ganapatrao Kulkarni <gankulkarni@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@os.amperecomputing.com> wrote:
> >> 
> >> From: D Scott Phillips <scott@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@os.amperecomputing.com>
> >> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@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).

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

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
Ganapatrao Kulkarni Jan. 9, 2023, 2:03 p.m. UTC | #4 
On 09-01-2023 07:14 pm, Marc Zyngier wrote:
> On Mon, 09 Jan 2023 12:25:13 +0000,
> Ganapatrao Kulkarni <gankulkarni@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@os.amperecomputing.com> wrote:
>>>>
>>>> From: D Scott Phillips <scott@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@os.amperecomputing.com>
>>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@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
diff mbox series

Patch

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)