Re: [PATCH v4 1/2] xen: asm-generic support

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Oleksii,

> On 1 Sep 2023, at 18:02, Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx> wrote:
> 
> Some headers are shared between individual architectures or are empty.
> To avoid duplication of these headers, asm-generic is introduced.
> 
> With the following patch, an architecture uses generic headers
> mentioned in the file arch/$(ARCH)/include/asm/Kbuild.

Kbuild refers to "Kernel build" I guess.
I am ok to keep that name to keep things simpler when compared to
Linux scripts but it would be good to mention that in the commit
message for future reference. 

> 
> To use a generic header is needed to add to
> arch/$(ARCH)/include/asm/Kbuild :
>        generic-y += <name-of-header-file.h>
> 
> For each mentioned header in arch/$(ARCH)/include/asm/Kbuild,
> kbuild will generate the necessary wrapper in
> arch/$(ARCH)/include/generated/asm.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V4:
> - introduce asm-generic support
> - update commit message
> ---
> Changes in V3:
> - Rename stubs dir to asm-generic
> ---
> Changes in V2:
> - Nothing changed.
> ---
> xen/Makefile                      | 10 +++++++++-
> xen/arch/arm/include/asm/Kbuild   |  1 +
> xen/arch/ppc/include/asm/Kbuild   |  1 +
> xen/arch/riscv/include/asm/Kbuild |  1 +
> xen/arch/x86/include/asm/Kbuild   |  1 +
> xen/scripts/Makefile.asm-generic  | 23 +++++++++++++++++++++++
> 6 files changed, 36 insertions(+), 1 deletion(-)
> create mode 100644 xen/arch/arm/include/asm/Kbuild
> create mode 100644 xen/arch/ppc/include/asm/Kbuild
> create mode 100644 xen/arch/riscv/include/asm/Kbuild
> create mode 100644 xen/arch/x86/include/asm/Kbuild
> create mode 100644 xen/scripts/Makefile.asm-generic
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index f57e5a596c..698d6ddeb8 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -438,6 +438,7 @@ ifdef building_out_of_srctree
> endif
> CFLAGS += -I$(srctree)/include
> CFLAGS += -I$(srctree)/arch/$(SRCARCH)/include
> +CFLAGS += -I$(srctree)/arch/$(SRCARCH)/include/generated

Why are we generating files in the source tree ? 
Shouldn't we keep it unmodified ?

> 
> # Note that link order matters!
> ALL_OBJS-y                := common/built_in.o
> @@ -580,6 +581,7 @@ _clean:
> rm -f $(TARGET).efi $(TARGET).efi.map $(TARGET).efi.elf $(TARGET).efi.stripped
> rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
> rm -f .banner .allconfig.tmp include/xen/compile.h
> + rm -rf arch/*/include/generated
> 

You must use the same as for CFLAGS here so $(srctree) would be needed.
(or something else depending on the fix for previous comment)

> .PHONY: _distclean
> _distclean: clean
> @@ -589,7 +591,7 @@ $(TARGET).gz: $(TARGET)
> gzip -n -f -9 < $< > $@.new
> mv $@.new $@
> 
> -$(TARGET): outputmakefile FORCE
> +$(TARGET): outputmakefile asm-generic FORCE
> $(Q)$(MAKE) $(build)=tools
> $(Q)$(MAKE) $(build)=. include/xen/compile.h
> $(Q)$(MAKE) $(build)=include all
> @@ -667,6 +669,12 @@ endif # need-sub-make
> PHONY += FORCE
> FORCE:
> 
> +# Support for using generic headers in asm-generic
> +PHONY += asm-generic
> +asm-generic:
> + $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.asm-generic \
> +            obj=arch/$(SRCARCH)/include/generated/asm
> +
> # Declare the contents of the PHONY variable as phony.  We keep that
> # information in a variable so we can use it in if_changed and friends.
> .PHONY: $(PHONY)
> diff --git a/xen/arch/arm/include/asm/Kbuild b/xen/arch/arm/include/asm/Kbuild
> new file mode 100644
> index 0000000000..a4e40e534e
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/Kbuild
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> diff --git a/xen/arch/ppc/include/asm/Kbuild b/xen/arch/ppc/include/asm/Kbuild
> new file mode 100644
> index 0000000000..a4e40e534e
> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/Kbuild
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> diff --git a/xen/arch/riscv/include/asm/Kbuild 
> b/xen/arch/riscv/include/asm/Kbuild
> new file mode 100644
> index 0000000000..a4e40e534e
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/Kbuild
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> diff --git a/xen/arch/x86/include/asm/Kbuild b/xen/arch/x86/include/asm/Kbuild
> new file mode 100644
> index 0000000000..a4e40e534e
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/Kbuild
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> diff --git a/xen/scripts/Makefile.asm-generic 
> b/xen/scripts/Makefile.asm-generic
> new file mode 100644
> index 0000000000..0aac3f50b8
> --- /dev/null
> +++ b/xen/scripts/Makefile.asm-generic
> @@ -0,0 +1,23 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# include/asm-generic contains a lot of files that are used
> +# verbatim by several architectures.
> +#
> +# This Makefile reads the file arch/$(SRCARCH)/include/asm/Kbuild
> +# and for each file listed in this file with generic-y creates
> +# a small wrapper file in $(obj) (arch/$(SRCARCH)/include/generated/asm)
> +
> +kbuild-file := $(srctree)/arch/$(SRCARCH)/include/asm/Kbuild
> +include $(kbuild-file)
> +
> +include scripts/Kbuild.include
> +
> +# Create output directory if not already present
> +_dummy := $(shell [ -d $(obj) ] || mkdir -p $(obj))
> +
> +quiet_cmd_wrap = WRAP    $@
> +cmd_wrap = echo "\#include <asm-generic/$*.h>" >$@
> +
> +all: $(patsubst %, $(obj)/%, $(generic-y))
> +
> +$(obj)/%.h:
> + $(call cmd,wrap)

In fact i think your files are not generated in the srctree so this
will break if you use out of tree compilation.

Cheers
Bertrand

> -- 
> 2.41.0
>