Re: [PATCH 4/6] LoongArch: KVM: Rebase steal time counter in vcpu context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: LoongArch: steal time drops first preemption delay due to 0 sentinel
--

commit c0824aa7f32e751198a471b8af7a3432b9a7ad2e
Author: Tao Cui <cuitao@kylinos.cn>

LoongArch: KVM: Rebase steal time counter in vcpu context

KVM_SET_DEVICE_ATTR(PVTIME GPA) initializes st.last_steal from the
ioctl thread's run_delay, but kvm_update_stolen_time() accumulates the
vcpu thread's run_delay; the first delta can be negative and wraps in
u64, so the guest reads a steal time close to 2^64.

Drop the initialization from the attr path and lazily rebase the
counter on the first steal update, which runs in vcpu context like the
hypercall path.

> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index ed9e092c97ba7..714bd87d1cea5 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
[ ... ]
> @@ -187,8 +188,14 @@ static void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
>  	smp_wmb();
>  
>  	unsafe_get_user(steal, &st->steal, out);
> -	steal += current->sched_info.run_delay - vcpu->arch.st.last_steal;
> -	vcpu->arch.st.last_steal = current->sched_info.run_delay;
> +	/* acquire pairs with the smp_wmb in the attr path */
> +	smp_rmb();
> +	last_steal = READ_ONCE(vcpu->arch.st.last_steal);
> +	if (!last_steal)
> +		/* first update in vcpu context: rebase the counter */
> +		last_steal = current->sched_info.run_delay;
> +	steal += current->sched_info.run_delay - last_steal;
> +	WRITE_ONCE(vcpu->arch.st.last_steal, current->sched_info.run_delay);

[Severity: Medium]
Since last_steal uses 0 as a sentinel value to indicate an uninitialized
baseline, what happens if a fresh VCPU thread's current->sched_info.run_delay
is exactly 0 during its first call to kvm_update_stolen_time()?

If run_delay is 0, last_steal remains 0. When the VCPU is later preempted
and run_delay grows, the next update will still see last_steal == 0.
It appears this treats it as an uninitialized state, rebasing to the new
run_delay without adding the preemption delay to the guest's steal time.
Does this permanently swallow the first preemption delay?

[ ... ]
> @@ -1204,8 +1211,10 @@ static int kvm_loongarch_pvtime_set_attr(struct kvm_vcpu *vcpu,
>  	srcu_read_unlock(&kvm->srcu, idx);
>  
>  	if (!ret) {
> -		vcpu->arch.st.guest_addr = gpa;
> -		vcpu->arch.st.last_steal = current->sched_info.run_delay;
> +		WRITE_ONCE(vcpu->arch.st.last_steal, 0);

[Severity: Medium]
This sets 0 as the sentinel, establishing the ambiguity between an
uninitialized state and a valid run_delay value of 0. Could a different
sentinel value (like U64_MAX) be used instead to avoid overlapping with
a zero run_delay?

> +		/* publish the new address only after clearing the rebase sentinel */
> +		smp_wmb();
> +		WRITE_ONCE(vcpu->arch.st.guest_addr, gpa);
>  		kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>  	}

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