-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add 4kb paging #84
Add 4kb paging #84
Conversation
a65fabb
to
4407e9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few points ;)
@@ -1,83 +1,138 @@ | |||
const std = @import("std"); | |||
const builtin = @import("builtin"); | |||
const panic = @import("../../kmain.zig").panic; | |||
const arch = @import("../../arch.zig").internals; | |||
const isr = @import("isr.zig"); | |||
const assert = std.debug.assert; | |||
const MemProfile = @import("../../mem.zig").MemProfile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const mem= @import("../../mem.zig");
const MemProfile = mem.MemProfile;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But mem is only used to get MemProfile so there's no point importing it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in that case, surly just const mem = @import("../../mem.zig");
and use `mem.MemProfile. Bc what if you want to use something else from mem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use const MemProfile = @import("../../mem.zig").MemProfile
and later on want to use something else from mem, then you can change it to
const mem = @import("../../mem.zig");
const MemProfile = mem.MemProfile;
and use mem
wherever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a few comments
62b4fa7
to
5c15376
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but confused why you use 4KB in places and others 4MB.
ac13ed0
to
479bd2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No cheating ;)
src/kernel/arch/x86/paging.zig
Outdated
@@ -372,7 +372,8 @@ test "virtToPhys" { | |||
const offset: usize = @ptrToInt(&KERNEL_ADDR_OFFSET); | |||
expectEqual(virtToPhys(offset + 0), 0); | |||
expectEqual(virtToPhys(offset + 123), 123); | |||
expectEqual(@ptrToInt(virtToPhys(@intToPtr(*usize, offset + 123))), 123); | |||
if (builtin.mode != builtin.Mode.ReleaseSafe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XD, This is cheating. I think it is bc ur comparing a pointer to a uint. @ptrToInt() != u32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:( I hoped you wouldn't notice the sneakyness. How do you think I could fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a workaround. It's not nice, but is all I can get working at the moment. I'll add an issue to monitor it once this is merged.
c68fa6a
to
59d78c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR adds 4KB paging support. Closes #79