| Message ID | 20190802170300.20662-1-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add arm64/signal initial kselftest support | expand |
On Fri, Aug 02, 2019 at 06:02:49PM +0100, Cristian Marussi wrote: > Hi > > this patchset aims to add the initial arch-specific arm64 support to > kselftest starting with signals-related test-cases. > A common internal test-case layout is proposed which then it is anyway > wired-up to the toplevel kselftest Makefile, so that it should be possible > at the end to run it on an arm64 target in the usual way with KSFT. The tests look like a reasonable base overall and something that we can extend later as needed. There are various minor things that need attention -- see my comments on the individual patches. Apart for some things that can be factored out, I don't think any of it involves redesign. A few general comments: * Please wrap all commit messages to <= 75 chars, and follow the other guidelines about commit messages in Documentation/process/submitting-patches.rst). * Remember to run scripts/checkpatch.pl on your patches. Currently various issues are reported: they should mostly be trivial to fix. checkpatch does report some false positives, but most of the warnings I see look relevant. * If you like, you can add an Author: line alongside the copyright notice in new files that you create. (You'll see this elsewhere in the kernel if you grep.) One general stylistic issue (IMHO): * Try to avoid inventing names for things that have no established name (for example "magic0" to mean "magic number 0"). The risk is that the reader wastes time grepping for the definition, when really the text should be read at face value. It's best to use all caps just for #define names, abbreviations, and other things that are customarily capitalised (like "CPU" etc.). Other words containing underscores may resemble variable / function names, and may cause confusion of there is no actual variable or function with that name. I don't think it's worth heavily reworking the patches for this, but it's something to bear in mind. [...] Cheers ---Dave
Hi On 13/08/2019 17:22, Dave Martin wrote: > On Fri, Aug 02, 2019 at 06:02:49PM +0100, Cristian Marussi wrote: >> Hi >> >> this patchset aims to add the initial arch-specific arm64 support to >> kselftest starting with signals-related test-cases. >> A common internal test-case layout is proposed which then it is anyway >> wired-up to the toplevel kselftest Makefile, so that it should be possible >> at the end to run it on an arm64 target in the usual way with KSFT. > > The tests look like a reasonable base overall and something that we can > extend later as needed. > > There are various minor things that need attention -- see my comments on > the individual patches. Apart for some things that can be factored out, > I don't think any of it involves redesign. > > > A few general comments: > > * Please wrap all commit messages to <= 75 chars, and follow the other > guidelines about commit messages in > Documentation/process/submitting-patches.rst). > > * Remember to run scripts/checkpatch.pl on your patches. Currently > various issues are reported: they should mostly be trivial to fix. > checkpatch does report some false positives, but most of the warnings > I see look relevant. > Thanks for the review. I addressed latest issues in V4, published now. I kept tests verbose (outputting to stderr) as of now. Removed as a whole standalone build/run. Thanks Cristian > * If you like, you can add an Author: line alongside the copyright > notice in new files that you create. (You'll see this elsewhere in > the kernel if you grep.) > > One general stylistic issue (IMHO): > > * Try to avoid inventing names for things that have no established > name (for example "magic0" to mean "magic number 0"). > > The risk is that the reader wastes time grepping for the definition, > when really the text should be read at face value. It's best to use > all caps just for #define names, abbreviations, and other things > that are customarily capitalised (like "CPU" etc.). Other words > containing underscores may resemble variable / function names, and > may cause confusion of there is no actual variable or function with > that name. > > I don't think it's worth heavily reworking the patches for this, but > it's something to bear in mind. > > [...] > > Cheers > ---Dave >
On Fri, Aug 30, 2019 at 05:40:42PM +0100, Cristian Marussi wrote: > Hi > > On 13/08/2019 17:22, Dave Martin wrote: > > On Fri, Aug 02, 2019 at 06:02:49PM +0100, Cristian Marussi wrote: > >> Hi > >> > >> this patchset aims to add the initial arch-specific arm64 support to > >> kselftest starting with signals-related test-cases. > >> A common internal test-case layout is proposed which then it is anyway > >> wired-up to the toplevel kselftest Makefile, so that it should be possible > >> at the end to run it on an arm64 target in the usual way with KSFT. > > > > The tests look like a reasonable base overall and something that we can > > extend later as needed. > > > > There are various minor things that need attention -- see my comments on > > the individual patches. Apart for some things that can be factored out, > > I don't think any of it involves redesign. > > > > > > A few general comments: > > > > * Please wrap all commit messages to <= 75 chars, and follow the other > > guidelines about commit messages in > > Documentation/process/submitting-patches.rst). > > > > * Remember to run scripts/checkpatch.pl on your patches. Currently > > various issues are reported: they should mostly be trivial to fix. > > checkpatch does report some false positives, but most of the warnings > > I see look relevant. > > > > Thanks for the review. I addressed latest issues in V4, published now. > > I kept tests verbose (outputting to stderr) as of now. > Removed as a whole standalone build/run. The responses look reasonable, thanks for repost. I'll take a look. [...] Cheers ---Dave
Hi On 02/09/2019 11:53, Dave Martin wrote: > On Fri, Aug 30, 2019 at 05:40:42PM +0100, Cristian Marussi wrote: >> Hi >> >> On 13/08/2019 17:22, Dave Martin wrote: >>> On Fri, Aug 02, 2019 at 06:02:49PM +0100, Cristian Marussi wrote: >>>> Hi >>>> >>>> this patchset aims to add the initial arch-specific arm64 support to >>>> kselftest starting with signals-related test-cases. >>>> A common internal test-case layout is proposed which then it is anyway >>>> wired-up to the toplevel kselftest Makefile, so that it should be possible >>>> at the end to run it on an arm64 target in the usual way with KSFT. >>> >>> The tests look like a reasonable base overall and something that we can >>> extend later as needed. >>> >>> There are various minor things that need attention -- see my comments on >>> the individual patches. Apart for some things that can be factored out, >>> I don't think any of it involves redesign. >>> >>> >>> A few general comments: >>> >>> * Please wrap all commit messages to <= 75 chars, and follow the other >>> guidelines about commit messages in >>> Documentation/process/submitting-patches.rst). >>> >>> * Remember to run scripts/checkpatch.pl on your patches. Currently >>> various issues are reported: they should mostly be trivial to fix. >>> checkpatch does report some false positives, but most of the warnings >>> I see look relevant. >>> >> >> Thanks for the review. I addressed latest issues in V4, published now. >> >> I kept tests verbose (outputting to stderr) as of now. >> Removed as a whole standalone build/run. > > The responses look reasonable, thanks for repost. > > I'll take a look. > Ok Thanks...but... I'm re-posting now a further V5 which is also rebased on arm64/for-next/core and so deals with the conflicts against queued commit: https://lore.kernel.org/linux-arm-kernel/c1e6aad230658bc175b42d92daeff2e30050302a.1563904656.git.andreyknvl@google.com/ Subject: [PATCH v19 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel Differences from v4 are limited to 01/02 and reported in changelog. Thanks Cristian > [...] > > Cheers > ---Dave >
Hi this patchset aims to add the initial arch-specific arm64 support to kselftest starting with signals-related test-cases. A common internal test-case layout is proposed which then it is anyway wired-up to the toplevel kselftest Makefile, so that it should be possible at the end to run it on an arm64 target in the usual way with KSFT. ~/linux# make TARGETS=arm64 kselftest New KSFT arm64 testcases live inside tools/testing/selftests/arm64 grouped by family inside subdirectories: arm64/signal is the first family proposed with this series. arm64/signal tests can be run via KSFT or standalone. Thanks Cristian Notes: ----- - further details in the included READMEs - more tests still to be written (current strategy is going through the related Kernel signal-handling code and write a test for each possible and sensible code-path) A few ideas in testcases/TODO.readme - a bit of overlap around KSFT arm64/ Makefiles is expected with this: https://lore.kernel.org/linux-arm-kernel/c1e6aad230658bc175b42d92daeff2e30050302a.1563904656.git.andreyknvl@google.com/ Changes: -------- v2-->v3: - rebased on v5.3-rc2 - better test result characterization looking for SEGV_ACCERR in si_code on SIGSEGV - using KSFT Framework macros for retvalues - removed SAFE_WRITE()/dump_uc: buggy, un-needed and unused - reviewed generation process of test_arm64_signals.sh runner script - re-added a fixed fake_sigreturn_misaligned_sp testcase and a properly extended fake_sigreturn() helper - added tests' TODO notes v1-->v2: - rebased on 5.2-rc7 - various makefile's cleanups - mixed READMEs fixes - fixed test_arm64_signals.sh runner script - cleaned up assembly code in signal.S - improved get_current_context() logic - fixed SAFE_WRITE() - common support code splitted into more chunks, each one introduced when needed by some new testcases - fixed some headers validation routines in testcases.c - removed some still broken/immature tests: + fake_sigreturn_misaligned + fake_sigreturn_overflow_reserved + mangle_pc_invalid + mangle_sp_misaligned - fixed some other testcases: + mangle_pstate_ssbs_regs: better checks of SSBS bit when feature unsupported + mangle_pstate_invalid_compat_toggle: name fix + mangle_pstate_invalid_mode_el[1-3]: precautionary zeroing PSTATE.MODE + fake_sigreturn_bad_magic, fake_sigreturn_bad_size, fake_sigreturn_bad_size_for_magic0: - accounting for available space...dropping extra when needed - keeping alignent - new testcases on FPSMID context: + fake_sigreturn_missing_fpsimd + fake_sigreturn_duplicated_fpsimd Cristian Marussi (11): kselftest: arm64: introduce new boilerplate code kselftest: arm64: adds first test and common utils kselftest: arm64: mangle_pstate_invalid_daif_bits kselftest: arm64: mangle_pstate_invalid_mode_el kselftest: arm64: mangle_pstate_ssbs_regs kselftest: arm64: fake_sigreturn_bad_magic kselftest: arm64: fake_sigreturn_bad_size_for_magic0 kselftest: arm64: fake_sigreturn_missing_fpsimd kselftest: arm64: fake_sigreturn_duplicated_fpsimd kselftest: arm64: fake_sigreturn_bad_size kselftest: arm64: fake_sigreturn_misaligned_sp tools/testing/selftests/Makefile | 1 + tools/testing/selftests/arm64/Makefile | 51 +++ tools/testing/selftests/arm64/README | 43 +++ .../testing/selftests/arm64/signal/.gitignore | 6 + tools/testing/selftests/arm64/signal/Makefile | 88 +++++ tools/testing/selftests/arm64/signal/README | 59 +++ .../testing/selftests/arm64/signal/signals.S | 73 ++++ .../arm64/signal/test_arm64_signals.src_shell | 55 +++ .../selftests/arm64/signal/test_signals.c | 26 ++ .../selftests/arm64/signal/test_signals.h | 141 +++++++ .../arm64/signal/test_signals_utils.c | 354 ++++++++++++++++++ .../arm64/signal/test_signals_utils.h | 16 + .../arm64/signal/testcases/.gitignore | 11 + .../arm64/signal/testcases/TODO.readme | 7 + .../testcases/fake_sigreturn_bad_magic.c | 63 ++++ .../testcases/fake_sigreturn_bad_size.c | 85 +++++ .../fake_sigreturn_bad_size_for_magic0.c | 57 +++ .../fake_sigreturn_duplicated_fpsimd.c | 62 +++ .../testcases/fake_sigreturn_misaligned_sp.c | 30 ++ .../testcases/fake_sigreturn_missing_fpsimd.c | 44 +++ .../mangle_pstate_invalid_compat_toggle.c | 25 ++ .../mangle_pstate_invalid_daif_bits.c | 28 ++ .../mangle_pstate_invalid_mode_el1.c | 29 ++ .../mangle_pstate_invalid_mode_el2.c | 29 ++ .../mangle_pstate_invalid_mode_el3.c | 29 ++ .../testcases/mangle_pstate_ssbs_regs.c | 56 +++ .../arm64/signal/testcases/testcases.c | 150 ++++++++ .../arm64/signal/testcases/testcases.h | 83 ++++ 28 files changed, 1701 insertions(+) create mode 100644 tools/testing/selftests/arm64/Makefile create mode 100644 tools/testing/selftests/arm64/README create mode 100644 tools/testing/selftests/arm64/signal/.gitignore create mode 100644 tools/testing/selftests/arm64/signal/Makefile create mode 100644 tools/testing/selftests/arm64/signal/README create mode 100644 tools/testing/selftests/arm64/signal/signals.S create mode 100755 tools/testing/selftests/arm64/signal/test_arm64_signals.src_shell create mode 100644 tools/testing/selftests/arm64/signal/test_signals.c create mode 100644 tools/testing/selftests/arm64/signal/test_signals.h create mode 100644 tools/testing/selftests/arm64/signal/test_signals_utils.c create mode 100644 tools/testing/selftests/arm64/signal/test_signals_utils.h create mode 100644 tools/testing/selftests/arm64/signal/testcases/.gitignore create mode 100644 tools/testing/selftests/arm64/signal/testcases/TODO.readme create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_misaligned_sp.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/testcases.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/testcases.h