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

virt_to_phys: breaks if guest_address is not equal to 0 #713

Open
n0toose opened this issue Jun 19, 2024 · 6 comments · May be fixed by #725
Open

virt_to_phys: breaks if guest_address is not equal to 0 #713

n0toose opened this issue Jun 19, 2024 · 6 comments · May be fixed by #725

Comments

@n0toose
Copy link
Member

n0toose commented Jun 19, 2024

This is the result of an excruciatingly long debugging session for #711.

Say that we want to replace arch::RAM_START in the line 98 of vm.rs:

let mem = MmapMemory::new(0, memory_size, arch::RAM_START, params.thp, params.ksm);

... to let mem = MmapMemory::new(0, memory_size, GuestPhysAddr::new(0x20000), params.thp, params.ksm);.

So, guest_address will be equal to 0x20000. There are many parts of the code which effectively already consider this case, by subtracting self.mem.guest_address. This is very often the case in vm.rs.

Debugging this was much more difficult because of the fact that the process "froze". With cargo test gdb (I could've used gdb) and LLDB running on the side. A previous indicator that something in phys_to_virt is broken was running the following test with a similar change:

fn test_virt_to_phys() {

This would always return a WrongMemoryError.

virt_to_phys invokes a function called mem.host_address(pagetable_l0):

unsafe { (mem.host_address(pagetable_l0).unwrap() as *mut PageTable).as_mut() }.unwrap();

That function has two conditions (function slightly modified for clarity and because it was easier for me to debug that way):

	pub fn host_address(&self, addr: GuestPhysAddr) -> Result<*const u8, MemoryError> {
		let addr_lt_guest: bool = addr < self.guest_address;
		let addr_ht_guest_plus_memsize: bool = addr.as_u64() as usize > self.guest_address.as_u64() as usize + self.memory_size;
		let expression: bool = addr_lt_guest || addr_ht_guest_plus_memsize;
		if expression
		{
			return Err(MemoryError::WrongMemoryError);
		}
		Ok(

Let's take a look at read_addrs in uhyve/src/linux/gdb.rs:

fn read_addrs(&mut self, start_addr: u64, data: &mut [u8]) -> TargetResult<usize, Self> {

This uses a constant: pub const BOOT_PML4: GuestPhysAddr = GuestPhysAddr::new(0x10000);

pub const BOOT_PML4: GuestPhysAddr = GuestPhysAddr::new(0x10000);

which is passed onto:

virt_to_phys(guest_addr, &self.vm.mem, BOOT_PML4).map_err(|_err| ())?,

... as the pagetable_l0 variable, which is in turn passed as the addr variable. If we choose a guest_address equal to 0x20000, the first condition will fail.


This error basically pops up everywhere, one way or another, when the guest_address is modified. This was the best and most reproducible case that I could find after spending hours of trying to crack it for #711.

@n0toose
Copy link
Member Author

n0toose commented Jun 19, 2024

The conclusion is that the physical address of the PML4 has to be changed accordingly, and that the control register that stores 0x10000 will have to be informed of that said change as well.

By adding the offset 0x20000 to all of these constants, the initialize_pagetables function executes correctly and actually does initialize all of these tables. However, the process still ends up exiting ungracefully. The vm runs, but Hermit refuses to boot.

An open question is whether this can be fixed in Uhyve. (Edit: Identical behavior to virtio-net, despite the memory implementations being slightly different.)


https://github.com/hermit-os/kernel/blob/2143b3bc85fb274eedfac5a0e0794b1151f9f68c/src/arch/x86_64/kernel/boot.s#L1-L4

https://github.com/hermit-os/kernel/blob/2143b3bc85fb274eedfac5a0e0794b1151f9f68c/src/arch/x86_64/kernel/boot.s#L25-L30

https://github.com/hermit-os/kernel/blob/2143b3bc85fb274eedfac5a0e0794b1151f9f68c/src/arch/x86_64/kernel/boot.s#L122

uhyve/src/vm.rs

Line 223 in efc3563

*raw_boot_info_ptr = RawBootInfo::from(boot_info);

@jounathaen
Copy link
Member

Related: #719

@jounathaen
Copy link
Member

To summarize the issue: The page tables must reside within the physical memory space.

Hermit uses recursive pagetables, so it is agnostic to the location of PML4, as long as the location is written to the last entry in PML4:

pml4[0].set_addr(
BOOT_PDPTE,
PageTableFlags::PRESENT | PageTableFlags::WRITABLE,
);
pml4[511].set_addr(
BOOT_PML4,
PageTableFlags::PRESENT | PageTableFlags::WRITABLE,
);
pdpte[0].set_addr(BOOT_PDE, PageTableFlags::PRESENT | PageTableFlags::WRITABLE);

So, I think this is quite solvable by modifying initialize_pagetables:

  • make BOOT_PML4 a static variable (OnceCell or similar).
  • remove BOOT_PGT/BOOT_PDPTE/BOOT_PDE constants and calculate these values via offset to BOOT_PML4 instead.
  • Set the CR3 register to the appropriate value of BOOT_PML4:
    sregs.cr3 = BOOT_PML4.as_u64();

@n0toose
Copy link
Member Author

n0toose commented Jun 25, 2024

Set the CR3 register to the appropriate value of BOOT_PML4:

sregs.cr3 = BOOT_PML4.as_u64();

Completely missed that part, I will take a look. Thank you!

@n0toose
Copy link
Member Author

n0toose commented Jun 25, 2024

I tried an entirely different method other than using OnceCell (I need help with that, as I'm not sure how to structure this) so as to be able to get a "working proof of concept" of a codebase that is aware of the offsets and different addresses by dynamically calculating and storing all of these values inside of a struct instead of repeatedly calculating them (that the VirtualCPU and UhyveVm are aware of), as well as moving some of the functions relying on these values in the impl. I also had some difficulties with OnceCell.

It still doesn't 100% work and I am not 100% pleased with it - I will have to go over this one with you. The read and write hypercall handlers now also rely on passing an object called pagetable, and it is not architecture-agnostic. However, I also had the implicit goal of enumerating all of the places in the codebase that depend on hardcoded addresses and getting a proof of concept ready before refactoring again. I will share the tree here in a bit once I look after some details.

image

@n0toose
Copy link
Member Author

n0toose commented Jun 25, 2024

First attempt (lots of dirty hacks, some hardcoded things, non-conventional variable names etc. etc., get_min_physmem_size is broken, and I'm aware that I will probably have to rewrite large parts of it - this is an experiment, as I'm unfamiliar with OnceCell): n0toose@03842a0

It seems like I've gotten a little bit further as the tests pass, but I am getting an error here - so something is not writing in something else properly:

https://github.com/n0toose/uhyve/blob/03842a0b202892663ef5529441a5defce3d949bb/src/arch/x86_64/mod.rs#L145

As I am not completely sure whether essentially ignoring the OnceCell tip / instruction was a good idea, I'll call it a day and continue working on it later.

(Problem: entry = page_table[index].clone(); returns 0 instead of the right value, i.e. 0x30003. Primary suspect is initialize_pagetables - I am probably using something with an offset of + guest_address when I am not supposed to)

EDIT: That was the case! Solved in 96e7939, but now I'm dealing with another error. Tree: https://github.com/n0toose/uhyve/tree/make-it-boot

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

Successfully merging a pull request may close this issue.

2 participants