Re: [PATCH v2 10/20] KVM: pfncache: Add guest-mode pinning (GUEST_USES_PFN successor)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: pfncache: gpc_kick_pinned_vcpu() re-posts vcpu_req on self-refresh
--

commit 9027e5fc5e4260b0089300dc0cf3173581bbcfed
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: pfncache: Add guest-mode pinning (GUEST_USES_PFN successor)

This commit adds guest-mode pinning support to gfn_to_pfn_cache so that
caches whose pfn is used directly from guest mode force the vCPU out of
guest mode and post a per-cache service request upon invalidation or
mutation.

> diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
> index 1c98b7bdbf09c..5cb30525e577c 100644
> --- a/virt/kvm/pfncache.c
> +++ b/virt/kvm/pfncache.c
> @@ -25,7 +25,9 @@
>  void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
>  				       unsigned long end)
>  {
> +	DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS);
>  	struct gfn_to_pfn_cache *gpc;
> +	bool evict_vcpus = false;
>  	bool cleared = false;
>  
>  	spin_lock(&kvm->gpc_lock);
> @@ -127,10 +129,60 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
>  			 */
>  			if (old & (GPC_VALID | GPC_INVALIDATING))
>  				cleared = true;
> +
> +			/*
> +			 * If a vCPU has the pfn pinned for direct use from
> +			 * guest mode, it must be forced out of guest mode
> +			 * before the invalidation may complete. The pin bit
> +			 * was set with a cmpxchg only while GPC_VALID held,
> +			 * so this same read of 'old' cannot miss it. It is
> +			 * NOT cleared here: only the vCPU itself (on request
> +			 * service, before re-pinning) or a mutator's xchg
> +			 * (which inherits the kick duty) may clear it. A
> +			 * stale pin costs at most a spurious request bit:
> +			 * kvm_make_vcpus_request_mask() only IPIs and waits
> +			 * for targets actually in guest mode.
> +			 */
> +			if (old & GPC_GUEST_USING) {
> +				if (!evict_vcpus) {
> +					evict_vcpus = true;
> +					bitmap_zero(vcpu_bitmap, KVM_MAX_VCPUS);
> +				}
> +				__set_bit(gpc->vcpu->vcpu_idx, vcpu_bitmap);
> +				/*
> +				 * Set this cache's own service request (what
> +				 * the vCPU must do before re-entering); the
> +				 * force-out below is common to all of them.
> +				 */
> +				__kvm_make_request(gpc->vcpu_req, gpc->vcpu);
> +			}
>  		}
>  	}
>  	spin_unlock(&kvm->gpc_lock);

[ ... ]

> @@ -224,6 +276,107 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
>  	return true;
>  }
>  
> +/*
> + * A mutator's xchg of the state word to zero may have consumed a live
> + * GPC_GUEST_USING pin. Clearing that bit obliges the clearer to do what
> + * the invalidation walk would have done: post the cache's service
> + * request and force the vCPU out of guest mode, waiting until it has
> + * left, before mutating or tearing down what the guest may be using.
> + *
> + * The service request is posted unconditionally: consuming a pin means
> + * whatever physical address the vCPU had latched (e.g. into a hardware
> + * control structure) is stale, and only the request handler makes that
> + * good. That holds even when the pinning vCPU is the caller itself —
> + * a vCPU refreshing its own pinned cache may re-map at a different pfn,
> + * and without the request nothing would rewrite the stale address
> + * before the next guest entry. Only the waited force-out is skipped in
> + * that case: a vCPU running a refresh is by definition not in guest
> + * mode. (The vCPU's own unpin is the one silent path, in
> + * kvm_gpc_unpin_for_guest(), because unpinning asserts the address is
> + * no longer latched anywhere.)
> + */
> +static void gpc_kick_pinned_vcpu(struct gfn_to_pfn_cache *gpc, int old_state)
> +{
> +	DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS);
> +
> +	if (!(old_state & GPC_GUEST_USING))
> +		return;
> +	if (WARN_ON_ONCE(!gpc->vcpu))
> +		return;
> +
> +	__kvm_make_request(gpc->vcpu_req, gpc->vcpu);
> +
> +	if (kvm_get_running_vcpu() == gpc->vcpu)
> +		return;

[Severity: Medium]
When gfn_to_pfn_cache_invalidate_start() invalidates a pinned cache, it
clears GPC_VALID, sets GPC_INVALIDATING, leaves GPC_GUEST_USING set in
gpc->state, and posts gpc->vcpu_req to gpc->vcpu:

	do {
		new = old & ~(GPC_VALID | GPC_BECOMING_VALID);
		if (old & GPC_VALID)
			new |= GPC_INVALIDATING;
	} while (!atomic_try_cmpxchg(&gpc->state, &old, new));

When gpc->vcpu subsequently services gpc->vcpu_req on the next guest entry
attempt, kvm_check_request() clears gpc->vcpu_req and the request handler
calls kvm_gpc_refresh() (or kvm_gpc_activate()) before re-pinning with
kvm_gpc_pin_for_guest(), as described in kvm_gpc_init_for_vcpu().

Because GPC_VALID is clear, __kvm_gpc_refresh() does not take the fast path
and instead consumes the leftover GPC_GUEST_USING bit via atomic_xchg():

	old_state = atomic_xchg(&gpc->state, GPC_INVALIDATING);
	gpc_kick_pinned_vcpu(gpc, old_state);

Won't gpc_kick_pinned_vcpu() see GPC_GUEST_USING in old_state and
unconditionally re-post gpc->vcpu_req to gpc->vcpu right in the middle of
handling that very request, causing the subsequent guest entry to abort and
run the request handler a second time (or loop if the handler does not hit
the __kvm_gpc_refresh() fast path on the next pass)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105020104.2507083-1-dwmw2@infradead.org?part=10