redo map(), add unmap(), invalidate tlb entries #62

wjhun · 2018-09-05T02:57:16Z

This implements a recursive version of map() / unmap() which writes page table entries from the page level upward in an attempt to insure a coherent table to avoid the TLB filling entries from an incomplete table. (Please see discussion in #33) It also uses the invlpg instruction on each page where a "present" (bit 0 set) page table entry was replaced.

The backed heap now calls unmap() after a page dealloc, as it should. network_test appears to run without problems.

I am also invalidating when adding a new upper-level table. While it shouldn't hurt, is this necessary?
The "fixup" for the stack mapping in stage2.c needs some scrutiny. It previously was calling map() with an unaligned address (stack end - 4) and length of two pages. It seems it was probably only actually mapping the second of the two pages - and the following, unallocated page. If I try to fix the parameters in centry(), I get strange results. Rewinding the stack argument before calling map() is a kludge.

…consistent table state from getting cached in TLB (see discussion in #33), do invalidate (invlpg) when necessary, introduce unmap()

convolvatron · 2018-09-05T04:27:54Z

boot/stage2.c

@@ -58,6 +58,7 @@ void kernel_read_complete(heap physical, heap working, u64 stack, u32 stacklen,
    map(pmem, pmem, identity_length, pages);
    // going to some trouble to set this up here, but its barely
    // used in stage3
+    stack -= (stacklen - 4);	/* XXX b0rk b0rk b0rk */


you know i'm not even sure thats necessary (the 4 byte offset)

its also* the case that stage3 now constructs and jumps to its own stack, which wasn't originally the case.

so idk, maybe worth looking at some point as to whether we can get through stage2 on the original bios stack, or put this one in the identity map

Thanks, sounds like a future sweep of boot/stage2 would be in order.

convolvatron · 2018-09-05T04:33:38Z

x86_64/page.c

+{
+    u64 flags;
+
+    /* Special case for zero page...? see init_service_new_task() */


i dont think you need to maintain this really, I just thought it was cute to equate unmap to mapping with invalid physical. so unmap should really be fine.

actually...we also dont need to map this at all anymore, originally since we were running on the bios stack when entering 64 bit mode we had to have it mapped on entry and then remove it..but i believe thats no longer the case.

and just for background the only reason this is unmapped is so that we trap on writing to zero, if we dont alot goes uncaught

I can check if trapping on write to zero works without it and remove if so.

Removing the map(0, 0, ...) from stage2 doesn't work. Still need to unmap() in service.c or else 0 page accesses don't throw a page fault.

convolvatron · 2018-09-05T04:41:19Z

x86_64/page.c

@@ -52,55 +56,72 @@ physical physical_from_virtual(void *x)
 }
 #endif

-static void write_pte(page target, physical to, boolean fat)
+/* virtual from physical of n required if we move off the identity map for pages */


does this seem straightforward to you? the identity map isn't the end of the world, but it does constrain the virtual layout in the low space...particularly given that we arent tracking mappings

i guess I was afraid of managing the recurrence of finding and mapping pages to express the mapping of the pages. not sure its thats scary

The identity map certainly is convenient for us. Is there some reason we would need to keep that low virtual space open, e.g. compatibility in userspace?

some programs have traditionally been linked to run at 0x100000, but 0x400000 seems
to be standard. I might be confusing my systems. that leaves us 4m or 1k pages for
mapping, which maybe if we use 2m pages is enough.

now that the allocation tools are a little more flexible we can maybe consider putting it up at the top of available physical?

oh wait, thats 2G max memory, that probably no where near enough in the limit

Please see my latest update pushed to page.c: now invalidate is set by write_pte() on the condition that an entry with a "present" bit has changed. As discussed, it's not clear if this is sufficient for every type of x86 CPU, but it appears to be sufficient for AMD and Intel, AFAIK.

There's a problem uncovered here: I'm seeing the identity map being placed at 0x21e000 with a length of 0x300000. There's a clash with the program being mapped at 0x400000... this will invariably lead to a crash down the road.

Also, since this address is not 2M aligned, and since there isn't a 2M stretch within that region, it's all being mapped as 4K pages.

If you want to try it out, uncomment the "#define PAGE_DEBUG" at the top of page.c to see detailed output...look for "invalidate" and you'll see the identity mappings getting knocked out. This output allows you to clearly see how everything is being mapped - and what entries are being displaced (intentionally or not).

We'll have a better defense against this when the rtrie / vm info work is in place...which I really hope to get to soon.

convolvatron · 2018-09-05T04:43:12Z

x86_64/page.c

+		return false;
+	    if (!force_entry(h, n, v, p, level + 1, fat, flags, invalidate))
+		return false;
+	    *invalidate = true;	/* XXX necessary when adding table entry? */


if you are adding a new entry, there shouldn't be a conflicting one present in the tlb? maybe there are some gotchas in the multiprocessor case?

I couldn't think of a reason to invalidate, especially since writing entries from the bottom-up should avoid that case mentioned in the AMD manual. But I wasn't completely sure, so it's out of an abundance of caution.

convolvatron

sorry, I go over this one more time in the morning. i just want to trace the invalidate path

but its a lot prettier

convolvatron · 2018-09-05T14:20:15Z

x86_64/page.c

+
+    if (level == (fat ? 3 : 4)) {
+	if (present)
+	    *invalidate = true;


sorry if I'm missing something, but doesn't this always force invalidate down the tree, even if the existing mapping is the same (the common case)

It's only for the terminal (page) entry and if the existing one was marked present. So it should not be invalidate for new entries.

But when do we write over existing entries (besides deletion)?

But, yes, it should check if it's the same and do a no-op if so.

The answer to this stackoverflow question is interesting...and suggests that, on some CPUs, an invalidate is needed even if a non-present entry is replaced with present...

https://stackoverflow.com/questions/28384234/when-to-do-or-not-do-invlpg-mov-to-cr3-to-minimize-tlb-flushing#28384866

convolvatron · 2018-09-05T14:43:12Z

x86_64/page.c

-        flags = PAGE_WRITABLE | PAGE_PRESENT | PAGE_USER;
-    }
+    // really set user?
+    u64 flags = PAGE_WRITABLE | PAGE_PRESENT | PAGE_USER;
    map_range(virtual, p, length, flags, h);


i guess depending on the outcome of the boot stack issue we may not need to map this in the first place, and thus won't need to unmap it here

…esent pte (for now), enhanced debugging output

Will Jhun added 3 commits September 4, 2018 21:53

map() rewrite, builds table entries from bottom (page) up to avoid in…

4876e7b

…consistent table state from getting cached in TLB (see discussion in #33), do invalidate (invlpg) when necessary, introduce unmap()

backed heap unmap() on dealloc

f4ae6df

sanity-check map alignment, "fix" two unaligned map()s in stage2

4b487cb

convolvatron reviewed Sep 5, 2018

View reviewed changes

remove special case, call unmap() in init_service_new_stack()

7e169fd

convolvatron reviewed Sep 5, 2018

View reviewed changes

Will Jhun added 2 commits September 5, 2018 11:37

move invalidation up to write_pte, only invalidate when changing a pr…

904590c

…esent pte (for now), enhanced debugging output

no longer need to flush tlb on mmap()

525d7d7

convolvatron approved these changes Sep 5, 2018

View reviewed changes

Merge branch 'master' into memory2

b2836ae

wjhun merged commit f8e9c14 into master Sep 5, 2018

wjhun mentioned this pull request Sep 5, 2018

dont flush tlb on every map #33

Closed

wjhun deleted the memory2 branch September 7, 2018 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redo map(), add unmap(), invalidate tlb entries #62

redo map(), add unmap(), invalidate tlb entries #62

wjhun commented Sep 5, 2018

convolvatron Sep 5, 2018

wjhun Sep 5, 2018

convolvatron Sep 5, 2018

wjhun Sep 5, 2018

wjhun Sep 5, 2018

convolvatron Sep 5, 2018

wjhun Sep 5, 2018

convolvatron Sep 5, 2018

convolvatron Sep 5, 2018

wjhun Sep 5, 2018

convolvatron Sep 5, 2018

wjhun Sep 5, 2018

convolvatron left a comment

convolvatron Sep 5, 2018

wjhun Sep 5, 2018

wjhun Sep 5, 2018

convolvatron Sep 5, 2018

redo map(), add unmap(), invalidate tlb entries #62

redo map(), add unmap(), invalidate tlb entries #62

Conversation

wjhun commented Sep 5, 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

convolvatron left a 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