Re: [PATCH 19/60] kvm: Introduce accessors for kvm_vcpu->mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly introduced accessors `kvm_vcpu_mode()` and `kvm_vcpu_set_mode()` perform plain memory accesses, which causes the inadvertent removal of `READ_ONCE()` and `WRITE_ONCE()` annotations from existing concurrent code paths.
--

commit e412a3aa4c759d9d7837663a68f8f88fd426f534
Author: Joerg Roedel <joerg.roedel@amd.com>

kvm: Introduce accessors for kvm_vcpu->mode

Introduce accessors to make it easier to move this member of struct kvm_vcpu.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -2272,7 +2272,7 @@ static inline bool kvm_vcpu_exit_request(struct kvm_vcpu *vcpu)
>  {
>  	xfer_to_guest_mode_prepare();
>  
> -	return READ_ONCE(vcpu->mode) == EXITING_GUEST_MODE ||
> +	return kvm_vcpu_mode(vcpu) == EXITING_GUEST_MODE ||
>  	       kvm_request_pending(vcpu) || xfer_to_guest_mode_work_pending();
>  }

[Severity: High]
Does the removal of READ_ONCE() here expose this to compiler optimization
issues?

Because kvm_vcpu_mode() is implemented as a plain read, this change silently
drops the data race prevention annotation. The vcpu->mode variable is accessed
locklessly and concurrently by multiple threads.

Without READ_ONCE(), could the compiler perform unsafe optimizations like
load-hoisting that lead to missed exit requests or KCSAN warnings?

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
[ ... ]
> @@ -3959,8 +3959,8 @@ void __kvm_vcpu_kick(struct kvm_vcpu *vcpu, bool wait)
>  	 * within the vCPU thread itself.
>  	 */
>  	if (vcpu == kvm_get_running_vcpu()) {
> -		if (vcpu->mode == IN_GUEST_MODE)
> -			WRITE_ONCE(vcpu->mode, EXITING_GUEST_MODE);
> +		if (kvm_vcpu_mode(vcpu) == IN_GUEST_MODE)
> +			kvm_vcpu_set_mode(vcpu, EXITING_GUEST_MODE);
>  		goto out;
>  	}

[Severity: High]
Can this plain write cause store-tearing or dropped wakeups?

Similar to the read in kvm_vcpu_exit_request(), replacing WRITE_ONCE() with
kvm_vcpu_set_mode() in __kvm_vcpu_kick() drops the compiler annotation since
the new setter performs a plain write.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=19