bhyve/riscv kernel part

[br]

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)

[br]

Regenerate

[br]

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!

[br]

SMP support added

[br]

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

[br]

Fix zero register getting in vmmops_getreg()

[br]

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

[br]

Handle exceptions during unprivileged read using hlvx.hu

[jrtc27]

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]

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]

sys/riscv/vmm/vmm_riscv.c

279

I don’t understand what you mean

[br]

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]

sys/riscv/vmm/vmm_riscv.c

281–283

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

[br]

remove htmp variable

[br]

Add a1 to the list of clobbers

[br]

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

[br]

Regenerate

[br]

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

[br]

Regenerate

[markj]

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?

[br]

Address markj@ comments, Thanks Mark

[br]

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.

[br]

Remove unrelated arm64/amd64 changes

[br]

unlock spin mutex when no interrupts found during APLIC CLAIM request

[br]

fix shift value of a register and remove bogus ones