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

feat: use RustSBI 0.4.0 proc macros to simplify VCpu SBI implementation #8

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

luojia65
Copy link
Contributor

@luojia65 luojia65 commented Oct 21, 2024

This pull request simplifies SBI implementation in RISCVVCpu by using RustSBI 0.4.0 Forward structure, it forwards all SBI calls on console, pmu, fence, reset and info (base extension) without missing any SBI features like extension detection and all functions in the extension.

For example, the original code only forwards remote_fence_i and remote_sfence_vma in fence (RFNC) extension, but by reusing RustSBI Forward feature we forward all 7 functions in the RFNC extension.

By using RustSBI 0.4.0 Forward, we save a lot of code on RISC-V hypervisor development.

Ref: https://github.com/KuangjuX/hypercraft/pull/3/files

r? @hky1999

src/detect.rs Outdated Show resolved Hide resolved
Signed-off-by: Zhouqi Jiang <luojia@hust.edu.cn>
@hky1999
Copy link
Contributor

hky1999 commented Oct 22, 2024

Thanks for your work, I'll review it later

hky1999
hky1999 previously approved these changes Oct 22, 2024
Copy link
Contributor

@hky1999 hky1999 left a comment

Choose a reason for hiding this comment

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

except for naked_asm modification, all LGTM

src/detect.rs Outdated Show resolved Hide resolved
src/detect.rs Outdated Show resolved Hide resolved
@luojia65
Copy link
Contributor Author

except for naked_asm modification, all LGTM

Thanks for review! For naked_asm, I have two solutions to this problem.

The first approach is using the rustversion crate, as is mentioned in this pull request: rcore-os/buddy_system_allocator#3. The second approach is just reverting it to asm! as we only build this project under ArceOS toolchain version.

What's your opinion on this? @hky1999

@hky1999
Copy link
Contributor

hky1999 commented Oct 23, 2024

except for naked_asm modification, all LGTM

Thanks for review! For naked_asm, I have two solutions to this problem.

The first approach is using the rustversion crate, as is mentioned in this pull request: rcore-os/buddy_system_allocator#3. The second approach is just reverting it to asm! as we only build this project under ArceOS toolchain version.

What's your opinion on this? @hky1999

Sorry for delay, a bit busy these days... (so much email send from github I missed this

The first approach is using the rustversion crate, as is mentioned in this pull request: rcore-os/buddy_system_allocator#3.

It's a good solution but I'm worried that this will further increase the complexity of the code

The second approach is just reverting it to asm! as we only build this project under ArceOS toolchain version.

Let's just go back to this solution, after that I think this PR is ready to merge

@hky1999
Copy link
Contributor

hky1999 commented Oct 23, 2024

also, pay attention to the ci error @luojia65

(#![feature(asm_const)] is needed in the “outdated“ of rust toolchain

@luojia65
Copy link
Contributor Author

@hky1999: Thanks! Will modify soon.

Signed-off-by: Zhouqi Jiang <luojia@hust.edu.cn>
Copy link
Contributor

@hky1999 hky1999 left a comment

Choose a reason for hiding this comment

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

LGTM
thanks for your work

@hky1999 hky1999 merged commit 3ae7deb into arceos-hypervisor:master Oct 23, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants