Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mtetest: Add a series of tests for the mte instruction set #2875

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Dec 2, 2024

Summary

Added basic mte instructions, ldg, stg, irg, gmi instruction tests

➜ NX git:(master) ✗ qemu-system-aarch64 -cpu max -nographic
-machine virt,virtualization=on,gic-version=3,mte=on
-chardev stdio,id=con,mux=on, -serial chardev:con
-mon chardev=con,mode=readline -kernel ./nuttx/nuttx

  • Ready to Boot Primary CPU
  • Boot from EL2
  • Boot from EL1
  • Boot to C runtime for OS Initialize

NuttShell (NSH)
nsh>
nsh>
nsh> mtetest
Spawning process for test: mtetest1
Running test: mtetest1
Test 'mtetest1' completed
Spawning process for test: mtetest2
Running test: mtetest2
Test 'mtetest2' completed
Spawning process for test: mtetest3
Running test: mtetest3
Test 'mtetest3' completed
Spawning process for test: mtetest4
Running test: mtetest4
Test 'mtetest4' completed
Spawning process for test: mtetest5
Running test: mtetest5
Test 'mtetest5' completed
All tests completed.
nsh>

Impact

Depends on apache/nuttx#14978

Testing

@nuttxpr
Copy link

nuttxpr commented Dec 2, 2024

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements, but is missing some key information. Here's a breakdown:

Strengths:

  • Clear Summary of Changes: The summary explains what was changed (MTE instruction tests) and why (presumably to verify MTE functionality). The how is somewhat implied (by adding tests), but could be more explicit.
  • Testing Logs Provided: Logs demonstrating the tests running before and after the change are included. This is excellent.
  • Dependency Noted: Referencing the dependent PR is crucial for reviewers.

Weaknesses:

  • Missing Issue References: If this relates to a specific issue in either nuttx or nuttx-apps, those links should be provided.
  • Incomplete Impact Assessment: The "Impact" section is very sparse. It acknowledges a dependency, but doesn't address most of the required points. Critically, it doesn't mention the impact on hardware (clearly this is AArch64 specific with MTE enabled). Each "NO" answer should have a brief justification (e.g., "Impact on build: NO - No changes to the build system were required"). "YES" answers need details.
  • Insufficient "How" in Summary: While the changes are described, more detail is needed on how they work. For example, what specific aspects of the MTE instructions are being tested? What constitutes a pass/fail condition in each test?
  • Unclear "Before" Testing: The "before" logs show a standard NuttShell prompt. Was there any prior MTE testing code that failed? Or is this entirely new functionality, meaning there should be no output related to MTE tests before the change? This needs clarification.
  • Missing Build Host Information: The "Testing" section lacks information about the build host environment. OS, CPU architecture, compiler version, and ideally the NuttX configuration used should be specified.
  • Target Information Incomplete: While the QEMU command line is provided, it would be helpful to specify the NuttX configuration used for the target.

Recommendations for Improvement:

  1. Link Related Issues: Provide links to any relevant NuttX or NuttX-apps issues.
  2. Expand Impact Assessment: Address all points in the Impact section. For "NO" answers, briefly justify. For "YES" answers, provide details. Specifically mention the AArch64/MTE hardware dependency.
  3. Clarify "How" in Summary: Explain the test methodology. What aspects of MTE are being tested? What are the pass/fail criteria?
  4. Explain "Before" Testing Logs: If the tests are entirely new, state that explicitly. If there was existing behavior that changed, describe it and show the relevant "before" logs.
  5. Provide Build Host Details: Include OS, CPU, compiler version, and NuttX configuration used on the build host.
  6. Specify Target Configuration: In addition to the QEMU command line, specify the NuttX configuration used for the target.

By addressing these points, the PR will be significantly improved and easier for reviewers to evaluate.

Added basic mte instructions, ldg, stg, irg, gmi instruction tests

➜  NX git:(master) ✗ qemu-system-aarch64 -cpu max -nographic \
        -machine virt,virtualization=on,gic-version=3,mte=on \
        -chardev stdio,id=con,mux=on, -serial chardev:con \
        -mon chardev=con,mode=readline  -kernel ./nuttx/nuttx
- Ready to Boot Primary CPU
- Boot from EL2
- Boot from EL1
- Boot to C runtime for OS Initialize

NuttShell (NSH)
nsh>
nsh>
nsh> mtetest
Spawning process for test: mtetest1
Running test: mtetest1
Test 'mtetest1' completed
Spawning process for test: mtetest2
Running test: mtetest2
Test 'mtetest2' completed
Spawning process for test: mtetest3
Running test: mtetest3
Test 'mtetest3' completed
Spawning process for test: mtetest4
Running test: mtetest4
Test 'mtetest4' completed
Spawning process for test: mtetest5
Running test: mtetest5
Test 'mtetest5' completed
All tests completed.
nsh>

Signed-off-by: wangmingrong1 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants