Re: [PATCH v3 15/23] KVM: s390: arm64: Implement feature sanitisation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] KVM: s390: wrong ARM64_HAS_HCR_NV1 check in
  cpus_have_final_cap()
- [Low] KVM: s390: missing ID_AFR0_EL1 masking in
  kvm_arm_host_sanitize_features()
- [Low] KVM: s390: malformed kernel-doc comment for
  read_sanitised_ftr_reg()
--

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

KVM: s390: arm64: Implement feature sanitisation

This commit implements feature detection and ID register sanitization for
arm64 KVM guests on s390 using capabilities queried via QAAF.

> diff --git a/arch/s390/kvm/arm64/feature.c b/arch/s390/kvm/arm64/feature.c
> new file mode 100644
> index 0000000000000..f8b23ba85f9ef
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/feature.c
> @@ -0,0 +1,163 @@

[ ... ]

> +void __init kvm_arm_host_sanitize_features(struct qaaf_qmc_block *qaaf_qmc)
> +{

[ ... ]

> +	MASK_RESERVED(qaaf_qmc, ID_PFR0_EL1);
> +	MASK_RESERVED(qaaf_qmc, ID_PFR1_EL1);
> +	MASK_RESERVED(qaaf_qmc, ID_PFR2_EL1);
> +
> +	MASK_RESERVED(qaaf_qmc, ID_DFR0_EL1);
> +	MASK_RESERVED(qaaf_qmc, ID_DFR1_EL1);
> +
> +	MASK_RESERVED(qaaf_qmc, ID_MMFR0_EL1);

[Severity: Low]
Should kvm_arm_host_sanitize_features() also mask reserved bits for
ID_AFR0_EL1?  kvm_qaaf_read_ftr_reg() exposes
qmc_sanitized.regs[QAAF_REG_ID_AFR0_EL1], and without
MASK_RESERVED(qaaf_qmc, ID_AFR0_EL1) its reserved bits [63:16] remain
unmasked when queried via read_sanitised_ftr_reg().

[ ... ]

> +bool cpus_have_final_cap(unsigned int num)
> +{
> +	u64 reg_val;
> +
> +	switch (num) {

[ ... ]

> +	case ARM64_HAS_HCR_NV1:
> +		reg_val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR4_EL1);
> +		return SYS_FIELD_GET(ID_AA64MMFR4_EL1, E2H0, reg_val) ==
> +		       ID_AA64MMFR4_EL1_E2H0_IMP;

[Severity: Medium]
Does the ARM64_HAS_HCR_NV1 case check for FEAT_E2H0 instead of HCR_EL2.NV1
support?

In the Arm architecture, ID_AA64MMFR4_EL1.E2H0 is a signed field where 0
(ID_AA64MMFR4_EL1_E2H0_IMP) means HCR_EL2.E2H can be programmed to 0, 0xf
(ID_AA64MMFR4_EL1_E2H0_NI) means E2H0 is not implemented but HCR_EL2.NV1 is
supported if nested virtualization is present, and 0xe
(ID_AA64MMFR4_EL1_E2H0_NI_NV1) means neither is implemented.  On arm64,
has_nv1() checks for nested virtualization support and that E2H0 is not
NI_NV1:

arch/arm64/kernel/cpufeature.c:has_nv1() {
	return (__system_matches_cap(ARM64_HAS_NESTED_VIRT) &&
		!(has_cpuid_feature(entry, scope) ||
		  is_midr_in_range_list(nv1_ni_list)));
}

Checking E2H0 == ID_AA64MMFR4_EL1_E2H0_IMP causes cpus_have_final_cap() to
return true whenever E2H0 is 0 even though nested virtualization is not
supported on s390, and false when E2H0 is 0xf.

[ ... ]

> diff --git a/arch/s390/kvm/arm64/feature.h b/arch/s390/kvm/arm64/feature.h
> new file mode 100644
> index 0000000000000..0a59047fda6ef
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/feature.h
> @@ -0,0 +1,86 @@

[ ... ]

> +/**
> + * read_sanitised_ftr_reg() - Get the value of a sys register
> + *
> + * @id - ARM sysreg id
> + *
> + * Get the value of a sys register describing the host configuration.
> + * The returned value indicates support by the machine (HW/FW)
> + * after some sanitisation.
> + *
> + * Example:
> + *	read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1)
> + *
> + * Return: Sanitised HW value of the specified register
> + */
> +static inline u64 read_sanitised_ftr_reg(u32 id)

[Severity: Low]
This isn't a bug, but the kernel-doc comment uses "@id -" instead of "@id:",
which causes scripts/kernel-doc to warn that the parameter is not described
in read_sanitised_ftr_reg().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-sysreg-v3-0-75e95980fb10@linux.ibm.com?part=15