rust-osdev / x86_64 Goto Github PK

Hi !

I'm considering using your crate to use a standard definition of the page tables in my project.

My goal is to do low-level Virtual Machine Introspection (VMI) on Xen, KVM, VirtualBox or Hyper-V with a Rust API (libmicrovmi)

Given that these hypervisors provide an API to read physical memory, the next step for me is to expose an API to read from a virtual address.

I can see that you already provide a standard definition of these paging structures for x86_64:
https://docs.rs/x86_64/0.7.6/x86_64/structures/paging/index.html

That's great !

But I was wondering how we could both avoid reinventing the wheel and share the same page tables parsing code to translate a virtual address to a physical one ?

Note: I found this crate by reading the excellent blog from Philipp Oppermann, Writing an OS in Rust !
An article was detailing the page table implementation using the x86_64 crate.
cc @phil-opp

Related: Wenzel/libmicrovmi#31

Thanks !

We previously had some discussion of using some sort of Visitor pattern to visit and update page tables. I got some time to play around with it a bit today. I think it is promising, but there are some unresolved API questions and some issues that I don't know how to deal with.

Unresolved Problem:

• The visitor doesn't know where in virtual memory a page table is. We need some generic and ergonomic way to figure out this information for Visitors to really work. For now, I have left it abstracted as the Visit::get_page method, which is implemented by each Visitor as needed.

Unresolved questions:

• Should we have visit_huge_page<S: Size>(...)? Is this too fine-grained?

Here is a rough prototype. It doesn't work or compile, but it should give a rough idea: https://github.com/rust-osdev/x86_64/compare/master...mark-i-m:visitor?expand=1

cc #32 #96 #112 #113 #114

There are two methods in the Mapper trait that are unsafe. What would it take to make them safe ? Could we, e.g., error or panic if the frame is actually not unused ?

I'm working on a KVM library and I find myself reimplementing most of the types from this crate. I can't use this crate, however, because of your (legitimate) need for asm!(). Would this project consider merging a patch that splits this crate into two halves: one containing raw types (such as RFlags) and the other containing implementations for things like reading and writing the registers?

It would be convenient if there is a method to translate a virtual address to the corresping PageTableFlags, this way the interrupt handler can check which flags are set on the entry and handle faults accordingly.

At the moment, the flags are not easily accessible from a pagetable, which makes it hard to debug issues where the active pagetable flags are unclear.

