Details

Reviewers

mhorne
markj
jrtc27

Group Reviewers

bhyve
riscv

Commits

rGd3916eace506: riscv/vmm: Initial import.

Summary

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)

Page: https://wiki.freebsd.org/riscv/bhyve

Test Plan

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

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Regenerate

Address comments

share/mk/bsd.cpu.mk
354 ↗	(On Diff #139929)	Right we don't need this for userspace. Thanks.
sys/riscv/vmm/vmm_ktr.h
70	no such directory yet
sys/riscv/vmm/vmm_riscv.c
406	Fixed!

SMP support added

br edited the summary of this revision. (Show Details)Jul 15 2024, 9:34 PM

br edited the test plan for this revision. (Show Details)

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

Fix zero register getting in vmmops_getreg()

sync with CURRENT: vmm_dev and vmm_stat moved to dev/vmm/

Handle exceptions during unprivileged read using hlvx.hu

jrtc27 added inline comments.Aug 29 2024, 4:35 PM

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?...

br added inline comments.Aug 30 2024, 10:26 AM

sys/riscv/vmm/vmm_riscv.c
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. I wanted to make them used, otherwise compiler tells they are unused and terminates (even when listing them in clobbers section). Any idea?

jrtc27 added inline comments.Aug 30 2024, 5:29 PM

sys/riscv/vmm/vmm_riscv.c
279	I don’t understand what you mean

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

jrtc27 added inline comments.Aug 30 2024, 7:50 PM

sys/riscv/vmm/vmm_riscv.c
281–283	As in this should work, no need for the htmp variable

remove htmp variable

Add a1 to the list of clobbers

Move guest's hstatus bits initialization from vmmops_run() to vmmops_vcpu_init()

Regenerate

handle VM_EXITCODE_SUSPENDED.
this fixes re-creation of a VM with the same name after graceful shutdown

Regenerate

markj added inline comments.Tue, Oct 22, 2:54 PM

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
91 ↗	(On Diff #144397)	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?

Address markj@ comments, Thanks Mark

Herald added a reviewer: andrew. · View Herald TranscriptMon, Oct 28, 2:32 PM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added subscribers: bcran, rgrimes. · View Herald Transcript

br added inline comments.Mon, Oct 28, 2:32 PM

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. Maybe we can provide a common implementation in a separate change.
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). I did never try running multiple VMs on a single host, so that feature is unimplemented. We will need to support remote sfences in SBI as well (implementation is not trivial)
860	Ok
873	Yes, it allows to use more than 1 CPU. Otherwise you get this error: "8 vCPUs requested but only 1 available" See bhyverun.c.

Remove unrelated arm64/amd64 changes

br removed reviewers: andrew, manu.Mon, Oct 28, 2:38 PM

br removed subscribers: rgrimes, bcran.

unlock spin mutex when no interrupts found during APLIC CLAIM request

fix shift value of a register and remove bogus ones

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Oct 31, 8:25 PM

Closed by commit rGd3916eace506: riscv/vmm: Initial import. (authored by br). · Explain Why

This revision was automatically updated to reflect the committed changes.

br added a commit: rGd3916eace506: riscv/vmm: Initial import..

Herald added a subscriber: imp. · View Herald TranscriptThu, Oct 31, 8:25 PM

bhyve/riscv kernel part
ClosedPublic
Actions

Details

Spike

QEMU

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 141882

sys/conf/files.riscv

sys/conf/kern.mk

sys/riscv/include/cpu.h

sys/riscv/include/elf.h

sys/riscv/include/md_var.h

sys/riscv/include/riscvreg.h

sys/riscv/include/vmm.h

sys/riscv/include/vmm_dev.h

sys/riscv/include/vmm_instruction_emul.h

sys/riscv/include/vmm_snapshot.h

sys/riscv/riscv/genassym.c

sys/riscv/riscv/identcpu.c

sys/riscv/vmm/riscv.h

sys/riscv/vmm/vmm.c

sys/riscv/vmm/vmm_aplic.h

sys/riscv/vmm/vmm_aplic.c

sys/riscv/vmm/vmm_dev.c

sys/riscv/vmm/vmm_instruction_emul.c

sys/riscv/vmm/vmm_ktr.h

sys/riscv/vmm/vmm_riscv.c

sys/riscv/vmm/vmm_sbi.c

sys/riscv/vmm/vmm_stat.h

sys/riscv/vmm/vmm_stat.c

sys/riscv/vmm/vmm_switch.S

bhyve/riscv kernel partClosedPublicActions

Details

Spike

QEMU

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 141882

sys/conf/files.riscv

sys/conf/kern.mk

sys/riscv/include/cpu.h

sys/riscv/include/elf.h

sys/riscv/include/md_var.h

sys/riscv/include/riscvreg.h

sys/riscv/include/vmm.h

sys/riscv/include/vmm_dev.h

sys/riscv/include/vmm_instruction_emul.h

sys/riscv/include/vmm_snapshot.h

sys/riscv/riscv/genassym.c

sys/riscv/riscv/identcpu.c

sys/riscv/vmm/riscv.h

sys/riscv/vmm/vmm.c

sys/riscv/vmm/vmm_aplic.h

sys/riscv/vmm/vmm_aplic.c

sys/riscv/vmm/vmm_dev.c

sys/riscv/vmm/vmm_instruction_emul.c

sys/riscv/vmm/vmm_ktr.h

sys/riscv/vmm/vmm_riscv.c

sys/riscv/vmm/vmm_sbi.c

sys/riscv/vmm/vmm_stat.h

sys/riscv/vmm/vmm_stat.c

sys/riscv/vmm/vmm_switch.S

bhyve/riscv kernel part
ClosedPublic
Actions

Revision Contents
Changeset List