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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-09-05 at 14:10 +0000, Bertrand Marquis wrote:
> Hi Oleksii,
> 
> > On 5 Sep 2023, at 15:37, Oleksii <oleksii.kurochko@xxxxxxxxx>
> > wrote:
> > 
> > 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.
> 
> Ack
> 
> > > 
> > > > 
> > > > # 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 ).
> 
> Right they are generated in the build tree but if it is not the same
> as srctree it will not work due to CFLAGS.
> 
> > 
> > Anyway I think that obj should be equal to what is mentioned in
> > CFLAGS.
> 
> We generate other files like asm-offsets.h and it could in fact be
> that
> both srctree and build tree are already in include path.
> 
> There is in makefile a rule already removing asm-offsets.h which is
> generated
> and this one does not use a prefix so we should follow this standard
> in fact.
> 
> So we should not use any prefix (srctree or obj) i think.
I think you are right. Lets avoid usage of srctree/obj.


~ Oleksii