Currently the MappedPageTable is generic over a PhysToVirt function (Fn(PhysFrame) -> *mut PageTable). This makes it quite awkward to use in statics (see phil-opp/blog_os#639 (comment)).

The question is: Does anybody use anything other than a simple offset to implement this function? If not, we could replace the generic PhysToVirt closure with a simple physical_memory_offset integer to make it much simpler.

This issue is mostly for discussion purposes, to clarify my understanding.

RecursivePageTable::new() requires the passed page table to be active, checked through CR3. However, I'm not entirely sure I grok why that is. Nothing else in RecursivePageTable references CR3 in any way, other than that one particular check.

The reason I'm looking to remove this, is because I'd like to be able to have my kernel map in new page tables, for processes/drivers/etc, by mapping in the new page table at some address within the kernel space, and calling that "recursive" - the algorithms would all be the same, it would simply be a different P4 being referenced. In this way, I don't need to have every page table mapped into the kernel constantly for no reason, I don't need to manually manage pages when there's a perfectly good implementation already written, and generally it would simplify the setup.

However, I don't consider myself familiar enough with the implementation to know why the CR3 check is in place, and whether or not it is safe to remove. Could somebody with this knowledge advise please?

I think x86_64::structures::idt::Entry::missing should be a public function, and should implement PartialEq, so we can check if an IDT entry is filled (unless I'm being stupid and missing something totally obvious). I'll file a PR

The clobber is "edi" but used "esi".
Here and two more lines.

Using the frame allocator to map pages with USER_ACCESSIBLE will create new p3 and p2 pages without USER_ACCESSIBLE.

I had to change

to

                entry.set_frame(
                    frame.frame(),
                    PageTableFlags::PRESENT
                        | PageTableFlags::WRITABLE
                        | PageTableFlags::USER_ACCESSIBLE,
                );

Otherwise a ring3 cannot access the page.

Not sure, if the code traversing the p2 and p3 should also check for USER_ACCESSIBLE and correct that.

The current implementations of offset and recursive page table require that new to be called with an already existing page table. Provide convenience methods to create more page tables from an existing page table or by taking limits as a parameter to the new function.

Yesterday I released version 0.2.0-beta of x86_64. There have been a lot of changes since version 0.1.2, some of them breaking.

The new version is a bit stripped down in some areas to keep it maintainable. Please let me know if you are missing any functionality. There are also a few new modules, the most significant beeing the new abstractions for recursive paging.

The crate is fully documented now and we have a CI job that checks that no undocumented items exist and also that no warnings occur.

cc @lachlansneff @4e554c4c @Restioson @gegy1000 @robert-w-gries

The MapperAllSizes trait currently provides only methods for translating addresses. While it is currently a supertrait of Mapper<{Size4KiB, Size2MiB, Size1GiB}, this isn't really required for any of its methods. It might thus be a good idea to remove the supertrait relationship and rename the trait to e.g. Translate.

Currently, this function takes port object through a shared reference.
To my mind, it is not good, because port internal state does mutate after read() (e.g. read is not idempotent, which is unusual for &-functions).
Since Port is Sync, user can by mistake read from port on multiple threads and get strange behaviour.
Finally, this is inconsistent with other similar APIs, like cpuio::Port::read, and std::io::Read::read.

Currently, unknown flags in bitfields (e.g. CR4) get automatically cleared by the library. This means that if part of the kernel uses a flag this library is not yet aware of, changing flags with this library will corrupt the environment.

This doesn't matter if all parts of the kernel use (the same version of) this crate to access the registers, but that doesn't always need to be the case. I think it would be safer to make sure unknown flags are kept unchanged (by adding a mask with all bits set? I'm not sure about that part).

The change to rename the asm! macro to llvm_asm! was just approved in rust-lang/rust#68404, so I expect that it will be in tomorrow's nightly. As far as I understand it, the old asm! will continue to work for now, but deprecated soon.

To avoid breakage, I think it makes sense to make it a minor release to avoid breakage for people that pinned an older nightly. The deprecation period should then be enough time for users to switch to the new version

I measured the compile time of the blog_os project with the new -Z timings flag:

The x86_64 crate and especially its ux and array-init dependencies take very long to compile. Maybe it's possible to remove or replace these dependencies somehow.

should probably be

pub enum MapToError<S: PageSize> {
    /// An additional frame was needed for the mapping process, but the frame allocator
    /// returned `None`.
    FrameAllocationFailed,
    /// An upper level page table entry has the `HUGE_PAGE` flag set, which means that the
    /// given page is part of an already mapped huge page.
    ParentEntryHugePage,
    /// The given page is already mapped to a physical frame.
    PageAlreadyMapped(UnusedPhysFrame<S>),
}

and methods returning PageAlreadyMapped should give back the frame.

The idt mod exports a structure named Idt which is an abbreviation for Interrupt Descriptor Table. All the other modules from structures export structs with full names, i.e., GlobalDescriptorTable and TaskStateSegment. Should we use full name for the Idt structure or refactor others to abbreviation form?

Showing up on attempt to run Travis-CI builds on Pull Request #154 ― in particular:

• Commits dec513c and 90d7219 contain identical code
• Despite containing identical code, the former failed on Linux and macOS but not Windows while compiling not my contribution but the dependency of this crate mentioned in the title, while the latter failed on Windows but not Linux and macOS while compiling not my contribution but the dependency of this crate mentioned in the title

Also, not sure why formatting check is failing at random intervals either ― it's consistently passing on my Arch Linux end.

To be certain that when update will not break anything, and if will, it's documented somehow

Why is the privilege level hard coded?

Hello

I am working with this crate to complete my honours project, but I encountered problems a couple of things.
I looked on the documentation on www.docs.rs, but I could not find any examples, and even worse I am new to Rust, so I'm a bit lost.
How can I add to the IDT an exception handler of the type HandlerFuncWithErrCode ?
Apparently the IDT structure only expects a HandlerFunc handler.
And how would you trigger a custom exception in the code (a kind of throw instruction) ?
Or maybe are you waiting for the stable release 1.0.0 to write the examples ?

Hello there,
It would be great if there was an rdrand function in the instructions submodule for seeding RNGs. It's relatively simple to implement and I believe many people would find it useful.

What are your thoughts on this?

The current implementation ORs the existing value with the flags bits:

I think the intention was to preserve unknown flags (flags that are missing in Cr0Flags). However, the current implementation preserves all flags.

• Review the rest of the code, the same issue might occur in more places (e.g. due to copy&paste).

The UnusedPhysFrame type was added in #89 in order to make the map_to method safe. It has an unsafe constructor function that requires a "physical frame that is not used for any mapping".

As reported in phil-opp/blog_os#570 (comment), there are however valid use cases for using the same frame in multiple mappings. I therefore think that we should reduce the (documented) requirements or introduce a new wrapper type.

One potential solution could be the following:

• Add a new UnaliasedPhysFrame wrapper type that represents a PhysFrame where no undefined (mutable) aliasing can occur. Examples are copy-on-write mappings or duplicate mappings where it is guaranteed that one mapping isn't used. It's up to the programmer to ensure this invariant, so it's unsafe to construct the type.
• Add a impl From<UnusedPhysFrame> for UnaliasedPhysFrame implementation since every unused physical frame is also guaranteed to be unaliased.
• Change map_to to expect an Into<UnaliasedPhysFrame> so that it works with both frame wrapper types.

Alternatively, we could rename the UnusedPhysFrame type to UnaliasedPhysFrame without introducing a second type. The main argument for this is that no frame is really unused in kernels that use a mapping of the complete physical memory for accessing page tables. To avoid breakage, we could add a deprecated reexport of the old type name for some time.

Our crate is currently broken on nightly because we need to update our assembly syntax or keep using legacy assembly. I will get around to this if nobody else does soon. This will break compatibility with nightlies older than a few weeks at maximum, so this will probably be a breaking change (@phil-opp please confirm this).

The map_to method of the Mapper trait is not unsafe, but it is possible to break memory safety by passing a FrameAllocator that returns frames that are already in use.

To avoid this, we should either make the map_to method unsafe or make FrameAllocator an unsafe trait.

This issue proposes to make the FrameAllocator trait unsafe to implement. This would allow functions to trust FrameAllocator parameters.

Currently it is safe to implement the FrameAllocator trait for a type. So it's entirely possible to safely construct a type that yields the same frame over and over again, even though it is not unused anymore. For this reason, all functions that rely on FrameAllocator parameter should be unsafe because the caller has to guarantee that the passed frame allocator is implemented correctly. If we make the FrameAllocator trait an unsafe trait, we can rule out invalid frame allocators.

This is similar to the Alloc trait of the standard library. Instead of making it safe to implement the trait and mistrusting all Alloc instances, the trait is unsafe so that it can be used in types/functions without making all (constructor) functions unsafe.

This would be a breaking change, but I think that it is worth it. What do you think?

Example

With the current FrameAllocator trait, we have to write code like this:

pub trait FrameAllocator<S: PageSize> {
    fn allocate_frame(&mut self) -> Option<PhysFrame<S>>;
}

unsafe fn create_mapping(
    mapper: &mut impl Mapper,
    frame_allocator: &mut impl FrameAllocator
) {
   let frame = frame_allocator.allocate_frame();
   mapper.map_to(some_page, frame, some_flags, frame_allocator);
}

We can't trust the frame_allocator, because it is possible to create invalid frame allocators in safe code. Therefore, this must be unsafe because the caller has to guarantee that the frame allocator is valid.

If we make the FrameAllocator trait unsafe to implement, the create_mapping function can be safe:

pub unsafe trait FrameAllocator<S: PageSize> {
    fn allocate_frame(&mut self) -> Option<PhysFrame<S>>;
}

fn create_mapping(
    mapper: &mut impl Mapper,
    frame_allocator: &mut impl FrameAllocator
) {
    […]
}

Now we know the frame_allocator can be only invalid if somebody messed up inside an unsafe block (when implementing the trait).

Hello,

I've been trying to setup system calls in my kernel, and I noticed a weird bug.

//...
idt.interrupts[0x80].set_handler_fn(syscall::syscall_handler);

When trying to trigger a test system call, the system experienced a double fault. After a bit of debugging, I realised that the first exception was a Segment Not Present exception. The instruction pointer pointed to the exact instruction int 0x80. This led me to believe that somehow the present bit was being unset. I tested this theory by cloning the blog_os repo, and registering a dummy handler on the same vector 0x80. Attempting to trigger a test interrupt caused the same Segment Not Present exception as I had experienced in my own kernel. I wonder if you can shed some light on this. I have a suspicion that this might be related to #7, but I'm not certain.

Thanks.

This doesn't look right to me, but I could be misunderstanding something.

In x86_64/src/instructions/port.rs

impl PortReadWrite for u16 {
    #[inline]
    unsafe fn read_from_port(port: u16) -> u16 {
        let value: u16;
        asm!("inw %dx, %ax" : "={ax}"(value) : "{dx}"(port) :: "volatile");
        value
    }

    #[inline]
    unsafe fn write_to_port(port: u16, value: u16) {
        asm!("outw %ax, %dx" :: "{dx}"(port), "{al}"(value) :: "volatile");
    }
}

impl PortReadWrite for u32 {
    #[inline]
    unsafe fn read_from_port(port: u16) -> u32 {
        let value: u32;
        asm!("inl %dx, %eax" : "={ax}"(value) : "{dx}"(port) :: "volatile");
        value
    }

    #[inline]
    unsafe fn write_to_port(port: u16, value: u32) {
        asm!("outl %eax, %dx" :: "{dx}"(port), "{al}"(value) :: "volatile");
    }
}

Shouldn't write for u16 be:
asm!("outw %ax, %dx" :: "{dx}"(port), "{ax}"(value) :: "volatile");,
and for u32, read:
asm!("inl %dx, %eax" : "={eax}"(value) : "{dx}"(port) :: "volatile");
and write:
asm!("outl %eax, %dx" :: "{dx}"(port), "{eax}"(value) :: "volatile"); ?

Apparently it can also occur on INT_MIN / -1: phil-opp/blog_os#449 (comment)

Edit: There are even more situations where a divide error can occur:

There are other cases which can error. x86 can divide a 64bit number by a 32bit number even in 32bit code, using the encodings which take an implicit %eax:%edx input. You also get #DE if either the quotient or remainder is too large for the 32bit destination registers. All divide errors are technically "result out of range", including a divide by 0, as it is tricky to fit infinity in %eax.

(From phil-opp/blog_os#449 (comment))

The Mapper::map_to method used to be unsafe because it is possible to break memory safety by pointing to unrelated pages to the same physical frame. In #89, we made the function safe by introducing a new UnusedPhysFrame wrapper type that is unsafe to construct.

In retrospect, I think this was a mistake because it is now possible to break memory safety using only the safe methods the Mapper trait. For example:

• Call unmap for some page and then map the page to a different UnusedPhysFrame. Now we invalidated all values that are stored in this page.
• Call map with the GLOBAL and MUTABLE flags. This crates a mapping that is not flushed from the TLB on address space switches, so that other processes can manipulate the data of the page.
  • (Technically, such a mapping alone cannot lead to undefined behavior since unsafe code is required in order to use the new mapping (e.g. for converting the page address to a reference type). However, I still think that we should treat this operation as unsafe given that it is a clear footgun.)

Given the complexity of page table mappings, I think the best solution to this problem is to treat the map_to method as a fundamentally unsafe operation. This means that we should not try to make it "less unsafe" by introducing wrapper types like UnusedPhysFrame because there will probably always be a way to still do something unsafe.

So I propose to revert #89, thereby making map_to unsafe again. Instead of completely removing the UnusedPhysFrame type, we could deprecate it to reduce breakage and make its new function return a PhysFrame, so that it can still be used with map_to. This way, we would also resolve #133 and #123.

Currently, this crate requests a version compatible with 1.0.1, but uses the bitflags macro in 2018 style which wasn't supported until bitflags 1.0.4. While running cargo update or starting a new project would bypass the issue, older projects updating their dependencies will see build errors from this crate being unable to find __bitflags. Specifically, I got 41 errors between cannot find macro `__bitflags!` in this scope and various undefined structures defined within bitflags.

Essentially, x86_64 already depends on 1.0.4, but claims to be able to work with 1.0.1 when it really can't.

Hi guys, first of all thanks for the great work,

I'm maintaining a 32bit (aka i386, aka IA-32, aka protected mode) fork of this library my own purposes. It just hit me that everything I've rewritten could be included inside of this crate with conditional compilation ala #[cfg(target_arch="x86-64"]. The main differences obviously concern address sizes, but also page table topology isn't the same in 32bit, and some quirks here and there, nothing huge really.

Your crate is named x86-64 so it's not obvious that these changes would be welcome... should i start writing a draft and make a pull request or should i maintain my own crate on the side?

ps: my version was forked at v0.2.0-alpha-009 so there's not much to catch up with

There are lots of trivial and very small functions in this crate that could (should?) be marked #[inline]. E.g. VirtAddr::p1_index, Add(Assign)/Sub(Assign) implementations, PrivilegeLevel::from_u16, etc. etc.

Should all of them be marked #[inline]? Or should users of this crate always depend on LTO?

I was trying to use the new Paging Implementation post and got the following error from this crate:

error[E0658]: use of unstable library feature 'try_from' (see issue #33417)
 --> /Users/subsid/.cargo/registry/src/github.com-1ecc6299db9ec823/x86_64-0.5.2/src/addr.rs:1:27
  |
1 | use core::convert::{Into, TryInto};
  |                           ^^^^^^^
  |
  = help: add #![feature(try_from)] to the crate attributes to enable

Adding #![feature(try_from)] to this crate's lib.rs solved the issue.

Thanks!

For some exceptions the handler function must not return. For example, the double fault exception does not guarantee an accurate interrupt stack frame, so that undefined behavior can occur on a return. We should encode this in the type system by requiring a diverging handler function (i.e. returning !) for such exceptions.

Inspired by phil-opp/blog_os#449 (comment)

I see that there used to be wrappers around these. Is there a reason they were removed?

Logically, the halt instruction can never return. Thus, it should be marked as such by declaring it as diverging (returning !). Since the compiler doesn't know that the asm block itself is diverging, one could add a loop {} at the end of the function to trick it.

This issue bugged me over the weekend. I tried to convert an os_bootinfo::FrameRange struct to PhysFrameRange using the From trait and saw a compilation error. I tried cargo clean and made sure all crates were up-to-date, and I still couldn't get it working.

I have a sinking feeling that this is a simple mistake on my end, but I wasn't able to search for a similar issue.

Code

let mut range: PhysFrameRange = PhysFrameRange::from(frame_range);

Error

The error message doesn't seem particuarly helpful.

error[E0277]: the trait bound `x86_64::structures::paging::PhysFrameRange<_>: core::convert::From<os_bootinfo::FrameRange>` is not satisfied
  --> src/arch/x86/memory/area_frame_allocator.rs:36:41
   |
36 |         let mut range: PhysFrameRange = PhysFrameRange::from(frame_range);
   |                                         ^^^^^^^^^^^^^^^^^^^^ the trait `core::convert::From<os_bootinfo::FrameRange>` is not implemented for `x86_64::structures::paging::PhysFrameRange<_>`
   |
   = help: the following implementations were found:
             <x86_64::structures::paging::PhysFrameRange as core::convert::From<os_bootinfo::memory_map::FrameRange>>
   = note: required by `core::convert::From::from`

Other attempts

I tried passing a size type with the PhysFrameRange creation, and I still couldn't get it working.

let mut range: PhysFrameRange = PhysFrameRange::<Size4KB>::from(frame_range);

error[E0277]: the trait bound `x86_64::structures::paging::PhysFrameRange: core::convert::From<os_bootinfo::FrameRange>` is not satisfied
  --> src/arch/x86/memory/area_frame_allocator.rs:36:41
   |
36 |         let mut range: PhysFrameRange = PhysFrameRange::<Size4KB>::from(frame_range);
   |                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `core::convert::From<os_bootinfo::FrameRange>` is not implemented for `x86_64::structures::paging::PhysFrameRange`
   |
   = help: the following implementations were found:
             <x86_64::structures::paging::PhysFrameRange as core::convert::From<os_bootinfo::memory_map::FrameRange>>
   = note: required by `core::convert::From::from`

Reported in phil-opp/blog_os#639.

The Mapper trait is not object safe because the map_to and identity_map methods are generic (prevents object safety since they can't be monomorphized).

To fix this, we could add Self: Sized bounds to the two methods, but I think this would make the methods not callable on a &dyn Mapper object. So maybe we should additionally add non-generic map_to_dyn/identity_map_dyn methods that take a &dyn mut FrameAllocator.

There seems to be no Descriptor::kernel_data_segment(). If I understand correctly, the presence of such a segment is expected in IA32_STAR when doing a syscall, as it is used to load the SS register. Or did I misunderstand the x86_64/amd64 manual?

If needed, I'd be happy to propose a PR.

This crate fails to build on the currently nightly (as of April 4, 2018) because TryFrom implementations for usize that are sometimes infallible have been removed.

See: rust-lang/rust#49305

I've been following you blog for a while now trying to understand more about Rust and OS development and it's been a huge help so far. I did all posts you have so far, so I started working on VGA and trying to figure out how it all works. One thing I've noticed is that a lot of the VGA registers have weird rules associated with them to remain backwards compatible. This leads to ports that are readonly, write only, read from one port and write to another port. With the current implementation of the ports, I found it weird have have a port that's read-only have a write command since it implements the PortReadWrite trait together.

Have you ever thought of breaking that trait out and allowing the current Port to read and write so it doesn't break functionality, but including a read-only and write only version of ports. I took a look at the current code and since I'm fairly new at Rust, there's probably a better way to do what I did, but if you have a chance, could you check out the changes I made and let me know what you think? I mainly like the fact that it removes the chance of accidentally writing to a port that's supposed to be read-only by specifying which type of port you'd like to use, with the Port struct remaining the same, as to not break anything with previous version.

Here's a simple gist I made showing how I adapted your code to get it working in your project. If you're interested let me know, I'd love to talk more about it.

https://gist.github.com/RKennedy9064/3446beb0057b4f5f271b3606f5c6ddc6

Thanks,

Ryan Kennedy

The try_new() method of VirtAddr and PhysAddr may return respectively VirtAddrNotValid and PhysAddrNotValid. Unfortunately, those error structs are defined in the private module addr and are not reexported, so their content cannot be accessed and they can't be stored in other errors.

It would be great to export both of them.

A fix for #53 requires a semver-incompatible release. Do we want to perform any other non backwards compatible changes?

I think we forgot to do this in #89.

The struct InterruptStackFrame implemements a function as_mut() as unsafe (https://github.com/rust-osdev/x86_64/blob/master/src/structures/idt.rs#L763), claiming:

/// This function is unsafe since modifying the content of the interrupt stack frame
/// can easily lead to undefined behavior.

However the documentation at https://doc.rust-lang.org/std/keyword.unsafe.html specifies:

Code or interfaces whose memory safety cannot be verified by the type system.

Since the type system can verify the function's memory safety, I suggest to remove the unsafe keyword.