[v3,00/11] Add arm64/signal initial kselftest support
mbox series

Message ID 20190802170300.20662-1-cristian.marussi@arm.com
Headers show
Series
  • Add arm64/signal initial kselftest support
Related show

Message

Cristian Marussi Aug. 2, 2019, 5:02 p.m. UTC
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

Comments

Dave Martin Aug. 13, 2019, 4:22 p.m. UTC | #1
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
Cristian Marussi Aug. 30, 2019, 4:40 p.m. UTC | #2
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
>
Dave Martin Sept. 2, 2019, 10:53 a.m. UTC | #3
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
Cristian Marussi Sept. 2, 2019, 11:30 a.m. UTC | #4
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
>