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

qemu:fix sabre-6quad boot failed with kernel mode #13518

Closed
wants to merge 1 commit into from

Conversation

gneworld
Copy link

@gneworld gneworld commented Sep 18, 2024

Summary

fix sabre-6quad boot failed with kernel mode, for the lack of two syscall api nxsched_self and nxsched_get_tcb

image

Impact

  • Is new feature added? NO
  • Is existing feature changed? YES (Kernel mode functionality on sabre-6quad).
  • Impact on user? Potentially YES, users may need to rebuild their kernel modules for kernel mode."
  • Impact on build? NO
  • Impact on hardware? YES (specifically sabre-6quad).
  • Impact on documentation? NO
  • Impact on security? /NO
  • Impact on compatibility? NO

Testing

./tools/configure.sh sabre-6quad:knsh
make -j10
make export V=1
cd ../apps
./tools/mkimport.sh -z -x ../nuttx/nuttx-export-*.tar.gz
make import
cd ../nuttx
qemu-system-arm -semihosting -M sabrelite -m 1024 -smp 4 -nographic -kernel ./nuttx

@lupyuen
Copy link
Member

lupyuen commented Sep 18, 2024

[Experimental Bot, please feedback here]

Let's break down whether this Pull Request (PR) description meets the NuttX requirements.

Strengths:

  • Summary: You clearly state the problem ("sabre-6quad boot failed with kernel mode") and imply it's a fix. However, it would be even better to briefly explain why it failed.
  • Impact: You correctly identify the affected area (kernel mode of sabre-6quad), but you should expand on the other impact points (see below).
  • Testing: You provide a solid testing procedure, which is excellent.

Areas for Improvement:

  • Summary:

    • Why is the change necessary? Be more specific about the cause of the boot failure. Was it a bug, a configuration issue, etc.?
    • What functional part of the code is changed? Mention the specific files, modules, or functions you modified.
    • How does the change work? Provide a concise explanation of your solution. For example, "Fixed an incorrect memory mapping configuration that prevented kernel mode initialization."
    • Related Issues: If there's a corresponding issue on the NuttX or NuttX Apps repositories, link it here.
  • Impact: You need to address all impact points, even if the answer is "NO." Provide brief explanations where relevant. For example:

    • New feature? NO
    • Existing feature changed? YES (Kernel mode functionality on sabre-6quad)
    • Impact on user? Potentially YES (if they rely on kernel mode). Describe any changes users might need to make.
    • Impact on build? NO (assuming your changes don't affect the build process).
    • Impact on hardware? YES (specifically sabre-6quad).
    • Impact on documentation? Maybe YES (if the fix involves configuration changes users need to be aware of).
    • Impact on security? Consider if your fix has any security implications (positive or negative).
    • Impact on compatibility? Assess potential compatibility issues with older NuttX versions or other hardware.
  • Testing:

    • Build Host(s): Specify your development environment (OS, CPU, compiler version).
    • Target(s): You mention the board (sabre-6quad:knsh), but clarify the architecture (likely ARM).
    • Testing logs: While you provide the commands, it's essential to include the actual output of those commands before and after your changes. This helps reviewers understand the problem and verify the fix.

Revised PR Description Template:

## Summary

The sabre-6quad board was failing to boot in kernel mode due to [briefly explain the root cause, e.g., an incorrect memory mapping configuration]. This PR fixes the issue by [concisely describe your solution, e.g., adjusting the memory regions assigned to the kernel during initialization in the board-specific configuration file]. 

This change modifies [mention the specific files or modules affected, e.g., `boards/arm/armv7-m/sabre-6quad/src/sabre_boot.c`].

* Related Issue: [Link to NuttX issue if applicable]

## Impact

* Is new feature added? NO
* Is existing feature changed? YES (Kernel mode functionality on sabre-6quad).
* Impact on user? Potentially YES (if they rely on kernel mode).  [Describe any user-facing changes, e.g.,  "Users may need to rebuild their kernel modules for kernel mode."]
* Impact on build? NO
* Impact on hardware? YES (specifically sabre-6quad).
* Impact on documentation? [YES/NO] [Explain if documentation needs updates.]
* Impact on security? [YES/NO] [Explain any security implications.]
* Impact on compatibility? [YES/NO] [Explain any compatibility concerns.]

## Testing

I confirm that changes are verified on my local setup and work as intended:

* Build Host(s): [Your OS] ([Your CPU]), [Your compiler] ([compiler version])
* Target(s): ARM, sabre-6quad:knsh

Testing logs **before** change:

[Paste the output of your test commands here]


Testing logs **after** change:

[Paste the output of your test commands after applying your fix]

Remember: A well-written PR description significantly increases the chances of your contribution being reviewed and merged quickly.

@acassis
Copy link
Contributor

acassis commented Sep 18, 2024

@lupyuen this AI bot is very useful to highlight missing things! It will avoid us repeating same thing in all PRs!

@acassis
Copy link
Contributor

acassis commented Sep 18, 2024

@gneworld please include commit log message!

@acassis
Copy link
Contributor

acassis commented Sep 18, 2024

@lupyuen is there a way to highlight the PR is missing commit log message? Maybe we could issue automatically a request change to avoid us to repeating it too

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include commit log message

@gneworld
Copy link
Author

@gneworld please include commit log message!

@acassis done

@gneworld gneworld closed this Sep 19, 2024
@gneworld gneworld deleted the work8 branch September 19, 2024 08:11
@gneworld gneworld restored the work8 branch September 19, 2024 08:57
@gneworld gneworld reopened this Sep 19, 2024
@@ -44,6 +44,8 @@ SYSCALL_LOOKUP(sched_setscheduler, 3)
SYSCALL_LOOKUP(sched_unlock, 0)
SYSCALL_LOOKUP(sched_yield, 0)
SYSCALL_LOOKUP(nxsched_get_stackinfo, 2)
SYSCALL_LOOKUP(nxsched_self, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these exposed to userspace ?

@@ -84,6 +84,8 @@
"nx_pthread_exit","nuttx/pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
"nx_vsyslog","nuttx/syslog/syslog.h","","int","int","FAR const IPTR char *","FAR va_list *"
"nxsched_get_stackinfo","nuttx/sched.h","","int","pid_t","FAR struct stackinfo_s *"
"nxsched_self","nuttx/sched.h","","FAR struct tcb_s *"
Copy link
Contributor

@pussuw pussuw Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does userspace need to know the composition of TCB ?

@gneworld
Copy link
Author

gneworld commented Sep 22, 2024

@anjiahao1 When will this feature "nsh:support wait command to wait task exit" which has calling of nxsched_self and nxsched_get_tcb be upstreamed to the community? :)

Signed-off-by: wanggang26 <wanggang26@xiaomi.com>
@github-actions github-actions bot added Area: OS Components OS Components issues Size: XS The size of the change in this PR is very small labels Sep 22, 2024
@xiaoxiang781216
Copy link
Contributor

@anjiahao1 When will this feature "nsh:support wait command to wait task exit" which has calling of nxsched_self and nxsched_get_tcb be upstreamed to the community? :)

let's replace nxsched_self with pthread_self.

@xiaoxiang781216
Copy link
Contributor

@gneworld please revert this patch, we don't need it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: XS The size of the change in this PR is very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants