[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <379673da-9f2f-484a-be04-a62d916fa25e@collabora.com>
Date: Tue, 20 Aug 2024 12:26:23 +0500
From: Muhammad Usama Anjum <Usama.Anjum@...labora.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Usama.Anjum@...labora.com, Paolo Bonzini <pbonzini@...hat.com>,
Shuah Khan <shuah@...nel.org>, kernel@...labora.com, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
Marc Zyngier <maz@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Janosch Frank <frankja@...ux.ibm.com>,
Claudio Imbrenda <imbrenda@...ux.ibm.com>, Anup Patel <anup@...infault.org>
Subject: Re: [PATCH v2] selftests: kvm: fix mkdir error when building for
unsupported arch
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@...hat.com>
>> Cc: Sean Christopherson <seanjc@...gle.com>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@...labora.com>
>> ---
>> 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
Powered by blists - more mailing lists