bhyve: Resume fails due to invalid guest state if guest_ncpus > 1

[gusev.vitaliy_gmail.com]

Summary

Error message:

vm exit[1]
        reason          VMX
        rip             0x0000000000000000
        inst_length     0
        status          0
        exit_reason     33 (VM-entry failure due to invalid guest state)
        qualification   0x0000000000000000
        inst_type               0
        inst_error              0
Abort trap (core dumped)

This error occurs because vm->vcpu[1] has not been allocated yet when vm_snapshot_vm() is called.

To fix this, move spinup_vcpu() before restore code. Use vm_vcpu_pause() before resume is started to avoid additional busy loop of all vcpu-s while resume is performing..

Sponsored by: vStack

Test Plan

Run VM, check working, then do suspend.

Restore previously saved snapshot.

VM Ubuntu 22.04.

[corvink]

usr.sbin/bhyve/bhyverun.c

1493

You should add a comment. Something like "Allocate all vcpus by performing a dummy read".

An even better solution would be to spin up all vcpus in suspended state early.

[gusev.vitaliy_gmail.com]

usr.sbin/bhyve/bhyverun.c

1493

I beleive this is the simplest solution. Spin up all vcpus would require additional loop at the end of main function that pass state from suspended to active.

[markj]

usr.sbin/bhyve/bhyverun.c

1493

I don't think we should use this kind of hack. It depends too much on the details of the kernel behaviour, and we are already stuck with some backwards compatibility cruft.

I agree with Corvin that we should explicitly create vcpus in the suspended state. An extra loop to transition from suspended to active is fine.

[jhb]

usr.sbin/bhyve/bhyverun.c

1493

I actually almost made the change of starting all vCPUs in suspended state and then later explicitly resuming the BSP when working on the latest refactoring of userspace (that I need to finish testing and merge). If you do that then you just need to rearrange the order here to make that initial loop run before snapshot restore.

[gusev.vitaliy_gmail.com]

usr.sbin/bhyve/bhyverun.c

1493

Just note, that there is misleading of "suspended" state. Generally it is "debug" state, and is seen as vm->debug_cpus.

Real suspended state can not be modified from userspace.

vm_suspend_cpu() from userspace only modifies vm->debug_cpus. And as side effect - all vCPU spin-up threads eat guest_cpus*100% CPU for any case.

Do you still want to add spin-up before resume steps ?

[gusev.vitaliy_gmail.com]

usr.sbin/bhyve/bhyverun.c

1493

@jhb Again, during startup in boot loader, VM ate about 100% CPU w/o any dependency of number of vcpus. Current code eats 100% * guest_ncpus. To verify it just stay in boot loader for some time and look at the host's cpus.

What's why I don't like solution with early spin up - it will eat 100% *N during all resume steps: reading image, reading VM memory, etc. For the instance, for 4 guest_ncpus it will be 400%.

[gusev.vitaliy_gmail.com]

@jhb @corvink Please look at updated diff. Is it good enough and perform what you suggested?

[corvink]

usr.sbin/bhyve/bhyverun.c

1493

We shouldn't implement some "hacks" to avoid eating up so much cpu time. Instead, we should just fix vm_supsend_cpu().

1503

suspend always has to be set to true at this point.

smbios and mptables are created in a later step. So, there's a race between the BSP and the creation of these tables.

[gusev.vitaliy_gmail.com]

spinup_vcpu all guest-cpus with suspended state and later resume it. Resume only BSP in case of startup.

[gusev.vitaliy_gmail.com]

usr.sbin/bhyve/bhyverun.c

1493

@corvink I am afraid of fixing all of them in one review and that would require more testing. So I implemented fix as you suggested initially, but w/o fix vm_suspend_cpu() (as you suggested later).

Current code works in both cases: startup and suspend/resume. vm_suspend_cpu can be improved in other review.

Do you agree?

1503

@corvink Corrected.

[corvink]

LGTM

[gusev.vitaliy_gmail.com]

Could anyone commit it ?