search

rust-osdev / x86_64

Library to program x86_64 hardware.
https://docs.rs/x86_64
Apache License 2.0
786 stars 130 forks source link

Returning FrameAlreadyMapped error when it shouldn't #461

Closed Sxmourai closed 6 months ago

Sxmourai commented 7 months ago

The MapToError::FrameAlreadyMapped is returned when we try to map a frame that is already mapped. From the offset_page_table -> Size4Kib code:

if !p1[page.p1_index()].is_unused() {
      return Err(MapToError::PageAlreadyMapped(frame));
}

We get this error when we try to change the page flags, which isn't right, we should only return this error if the frame address's are different. Something like:

if !p1[page.p1_index()].is_unused() && p1[page.p1_index()].addr() != frame.start_address() {
     return Err(MapToError::PageAlreadyMapped(frame));
}

Also why do we return the user inputted frame and not the previously mapped frame ?

Freax13 commented 7 months ago

We get this error when we try to change the page flags, which isn't right, we should only return this error if the frame address's are different.

Is there a reason why you aren't using Mapper::update_flags if you only want to change the flags and not the frame?

Also why do we return the user inputted frame and not the previously mapped frame ?

See #117 and #118.

Sxmourai commented 7 months ago

Sorry, I didn't see this function ! But maybe we should use the update_flags implicitly ?

Freax13 commented 7 months ago

I think there are some users of the mapping function that expect it to return an error if the entry is already present even if that entry contains the same frame. Changing this behavior would be a breaking change and a subtle one.

Sxmourai commented 7 months ago

Yes I'm okay, but also why do we have Mapper::update_flags but not Mapper::get_flags, I think it would be a cool addition

Freax13 commented 7 months ago

Yes I'm okay, but also why do we have Mapper::update_flags but not Mapper::get_flags, I think it would be a cool addition

That sounds reasonable to me. Feel free to open a PR :)

phil-opp commented 6 months ago

Yes I'm okay, but also why do we have Mapper::update_flags but not Mapper::get_flags, I think it would be a cool addition

The Mapper trait is about updating the page tables, i.e. creating or changing some mappings. We have a separate trait called Translate for inspecting the page tables without modification. For example, you can use the Translate::translate to get all the information about a virtual page, including its flags.

Sxmourai commented 6 months ago

Okay thanks ! Well I guess my issue is useless... So I can close it... Although I think it's not really straight forward...

phil-opp commented 6 months ago

Although I think it's not really straight forward...

Do you think it's mainly a documentation issue or do you see a way how we could make the API itself simpler?

Sxmourai commented 6 months ago

The API should change, because I think the update_flags and get_flags should be on the same object But we can also just add in comments for the update_flags function: /// See Translate::get_flags to get the flags on a specific page/frame

phil-opp commented 6 months ago

You can check the docs for a specific page table type to see all supported methods, e.g. https://docs.rs/x86_64/latest/x86_64/structures/paging/mapper/struct.OffsetPageTable.html. Keeping the traits separate allows creating custom read-only or write-only page table types.

But we can also just add in comments for the update_flags function: /// See Translate::get_flags to get the flags on a specific page/frame

I agree that such a comment would be useful. Could you submit a PR?