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 p23_insert_flag_mask argument to mapper.map_to() #114

Open
wants to merge 1 commit into
base: master
from

Conversation

@haraldh
Copy link
Contributor

haraldh commented Jan 8, 2020

It will update the parent hierarchy tables with (flags & p23_insert_flag_mask).

    mapper
        .map_to(
            page,
            frame,
            PageTableFlags::PRESENT
                | PageTableFlags::WRITABLE
                | PageTableFlags::USER_ACCESSIBLE,
            PageTableFlags::USER_ACCESSIBLE,
            frame_allocator,
        )
        .unwrap()
        .flush();

will update the hierarchy with PageTableFlags::USER_ACCESSIBLE. If the
flags would not contain USER_ACCESSIBLE, the hierarchy would not be
updated.

If you don't want any updates, pass PageTableFlags::empty() as the
mask.

@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Jan 8, 2020

This is my take on #112

@haraldh haraldh force-pushed the haraldh:p23_insert_flags branch from d6b9aed to 247d02f Jan 8, 2020
@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Jan 8, 2020

ah, doh.. should have named it p321_insert_flag_mask

@haraldh haraldh force-pushed the haraldh:p23_insert_flags branch from 247d02f to fce2df4 Jan 8, 2020
@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Jan 8, 2020

ah, doh.. should have named it p321_insert_flag_mask

done

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Jan 8, 2020

Thanks for the pull request! I like your approach more than the approach in #112 because it is more predictable. However, I'm still not sure if this is the best design. Mainly, I have the following design questions:

  • Do we really need the additional p321_insert_flag_mask parameter? We could alternatively derive it from the flags parameter, assuming that all given flags should apply to the mapping. Or is there a use case for adding e.g. USER_ACCESSIBLE to the entry of the level 1 table but not to higher level tables?
  • We currently add the PRESENT and WRITABLE flags unconditionally. AFAIK they behave exactly like USER_ACCESSIBLE, so it might make sense to incorporate them into the flag_mask or derive them from the flag set.
@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Jan 8, 2020

* Do we really need the additional `p321_insert_flag_mask` parameter? We could alternatively derive it from the `flags` parameter, assuming that all given flags should apply to the mapping. Or is there a use case for adding e.g. `USER_ACCESSIBLE` to the entry of the level 1 table but not to higher level tables?

There might be.. there are other flags, too.

* We currently add the `PRESENT` and `WRITABLE` flags unconditionally. AFAIK they behave exactly like `USER_ACCESSIBLE`, so it might make sense to incorporate them into the `flag_mask` or derive them from the `flag` set.

I don't know, we need them to build the entries, so it probably would lead to failures, if someone forgets them.

@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Jan 8, 2020

* Do we really need the additional `p321_insert_flag_mask` parameter? We could alternatively derive it from the `flags` parameter, assuming that all given flags should apply to the mapping. Or is there a use case for adding e.g. `USER_ACCESSIBLE` to the entry of the level 1 table but not to higher level tables?

There might be.. there are other flags, too.

like NO_CACHE and WRITE_THROUGH

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Jan 8, 2020

I don't know, we need them to build the entries, so it probably would lead to failures, if someone forgets them.

Good point! Note that this is only true for the recursive page table. For the mapped page table we use the separate physical mapping for modifying the page tables.

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Jan 8, 2020

There might be.. there are other flags, too.

like NO_CACHE and WRITE_THROUGH

Good examples… However, I still think we can set some flags automatically, such as USER_ACCESSIBLE or WRITABLE. One might even argue that we could even always set these flags since they only have an effect if they are set on the level 1 entry too.

I think my preferred solution is currently this:

  • Add a new map_to_with_table_flags (placeholder name) method with an additional parent_table_flags argument to the flags of the parent table entries. This is similar to your p321_insert_flag_mask argument with the difference that it isn't a mask. This way, it's also possible to set flags on parent tables without setting them on the level 1 table entry (which might make sense for flags that have a different effect on those tables such as NO_CACHE).
  • Keep map_to without API changes as a convenience method. Instead of hardcoding PRESENT and WRITABLE, it automatically infers the parent table flags from the flags of the mapping and then calls into map_to_with_table_flags.
    • To ensure that we don't automatically set flags like NO_CACHE, which have a different meaning on higher level tables, we use a hardcoded mask of "safe" flags (e.g. PRESENT | WRITABLE | USER_ACCESSIBLE).
    • Automatically infering these parent flags makes sense to me because otherwise the entry flags have no effect. For example, it does not make sense to set USER_ACCESSIBLE only for the level 1 entry.
    • To keep the recursive page table working, we need to always set the PRESENT and WRITABLE flags for parent entries (as we do today).
  • Add methods to get/set/update the flags of the level 4/3/2 tables.

