Re: [PATCH v5 01/11] kselftest: arm64: add skeleton Makefile

[Cristian Marussi <cristian.marussi@arm.com>]

On 04/09/2019 12:47, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:22pm +0100, Cristian Marussi wrote:
>> Add a new arm64-specific empty subsystem amongst TARGETS of KSFT build
>> framework; keep these new arm64 KSFT testcases separated into distinct
> 
> Nit: this isn't true any more, since the tags tests already added the
> arm64 subsystem here.

Ok
> 
>> subdirs inside tools/testing/selftests/arm64/ depending on the specific
>> subsystem targeted.
>>
>> Add into toplevel arm64 KSFT Makefile a mechanism to guess the effective
>> location of Kernel headers as installed by KSFT framework.
> 
> This:
> 
>> Merge with
>>
>> commit 9ce1263033cd ("selftests, arm64: add a selftest for passing
>> 		     tagged pointers to kernel")
>>
>> while moving such KSFT tags tests inside their own subdirectory
>> (arm64/tags).
> 
> ...could be put under the tearoff, but it doesn't really belong in the
> commit message IMHO.
> 
> I suggest rewriting the commit message to reflect the current
> situation (but it can be kept brief).
> 
> Basically, what this patch now seems to do is to prepare for adding
> more arm64 tests, by moving the tags tests into their own subdirectory
> and extending the existing skeleton Makefile as appropriate.
> 

Ok
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v4 --> v5
>> - rebased on arm64/for-next/core
>> - merged this patch with KSFT arm64 tags patch, while moving the latter
>>   into its own subdir
>> - moved kernel header includes search mechanism from KSFT arm64
>>   SIGNAL Makefile
>> - export proper top_srcdir ENV for lib.mk
>> v3 --> v4
>> - comment reword
>> - simplified documentation in README
>> - dropped README about standalone
>> ---
> 
> [...]
> 
>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
>> index a61b2e743e99..5dbb0ffdfc9a 100644
>> --- a/tools/testing/selftests/arm64/Makefile
>> +++ b/tools/testing/selftests/arm64/Makefile
>> @@ -1,11 +1,69 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2019 ARM Limited
> 
> Change of copyright?  This isn't pure Arm IP upstream IIUC.
> 
> Maybe just drop it: Makefiles don't usually contain significant IP, so
> many have no copyright message anyway.
> 
Right. I'll drop.
>> -# ARCH can be overridden by the user for cross compiling
>> -ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>> +# When ARCH not overridden for crosscompiling, lookup machine
>> +ARCH ?= $(shell uname -m)
>> +ARCH := $(shell echo $(ARCH) | sed -e s/aarch64/arm64/)
>>  
>> -ifneq (,$(filter $(ARCH),aarch64 arm64))
>> -TEST_GEN_PROGS := tags_test
>> -TEST_PROGS := run_tags_test.sh
>> +ifeq ("x$(ARCH)", "xarm64")
>> +SUBDIRS := tags
>> +else
>> +SUBDIRS :=
>>  endif
>>  
>> -include ../lib.mk
>> +CFLAGS := -Wall -O2 -g
>> +
>> +# A proper top_srcdir is needed by KSFT(lib.mk)
>> +top_srcdir = ../../../../..
>> +
>> +# Additional include paths needed by kselftest.h and local headers
>> +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/
>> +
>> +# Guessing where the Kernel headers could have been installed
>> +# depending on ENV config
>> +ifeq ($(KBUILD_OUTPUT),)
>> +khdr_dir = $(top_srcdir)/usr/include
>> +else
>> +# the KSFT preferred location when KBUILD_OUTPUT is set
>> +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include
>> +endif
> 
> Looking at this, can we just pass the directory in from the toplevel
> "all" rule instead of guessing?
> 
Do you mean toplevel in KSFT ?
I think it's how should be done at the end, but I was trying to keep this series on
arm64/ lands only. (also maybe I'm missing something obvious in KSFT handling of this
situation....even though many other KSFT use built CFLAGS like: -I../../../usr/include
or similar)

> Maybe don't churn this for now though.  It's something that could be
> looked at later.
> 

Ok. I'll leave here and fix it to avoid relative paths...which could be problematic
when exported to lower level Makefiles.

Cheers

Cristian

> [...]
> 
> Apart from the comments above, the patch looks reasonable to me.
> 
> Cheers
> ---Dave
>