Re: [PATCH v18 04/14] KVM: arm64: Ensure GCS memory effects are visible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Coherency bug and asynchronous memory corruption risk in VHE mode due to delayed `gcsb_dsync()`.
- [High] Logical ordering and missing synchronization (ISB) violation when executing `gcsb_dsync()` in VHE exit path.
- [Medium] Incorrect memory barrier ordering around `gcsb_dsync()` on guest exit/load paths leaves normal memory accesses unsynchronized.
--

--- Patch [4]: [PATCH v18 04/14] KVM: arm64: Ensure GCS memory effects are visible ---
commit 1407ce4bfae0ac123f8195fde84320f01f5496bb
Author: Mark Brown <broonie@kernel.org>

KVM: arm64: Ensure GCS memory effects are visible

This commit inserts explicit GCSB DSYNC barriers during vCPU state
loading and saving to ensure GCS memory effects are visible. This is
required because the hypervisor cannot rely on implicit barriers in
vCPU context switches.

> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c

[ ... ]

> @@ -345,6 +351,13 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  	 */
>  	dsb(nsh);
>  
> +	/*
> +	 * Ensure any GCS memory effects from the outgoing vCPU are
> +	 * visible elsewhere.
> +	 */
> +	if (ctxt_has_gcs(guest_ctxt))
> +		gcsb_dsync();

[Severity: Medium]
Does this sequence require a DSB after the GCSB DSYNC instruction?

To guarantee that GCS memory effects are globally visible before subsequent
operations like page table unmapping, a DSB is typically required after
the GCSB DSYNC. In __kvm_vcpu_run(), the DSB occurs before it.

> diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> @@ -239,6 +239,13 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
>  	if (vcpu_has_nv(vcpu))
>  		dsb(nsh);
>  
> +	/*
> +	 * Ensure any GCS memory effects are visible to the incoming
> +	 * vCPU.
> +	 */
> +	if (ctxt_has_gcs(guest_ctxt))
> +		gcsb_dsync();

[Severity: Medium]
Is another DSB needed here as well?

Similar to the nVHE path, a DSB after the GCSB DSYNC is needed to ensure
memory effects are globally visible. Also, the preceding dsb(nsh) is
conditionally skipped if the vCPU does not have nested virtualization enabled,
which might leave this barrier in __vcpu_load_switch_sysregs() without
preceding protection.

[ ... ]

> @@ -299,5 +306,12 @@ void __vcpu_put_switch_sysregs(struct kvm_vcpu *vcpu)
>  	/* Restore host user state */
>  	__sysreg_restore_user_state(host_ctxt);
>  
> +	/*
> +	 * Ensure any GCS memory effects from the outgoing vCPU are
> +	 * visible elsewhere.
> +	 */
> +	if (ctxt_has_gcs(guest_ctxt))
> +		gcsb_dsync();

[Severity: High]
Could placing this barrier in the lazy vcpu_put() path cause stale memory
reads in the host kernel?

In VHE mode, __vcpu_put_switch_sysregs() executes lazily during vcpu_put().
The host kernel will begin processing guest exits (hypercalls, page faults)
while the vCPU is still loaded. This leaves the CPU's GCS cache with dirty,
unwritten guest data while the host might attempt normal memory reads on that
same memory. 

Does this need to be in the immediate exit path instead?

[Severity: High]
Does this execute the barrier under the wrong GCS context and an undefined
pipeline state?

This gcsb_dsync() happens after __sysreg_restore_user_state(host_ctxt).
Restoring the host user state overwrites SYS_GCSCRE0_EL1 without a subsequent
ISB, violating system register synchronization rules and leaving the pipeline
architecturally undefined when the branch and GCSB DSYNC execute.

Additionally, flushing after tearing down the guest's GCS configuration means
the flush occurs under the host's context. 

Should gcsb_dsync() execute before restoring the host state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-arm64-gcs-v18-0-fd8c40f5ad88@kernel.org?part=4