This gives a high amount of control to the user but still keeps the API for common operations simple. What do you think?

@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Jan 8, 2020

Sounds good to me

@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Jan 22, 2020

I might work on this in a week.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Jan 24, 2020

Or is there a use case for adding e.g. USER_ACCESSIBLE to the entry of the level 1 table but not to higher level tables?

Ah, I just saw this thread. I think there is a use-case for mapping a page as user-accessible but not making it user-accessible yet. For example, if I was implementing a kernel-side elf-loader, I might want to set up mappings to a bunch of user pages but not allow user-mode to access the page yet. One could then flip the bit at the higher level to allow access to all of the pages.

This was referenced Jan 24, 2020
@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Jan 27, 2020

@mark-i-m

For example, if I was implementing a kernel-side elf-loader, I might want to set up mappings to a bunch of user pages but not allow user-mode to access the page yet. One could then flip the bit at the higher level to allow access to all of the pages.

Why would the user-mode program be running before it's ELF file is loaded? The table hierarchy is not used before you load it into the CR3 register, so I don't think that you need an additional "global" switch in this case.

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Jan 27, 2020

In general, I think there are two approaches that are both valid:

  1. Give the user full control of all page table bits of all levels.
  2. Pretend to the user that higher level table "off-switchs" does not exist. Automatically set such flags (PRESENT, WRITABLE, USER_ACCESSIBLE, …) for parent tables if they're set on a level 1 table.

For me, variant 2 makes most sense as a default because that's you want most of the case. For example, if you call map_to with the WRITABLE flag, you normally want to create a writable mapping. Requiring the user to also set the flag on all higher level tables could be unexpected given that map_to already handles the creation of non-existent higher level tables.

