Feature/process #49

jjgarzella · 2018-06-08T15:58:11Z

Add address space creation options, along with some testing and debug utilities for page tables.

Right now, the copying the higher half is optional because there is some sort of a bug in the higher half of the original p4 that we have. Code to trigger it is in the fixme.

Make a new module for creation of processes, and in that module, create a new page table that will become the address space for our new process Copy p3 entries to new p4 table Copy the kernel (in the higher half) into the new address space. Many memory-module functions are made public to allow this.

Done so that the code in process can be factored into the paging mod, removing the necessecity for everything to be public.

Add a method that copies a table into a new frame, copying the higher half of the entries.

Fix the copy_higher_half() method of Table to map the table before using it Found bug(?) in recursive mapping of original table, put in a fixme Add testing for copy_higher_half() Derive a few traits to help with debugging of entries. Also, wrote print_entries() method on Table Remove the now unused src/process.rs file Finally, add the option to copy the higher half upon creation of an InactivePageTable. This is not default behavior because of the aforementioned bug.

4e554c4c · 2018-06-08T20:10:48Z

src/memory/paging/entry.rs

@@ -12,6 +12,7 @@ use multiboot2::ElfSection;
 use memory::Frame;

 /// A representation of a page table entry.
+#[derive(Clone,PartialEq,Debug)]


How will we make sure this doesn't result in a double free?

I don't know how it would cause a double free... can you explain?

An entry owns a physical frame, and when it's deallocated so is it's backing frame. If Entrys are clone, then a frame that is in use could be cloned and freed to the allocator, leading to memory corruption. There was a hack around this with Frames in phil's blog.

4e554c4c · 2018-06-08T20:14:21Z

src/memory/paging/table.rs

