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

[Marc Zyngier <maz@kernel.org>]

On Mon, 20 Jul 2026 14:46:28 +0100,
sashiko-bot@kernel.org wrote:
> 
> 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?

Yeah, that's borderline stupid. This should be "cpus_have_cap()".

> 
> 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()?

I guess that if that fails, we will already have failed much earlier,
while probing for TDIR. I'll drop that.

> 
> > +
> > +	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)?

It isn't missing. It is just that the relevant hunk is in the wrong
patch and needs to be moved here.

> 
> 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?

That's something that needs to be separately addressed. There was some
work from Ada a long while ago that needs to be brought back to life.

	M.

-- 
Without deviation from the norm, progress is not possible.