Re: [PATCH v4 08/27] KVM: arm64: Prepare sharing arm64 code with s390

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The patch misuses C preprocessor `#ifdef` directives to exclude tracepoints, resulting in catastrophic syntax errors because it leaves statements in the global scope.
- [Low] Critical ARM64 functions and definitions are wrapped in `#ifdef ARM64_S390_COMMON`, stripping them from standard ARM64 builds and causing undefined reference errors because their callers remain unconditionally compiled.
--

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

KVM: arm64: Prepare sharing arm64 code with s390

This commit marks functions that s390 can use to implement arm on s390 as
shared functions, without introducing functional changes.

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 50adfff75be82..c992308b2dceb 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1580,6 +1580,7 @@ static unsigned long system_supported_vcpu_features(void)
>  	return features;
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static int kvm_vcpu_init_check_features(struct kvm_vcpu *vcpu,
>  					const struct kvm_vcpu_init *init)
>  {

[Severity: Low]
Will this cause undefined reference errors on standard ARM64 builds? 

The caller of kvm_vcpu_init_check_features() remains unconditionally compiled,
so the symbol will be missing when ARM64_S390_COMMON is not defined.

> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index b01d6622b8720..5a202cfd27bcb 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -543,6 +543,7 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>  	return -EINVAL;
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static int copy_core_reg_indices(const struct kvm_vcpu *vcpu,
>  				 u64 __user *uindices)
>  {

[Severity: Low]
Does this missing symbol cause a link failure for kvm_arm_copy_reg_indices()?
The function is compiled out here, but the caller isn't.

> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 54aedf93c78b6..d511af145ee6a 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -213,6 +213,7 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu)
>  {

[Severity: Low]
Will this lead to an undefined reference? 

kvm_handle_unknown_ec() is conditionally removed here, but it remains
referenced in the unconditionally compiled arm_exit_handlers array.

[ ... ]

> @@ -404,6 +407,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_ELx_EC_GCS]	= kvm_handle_gcs,
>  };
>  
> +#ifdef ARM64_S390_COMMON
>  static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>  {

[Severity: Low]
Is it intentional to compile out kvm_get_exit_handler() and
handle_trap_exceptions()? 

The handle_exit() caller relies on them unconditionally, which may cause a
build failure.

> diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
> index e2285ed8c91de..876d56dd5b640 100644
> --- a/arch/arm64/kvm/mmio.c
> +++ b/arch/arm64/kvm/mmio.c

[ ... ]

> @@ -135,8 +136,11 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu)
>  		if (!kvm_vcpu_dabt_issf(vcpu))
>  			data = data & 0xffffffff;
>  
> +#endif /* ARM64_S390_COMMON */
>  		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
>  			       &data);
> +		//TODO  tracing
> +#ifdef ARM64_S390_COMMON
>  		data = vcpu_data_host_to_guest(vcpu, data, len);
>  		vcpu_set_reg(vcpu, kvm_vcpu_dabt_get_rd(vcpu), data);
>  	}

[Severity: Low]
Does this inverted preprocessor logic break the build on standard ARM64?

Since kvm_handle_mmio_return() starts inside an ifdef ARM64_S390_COMMON block,
placing an endif here exposes the trace_kvm_mmio() call to the global scope
when the flag is not set, resulting in invalid C syntax.

[ ... ]

> @@ -171,8 +175,11 @@ int io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>  	 * though, so directly deliver an exception to the guest.
>  	 */
>  	if (!kvm_vcpu_dabt_isvalid(vcpu)) {
> +#endif /* ARM64_S390_COMMON */
>  		trace_kvm_mmio_nisv(*vcpu_pc(vcpu), esr,
>  				    kvm_vcpu_get_hfar(vcpu), fault_ipa);
> +		//TODO tracing
> +#ifdef ARM64_S390_COMMON
>  
>  		if (vcpu_is_protected(vcpu))
>  			return kvm_inject_sea_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));

[Severity: Low]
Similar to the issue above, does this leave trace_kvm_mmio_nisv() in the
global scope and cause syntax errors when ARM64_S390_COMMON is undefined?

> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index b963fd975aaca..faab6f03f3655 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -34,6 +34,7 @@
>  static u32 __ro_after_init kvm_ipa_limit;
>  unsigned int __ro_after_init kvm_host_sve_max_vl;
>  
> +#ifdef ARM64_S390_COMMON
>  /*
>   * ARMv8 Reset Values
>   */
> @@ -46,6 +47,8 @@ unsigned int __ro_after_init kvm_host_sve_max_vl;
>  #define VCPU_RESET_PSTATE_SVC	(PSR_AA32_MODE_SVC | PSR_AA32_A_BIT | \
>  				 PSR_AA32_I_BIT | PSR_AA32_F_BIT)
>  
> +#endif /* ARM64_S390_COMMON */

[Severity: Low]
Could stripping out these reset value macros cause compilation to fail? 

They appear to be used in unconditional kvm_reset_vcpu() initialization logic.

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