| Message ID | 20221213001653.3852042-7-seanjc@google.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | KVM: selftests: Clang fixes, Makefile cleanup | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Guessing tree name failed |
+David On Tue, Dec 13, 2022, Sean Christopherson wrote: > Rename UNAME_M to ARCH_DIR and explicitly set it directly for x86. At > this point, the name of the arch directory really doesn't have anything > to do with `uname -m`, and UNAME_M is unnecessarily confusing given that > its purpose is purely to identify the arch specific directory. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > -# No change necessary for x86_64 > -UNAME_M := $(shell uname -m) > - > -# Set UNAME_M for arm64 compile/install to work > -ifeq ($(ARCH),arm64) > - UNAME_M := aarch64 > -endif > -# Set UNAME_M s390x compile/install to work > -ifeq ($(ARCH),s390) > - UNAME_M := s390x > -endif > -# Set UNAME_M riscv compile/install to work > -ifeq ($(ARCH),riscv) > - UNAME_M := riscv > +ifeq ($(ARCH),x86) As discovered by by David, this breaks doing "ARCH=x86_64 make", which is an allowed/supported variant in the kernel proper, so this needs to be: ifneq (,$(filter $(ARCH),x86 x86_64)) or alternatively ifeq ($(ARCH),x86_64) ARCH := x86 endif Hmm, unless there's a reason to keep ARCH=x86_64, the latter appears to be the better option as lib.mak doesn't play nice with x86_64 either, e.g. `ARCH=x86_64 LLVM=1 make` fails. That's arguably a lib.mak bug, but it's trivial to handle in KVM's makefile so forcing lib.mak to handle both seems unnecessary. I'll also add a comment to call out that $(ARCH) follows the kernel's terminology for arch/*, whereas for whatever reason KVM selftests effectively uses `uname -m` terminology. One last thought/question, what do y'all think about renaming directories to follow the kernel proper? I.e. aarch64=>arm64, s390x=>s390, and x86_64=>x86. Then $(ARCH_DIR) would go away. The churn would be unfortunate, but it would be nice to align with arch/ and tools/arch/. > + ARCH_DIR := x86_64 > +else ifeq ($(ARCH),arm64) > + ARCH_DIR := aarch64 > +else ifeq ($(ARCH),s390) > + ARCH_DIR := s390x > +else ifeq ($(ARCH),riscv) > + ARCH_DIR := riscv > +else > +$(error Unknown architecture '$(ARCH)') > endif
On 2022-12-13 20:03, Sean Christopherson wrote: > One last thought/question, what do y'all think about renaming > directories to > follow the kernel proper? I.e. aarch64=>arm64, s390x=>s390, and > x86_64=>x86. > Then $(ARCH_DIR) would go away. The churn would be unfortunate, but it > would be > nice to align with arch/ and tools/arch/. aarch64->arm64 makes sense to me. Whether it is worth the churn is another question. As long as we don't try to backport tests, the damage should be limited to a single merge window. M.
On 12/13/22 01:16, Sean Christopherson wrote: > -ifeq ($(ARCH),riscv) > - UNAME_M := riscv > +ifeq ($(ARCH),x86) > + ARCH_DIR := x86_64 > +else ifeq ($(ARCH),arm64) > + ARCH_DIR := aarch64 > +else ifeq ($(ARCH),s390) > + ARCH_DIR := s390x > +else ifeq ($(ARCH),riscv) > + ARCH_DIR := riscv > +else > +$(error Unknown architecture '$(ARCH)') > endif $(error) would break compiling via tools/testing/selftests/Makefile, so I am squashing this: diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index d761a77c3a80..59f3eb53c932 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -13,10 +13,8 @@ else ifeq ($(ARCH),arm64) ARCH_DIR := aarch64 else ifeq ($(ARCH),s390) ARCH_DIR := s390x -else ifeq ($(ARCH),riscv) - ARCH_DIR := riscv else -$(error Unknown architecture '$(ARCH)') + ARCH_DIR := $(ARCH) endif LIBKVM += lib/assert.c Then the aarch64 and s390x directories can be renamed---x86 too, but the ifeq needs to stay (just changed to do x86_64->x86 instead of the other way round). Paolo
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 947676983da1..d761a77c3a80 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -7,35 +7,16 @@ top_srcdir = ../../../.. include $(top_srcdir)/scripts/subarch.include ARCH ?= $(SUBARCH) -# For cross-builds to work, UNAME_M has to map to ARCH and arch specific -# directories and targets in this Makefile. "uname -m" doesn't map to -# arch specific sub-directory names. -# -# UNAME_M variable to used to run the compiles pointing to the right arch -# directories and build the right targets for these supported architectures. -# -# TEST_GEN_PROGS and LIBKVM are set using UNAME_M variable. -# LINUX_TOOL_ARCH_INCLUDE is set using ARCH variable. -# -# x86_64 targets are named to include x86_64 as a suffix and directories -# for includes are in x86_64 sub-directory. s390x and aarch64 follow the -# same convention. "uname -m" doesn't result in the correct mapping for -# s390x and aarch64. -# -# No change necessary for x86_64 -UNAME_M := $(shell uname -m) - -# Set UNAME_M for arm64 compile/install to work -ifeq ($(ARCH),arm64) - UNAME_M := aarch64 -endif -# Set UNAME_M s390x compile/install to work -ifeq ($(ARCH),s390) - UNAME_M := s390x -endif -# Set UNAME_M riscv compile/install to work -ifeq ($(ARCH),riscv) - UNAME_M := riscv +ifeq ($(ARCH),x86) + ARCH_DIR := x86_64 +else ifeq ($(ARCH),arm64) + ARCH_DIR := aarch64 +else ifeq ($(ARCH),s390) + ARCH_DIR := s390x +else ifeq ($(ARCH),riscv) + ARCH_DIR := riscv +else +$(error Unknown architecture '$(ARCH)') endif LIBKVM += lib/assert.c @@ -196,10 +177,10 @@ TEST_GEN_PROGS_riscv += kvm_page_table_test TEST_GEN_PROGS_riscv += set_memory_region_test TEST_GEN_PROGS_riscv += kvm_binary_stats_test -TEST_PROGS += $(TEST_PROGS_$(UNAME_M)) -TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(UNAME_M)) -TEST_GEN_PROGS_EXTENDED += $(TEST_GEN_PROGS_EXTENDED_$(UNAME_M)) -LIBKVM += $(LIBKVM_$(UNAME_M)) +TEST_PROGS += $(TEST_PROGS_$(ARCH_DIR)) +TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(ARCH_DIR)) +TEST_GEN_PROGS_EXTENDED += $(TEST_GEN_PROGS_EXTENDED_$(ARCH_DIR)) +LIBKVM += $(LIBKVM_$(ARCH_DIR)) INSTALL_HDR_PATH = $(top_srcdir)/usr LINUX_HDR_PATH = $(INSTALL_HDR_PATH)/include/ @@ -212,7 +193,7 @@ endif CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ - -I$(<D) -Iinclude/$(UNAME_M) -I ../rseq -I.. $(EXTRA_CFLAGS) \ + -I$(<D) -Iinclude/$(ARCH_DIR) -I ../rseq -I.. $(EXTRA_CFLAGS) \ $(KHDR_INCLUDES) no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \
Rename UNAME_M to ARCH_DIR and explicitly set it directly for x86. At this point, the name of the arch directory really doesn't have anything to do with `uname -m`, and UNAME_M is unnecessarily confusing given that its purpose is purely to identify the arch specific directory. Signed-off-by: Sean Christopherson <seanjc@google.com> --- tools/testing/selftests/kvm/Makefile | 49 +++++++++------------------- 1 file changed, 15 insertions(+), 34 deletions(-)