Page MenuHomeFreeBSD

bhyve/riscv kernel part
ClosedPublic

Authored by br on Jun 10 2024, 8:52 PM.
Tags
None
Referenced Files
F104060871: D45553.diff
Tue, Dec 3, 12:26 AM
Unknown Object (File)
Sun, Dec 1, 12:02 AM
Unknown Object (File)
Fri, Nov 29, 1:08 PM
Unknown Object (File)
Fri, Nov 29, 10:07 AM
Unknown Object (File)
Fri, Nov 29, 8:18 AM
Unknown Object (File)
Thu, Nov 28, 6:52 PM
Unknown Object (File)
Thu, Nov 28, 6:07 AM
Unknown Object (File)
Thu, Nov 28, 1:09 AM

Details

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

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

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
69 ↗(On Diff #139929)

no such directory yet

sys/riscv/vmm/vmm_riscv.c
406

Fixed!

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

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

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
sys/riscv/vmm/vmm_riscv.c
281–283

As in this should work, no need for the htmp variable

Add a1 to the list of clobbers

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

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?

br marked 33 inline comments as done.

Address markj@ comments, Thanks Mark

sys/riscv/include/vmm.h
280

Removed most of them

sys/riscv/vmm/vmm.c
337
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

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.Oct 31 2024, 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.