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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Bertrand,

On Mon, 2023-09-04 at 13:39 +0000, Bertrand Marquis wrote:
> 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.
Sure. I'll add that Kbuild refers to "Kernel build"
> 
> > 
> > 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 ?
I think you are right.
I got myself confused because I do not use out-of-tree compilation so
for me it looked like $(srctree).

It would be better to use $(objtree) instead.
> 
> > 
> > # 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)
Thanks. I'll change to the value mentioned in CFLAGS.

> 
> > .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.
Why not? They will be generated in $obj which equal to
obj=arch/$(SRCARCH)/include/generated/asm ( in my case ).

Anyway I think that obj should be equal to what is mentioned in CFLAGS.

~ Oleksii