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 dedicated mechanism for page tables mapping/unmapping #333

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

wipawel
Copy link
Contributor

@wipawel wipawel commented Nov 14, 2023

This closes #66.

@wipawel wipawel added feature New feature or request Priority: 3 Regular feature labels Nov 14, 2023
@wipawel wipawel requested a review from minipli-oss November 14, 2023 11:19
@wipawel wipawel requested a review from a team as a code owner November 14, 2023 11:19
Copy link
Contributor

@minipli-oss minipli-oss left a comment

Choose a reason for hiding this comment

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

I guess the overall functionality makes sense but I have my problems with wrapping my head around the prototypes of [un]map_pagetables(cr3_t *, cr3_t *) .... I just cannot compute to map/unmap a "page table" in a page table.

I see that the implementation is walking the "from" page table and adding/removing existing mappings in the target page table. But, IMHO, it would be much more intuitive if the interface would simply to map/unmap a range of pages at a given virtual address. But then, we already have that -- kind of.

So, what's the use case for all of this? Merging page tables seems to be really a special case I simply cannot think of being useful in a general manor.

arch/x86/pagetables.c Outdated Show resolved Hide resolved
arch/x86/pagetables.c Outdated Show resolved Hide resolved
arch/x86/pagetables.c Outdated Show resolved Hide resolved
@wipawel
Copy link
Contributor Author

wipawel commented Nov 17, 2023

I guess the overall functionality makes sense but I have my problems with wrapping my head around the prototypes of [un]map_pagetables(cr3_t *, cr3_t *) .... I just cannot compute to map/unmap a "page table" in a page table.

I see that the implementation is walking the "from" page table and adding/removing existing mappings in the target page table. But, IMHO, it would be much more intuitive if the interface would simply to map/unmap a range of pages at a given virtual address. But then, we already have that -- kind of.

So, what's the use case for all of this? Merging page tables seems to be really a special case I simply cannot think of being useful in a general manor.

In KTF the page tables themselves are not mapped in the address space. Before the 4978793 only initial page tables were mapped and now none of them except the tmp_mfn used for temporal mappings during page tables operations. However, for various tests (PoCs) we do need to access and manipulate the page tables to construct specific scenarios. There is a bunch of helper functions like get_pdpe(), get_pde, get_pte() , which do not map the page table entry, yet they return a pointer to its entry. To make them usable user has to map the page table upfront. Before it was necessary to use map_used_memory() which was an ugly hack to map the page tables among other unnecessary things. Now there is the (un)map_pagetables_va() coming with this change. The (un)map_pagetables() are just here to map entire page table at once without specifying particular virtual address. The from_cr3 and to_cr3() just indicate which page table into what address space. It's possible for example to map user_cr3 page tables into cr3 (kernel) address space although I don't think it would be every used. The main purpose is to map "our own" page tables into "our own" address space.

@minipli-oss
Copy link
Contributor

In KTF the page tables themselves are not mapped in the address space.

Ok

Before the 4978793 only initial page tables were mapped and now none of them except the tmp_mfn used for temporal mappings during page tables operations. However, for various tests (PoCs) we do need to access and manipulate the page tables to construct specific scenarios.

Makes sense.

There is a bunch of helper functions like get_pdpe(), get_pde, get_pte() , which do not map the page table entry, yet they return a pointer to its entry. To make them usable user has to map the page table upfront. Before it was necessary to use map_used_memory() which was an ugly hack to map the page tables among other unnecessary things. Now there is the (un)map_pagetables_va() coming with this change. The (un)map_pagetables() are just here to map entire page table at once without specifying particular virtual address. The from_cr3 and to_cr3() just indicate which page table into what address space. It's possible for example to map user_cr3 page tables into cr3 (kernel) address space although I don't think it would be every used. The main purpose is to map "our own" page tables into "our own" address space.

And I guess that's where my confusion came from. I was under the assumption that you're trying to merge address spaces but simply mapping the page table pages is something completely different.

Sorry my confusion :D

Copy link
Contributor

@minipli-oss minipli-oss left a comment

Choose a reason for hiding this comment

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

Looks good, only some nits left.

arch/x86/pagetables.c Outdated Show resolved Hide resolved
include/arch/x86/page.h Outdated Show resolved Hide resolved
arch/x86/pagetables.c Outdated Show resolved Hide resolved
arch/x86/pagetables.c Outdated Show resolved Hide resolved
arch/x86/pagetables.c Outdated Show resolved Hide resolved
The map_pagetables() function maps all page tables specified in
its second parameter into the address space specified by its
first parameter. If second parameter is NULL, the current address
space page tables are mapped. Only page table frames are being
mapped.
After mapping a helper function family: get_pte(), get_pde(),
... can be used without causing exceptions.

The unmap_pagetables() function unmaps all page tables specified
in its second parameter from address space indicated by its first
parameter. If second parameter is NULL, the current address space
page tables are being unmapped.

Signed-off-by: Pawel Wieczorkiewicz <[email protected]>
The map_pagetables_va() function maps page tables pertinent to the
virtual address specified by its second parameter into the address
space indicated by its first parameter.
It is a fine-grained, lightweight version of the map_pagetables().

The unmap_pagetables_va() function unmaps page tables pertinent to
the virtual address specified by its second parameter into the
address space indicated by its first parameter.
It is a fine-grained, lightweight version of the unmap_pagetables().
The unmap_pagetables_va() performs the unmapping regardless of the
mapping status of any of the page tables levels.

Signed-off-by: Pawel Wieczorkiewicz <[email protected]>
Finally, this ugly hack goes away.

Signed-off-by: Pawel Wieczorkiewicz <[email protected]>
Copy link
Contributor

@minipli-oss minipli-oss left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Pawel!

@wipawel wipawel merged commit 43b2ae9 into KernelTestFramework:mainline Nov 21, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request Priority: 3 Regular feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add dedicated mechanism for pagetable frames allocation and mapping
2 participants