Re: [PATCH 2/5] KVM: arm64: Add a helper providing an inlined literal value for ICH_VTR_EL2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `kvm_patch_ich_vtr_el2()` helper fails to apply the `vgic_v3_broken_seis()` workaround mask to the `ICH_VTR_EL2` value.
- [High] The `noinstr` function `kvm_patch_ich_vtr_el2()` calls out-of-line instrumentable functions, which violates `.noinstr.text` safety rules.
--

--- Patch [2]: [PATCH 2/5] KVM: arm64: Add a helper providing an inlined literal value for ICH_VTR_EL2 ---
commit aedd950f23aaa5667277a7a495b598168c06eb84
Author: Marc Zyngier <maz@kernel.org>

KVM: arm64: Add a helper providing an inlined literal value for ICH_VTR_EL2

This commit adds an alternative patching callback to provide an inlined
literal value for the ICH_VTR_EL2 system register. It computes the value
at boot time and patches it into the relevant code locations.

> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 9e841e7afd4a7..ef62dd498f5cf 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -876,6 +876,57 @@ void noinstr kvm_compute_ich_hcr_trap_bits(struct alt_instr *alt,
>  	*updptr = cpu_to_le32(insn);
>  }
>  
> +void noinstr kvm_patch_ich_vtr_el2(struct alt_instr *alt,
> +				   __le32 *origptr, __le32 *updptr,
> +				   int nr_inst)
> +{
> +	struct arm_smccc_res res = {};
> +	u32 insn, oinsn, rd, vtr;
> +
> +	/* No KVM? Nothing to do */
> +	if (!is_hyp_mode_available())
> +		return;
> +
> +	/* No v3 (or v3 compat)? Nothing to do either */
> +	if (!this_cpu_has_cap(ARM64_HAS_GICV5_LEGACY) &&
> +	    !this_cpu_has_cap(ARM64_HAS_GICV3_CPUIF))
> +		return;

[Severity: High]
Is it safe to call out-of-line and instrumentable functions like
this_cpu_has_cap() from a noinstr context?

This violates the .noinstr.text safety rules and might expose the
alternative patching sequence to ftrace, KASAN, and kprobes, potentially
leading to recursive faults.

[ ... ]

> +	if (WARN_ON(res.a0 == HVC_STUB_ERR))
> +		return;

[Severity: High]
Could WARN_ON() also expand to out-of-line instrumentable trap or warning
functions, further violating the noinstr constraints of
kvm_patch_ich_vtr_el2()?

> +
> +	vtr = res.a1;
> +
> +	/* Compute target register */
> +	oinsn = le32_to_cpu(*origptr);

[Severity: High]
Does this sequence miss the workaround mask for broken locally generated
System Error Interrupts (SEI)?

The code reads SYS_ICH_VTR_EL2 and directly bakes it into instructions
without applying the vgic_v3_broken_seis() mask to clear the
ICH_VTR_EL2_SEIS bit. Affected hardware might incorrectly advertise SEI
support as a result.

> +	rd = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RD, oinsn);
> +
> +	/* movz rd, #(vtr & 0xffff) */
> +	insn = aarch64_insn_gen_movewide(rd,
> +					 (u16)vtr,
> +					 0,
> +					 AARCH64_INSN_VARIANT_64BIT,
> +					 AARCH64_INSN_MOVEWIDE_ZERO);
> +	*updptr++ = cpu_to_le32(insn);

[Severity: High]
Like this_cpu_has_cap() above, aarch64_insn_decode_register() and
aarch64_insn_gen_movewide() are out-of-line C functions. Should these
calls be avoided inside a noinstr patching callback to prevent recursive
faulting during instruction patching?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720132220.2143486-1-maz@kernel.org?part=2