Basic VMM support for RISC-V.
Lots of TODOs indicated within the code, but working for me on multi-core host with multi-core guest (single guest VM instance only)
Details
Details
Spike
tested in Spike
./spike -m4096 -d --isa RV64IMAFDCH_zicntr_zihpm_sstc --kernel /usr/obj/usr/home/br/dev/freebsd/riscv.riscv64/sys/GENERIC/kernel.bin ../../opensbi/build/platform/generic/firmware/fw_jump.elf
and then
bhyve -m 2560 -o bootrom=/kernel.bin -o console=stdio -s 4,ahci-hd,/bin/ls test
QEMU
tested in Qemu 8-core:
./qemu-system-riscv64 -nographic \ -machine virt \ -cpu 'rv64,h=true' \ -smp 8 \ -m 8G \ -bios ../../opensbi/build/platform/generic/firmware/fw_jump.elf \ -kernel /usr/obj/usr/home/br/dev/freebsd/riscv.riscv64/sys/GENERIC/kernel.bin
and then
bhyve -c 8 -m 2560 -o bootrom=/kernel.bin -o console=stdio -s 4,ahci-hd,/bin/ls test
(in both host&guest kernels I have a small mdroot embedded)
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
APLIC emul: allow injection of pending bit when interrupt source disabled (It wont fire until OS enable it). This may fix AHCI attachment on boot
| sys/riscv/vmm/vmm_riscv.c | ||
|---|---|---|
| 260 | What's wrong with a real pointer type? | |
| 261 | Use uintptr_t or a pointer type so it's compatible with CHERI | |
| 263 | Ditto | |
| 279 | I cannot figure out what these instructions are for, the trap handler doesn't read htmp, nor does the code in this function. As far as I can tell, htmp exists solely to reserve a1 for use in the trap handler, but you can do that by just adding a1 to the clobbers. | |
| 282 | Why is guest_addr marked as an output? It's just an address that's read from. Why is val early-clobber? It's written to at the end after all instructions have executed. Same(ish) goes for htrap, it's written during the final instruction after guest_addr has been read. And if the add htmp above is removed but htmp remains as an operand rather than a clobber then htmp follows htrap and doesn't need an early-clobber. | |
| 286 | Shouldn't we check we didn't trap first?... | |
| sys/riscv/vmm/vmm_riscv.c | ||
|---|---|---|
| 279 |
I wanted to make them used, otherwise compiler tells they are unused and terminates (even when listing them in clobbers section). Any idea? | |
| sys/riscv/vmm/vmm_riscv.c | ||
|---|---|---|
| 279 | I don’t understand what you mean | |
Comment Actions
Try to address issues discovered by jrtc27:
- htrap is a real pointer type
- old_hstatus, old_stvec are pointer types
- remove "add htmp, ..." instruction
- no early clobbers as everything written after hlvx.hu finished consuming input operands
- htrap and guest_addr are input operands
- second hlvx.hu not issued unless first one succeeded
| sys/riscv/vmm/vmm_riscv.c | ||
|---|---|---|
| 281–283 | As in this should work, no need for the htmp variable | |
Comment Actions
handle VM_EXITCODE_SUSPENDED.
this fixes re-creation of a VM with the same name after graceful shutdown
| sys/riscv/include/vmm.h | ||
|---|---|---|
| 2–31 | +1 for the SPDX tag. Yes, it is missing in arm64, and that's a bug. | |
| 280 | Are these actually referenced somewhere? I cannot see it if so. MTRAP is definitely amd64-specific, at least. | |
| sys/riscv/vmm/vmm.c | ||
| 337 | This should be a load-acquire to synchronize with the release store below, no? If so, arm64 has the same bug. (Really, this code needs to be deduplicated.) | |
| 805 | This should be passing up the return value of vmmops_gla2gpa(). That's a bug in arm64 too. | |
| 1460 | Why are we returning success when pmap_fault() fails? It looks like the check is backwards. | |
| sys/riscv/vmm/vmm_aplic.c | ||
| 113 | Assert that i is within bounds? | |
| 215 | Why do we panic here? This looks like it is triggerable from the guest. | |
| 261 | Same here, it looks like a malicious guest can panic the host system, or I'm missing something. | |
| 359 | Stray debug print? | |
| 365 | Why is 63 hard-coded? Shouldn't this be a defined constant? | |
| 383 | Debug print. | |
| sys/riscv/vmm/vmm_dev_machdep.c | ||
| 92 | This check isn't needed. | |
| sys/riscv/vmm/vmm_riscv.c | ||
| 77 | This looks like it should be in a header. It is duplicated with riscv/db_disasm.c. | |
| 289 | Please add some comments explaining what this and the asm above is doing. Should the first condition be spelled trap->scause == SCAUSE_INST_MISALIGNED? | |
| 324 | ||
| 325 | I think this should assert that vme_ret->scause has an expected value. | |
| 471 | This needs to be handled somehow. Guests shouldn't be able to panic the host. | |
| 511 | This should return an error instead. (And if it's not implemented, why provide the VM_GLA2GPA_NOFAULT ioctl at all?) | |
| 584 | We're not assigning a VMID field here, and the riscv pmap doesn't implement ASIDs. How is the CPU able to distinguish between TLB entries for different VMs? Don't we at least need an hfence instruction here? | |
| 860 | This is effectively unimplemented. IMO this should simply return an error instead. | |
| 873 | Is this actually used? | |
| sys/riscv/vmm/vmm_sbi.c | ||
| 79 | What needs to be done? | |
| sys/riscv/include/vmm.h | ||
|---|---|---|
| 280 | Removed most of them | |
| sys/riscv/vmm/vmm.c | ||
| 337 | fixed in https://reviews.freebsd.org/D47306 | |
| 1460 | Because our pmap_fault() returns 1 on success. I have changed the if statement to avoid confusion... | |
| sys/riscv/vmm/vmm_aplic.c | ||
| 215 | Fixed. | |
| 261 | Fixed. | |
| 365 | A macro added | |
| sys/riscv/vmm/vmm_riscv.c | ||
| 77 | That is slightly different variant to the one found in riscv/db_disasm.c. | |
| 289 | I added comments and thanks for catching the bug as scause 0 is actually a valid scause, but here we wanted to check that previous hlvx.hu did not raise any exception. So I changed to -1. | |
| 325 | Assertion added. | |
| 471 | Agree, let's just return to userspace for now and try to continue execution. | |
| 511 | Ok. I guess we could leave it so that we can implement it later for gdb, same to exception injecting. | |
| 584 | I added hfence.gvma per you request (with a comment from ISA manual). | |
| 860 | Ok | |
| 873 | Yes, it allows to use more than 1 CPU. Otherwise you get this error: "8 vCPUs requested but only 1 available" | |