Re: [PATCH v5 11/31] arm64: Share arm64 headers with s390

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Sharing architecture-private headers directly with another architecture (s390) violates fundamental kernel design principles.
- [High] The `#ifdef ARM64_S390_COMMON` macro creates an incomplete type build breakage and inverted logic.
--

--- Patch [11]: [PATCH v5 11/31] arm64: Share arm64 headers with s390 ---
commit ab90259bd4d46a7a1b7b9c187fcf2bfc3985993e
Author: Steffen Eiden <[email protected]>

arm64: Share arm64 headers with s390

This commit modifies arm64 Makefiles and headers to allow sharing them with
the s390 architecture. It introduces an ARM64_S390_COMMON preprocessor macro
and adds comments to mark shared regions.

> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 6b005c8fef706..1cd7cd94a8554 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -43,7 +43,9 @@ CC_FLAGS_NO_FPU	:= -mgeneral-regs-only
>  KBUILD_CFLAGS	+= $(CC_FLAGS_NO_FPU) \
>  		   $(compat_vdso) $(cc_has_k_constraint)
>  KBUILD_CFLAGS	+= $(call cc-disable-warning, psabi)
> +KBUILD_CFLAGS	+= -DARM64_S390_COMMON

[Severity: High]
Is it appropriate to add a cross-architecture define directly to the arm64
Makefile?

Sharing architecture-private headers directly with another architecture (s390)
violates strict architectural separation. Could this shared code be abstracted
into generic kernel headers (e.g., under include/linux/, include/kvm/, or
include/asm-generic/) instead of exposing arch/arm64/ headers to s390?

[ ... ]
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 875fc7c3366be..83fefceed952b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
[ ... ]
> @@ -832,6 +835,8 @@ extern s64 kvm_nvhe_sym(hyp_physvirt_offset);
>  extern u64 kvm_nvhe_sym(hyp_cpu_logical_map)[NR_CPUS];
>  #define hyp_cpu_logical_map CHOOSE_NVHE_SYM(hyp_cpu_logical_map)
>  
> +#ifdef ARM64_S390_COMMON
> +
>  struct vcpu_reset_state {
>  	unsigned long	pc;
>  	unsigned long	r0;
> @@ -839,6 +844,8 @@ struct vcpu_reset_state {
>  	bool		reset;
>  };
>  
> +#endif /* ARM64_S390_COMMON */
> +

[Severity: High]
Will this cause an incomplete type compilation error if ARM64_S390_COMMON is
not defined?

Looking further down in this file, struct kvm_vcpu_arch instantiates this
struct by value outside of any macro guards:

struct kvm_vcpu_arch {
	struct kvm_cpu_context ctxt;
...
	/* Additional reset state */
	struct vcpu_reset_state	reset_state;
...
};

If an architecture includes asm/kvm_host.h without defining the macro, the
compiler skips the definition of struct vcpu_reset_state, leading to a hard
build failure when it parses struct kvm_vcpu_arch.

Additionally, does it make sense that ARM64_S390_COMMON guards
strictly ARM-specific registers (like pc, r0, and pstate)? If s390 defines
this macro to access shared code, it will also pull in these architecture
specific definitions that it cannot use.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11