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

arch/riscv64: revise comments #1355

Merged
merged 1 commit into from
Nov 26, 2024
Merged

arch/riscv64: revise comments #1355

merged 1 commit into from
Nov 26, 2024

Conversation

yf13
Copy link
Contributor

@yf13 yf13 commented Nov 25, 2024

This revises comments of riscv64 kernel address mapping for easier reading purposes.

@yf13 yf13 force-pushed the comments branch 7 times, most recently from 2c4b910 to 0d60825 Compare November 25, 2024 10:04
@Indanz
Copy link
Contributor

Indanz commented Nov 25, 2024

First impression is that you're changing too much at once and it's overall not obviously better than what we have now, when comparing these side-by-side:

https://raw.githubusercontent.com/seL4/seL4/refs/heads/master/include/arch/riscv/arch/64/mode/hardware.h
https://raw.githubusercontent.com/seL4/seL4/0d608251f653679de2b7cf93a784f89c97dc6a67/include/arch/riscv/arch/64/mode/hardware.h

Maybe have separate commits for content changes/fixes and indentation/layout changes.

I like the addition of PPTR_TOP, removal of the nonsensical (KERNEL_ELF_BASE % 1GiB). I don't like the squashing things to the left so much.

Frankly, I think the whole thing would be clearer if the addresses were printed too, like Arm does. There they keep it narrow by putting the next level translations underneath instead of next to the previous block.

This revises comments of virtual space mapping.

Signed-off-by: Yanfeng Liu <[email protected]>
@yf13
Copy link
Contributor Author

yf13 commented Nov 26, 2024

@Indanz, okay I will split contents change from layout changes.

@Indanz
Copy link
Contributor

Indanz commented Nov 26, 2024 via email

Copy link
Member

@kent-mcleod kent-mcleod left a comment

Choose a reason for hiding this comment

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

I agree that the 2 line change makes the diagram clearer.

@kent-mcleod kent-mcleod merged commit 58674cd into seL4:master Nov 26, 2024
36 of 43 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.

3 participants