On 8/19/24 9:33 PM, Sean Christopherson wrote: > +KVM arch maintainers > > On Mon, Aug 19, 2024, Muhammad Usama Anjum wrote: >> The tests are built on per architecture basis. When unsupported >> architecture is specified, it has no tests and TEST_GEN_PROGS is empty. >> The lib.mk has support for not building anything for such case. But KVM >> makefile doesn't handle such case correctly. It doesn't check if >> TEST_GEN_PROGS is empty or not and try to create directory by mkdir. >> Hence mkdir generates the error. >> >> mkdir: missing operand >> Try 'mkdir --help' for more information. >> >> This can be easily fixed by checking if TEST_GEN_PROGS isn't empty >> before calling mkdir. >> >> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx> >> Cc: Sean Christopherson <seanjc@xxxxxxxxxx> >> Signed-off-by: Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx> >> --- >> Changes since v1: >> - Instead of ignoring error, check TEST_GEN_PROGS's validity first >> --- >> tools/testing/selftests/kvm/Makefile | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile >> index 48d32c5aa3eb7..9f8ed82ff1d65 100644 >> --- a/tools/testing/selftests/kvm/Makefile >> +++ b/tools/testing/selftests/kvm/Makefile >> @@ -317,7 +317,9 @@ $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S $(GEN_HDRS) >> $(LIBKVM_STRING_OBJ): $(OUTPUT)/%.o: %.c >> $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c -ffreestanding $< -o $@ >> >> +ifneq ($(strip $(TEST_GEN_PROGS)),) >> $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS)))) >> +endif > This just suppresses an error, it doesn't fix the underlying problem. E.g. there > are other weird side effects, such as an above mkdir creating the $(ARCH) directory > even though it shouldn't exist in the end. > > It's also very opaque, e.g. without a comment or the context of the changelog, > I'd have no idea what purpose the above serves. > > Rather than bury the effective "is this arch supported" check in the middle of > the Makefile, what if we wrap the "real" makefile and include it only for > supported architectures, and provide dummy targets for everything else? > > E.g. > > --- > # SPDX-License-Identifier: GPL-2.0-only > top_srcdir = ../../../.. > include $(top_srcdir)/scripts/subarch.include > ARCH ?= $(SUBARCH) > > ifeq ($(ARCH),$(filter $(ARCH),arm64 s390 riscv x86 x86_64)) > ifeq ($(ARCH),x86) > ARCH_DIR := x86_64 > else ifeq ($(ARCH),arm64) > ARCH_DIR := aarch64 > else ifeq ($(ARCH),s390) > ARCH_DIR := s390x > else > ARCH_DIR := $(ARCH) > endif > > include Makefile.kvm > else > all: > clean: > endif > --- > > And other KVM maintainers, the big question is: if we do the above, would now be > a decent time to bite the bullet and switch to the kernel's canonical arch paths, > i.e. arm64, s390, and x86? I feel like if we're ever going to get away from > using aarch64, x86_64, and s390x, this is as about a good of an opportunity as > we're going to get. > > The annoying x86_64=>x86 alias still needs to be handled to avoid breaking explicit > ARCH=x86_64 builds (which apparently are allowed, *sigh*), but we can ditch ARCH_DIR > and the KVM selftests dirs match tools' include paths. > > --- > # SPDX-License-Identifier: GPL-2.0-only > top_srcdir = ../../../.. > include $(top_srcdir)/scripts/subarch.include > ARCH ?= $(SUBARCH) > > ifeq ($(ARCH),$(filter $(ARCH),arm64 s390 riscv x86 x86_64)) > # Top-level selftests allows ARCH=x86_64 🙁 > ifeq ($(ARCH),x86_64) > ARCH := x86 > endif > include Makefile.kvm > else > all: > clean: > endif > --- > > If no one objects or has a better idea, I'll post a series to do the above. I didn't had enough knowledge to attempt a better fix. Thank you. -- BR, Muhammad Usama Anjum
