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

[sashiko-bot@kernel.org]

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

commit a7cb3705acf7ea96c94b5937368fbc6655fb99e7
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: s390: arm64: Query Available Arm features

This commit introduces the QAAF (Query Available Arm Features) instruction
support for s390 hosts. It allows KVM to query machine-supported ARM features
and provides an initialized save area for running ARM guests.

> diff --git a/arch/s390/include/asm/kvm_host_arm64_types.h b/arch/s390/include/asm/kvm_host_arm64_types.h
> index 2c922ef478d13..93fd649a26f47 100644
> --- a/arch/s390/include/asm/kvm_host_arm64_types.h
> +++ b/arch/s390/include/asm/kvm_host_arm64_types.h
[ ... ]
> +/* QAAF Query Model Capabilities */
> +struct qaaf_qmc_block {
> +	u64	_0000;			/* 0x0000 */
> +	u8	ssdf;			/* 0x0008 */
> +	u8	_0009;			/* 0x0009 */
> +	u8	ssaf;			/* 0x000a */
> +	u8	_000b[3];		/* 0x000b */
> +	u16	maxncpu;		/* 0x000e */
> +	u64	regs[0x1fe];		/* 0x0010 */
> +} __aligned(PAGE_SIZE);

[ ... ]
> +enum {
> +	QAAF_REG_MIDR_EL1		= 0x02,
[ ... ]
> +};

[ ... ]
> diff --git a/arch/s390/include/asm/sae.h b/arch/s390/include/asm/sae.h
> index 6580536544eef..f88d15b8f1e90 100644
> --- a/arch/s390/include/asm/sae.h
> +++ b/arch/s390/include/asm/sae.h
[ ... ]
> +static __always_inline int qaaf(u64 gr0, union qaaf_block *qaaf_block)
> +{
> +	int exception = 1;
> +
> +	asm volatile(
> +		"	lgr	0,%[r0]\n"
> +		"	.insn	rre,0xb9ad0000,%[r1],0\n"
> +		"0:	lhi	%[exc],0\n"
> +		"1:\n"
> +		EX_TABLE(0b, 1b)
> +		: "=m"(*qaaf_block), [exc] "+d"(exception)
> +		: [r1] "a"(qaaf_block), [r0] "d"(gr0)
> +		: "r0"
> +	);
> +
> +	return exception;
> +}
> +

[Severity: Critical]
Does the exception table entry point to the wrong instruction? 

The EX_TABLE(0b, 1b) targets the `lhi` instruction at label `0:`, but the
instruction that would actually fault is the preceding `.insn` instruction.
If this executes on hardware without the AEF feature, won't this result in
an unhandled hardware exception and a kernel panic?

[ ... ]
> 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?

[ ... ]
> +static int __init aef_sysfs_init(void)
> +{
> +	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?

[ ... ]
> diff --git a/arch/s390/kvm/arm64/qaaf.c b/arch/s390/kvm/arm64/qaaf.c
> new file mode 100644
> index 0000000000000..c3124d38afc23
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/qaaf.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/kvm_host.h>
> +#include <asm/aef.h>
> +#include "qaaf.h"
> +
> +#define _qaaf_reg_case(id) case SYS_##id: return aef_qmc()->regs[QAAF_REG_##id]

[Severity: High]
Does this index into the regs array using the wrong offset?

Looking at struct qaaf_qmc_block in asm/kvm_host_arm64_types.h, the regs
array already starts at offset 0x0010:

    u64 regs[0x1fe];        /* 0x0010 */

However, the QAAF_REG_* enums represent the absolute doubleword offset from the
start of the block (e.g., QAAF_REG_MIDR_EL1 is 0x02, which represents byte
offset 0x0010).

By using the enum to index into the regs array, doesn't this double-apply the
0x0010 offset, causing KVM to read incorrect values for all feature registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831145536.913567-1-seiden@linux.ibm.com?part=11