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-armv8a/ivshmem: add ivshmem chapter #15827

Merged
merged 1 commit into from
Feb 13, 2025
Merged

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Feb 13, 2025

Summary

qemu-armv8a/ivshmem: add ivshmem chapter

An example implementation for OpenAMP based on the Inter-VM share memory(ivshmem)::

rpproxy_ivshmem: Remote slave(client) proxy process.
rpserver_ivshmem: Remote master(host) server process.

Please refer to the official Qemu ivshmem documentation for more information:

https://www.qemu.org/docs/master/system/devices/ivshmem.html

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

a. Start rpserver_ivshmem::

  $ qemu-system-aarch64 -cpu cortex-a53 -nographic -machine virt,virtualization=on,gic-version=3 -kernel server/nuttx \
    -device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,addr=0xb \
    -object memory-backend-file,id=shmmem-shmem0,mem-path=/dev/shm/ivshmem0,size=4194304,share=yes

b. Start rpproxy_ivshmem::

  $ qemu-system-aarch64 -cpu cortex-a53 -nographic -machine virt,virtualization=on,gic-version=3 -kernel proxy/nuttx \
    -device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,addr=0xb \
    -object memory-backend-file,discard-data=on,id=shmmem-shmem0,mem-path=/dev/shm/ivshmem0,size=4194304,share=yes

c. Check the RPMSG Syslog in rpserver shell:

In the current configuration, the proxy syslog will be sent to the server by default.
You can check whether there is proxy startup log in the server shell.

RpServer bring up::

    $ qemu-system-aarch64 -cpu cortex-a53 -nographic -machine virt,virtualization=on,gic-version=3 -kernel server/nuttx \
      -device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,addr=0xb \
      -object memory-backend-file,id=shmmem-shmem0,mem-path=/dev/shm/ivshmem0,size=4194304,share=yes
    [    0.000000] [ 0] [  INFO] [server] pci_register_rptun_ivshmem_driver: Register ivshmem driver, id=0, cpuname=proxy, master=1
    ...
    [    0.033200] [ 3] [  INFO] [server] ivshmem_probe: shmem addr=0x10400000 size=4194304 reg=0x10008000
    [    0.033700] [ 3] [  INFO] [server] rptun_ivshmem_probe: shmem addr=0x10400000 size=4194304

After rpproxy bring up, check the log from rpserver::

    NuttShell (NSH) NuttX-10.4.0
    server>
    [    0.000000] [ 0] [  INFO] [proxy] pci_register_rptun_ivshmem_driver: Register ivshmem driver, id=0, cpuname=server, master=0
    ...
    [    0.031400] [ 3] [  INFO] [proxy] ivshmem_probe: shmem addr=0x10400000 size=4194304 reg=0x10008000
    [    0.031800] [ 3] [  INFO] [proxy] rptun_ivshmem_probe: shmem addr=0x10400000 size=4194304
    [    0.033100] [ 3] [  INFO] [proxy] rptun_ivshmem_probe: Start the wdog

d. IPC test via RPMSG socket:

Start rpmsg socket server::

    server> rpsock_server stream block test
    server: create socket SOCK_STREAM nonblock 0
    server: bind cpu , name test ...
    server: listen ...
    server: try accept ...
    server: Connection accepted -- 4
    server: try accept ...

Switch to proxy shell and start rpmsg socket client, test start::

    proxy> rpsock_client stream block test server
    client: create socket SOCK_STREAM nonblock 0
    client: Connecting to server,test...
    client: Connected
    client send data, cnt 0, total len 64, BUFHEAD process0007, msg0000, name:test
    client recv data process0007, msg0000, name:test
    ...
    client recv done, total 4096000, endflags, send total 4096000
    client: Terminating

Check the log on rpserver shell::

    server recv data normal exit
    server Complete ret 0, errno 0

An example implementation for OpenAMP based on the Inter-VM share memory(ivshmem)::

rpproxy_ivshmem:  Remote slave(client) proxy process.
rpserver_ivshmem: Remote master(host) server process.

Please refer to the official Qemu ivshmem documentation for more information:

https://www.qemu.org/docs/master/system/devices/ivshmem.html

Signed-off-by: chao an <[email protected]>
@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Size: M The size of the change in this PR is medium labels Feb 13, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 13, 2025

[Experimental Bot, please feedback here]

This PR mostly meets the NuttX requirements, but is lacking in key areas, making it difficult to review properly. Here's a breakdown:

Strengths:

  • Clear Summary of Functionality: The summary explains the core changes (adding ivshmem support, including client and server components) and provides a helpful link to QEMU documentation.
  • Detailed Testing Steps: The testing section provides clear, step-by-step instructions for reproducing the functionality. This is a significant plus.
  • Example Logs: Including example logs, though limited, gives reviewers a better idea of expected output and helps with verification.

Weaknesses:

  • Impact Section is Inadequate: "N/A" is not sufficient. Even if the user impact is minimal, there are clear impacts on the codebase (new features, hardware dependencies). This section needs to be filled out thoroughly. Consider:
    • Is new feature added? YES - ivshmem support
    • Impact on hardware? YES - Requires QEMU with ivshmem support, specific configuration. Specify the architectures affected (armv8a).
    • Impact on documentation? Potentially YES - If no documentation exists for this feature, it should be added. If this PR doesn't include documentation, explain why (e.g., planned for a separate PR).
    • Impact on compatibility: Possibly YES - Does this affect existing OpenAMP implementations? Is it backwards compatible?
  • Missing "Before Change" Logs: The testing section requests "Testing logs before change," but none are provided. This makes it impossible to assess the impact of the changes.
  • Limited Scope of Testing Logs: The logs primarily focus on successful communication. More testing is needed, including:
    • Error Handling: What happens if the shared memory setup fails? If the connection drops?
    • Edge Cases: Test with different message sizes, large data transfers, stress testing.
    • Different Configurations: Test with variations in QEMU parameters.

Recommendations for Improvement:

  1. Expand the Impact Section: Address all the points in the template, even if the answer is "NO." Justify "NO" answers briefly. Provide detail for "YES" answers.
  2. Add "Before Change" Logs: Capture logs from the system without the ivshmem changes to demonstrate the difference in behavior.
  3. Broaden Testing and Include More Logs: Test error conditions, edge cases, and different configurations. Include logs for these tests.
  4. Consider Documentation: If this feature warrants documentation (and it likely does), either include it in this PR or explain why it will be handled separately.

By addressing these weaknesses, the PR will be much stronger and easier to review, increasing its chances of being accepted.

@anchao anchao requested review from acassis and yf13 February 13, 2025 01:53
@acassis acassis merged commit 2e21426 into apache:master Feb 13, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants