Re: [PATCH v2 8/8] KVM: arm64: Add VNCR TLB tracking again

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The fast-path TLBI handler incorrectly skips the slow path for native EL2 TLBI instructions when E2H or TGE is clear, bypassing software VNCR TLB invalidation and leaving stale mappings.
--

--- Patch [8]: [PATCH v2 8/8] KVM: arm64: Add VNCR TLB tracking again ---
commit 65741607ee5db3e8d57cf0e1773bb1031e83a3ee
Author: Marc Zyngier <[email protected]>

KVM: arm64: Add VNCR TLB tracking again

This commit introduces a VM-wide counter to track valid VNCR TLBs. It aims
to improve performance by taking the TLBI slow path only when there are
active VNCR TLBs that need invalidation.

> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
[ ... ]
> @@ -424,10 +424,15 @@ static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code)
>  		return false;
>  
>  	/*
> -	 * If we have to check for any VNCR mapping being invalidated,
> -	 * go back to the slow path for further processing.
> +	 * If we have to check for any VNCR TLB being invalidated, go back
> +	 * to the slow path for further processing.
> +	 *
> +	 * The synchronisation betweem TLBI and walk is provided by the
> +	 * speculative increment of the TLB counter on walk, and the
> +	 * invalidation counter. Yes, this is fiddly.
>  	 */
> -	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
> +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu) &&
> +	    atomic_read(&vcpu->kvm->arch.vncr_tlb_count))
>  		return false;

[Severity: High]
This is a pre-existing issue, but does this condition incorrectly skip the
slow path for native EL2 TLBI instructions when E2H or TGE is clear?

If a nested hypervisor running in nVHE mode (E2H=0) or temporarily clearing
TGE executes an S1E2 TLBI instruction, kvm_hyp_handle_tlbi_el2() will execute
the hardware TLBI. However, since the fallback check requires both E2H and TGE
to be set, it evaluates to false. This returns true and bypasses the software
VNCR TLB invalidation.

Should the check instead read atomic_read(&vcpu->kvm->arch.vncr_tlb_count)
unconditionally for any successful EL2 S1 TLBI so that stale software VNCR
mappings aren't left behind?

>  
>  	__kvm_skip_instr(vcpu);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8