Re: [PATCH v2 11/20] KVM: s390: arm64: Query Available Arm features

[Janosch Frank <frankja@linux.ibm.com>]

On 8/31/26 9:46 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Critical] The EX_TABLE entry in the `qaaf` inline assembly incorrectly places the exception fixup label on the non-faulting `lhi` instruction instead of the potentially faulting `.insn` instruction, which will cause unhandled hardware exceptions to panic the kernel.
> - [High] `aef_sysfs_init()` unconditionally calls `aef_init_save_area()`, failing to skip it when the AEF facility is not present, causing a crash on older hardware.
> - [High] Off-by-two index error when accessing the QAAF registers array in `kvm_qaaf_read_ftr_reg()`, causing KVM to read incorrect memory offsets for all features.
> - [High] If QAAF initialization fails, the global capability flag `info.arm_guest_supp` is not cleared, leading KVM to initialize using uninitialized hardware capability data.
> --

[...]

>> diff --git a/arch/s390/kernel/aef.c b/arch/s390/kernel/aef.c
>> new file mode 100644
>> index 0000000000000..825e2643a8921
>> --- /dev/null
>> +++ b/arch/s390/kernel/aef.c
> [ ... ]
>> +static int  __init aef_query_info(void)
>> +{
>> +	int ret;
>> +
>> +	if (IS_ENABLED(CONFIG_KVM_S390_ARM64)) {
>> +		info.sae_avail = sclp.has_aef;
>> +		info.ptff_avail = ptff_query(PTFF_QAGTO) &&
>> +				  ptff_query(PTFF_QAGPT);
>> +		info.arm_guest_supp = info.sae_avail && info.ptff_avail;
>> +	}
>> +	if (!info.sae_avail)
>> +		return 0;
>> +
>> +	ret = qaaf(QAAF_FC_QMC, (union qaaf_block *)&qmc);
>> +	if (ret)
>> +		return ret;
> 
> [Severity: High]
> If the qaaf instruction fails and returns an error here, `info.arm_guest_supp`
> remains set to true.
> 
> Because aef_sysfs_init() is called via an arch_initcall, the initialization
> failure is ultimately ignored by the kernel. Later, kvm_s390_arm64_init()
> checks `aef_info()->arm_guest_supp` and will proceed to initialize KVM using
> the uninitialized `qmc` block.
> 
> Should `info.arm_guest_supp` be cleared before returning when qaaf() fails?

No, we can instead check the sclp bit for sae_avail and early return 
before the assignments.
I don't see a gain in having this information shadowed considering the 
two patchsets that are floating around. Maybe you need it at a later time?


sae_avail is also used in patch #20 where it makes even less sense.
Either the machine and KVM have sae capabilities or we don't need the 
sysfs files. So why even add it as a file?

It's not like UV where we need to opt in, it's either supported or not.
If it's not supported there's no need to have sysfs files.


> 
> [ ... ]
>> +static int __init aef_sysfs_init(void)

aef_info_init()?

>> +{
>> +	int rc = -ENOMEM;
>> +
>> +	if (!IS_ENABLED(CONFIG_KVM_S390_ARM64))
>> +		return 0;
>> +
>> +	rc = aef_query_info();
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = aef_init_save_area();
>> +	if (rc)
>> +		return rc;
> 
> [Severity: High]
> Will this execute the new qaaf instruction unconditionally on all hardware?
> 
> aef_query_info() correctly returns 0 if the AEF facility is absent (since
> `info.sae_avail` would be false). However, aef_init_save_area() is called
> immediately after without checking if the facility is actually supported.
> 
> Could this trigger an operation exception on older hardware?

>