Re: [PATCH V3 05/22] LoongArch: Add build infrastructure

[Huacai Chen <chenhuacai@gmail.com>]

Hi, Arnd,

On Fri, Sep 17, 2021 at 3:53 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Sep 17, 2021 at 5:57 AM Huacai Chen <chenhuacai@loongson.cn>
> wrote:> --- /dev/null
> > +++ b/arch/loongarch/.gitignore
> > @@ -0,0 +1,9 @@
> > +*.lds
> > +*.raw
> > +calc_vmlinuz_load_addr
> > +elf-entry
> > +relocs
> > +vmlinux.*
> > +vmlinuz.*
> > +
> > +!kernel/vmlinux.lds.S
>
> Can you double-check that 'make clean' and/or 'make mrproper' remove all the
> generated files? This may already be the case, but I don't see how this is
> done in your Makefile
The files listed in .gitignore are not generated by the current series
(except vmlinux, but it is cleaned, of course). Most of them are
generated by the zboot patch (not in this series). So, should I keep
the .gitignore file as is, or remove it at present?

>
> > +
> > +choice
> > +       prompt "Page Table Layout"
> > +       default 16KB_2LEVEL if 32BIT
> > +       default 16KB_3LEVEL if 64BIT
> > +       help
> > +         Allows choosing the page table layout, which is a combination
> > +         of page size and page table levels. The virtual memory address
> > +         space bits are determined by the page table layout.
> > +
> > +config 4KB_3LEVEL
> > +       bool "4KB with 3 levels"
> > +       select PAGE_SIZE_4KB
> > +       select PGTABLE_3LEVEL
> > +       help
> > +         This option selects 4KB page size with 3 level page tables, which
> > +         support a maximum 39 bits of application virtual memory.
> > + ...
>
> Nice, this choice statement looks a lot better than the version you had before.
>
> > +
> > +cflags-y += -ffreestanding
>
> I had not noticed this previously, but I think this should not be used here,
> as -ffreestanding disables a number of optimizations for compiler builtins.
>
> Did you just copy this from MIPS or do you have a particular reason this
> is used here?
If we use zboot (compressed kernel), there are some conflicting if
without -ffrestanding. E.g., the built-in malloc() conflicts with the
one defined in ./include/linux/decompress/mm.h.

>
> > +# Some distribution-specific toolchains might pass the -fstack-check
> > +# option during the build, which adds a simple stack-probe at the beginning
> > +# of every function.  This stack probe is to ensure that there is enough
> > +# stack space, else a SEGV is generated.  This is not desirable for LoongArch
> > +# as kernel stacks are small, placed in unmapped virtual memory, and do not
> > +# grow when overflowed.
> > +#
> > +cflags-y += -fno-stack-check
>
> This is already set in the global Makefile and can be removed as well
OK, this will be removed.

>
> > +cflags-y += $(call as-option,-Wa$(comma)-mno-fix-loongson3-llsc,)
> > +cflags-y += -U_LOONGARCH_ISA -D_LOONGARCH_ISA=_LOONGARCH_ISA_LOONGARCH64
> > +
> > +load-y                         = 0x9000000000200000
> > +
> > +drivers-$(CONFIG_PCI)          += arch/loongarch/pci/
> > +
> > +KBUILD_AFLAGS  += $(cflags-y)
> > +KBUILD_CFLAGS  += $(cflags-y)
> > +KBUILD_CPPFLAGS += -DVMLINUX_LOAD_ADDRESS=$(load-y)
> > +
> > +bootvars-y     = VMLINUX_LOAD_ADDRESS=$(load-y) PLATFORM="$(platform-y)"
>
> I would argue that VMLINUX_LOAD_ADDRESS should not be configurable
> here, instead all kernels should use the same value.
VMLINUX_LOAD_ADDRESS is not configurable, it is also used by the later
zboot patch (use VMLINUX_LOAD_ADDRESS to calculate the load address of
vmlinuz).

>
> > diff --git a/arch/loongarch/include/asm/Kbuild b/arch/loongarch/include/asm/Kbuild
> > new file mode 100644
> > index 000000000000..41a76e675321
> > --- /dev/null
> > +++ b/arch/loongarch/include/asm/Kbuild
> > @@ -0,0 +1,31 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +generic-y += dma-contiguous.h
> > +generic-y += export.h
> > +generic-y += mcs_spinlock.h
> > +generic-y += parport.h
> > +generic-y += early_ioremap.h
> > +generic-y += qrwlock.h
> > +generic-y += qspinlock.h
>
> The list is apparently from an older kernel and no longer needed for
> files that are listed in include/asm-generic/Kbuild.
>
> Please only list the files that are not already there.
Emmm, I have checked include/asm-generic/Kbuild and no duplication,
but I've found a self duplication (rwsem.h), thanks.

Huacai
>
>         Arnd