Variant 1 also has valid use cases, e.g. setting flags that behave differently on higher level tables such as the mentioned cache flags. So I think it makes sense to support this variant too, e.g. in form of a visitor API (proposed in #121).

I think we can ergonomically cover most use-cases by defaulting to variant 2, but additionally providing advanced methods that give the user full control (variant 1).

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Jan 27, 2020

Why would the user-mode program be running before it's ELF file is loaded? The table hierarchy is not used before you load it into the CR3 register, so I don't think that you need an additional "global" switch in this case.

Imagine for example, that the kernel loads another binary/library into the same address space as another thread.

Another example would be some sort of ipc mechanism for atomically bulk-transferring data.

I agree though that variant 1 makes sense to support via something like visitors, rather than the default.

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Jan 28, 2020

Thanks for the examples! I don't think that such a setup would be common, but I agree that we should still support it somehow.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Jan 29, 2020

Yep, admittedly I tend to have weird use cases because I like to play around with non-conventional OS designs...

Add a new map_to_with_table_flags method with an additional
parent_table_flags argument to the flags of the parent table entries.

map_to is kept without API changes as a convenience method.
It automatically infers the parent table flags from the flags of the mapping
and then calls into map_to_with_table_flags. To ensure that flags like
NO_CACHE are not automatically set, which have a different meaning on
higher level tables, a hardcoded mask of "safe" flags
(PRESENT | WRITABLE | USER_ACCESSIBLE) is used.

To keep the recursive page table working, PRESENT and WRITABLE flags
for parent entries are also added.

Also adds methods to update the flags of the level 4/3/2 tables.
@haraldh haraldh force-pushed the haraldh:p23_insert_flags branch from fce2df4 to 1388935 Feb 11, 2020
@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Feb 11, 2020

Updated PR with changes mentioned in #114 (comment)

Additional methods for p[432] entries can be added later in a separate PR.

@@ -462,13 +608,16 @@ impl<P: PhysToVirt> PageTableWalker<P> {
if let Some(frame) = allocator.allocate_frame() {
entry.set_frame(
frame.frame(),
PageTableFlags::PRESENT | PageTableFlags::WRITABLE,
PageTableFlags::PRESENT | PageTableFlags::WRITABLE | insert_flags,

This comment has been minimized.

Copy link
@phil-opp

phil-opp Feb 12, 2020

Member

I think we can just pass insert_flags here. Since we access the page table frame through a separate mapping, the PRESENT and WRITABLE flags should be only needed if the page is mapped with these flags (or if the user explicitly specified them in parent_table_flags).

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Feb 12, 2020

Thanks a lot for the update! I only took a quick glance, but what I saw looks very good. I try to do a full review soon.

@@ -82,13 +82,99 @@ pub trait Mapper<S: PageSize> {
///
/// This function might need additional physical frames to create new page tables. These
/// frames are allocated from the `allocator` argument. At most three frames are required.
///
/// Parent page table entries are automatically updated with `PRESENT | WRITABLE | USER_ACCESSIBLE`
/// if present in the `PageTableFlags`.

This comment has been minimized.

Copy link
@phil-opp

phil-opp Feb 19, 2020

Member

We should not here that, depending on the used mapper implementation, the PRESENT and WRITABLE flags might be set for parent tables even if they are not set for the mapping.

/// frames are allocated from the `allocator` argument. At most three frames are required.
///
/// The flags of the parent table(s) can be explicitly specified. Those flags are used for
/// newly created table entries, and for existing entries the flags are added.

This comment has been minimized.

Copy link
@phil-opp

phil-opp Feb 19, 2020

Member

As above, we should not here that, depending on the used mapper implementation, the PRESENT and WRITABLE flags might be set for parent tables even if they are not specified in parent_table_flags.

/// Create USER_ACCESSIBLE mapping and update the top hierarchy to be USER_ACCESSIBLE, too:
///
/// ```
/// # use x86_64::structures::paging::{
/// # Mapper, UnusedPhysFrame, Page, FrameAllocator,
/// # Size4KiB, OffsetPageTable, page_table::PageTableFlags
/// # };
/// # fn test(mapper: &mut OffsetPageTable, frame_allocator: &mut impl FrameAllocator<Size4KiB>,
/// # page: Page<Size4KiB>, frame: UnusedPhysFrame) {
/// mapper
/// .map_to_with_table_flags(
/// page,
/// frame,
/// PageTableFlags::PRESENT
/// | PageTableFlags::WRITABLE
/// | PageTableFlags::USER_ACCESSIBLE,
/// PageTableFlags::PRESENT
/// | PageTableFlags::WRITABLE
/// | PageTableFlags::USER_ACCESSIBLE,
/// frame_allocator,
/// )
/// .unwrap()
/// .flush();
/// # }
Comment on lines +145 to +168

This comment has been minimized.

Copy link
@phil-opp

phil-opp Feb 19, 2020

Member

Maybe make this example use different flags for the entry and the parent tables to show a potential use case for this method.

/// Updates the flags of an existing mapping.
fn update_flags_p4_entry(
&mut self,
page: Page<S>,
flags: PageTableFlags,
) -> Result<MapperFlush<S>, FlagUpdateError>;

/// Updates the flags of an existing mapping.
fn update_flags_p3_entry(
&mut self,
page: Page<S>,
flags: PageTableFlags,
) -> Result<MapperFlush<S>, FlagUpdateError>;

/// Updates the flags of an existing mapping.
fn update_flags_p2_entry(
&mut self,
page: Page<S>,
flags: PageTableFlags,
) -> Result<MapperFlush<S>, FlagUpdateError>;

Comment on lines +194 to +214

This comment has been minimized.

Copy link
@phil-opp

phil-opp Feb 19, 2020

Member

I think we should name these methods "set" instead of "update" to make it more clear that they overwrite the entry flags (in contrast to ORing the flags). Also, I think I'm in favor of naming the methods set_pX_entry_flags instead of set_flags_pX_entry, but this is only personal preference.

@@ -39,6 +41,7 @@ struct PhysOffset {
}

impl PhysToVirt for PhysOffset {
#[inline]

This comment has been minimized.

Copy link
@phil-opp

phil-opp Feb 19, 2020

Member

These #[inline] attributes are fine with me, but perhaps mention their addition in the pull request/commit description.

entry.set_frame(frame.frame(), Flags::PRESENT | Flags::WRITABLE);
entry.set_frame(
frame.frame(),
Flags::PRESENT | Flags::WRITABLE | insert_flags,

This comment has been minimized.

Copy link
@phil-opp

phil-opp Feb 19, 2020

Member

We should document that the PRESENT and WRITABLE flags are always set for higher level page table entries because the recursive page table approach requires it. Preferably we both add a code comment here and also add it to the documentation of the RecursivePageTable type so that users see it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.