Re: [PATCH 06/17] KVM: x86: Use gfn_to_pfn_cache for steal time / preempted status

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] build: Missing definitions for srcu_read_lock_atomic and synchronize_srcu_atomic
--

commit 73f7f7addf4a338b749e65de38df63e40795698c
Author: Carsten Stollmaier <stollmc@xxxxxxxxxx>

KVM: x86: Use gfn_to_pfn_cache for steal time / preempted status

This largely reverts commit 7e2175ebd695 ("KVM: x86: Fix recording of
guest steal time / preempted status"), which dropped the use of the
gfn_to_pfn_cache because it was not integrated with the MMU notifiers
at the time. That shortcoming has long since been addressed, making
the GPC work correctly for this use case.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3db1855aba798..116932e13d595 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2065,42 +2064,27 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
>  	if (WARN_ON_ONCE(current->mm != vcpu->kvm->mm))
>  		return;
>  
> -	slots = kvm_memslots(vcpu->kvm);
> +	/* We rely on the fact that it fits in a single page. */
> +	BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
>  
> -	if (unlikely(slots->generation != ghc->generation ||
> -		     gpa != ghc->gpa ||
> -		     kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
> -		/* We rely on the fact that it fits in a single page. */
> -		BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
> +	idx = srcu_read_lock_atomic(&vcpu->kvm->gpc_srcu);
> +	while (!kvm_gpc_check(gpc, sizeof(*st))) {
> +		srcu_read_unlock_atomic(&vcpu->kvm->gpc_srcu, idx);

[Severity: Low]
Are the definitions for srcu_read_lock_atomic(), srcu_read_unlock_atomic(),
and synchronize_srcu_atomic() missing from the kernel tree? Compiling this
patch results in an implicit declaration or undefined reference linker error.

[ ... ]

> @@ -2657,20 +2632,31 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
>  	if (unlikely(current->mm != vcpu->kvm->mm))
>  		return;
>  
> -	slots = kvm_memslots(vcpu->kvm);
> -
> -	if (unlikely(slots->generation != ghc->generation ||
> -		     gpa != ghc->gpa ||
> -		     kvm_is_error_hva(ghc->hva) || !ghc->memslot))
> -		return;
> -
> -	st = (struct kvm_steal_time __user *)ghc->hva;
> -	BUILD_BUG_ON(sizeof(st->preempted) != sizeof(preempted));
> -
> -	if (!copy_to_user_nofault(&st->preempted, &preempted, sizeof(preempted)))
> +	/*
> +	 * This is called from the scheduler path (via kvm_sched_out), so
> +	 * the cache cannot be refreshed here.  Setting 'preempted' is
> +	 * best-effort anyway; the old HVA-based code used
> +	 * copy_to_user_nofault(), which could also silently fail.  If the
> +	 * cache is invalid, it will be refreshed from task context by
> +	 * record_steal_time() before the vCPU next enters the guest, since
> +	 * kvm_arch_vcpu_load() unconditionally raises KVM_REQ_STEAL_UPDATE.
> +	 */
> +	guard(srcu_atomic)(&vcpu->kvm->gpc_srcu);

[Severity: Low]
Does the guard(srcu_atomic) definition in include/linux/srcu.h also reference
the undefined srcu_read_lock_atomic() function, which breaks the build?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920211920.928306-1-dwmw2@xxxxxxxxxxxxx?part=6