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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

@anthony: any suggestion here ?

Bertrand

> 
> ~ Oleksii