+    pub fn clone_higher_half_into<A>(&self, table: &mut Table<A>)
+        where A: TableLevel
+    {
+        let mut i = 0b1_0000_0000; // == 0x100 == 256


I would prefer 1 << 8 or 0x100

4e554c4c · 2018-06-08T20:16:01Z

src/memory/paging/table.rs

+                       "Higher half not identical to previous p4 (bottom)");
+        tap.assert_tap(new_p4[0b1_0000_1000] == active_p4[0b1_0000_1000],
+                       "Higher half not identical to previous p4 (random)");
+        tap.assert_tap(new_p4[510] == active_p4[510],


The recursive mapping /should not/ be the same between table copies. If it is, then trying to modify the new table will change the old one.

I'm all use cases, yes this is necessary. However, this is just a unit test, and the method just copies the higher half. In InactivePageTable, where this is used, it is recursively mapped properly. Technically, the function has the desired behavior and the unit test tests that. Do you think we should fold the recursive mapping functionality into here?

4e554c4c · 2018-06-08T20:16:27Z

src/memory/paging/table.rs

+        tap.assert_tap(new_p4[0b0_1111_1111].is_unused(),
+                       "Lower half not zeroed on copied p4 (top of lower half)");
+
+        tap.assert_tap(new_p4[0b1_0000_0000] == active_p4[0b1_0000_0000],


ditto here & below

4e554c4c · 2018-06-08T20:18:03Z

src/memory/paging/mod.rs

+    //       in this original p4 is not recursively mapped correctly.
+    //       This code triggers that.
+    //
+    //serial_println!("About to try to map at 511th p4, 510th p3, 0th p2");


I don't understand this print statement. there is only one p4 table. Also we recurvisely map at the 510th index, not 511

Here's the address in binary: 1111111111111111 | 1 1111 1111 | 1 1111 1110 | 0 0000 0000 | 1 0000 0001 | 0000 0000 0000

First, we get the last p4, then we recursively go through the p3, then we get the very first page (zero). However, through debugging I found that the 'p3' table and 'p2' tables were not identical... concluding that the p3 is not recursively mapped.

the p3 at 511 shouldn't be recursively mapped

If that's true, then that's the answer to the other problems we've been having then. The 511th p3 isn't recursively mapped. When we created a new p4 from scratch, when it mapped the kernel it just created new tables at the 511th position, but when I had it copying in the old p4 entries on the higher half, it tried to map something to that non-recursively-mapped page, causing a problem. The solution is to just not copy the entries over in this situation.

4e554c4c · 2018-06-08T20:23:17Z

src/memory/paging/mod.rs

+    //serial_println!("About to try to map at 511th p4, 510th p3, 0th p2");
+    //let frame = allocator.allocate_frame().unwrap();
+    //let page = Page::containing_address(0xffffffff80101000);
+    //active_table.mapper.map_to(page, frame, EntryFlags::PRESENT, &mut allocator).expect("Unable to map sample page");


This probably pagefaults because 0xffffffff80101000 is literally the first kernel page

This is done before remap_the_kernel does anything to the pages... the original bug was that the OS was trying to remap the kernel into this page and it pagefaulted.

odd. If it's faulting while remapping the kernel we can't merge until that's fixed.

4e554c4c · 2018-06-11T00:17:53Z

Just so we're on the same page. Are we merging this in or keeping process work on the feature/process branch?

jjgarzella · 2018-06-11T00:31:40Z

We can merge this in. It's relatively self-contained, but there are obviously two or three outstanding issues.

4e554c4c · 2018-06-11T19:00:20Z

src/lib.rs

@@ -106,6 +108,8 @@ pub extern "C" fn rust_main(multiboot_info_address: usize) -> ! {
    println!("Try to write some things!");
    vga_buffer::change_color(vga_buffer::Color::White, vga_buffer::Color::Black);

+ //   process::create_process();
+
    #[cfg(feature = "test")] {
        run_tests();


Changes in lib.rs don't seem needed

4e554c4c · 2018-06-11T19:02:09Z

src/memory/paging/entry.rs

@@ -12,6 +12,7 @@ use multiboot2::ElfSection;
 use memory::Frame;

 /// A representation of a page table entry.
+#[derive(Clone,PartialEq,Debug)]


An entry owns a physical frame, and when it's deallocated so is it's backing frame. If Entrys are clone, then a frame that is in use could be cloned and freed to the allocator, leading to memory corruption. There was a hack around this with Frames in phil's blog.

4e554c4c · 2018-06-11T19:03:55Z

src/memory/paging/mod.rs

+    //       in this original p4 is not recursively mapped correctly.
+    //       This code triggers that.
+    //
+    //serial_println!("About to try to map at 511th p4, 510th p3, 0th p2");


the p3 at 511 shouldn't be recursively mapped

4e554c4c · 2018-06-11T19:04:45Z

src/memory/paging/mod.rs

+    //serial_println!("About to try to map at 511th p4, 510th p3, 0th p2");
+    //let frame = allocator.allocate_frame().unwrap();
+    //let page = Page::containing_address(0xffffffff80101000);
+    //active_table.mapper.map_to(page, frame, EntryFlags::PRESENT, &mut allocator).expect("Unable to map sample page");


odd. If it's faulting while remapping the kernel we can't merge until that's fixed.

4e554c4c · 2018-06-11T19:05:56Z

src/memory/paging/table.rs

+    use tap::TestGroup;
+
+    pub fn run() {
+


extra newline

4e554c4c · 2018-06-11T19:07:08Z

src/memory/paging/table.rs

+                       "Higher half not identical to previous p4 (bottom)");
+        tap.assert_tap(new_p4[0x108] == active_p4[0x108],
+                       "Higher half not identical to previous p4 (random)");
+        tap.assert_tap(new_p4[510] == active_p4[510],


I think we should test something like a kernel page rather than the recursive mapping because this will be overwritten when the new table is used.

Remove references to deleted process.rs Add note about macros in lib.rs Small visual fixes Check kernel table instead of recursively mapped one in tests of clone_higher_half() Make entry not have the clone type, simply a clone method that could be unsafe.

The recursive mapping error has been deemed a non-issue, so the comment with the fixme has been removed.

jjgarzella added 4 commits June 8, 2018 09:53

Revert all public changes to paging mod

bc0addd

Done so that the code in process can be factored into the paging mod, removing the necessecity for everything to be public.

Add copying funcitonality to talbe mod

ef225d4

Add a method that copies a table into a new frame, copying the higher half of the entries.

jjgarzella requested a review from 4e554c4c June 8, 2018 15:58

4e554c4c reviewed Jun 8, 2018

View reviewed changes

Change binary constants to hex

cec0ca7

4e554c4c suggested changes Jun 11, 2018

View reviewed changes

jjgarzella added 2 commits June 11, 2018 15:05

Remove comment with paging error

8ed2f93

The recursive mapping error has been deemed a non-issue, so the comment with the fixme has been removed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/process #49

Feature/process #49

jjgarzella commented Jun 8, 2018

4e554c4c Jun 8, 2018

jjgarzella Jun 8, 2018

4e554c4c Jun 11, 2018

4e554c4c Jun 8, 2018

4e554c4c Jun 8, 2018

jjgarzella Jun 8, 2018

4e554c4c Jun 8, 2018

4e554c4c Jun 8, 2018

jjgarzella Jun 8, 2018

4e554c4c Jun 11, 2018

jjgarzella Jun 11, 2018

4e554c4c Jun 8, 2018

jjgarzella Jun 8, 2018

4e554c4c Jun 11, 2018

4e554c4c commented Jun 11, 2018

jjgarzella commented Jun 11, 2018

4e554c4c Jun 11, 2018

4e554c4c Jun 11, 2018

4e554c4c Jun 11, 2018

4e554c4c Jun 11, 2018

4e554c4c Jun 11, 2018

4e554c4c Jun 11, 2018

Feature/process #49

Are you sure you want to change the base?

Feature/process #49

Conversation

jjgarzella commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4e554c4c commented Jun 11, 2018

jjgarzella commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment