PageTableEntry::frame misleadingly returns 4KiB frames for >4KiB frames

[ChocolateLoverRaj]

Before #529 , it used to return an error (FrameError::HugeFrame). Now it just returns a PhysFrame<Size4KiB>. This is misleading because while the address mapping for those 4KiB is valid, the entry does not really point to a 4KiB frame if it's huge. It should be a 2MiB or 1GiB frame.

I ran into this when my code here stopped working after I switched from 0.15.2 to f01a291ecb0c1c8e8a5f5d58c29fbd9588a1486c.

I understand that because PhysFrame is generic and the frame size is only phantom data, and because PageTableEntry does not know what level it is, the frame method can't know what the actual frame size is. But imo this function's behavior needs to change.

[Freax13]

The frame function can't implement the behavior you want. The problem is that it doesn't know the level and so it can't know if a frame is huge or not. Instead, try manually checking the huge bit at the appropriate levels.

One way to solve this would be to add a generic parameter to the entry type that encodes the page table level, though it doesn't expect that to work well with existing code.

[ChocolateLoverRaj]

The frame function can't implement the behavior you want.

Can it at least be renamed to get_4kib_frame or something to reduce bugs when calling it? And also mark #529 as a breaking change?

[Freax13]

Can it at least be renamed to get_4kib_frame or something to reduce bugs when calling it?

I think that might still be confusing to users because they might still try to call it on a l2 or l3 entry. I'd rather add a generic parameter for the page size and force users to use the appropriate size. Alternatively, we could just remove it entirely and ask users to use .addr() instead.

All of these proposals are breaking changes.

And also mark #529 as a breaking change?

I'd be open to that given that you have demonstrated reasonable code that breaks after this change. Your code is technically incorrect, but we encouraged it with our incorrect API. @phil-opp what's your take on this?

Cc @adavis628

[phil-opp]

Thanks for bringing this to our attention @ChocolateLoverRaj!

Unfortunately, I don't see a good way to solve this either. The PageTableEntry comes from a PageTable, which is also not aware of its level. And recursive page tables don't even have a clear level because they are used at multiple levels. So I'm not sure if a generic parameter could even work.

Regarding the classification as breaking change:

Our documentation was explicitly specifying that the HugeFrame error is returned in that case:

pub fn frame(&self) -> Result<PhysFrame, FrameError>

Returns the physical frame mapped by this entry.

Returns the following errors:

• FrameError::FrameNotPresent if the entry doesn’t have the PRESENT flag set.
• FrameError::HugeFrame if the entry has the HUGE_PAGE flag set (for huge pages the addr function must be used)

So users might reasonably expect that the function behaves this way. They are probably not aware that the HUGE_PAGE bit is used as PAT bit in level 1 page tables (I missed this as well when I originally introduced this functionality in 2018).

So in my opinion this is clearly a breaking change. I think we should even try to turn it into a hard error instead of silently changing the behavior. I would also be fine with removing the frame method completely (and also the HugeFrame error). We should clearly document the reasoning for this in our Changelog though.

[phil-opp]

As an alternative, we could add an is_level_1_entry argument to the frame method. This way, we could keep the old behavior and keep returning the HUGE_PAGE error. This would also be a breaking change of course, but probably easier to resolve for users.

[ChocolateLoverRaj]

I would also be fine with removing the frame method completely (and also the HugeFrame error)

@phil-opp @Freax13 this makes the most sense to me.

[ChocolateLoverRaj]

Btw this is my new iterator for traversing page tables: https://github.com/ChocolateLoverRaj/code-runner/blob/9ffdb897e957fcac260908150190dbde455afa6d/kernel/src/traverse_cr3.rs