[PATCH v2] LoongArch: KVM: Reload one-shot TVAL on migration destination

[PATCH v2] LoongArch: KVM: Reload one-shot TVAL on migration destination

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

kvm_restore_timer() rebuilds the remaining timer countdown from
vcpu->arch.expire, which is host-internal and is not part of the migrated
vCPU state. On the migration destination it is still 0, so for a one-shot
timer that has not expired yet the computed delta is 0 and
write_gcsr_timertick(0) injects the timer interrupt immediately instead of
after the remaining time.

The expired one-shot case (TVAL = -1) is already handled earlier. When
expire has not been set (i.e. on the migration destination), reload the
remaining countdown from the migrated TVAL. The regular preempt/resume
path on the source, where expire is valid, is unchanged.

Suggested-by: Bibo Mao <maobibo@loongson.cn>
Fixes: a5857b9ff6e0 ("LoongArch: KVM: Implement vcpu timer operations")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>

---

Changes in v2:
- Move the expire check before ktime_before() (suggested by Bibo Mao)
- Remove the old else-if branch that was one-shot only.

---
 arch/loongarch/kvm/timer.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/loongarch/kvm/timer.c b/arch/loongarch/kvm/timer.c
index 3829f35a4070..08359a612bde 100644
--- a/arch/loongarch/kvm/timer.c
+++ b/arch/loongarch/kvm/timer.c
@@ -119,6 +119,17 @@ void kvm_restore_timer(struct kvm_vcpu *vcpu)
 	delta = 0;
 	now = ktime_get();
 	expire = vcpu->arch.expire;
+	if (!expire) {
+		/*
+		 * vcpu->arch.expire is host-internal and is not migrated,
+		 * so it is 0 after migration. Reload the remaining countdown
+		 * from the migrated TVAL.
+		 */
+		if (ticks < cfg)
+			delta = tick_to_ns(vcpu, ticks);
+		expire = ktime_add_ns(ktime_get(), delta);
+	}
+
 	if (ktime_before(now, expire))
 		delta = ktime_to_tick(vcpu, ktime_sub(expire, now));
 	else if (cfg & CSR_TCFG_PERIOD) {
-- 
2.43.0

Re: [PATCH v2] LoongArch: KVM: Reload one-shot TVAL on migration destination

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Redundant ktime_get() call introduces a race condition and non-deterministic timer inflation.

Pre-existing issues:
- [High] Unvalidated guest-controlled timer period leads to a division by zero and host kernel panic.
--

--- Patch [1]: [PATCH v2] LoongArch: KVM: Reload one-shot TVAL on migration destination ---
commit 76bdf1c5f64a97f768f72eb78cf6f70484cc8af9
Author: Tao Cui <cuitao@kylinos.cn>

LoongArch: KVM: Reload one-shot TVAL on migration destination

This commit handles restoring one-shot timers on migration destinations where
vcpu->arch.expire is uninitialized. It calculates the remaining countdown
using the migrated TVAL.

> diff --git a/arch/loongarch/kvm/timer.c b/arch/loongarch/kvm/timer.c
> index 3829f35a4070b..08359a612bdee 100644
> --- a/arch/loongarch/kvm/timer.c
> +++ b/arch/loongarch/kvm/timer.c

[ ... ]

> @@ -119,6 +119,17 @@ void kvm_restore_timer(struct kvm_vcpu *vcpu)
>  	delta = 0;
>  	now = ktime_get();
>  	expire = vcpu->arch.expire;
> +	if (!expire) {
> +		/*
> +		 * vcpu->arch.expire is host-internal and is not migrated,
> +		 * so it is 0 after migration. Reload the remaining countdown
> +		 * from the migrated TVAL.
> +		 */
> +		if (ticks < cfg)
> +			delta = tick_to_ns(vcpu, ticks);
> +		expire = ktime_add_ns(ktime_get(), delta);

[Severity: Medium]
Is it intentional to call ktime_get() again here instead of reusing the 'now'
timestamp captured just above?

If the host thread is preempted or delayed between capturing 'now' and
evaluating this second ktime_get(), the execution delay is effectively added
to the remaining timer duration.

For an already expired one-shot timer, could this cause the code to program
a small positive tick value instead of 0, delaying the interrupt injection?

> +	}
> +
>  	if (ktime_before(now, expire))
>  		delta = ktime_to_tick(vcpu, ktime_sub(expire, now));
>  	else if (cfg & CSR_TCFG_PERIOD) {
> 		period = cfg & CSR_TCFG_VAL;
> 		delta = ktime_to_tick(vcpu, ktime_sub(now, expire));
> 		delta = period - (delta % period);

[Severity: High]
This isn't a bug introduced by this patch, but does this code allow a guest
to trigger a host kernel panic?

If a malicious guest writes to LOONGARCH_CSR_TCFG, setting the CSR_TCFG_PERIOD
bit but leaving the CSR_TCFG_VAL bits as 0, 'period' will evaluate to 0.

When the vCPU is scheduled out and later restored, this branch executes
because the timer is past its expiration, resulting in a modulo by zero
operation.

Could this division by zero in the host kernel be prevented by validating the
guest-controlled timer period?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715072952.1433171-1-cui.tao@linux.dev?part=1