Re: [PATCH v2 1/7] KVM: arm64: Move OUTSIDE_GUEST_MODE publication past context being saved

[Will Deacon <will@kernel.org>]

Hi folks,

Sorry, but this is probably an incredibly unhelpful drive-by comment
but Marc was talking about vcpu->mode the other day and I couldn't
resist looking at it some more. Like a moth to a flame...

See below.

On Tue, Sep 29, 2026 at 03:13:28PM +0100, Marc Zyngier wrote:
> On Tue, 29 Sep 2026 13:59:23 +0100,
> Fuad Tabba <fuad.tabba@linux.dev> wrote:
> > On Tue, 29 Sep 2026 10:35:42 +0100, Marc Zyngier <maz@kernel.org> wrote:
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > [...]
> > > @@ -1386,6 +1385,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> > >
> > >               kvm_arch_vcpu_ctxsync_fp(vcpu);
> > >
> > > +             /*
> > > +              * All the state has been synchronised, let advertise
> > > +              * we're outside of the guest.
> > > +              */
> > > +             smp_store_release(&vcpu->mode, OUTSIDE_GUEST_MODE);
> > 
> > Pardon my atomics :)
> 
> This is not an atomic instruction. However, it composes with atomics.
> 
> > , but what does the release pair with? On the halt
> > path, the only reader I can find is the cmpxchg() in
> > kvm_vcpu_exiting_guest_mode()
> 
> From Documentation/atomic_t.txt:
> 
> <quote>
>  - RMW operations that have a return value are fully ordered;
> 
>  - RMW operations that are conditional are unordered on FAILURE,
>    otherwise the above rules apply.
> </quote>
> 
> The acquire side of cmpxchg() is therefore interacting with the above
> release, which gives us the required ordering.
> 
> However, there is a problem if cmpxchg() fails, as there is no
> ordering in that case, and I'm not sure the smp_mb__before_atomic()
> saves the bacon in that case. It feels we'd need an acquire
> somewhere, a bit like this:

(as discussed off list, you can use smp_acquire__after_ctrl_dep() if
you're feeling really brave)

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 03bfc92864b6e..2efb4febcb235 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -563,9 +563,15 @@ static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
>  	 * The memory barrier ensures a previous write to vcpu->requests cannot
>  	 * be reordered with the read of vcpu->mode.  It pairs with the general
>  	 * memory barrier following the write of vcpu->mode in VCPU RUN.
> +	 *
> +	 * cmpxchg() is not ordered when failing, so make sure we perform an
> +	 * acquire in that case.
>  	 */
>  	smp_mb__before_atomic();
> -	return cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE);
> +	if (cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE) != IN_GUEST_MODE)
> +		return smp_load_acquire(&vcpu->mode);
> +
> +	return IN_GUEST_MODE;
>  }
>  
>  /*
> 
> > , and the LPI-disable and MOVALL halts
> > then take ap_list_lock or irq_lock. Would WRITE_ONCE() be enough?
> 
> We need a release so that we know for sure that any state stored
> before is visible by the time we can observe OUTSIDE_GUEST_MODE, and
> WRITE_ONCE() doesn't provide that (it can be reordered).
> 
> I don't see what taking a lock changes to the ordering requirement.
> 
> >
> > Should the early exit path (the kvm_vcpu_exit_request() bail-out) get
> > the same treatment? I think that's what Sashiko is trying to say in
> > the patch 5 review [1].
> 
> I don't understand what sashiko is trying to say, but this is clearly
> missing from the patch, see below. Not sure how I missed that one.
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 9a4871cd796bc..1a3a15bc6f55c 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1333,13 +1333,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  		smp_store_mb(vcpu->mode, IN_GUEST_MODE);
>  
>  		if (ret <= 0 || kvm_vcpu_exit_request(vcpu, &ret)) {
> -			vcpu->mode = OUTSIDE_GUEST_MODE;
>  			isb(); /* Ensure work in x_flush_hwstate is committed */
>  			if (kvm_vcpu_has_pmu(vcpu))
>  				kvm_pmu_sync_hwstate(vcpu);
>  			if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
>  				kvm_timer_sync_user(vcpu);
>  			kvm_vgic_sync_hwstate(vcpu);
> +			smp_store_release(&vcpu->mode, OUTSIDE_GUEST_MODE);

I'm struggling to see why a release is sufficient here, but I'm also
struggling to understand the bigger picture so I'm probably just confused.

I can see why a release is necessary for the saved state to be visible
to another CPU that has kicked the vCPU out of the guest and then uses
vcpu->mode == OUTSIDE_GUEST_MODE as the indication that the state is
safe to consume. However, don't we also need to make sure that any
subsequent check for a pending request on _this_ vCPU is ordered after
that write to the mode? Now that we've toggled it away from IN_GUEST_MODE,
I think IPIs can be elided by the kick, so a subsequent call to
e.g. kvm_request_pending() must be observed after that toggle, otherwise
I think we could miss a request.

Can you see the tree I'm barking up here?

Will