Re: [PATCH v1 3/4] KVM: arm64: vgic: Do not access the GICv5 CPU interface from EL1

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

Pre-existing issues:
- [High] Missing Context Synchronization Event (CSE) after writing to SYS_ICC_ICSR_EL1.
--

--- Patch [3]: [PATCH v1 3/4] KVM: arm64: vgic: Do not access the GICv5 CPU interface from EL1 ---
commit 5b0cdbb37754741e9006b6abdf408b239ed6b887
Author: Fuad Tabba <[email protected]>

KVM: arm64: vgic: Do not access the GICv5 CPU interface from EL1

This commit fixes an issue where the GICv5 CPU interface was incorrectly
accessed from EL1 on systems without FEAT_GCIE_LEGACY. It restricts GICv5
access to VHE, similar to the existing GICv3 handling.

> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> index 74bace10a22ed..df58c0042ed63 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
> @@ -1042,11 +1042,15 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
>  static inline bool can_access_vgic_from_kernel(void)
>  {
>  	/*
> -	 * GICv2 can always be accessed from the kernel because it is
> -	 * memory-mapped, and VHE systems can access GICv3 EL2 system
> -	 * registers.
> +	 * GICv3 and GICv5 drive the CPU interface through EL2 system
> +	 * registers, so only VHE reaches them from the kernel. GICv2 is
> +	 * memory-mapped and always reachable.
>  	 */
> -	return !static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif) || has_vhe();
> +	if (kvm_vgic_global_state.type == VGIC_V5 ||
> +	    static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif))
> +		return has_vhe();
> +
> +	return true;
>  }

While reviewing the GICv5 access paths related to this change, I noticed a
related issue in the GICv5 context restore path:

arch/arm64/kvm/hyp/vgic-v5-sr.c:__vgic_v5_restore_state() {
    write_sysreg_s(cpu_if->vgic_icsr, SYS_ICC_ICSR_EL1);
}

[Severity: High]
This isn't a bug introduced by this patch, but does this write to
SYS_ICC_ICSR_EL1 require a subsequent isb() instruction?

According to the ARM64 KVM architectural rules, any write to an ICC_*_EL1
register (excluding ICC_PMR_EL1) must be immediately followed by an isb()
as the very next instruction. 

Because __vgic_v5_restore_state() writes to SYS_ICC_ICSR_EL1 and then
returns, the caller will execute subsequent instructions without a Context
Synchronization Event. Could this leave the CPU operating with an
architecturally undefined pipeline state during the vCPU switch?

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