Paging/Mapper: include active page table flags in translate() result

[tscs37]

This is code for the issue #149 to retrieve the active pagetable flags during translation. It does not provide the active flags if the table is not mapped but from what I've tested, works perfectly well in providing the flags for mapped pages.

It does break the API of the library as it adds a new field to the enum.

[josephlr]

This mostly looks good, it is annoying that there's not a good way to talk about a PhysFrame that you don't know the size of. Maybe we need a fourth "size": SizeDynamic. Then there would be way to convert a PhysFrame<SizeDynamic> to one of PhysFrame<Size4KiB>, PhysFrame<Size2MiB>, or PhysFrame<Size1GiB>. This would also be useful for Page.

We would then have:

pub enum TranslateResult {
    PageMapped {
        frame: PhysFrame<SizeDynamic>,
        offset: u64,
        flags: PageTableFlags,
    },
    PageNotMapped,
    InvalidFrameAddress(PhysAddr),
}

However, that's not exactly relevant to this PR. This change, while backwards-incompatible, is fairly minor compared to what I'm talking about above.

[josephlr]

Above we use the order (frame, offset, flags), the declaration here should be consistent. Either we should always use (frame, offset, flags) or (frame, flags, offset).

[tscs37]

I've reordered the fields to adhere to (frame, offset, flags), which is think is the neater way to add this field.

[josephlr]

Here and elsewhere, if we're making this struct larger, it might make more sense to initialize it like:

TranslateResult::Frame1GiB {
    frame: PhysFrame::containing_address(p3[addr.p3_index()].addr()),
    offset: addr.as_u64() & 0o_777_777_7777,
    flags:  p3[addr.p3_index()].flags(),
}

[tscs37]

I've changed the struct init accordingly, though I've left variables in two places, as it's defined in a match and I feel like that isn't as neat to look at.

[josephlr]

Looks good, this is a breaking change, so I'm not sure when @phil-opp will want to merge it.

[phil-opp]

Thanks a lot for the PR and sorry for the late review!

[phil-opp]

We should clarify here that these are only the flags of the last-level page table entry. (Flags of higher-level page table entries can affect the effective flags for an address too, for example when the WRITABLE flag is not set for a level 3 entry.)

[phil-opp]

Same as above.

[phil-opp]

Same as above.

[phil-opp]

@josephlr Thanks for reviewing!

This mostly looks good, it is annoying that there's not a good way to talk about a PhysFrame that you don't know the size of. Maybe we need a fourth "size": SizeDynamic. Then there would be way to convert a PhysFrame<SizeDynamic> to one of PhysFrame<Size4KiB>, PhysFrame<Size2MiB>, or PhysFrame<Size1GiB>. This would also be useful for Page.

Interesting idea! I agree that the current enum layout is a bit cumbersome.

How about the following:

pub enum TranslateResult {
    PageMapped {
        frame: PhysFrameDynamicSize,
        offset: u64,
        flags: PageTableFlags,
    },
    PageNotMapped,
    InvalidFrameAddress(PhysAddr),
}

pub enum PhysFrameDynamicSize {
    Size4KiB(PhysFrame<Size4KiB>),
    Size2MiB(PhysFrame<Size2MiB>),
    Size1GiB(PhysFrame<Size1GiB>),
}

This way, we can keep the TranslateResult type clean without extending the PhysFrame type iself. What do you two think about this (cc @tscs37)?

Given that this is a breaking change, I'm also thinking about bundling this change with a rename of the MapperAllSizes trait (e.g. to Translate) and a removal of the supertrait-relationship to Mapper<_>. I opened #152 to discuss this change.

[phil-opp]

I just found this PR again after merging #207, which implements the same changes. I'm sorry @tscs37, I didn't remember that we had an earlier PR for it. Thanks a lot for opening the pull request though and sorry again for my mess-up.

Since I still prefer the layout described in #150 (comment), I opened #211 as a follow-up PR to implement it.