Re: [PATCH v6 10/33] arm64: Share arm64 headers with s390

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Gating core arm64 macros behind `#ifdef ARM64_S390_COMMON` breaks userspace tooling (perf, KVM selftests, BPF) and creates inconsistent header views for linker scripts.
- [Medium] Using active C preprocessor `#ifdef` blocks as text-extraction markers for another architecture creates fragile coupling and pollutes the arm64 build system.
--

--- Patch [10]: [PATCH v6 10/33] arm64: Share arm64 headers with s390 ---
commit 20e1eb14886be076f6d4d8836cb6c7175057685e
Author: Steffen Eiden <[email protected]>

arm64: Share arm64 headers with s390

Allow sharing of arm64 headers with s390 by marking the shared regions
or add a comment that the whole file is shared. The preprocessor symbol
ARM64_S390_COMMON is always defined for arm64.

> 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
>  KBUILD_AFLAGS	+= $(compat_vdso)
> +KBUILD_AFLAGS	+= -DARM64_S390_COMMON

[Severity: Medium]
Since this macro is injected into C and assembly compilation flags, does this
omit it from KBUILD_CPPFLAGS used for linker scripts?

Could this create an inconsistency where the C and assembly code sees the shared
blocks, but the linker script preprocessing silently strips them?

[ ... ]

> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index a34a9417ff4da..09f04a5686a18 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -47,6 +47,7 @@ void kvm_skip_instr32(struct kvm_vcpu *vcpu);
>  int kvm_inject_dabt_excl_atomic(struct kvm_vcpu *vcpu, u64 addr);
>  int kvm_inject_serror_esr(struct kvm_vcpu *vcpu, u64 esr);
>  
> +#ifdef ARM64_S390_COMMON
>  void kvm_inject_undefined(struct kvm_vcpu *vcpu);
>  void kvm_inject_sync(struct kvm_vcpu *vcpu, u64 esr);

[Severity: Medium]
Is using an active C preprocessor macro as a text-extraction marker for
another architecture a robust design?

By using an active ifdef to delineate text for an s390 extraction script, it
forces arm64 to modify its global Makefiles just to parse its own native
headers. Would it be safer to use specially formatted code comments for script
scraping to avoid altering C preprocessor semantics?

[ ... ]

> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index ab205f9db94ad..1c5c4df260bea 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -16,6 +16,8 @@
>  
>  #include <asm/gpr-num.h>
>  
> +#ifdef ARM64_S390_COMMON
> +
>  /*
>   * ARMv8 ARM reserves the following encoding for system registers:
>   * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
> @@ -50,6 +52,8 @@
>  #define sys_reg_CRm(id)	(((id) >> CRm_shift) & CRm_mask)
>  #define sys_reg_Op2(id)	(((id) >> Op2_shift) & Op2_mask)
>  
> +#endif /* ARM64_S390_COMMON */

[Severity: Medium]
Will gating fundamental arm64 macros like sys_reg() behind this macro break
userspace tooling (such as perf, KVM selftests, and BPF)?

When these headers are synced to tools/, their independent Makefiles will not
inherit the kernel's KBUILD_CFLAGS. This means the macro is undefined and
core arm64 macros are completely hidden from those builds.

[ ... ]

> @@ -1251,6 +1262,8 @@
>  	par;								\
>  })
>  
> +#ifdef ARM64_S390_COMMON
> +
>  #define SYS_FIELD_VALUE(reg, field, val)	reg##_##field##_##val
>  
>  #define SYS_FIELD_GET(reg, field, val)		\

[Severity: Medium]
Similar to the sys_reg() macro above, does hiding widely used field-extraction
macros behind this conditional break builds for external tools that lack the
new compiler flag when headers are synced?

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