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

Add x86 paging rt test #73

Merged
merged 1 commit into from
Oct 1, 2019
Merged

Add x86 paging rt test #73

merged 1 commit into from
Oct 1, 2019

Conversation

SamTebbs33
Copy link
Collaborator

Adds a runtime test to x86 paging which ensures that unmapped memory causes a page fault. If you can think of any other tests I could add please say so. Part of #60

Copy link
Member

@DrDeano DrDeano left a comment

Choose a reason for hiding this comment

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

Can you use the full doc comment format for all functions/constants/variables and errors.
More unit tests :P

src/kernel/arch/x86/paging.zig Outdated Show resolved Hide resolved
src/kernel/arch/x86/paging.zig Outdated Show resolved Hide resolved
src/kernel/arch/x86/paging.zig Outdated Show resolved Hide resolved
src/kernel/arch/x86/paging.zig Outdated Show resolved Hide resolved
src/kernel/arch/x86/paging.zig Outdated Show resolved Hide resolved
src/kernel/arch/x86/paging.zig Show resolved Hide resolved
Copy link
Member

@DrDeano DrDeano left a comment

Choose a reason for hiding this comment

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

Only one comment :)

test/unittests/test_all.zig Outdated Show resolved Hide resolved
Copy link
Member

@DrDeano DrDeano left a comment

Choose a reason for hiding this comment

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

Another comment

test/unittests/test_all.zig Outdated Show resolved Hide resolved
@SamTebbs33
Copy link
Collaborator Author

We should wait until my 4kb paging PR is merged so I can add some new paging tests to this PR.

@SamTebbs33
Copy link
Collaborator Author

Ready for review again.

Copy link
Member

@DrDeano DrDeano left a comment

Choose a reason for hiding this comment

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

A few points

test/unittests/test_all.zig Outdated Show resolved Hide resolved
test/kernel/arch/x86/rt-test.py Outdated Show resolved Hide resolved
src/kernel/arch/x86/paging.zig Outdated Show resolved Hide resolved
src/kernel/arch/x86/paging.zig Outdated Show resolved Hide resolved
src/kernel/arch/x86/paging.zig Show resolved Hide resolved
Copy link
Member

@DrDeano DrDeano left a comment

Choose a reason for hiding this comment

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

Minor tweek

const tty_addr = tty.getVideoBufferAddress();
// If the previous mappping space didn't cover the tty buffer, do so now
if (v_start > tty_addr or v_end <= tty_addr) {
const tty_phys = virtToPhys(tty_addr);
const tty_buff_size = 32 * 1024;
mapDir(kernel_directory, tty_addr, tty_addr + tty_buff_size, tty_phys, tty_phys + tty_buff_size, allocator) catch panic(@errorReturnTrace(), "Failed to map vga buffer in kernel directory");
mapDir(kernel_directory, tty_addr, tty_addr + tty_buff_size, tty_phys, tty_phys + tty_buff_size, allocator) catch |e| panic(@errorReturnTrace(), "Failed to map vga buffer in kernel directory: {}", @errorName(e));
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the @errorName as fmt handles it for you. Also (u may reject), so the line isn't long, have this like:

mapDir(kernel_directory, tty_addr, tty_addr + tty_buff_size, tty_phys, tty_phys + tty_buff_size, allocator) catch |e| {
    panic(@errorReturnTrace(), "Failed to map vga buffer in kernel directory: {}\n", e);
};

Also for the other error printing.
Also add new lines (\n) to the end of the logging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh cool

Copy link
Member

@DrDeano DrDeano left a comment

Choose a reason for hiding this comment

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

bam

@SamTebbs33 SamTebbs33 merged commit 35dc8e3 into develop Oct 1, 2019
@SamTebbs33 SamTebbs33 deleted the feature/paging-rt-tests branch October 1, 2019 22:28
SamTebbs33 added a commit that referenced this pull request Oct 7, 2019
SamTebbs33 added a commit that referenced this pull request Oct 7, 2019
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