Re: [PATCH v5 21/31] s390: Use arm64 headers

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ARM64 headers are extracted unconditionally on s390 because they are explicitly listed as a dependency in the `kapi` target, and the conditional assignment uses the wrong Kconfig variable.
- [Low] The generated ARM64 headers are written to a path with a duplicated `$(objtree)` prefix, which causes them to be placed in an incorrect nested directory during out-of-tree builds and silently breaks Kbuild dependency tracking.
- [Low] Using AWK scripts to parse C headers and extract snippets based on s390-specific markers (`ARM64_S390_COMMON`) inside ARM64 architectural headers is a brittle design pattern.
--

--- Patch [21]: [PATCH v5 21/31] s390: Use arm64 headers ---
commit 0aa161b5c002de946626485d254baf5277503c38
Author: Steffen Eiden <[email protected]>
s390: Use arm64 headers

This commit introduces a system for sharing ARM64 headers with s390 KVM.
It uses awk scripts to extract marked sections from arm64 headers and
integrates this into the s390 build process.

> diff --git a/arch/s390/tools/Makefile b/arch/s390/tools/Makefile
> index f2862364fb422..3e0f21b552daf 100644
> --- a/arch/s390/tools/Makefile
> +++ b/arch/s390/tools/Makefile
[ ... ]
> @@ -3,12 +3,20 @@
>  # Makefile for s390 specific build tools
>  #
>  
> +include $(srctree)/arch/s390/tools/Makefile.arm64h
> +
>  kapi := arch/$(ARCH)/include/generated/asm
>  kapi-hdrs-y := $(kapi)/facility-defs.h $(kapi)/dis-defs.h
>  
> +targets += $(addprefix ../../../,$(kapi-hdrs-y))
> +# Convert ARM64 absolute paths to relative before Kbuild adds $(obj)/ prefix
> +targets += $(patsubst $(objtree)/%,../../../%,$(ARM64_TARGETS))
> +
> +kapi-hdrs-$(CONFIG_KVM_ARM64) += $(ARM64_TARGETS)
>  
>  PHONY += kapi
>  
> -kapi:	$(kapi-hdrs-y)
> +kapi:	$(kapi-hdrs-y) $(ARM64_TARGETS)

[Severity: Medium]
The commit message mentions this should be conditional on CONFIG_KVM_S390_ARM64,
but the code uses CONFIG_KVM_ARM64 instead. 

Additionally, since $(ARM64_TARGETS) is added directly as an unconditional
dependency to the kapi target here, does this mean the headers will be
extracted unconditionally on every s390 build regardless of the Kconfig value?

> diff --git a/arch/s390/tools/Makefile.arm64h b/arch/s390/tools/Makefile.arm64h
> new file mode 100644
> index 0000000000000..8fbd9cb486952
> --- /dev/null
> +++ b/arch/s390/tools/Makefile.arm64h
[ ... ]
> +arm64api := $(objtree)/arch/$(ARCH)/include/generated/arm64
> +arm64uapi := $(objtree)/arch/$(ARCH)/include/generated/uapi/arm64
[ ... ]
> +arm64-src = $(srctree)/arch/arm64/include/$(if $(filter uapi,$(call arm64-type,$(1))),uapi/)asm/$(call arm64-src-base,$(1))
> +arm64-dst = $(objtree)/$(if $(filter uapi,$(call arm64-type,$(1))),$(arm64uapi),$(arm64api))/$(call arm64-base,$(1))

[Severity: Low]
Does this result in a duplicated $(objtree) prefix? The arm64api and arm64uapi
variables defined earlier already contain $(objtree)/, so prepending it again
here could cause Kbuild to generate files in a malformed path like
$(objtree)/$(objtree)/... during out-of-tree builds and break dependency
tracking.

[ ... ]
> +quiet_cmd_gen_sysreg_arm64 = GEN     $@
> +      cmd_gen_sysreg_arm64 = mkdir -p $(dir $@); $(AWK) -f $(srctree)/arch/arm64/tools/gen-sysreg.awk $(srctree)/arch/arm64/tools/sysreg > $@

[Severity: Low]
This invokes an ARM64 internal build script directly from the s390 Makefile.
Could this tight coupling cause the s390 build to break silently if the ARM64
generator scripts are refactored?

> diff --git a/arch/s390/tools/copy-arm64h.awk b/arch/s390/tools/copy-arm64h.awk
> new file mode 100644
> index 0000000000000..d74df368c1b57
> --- /dev/null
> +++ b/arch/s390/tools/copy-arm64h.awk
[ ... ]
> +BEGIN {
> +	# Constants
> +	start_pattern = "^#ifdef ARM64_S390_COMMON$"

[Severity: Low]
Relying on rigid regex patterns to extract C code blocks and forcing
s390-specific markers into ARM64 headers appears brittle. Could minor
whitespace changes or formatting updates in the ARM64 headers break this
parsing pattern?

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