This probably triggers warnings. Pointer math on void * isn't defined in standard C. GNU C allows it as an extension. Char * would be better. It seems like 'offaddr' should also be vm_paddr_t, not off_t. FreeBSD's style also doesn't do spaces after casts, but that's minor.

This is normally spelled 'packed' in FreeBSD and would be added to the end of the struct normally. However, I don't think you need packed here since a char array is not going to have padding on any architectures FreeBSD supports.

Ideally this isn't really part of the API. We'd just want some kind of opaque "CPU context" perhaps that is VMCS on Intel, VMCB on AMD. This can be on the list for things to refine in tree.

These are a lot of nested includes for this header. Certainly <sys/param.h> shouldn't be a nested include. I wonder if we can get rid of all of these nested includes? It's fine to make certain things in the header conditional as the existing headers do with _SYS_CPUSET_H_ if you need to.

Why is this check here?

Not to change right now, but for the one-liner wrapper functions that are only used once like this (vm_snapshot_lapic) I would probably just inline the call to vlapic_snapshot() directly in this switch instead.

Missing a 'break' here now.

You can't call make_dev* while holding locks. There is a race with setting si_drv1 that can be fixed by using make_dev_s instead of make_dev_p, but that would be a separate change from save/restore I think,

For local variables, we can just use 'op' without leading underscores. FreeBSD generally only uses leading underscores for global symbols that are not part of the public API.

Maybe use __DEVOLATILE instead:

void *_data;

_data = __DEVOLATILE(void *, data);

Maybe to avoid the leading underscore you could call the function parameter 'datav' and the local variable in the function 'data'. Alternatively, if there are not many volatile pointers being passed to this function, the volatile could just be removed and __DEVOLATILE could be used in callers in the places where it is needed.

Are the libucl headers not installed?

When you are using stdio for the console, it leaves the console in raw mode so \n is only a newline and not a full CR+LF combo. Other printfs in bhyve only use \n though since stdio isn't always used as the console.

Why does this need packed? If it isn't stored in a file, we should let the compiler choose the optimal layout.

Why aren't the x86 generic registers being saved/restored by logic shared by both SVM and VMX?

Why are you saving host state?

The eptgen is specific to the state of the host machine and probably shouldn't be saved.

The ASID is potentially specific to the state of the host machine (it is on SmartOS bhyve at least) and shouldn't be saved.

As bhyve lacks AVIC support today, these should all be zero at snapshot time. Unused portions should probably be skipped.

I'm not sure it makes sense to record the ASID as part of snapshot state, as that's specific to the host for tracking vCPUs as they go on/off CPU

Rather than requiring forward definitions here and in the other headers (vlapic, vhpet, etc) for struct vm_snapshot_meta, why not include machine/vmm_snapshot.h? It's required for the function implementation anyways.

With descriptors being common across the two CPU types, why not use a general mechanism for saving them?

Why not save these MSRs in a general fasion? (Same goes for EFER too)

Saving/restoring control state like this with no validation seems dangerous. It's probably safest to allow the host to reconstruct that from present (and requested) features?

In SmartOS bhyve, we added a CPU-agnostic method for setting the tsc offset in vmm.c, which then VMX/SVM consume in order to do the CPU-specific settings. It might make sense to do something like that here.

The write operators for these register tables enforce certain invariants about their contents. The restore process likely needs to do the same enforcement. Additionally, I believe this is skipping TMR updates for the CPUs.

Why was this accessor added?

Why is this here with no consumer?

I'm uncomfortable about providing ioctl interfaces to these locking operations. All of the other vCPU lock consumers will take the needed lock(s), perform their action in kernel space, and the release the lock(s) prior to returning to userspace. These ioctls break that model. Furthermore, since the save/restore operation is driven from userspace, where the vCPU run threads are operating, there's no reason why the quiescence cannot start there.

No warnings were generated at compile time, but using char * makes more sense since baseaddr is also char *

Removed it; no longer used.

Removed all includes from this header. Replaced 'struct vm_snapshot_req' with 'struct vm_snapshot_meta' since the former had only one member of the latter type.

Removed it; was forgotten from a previous implementation.

Removed the one-line wrapper functions.

Removed the comment, since it was not related to save/restore.

Renamed the variable.

Used __DEVOLATILE here to be safe and keep the compatibility (same parameters and return type) with the functions used in userspace.

Build fails if this line is removed - #include <ucl.h> is unable to find the header.

Will keep it it in mind, thank you. We can remove the CR characters for coding style sometime down the line.

Perhaps because this is a private library?
/usr/include/private/ucl/ucl.h

The data structures that hold them differ. Registers on Intel are also saved like this. Using generics to read and write them may be in order.

It's similar to the Intel implementation. We are working on removing these registers from that implementation. We will do the same here when we fix it there, but we cannot test it on AMD currently.

Forward declarations were seemed like the preferred implementation. Including headers may be problematic, especially if those include other headers.

Simply meant to keep code for all data from vmcs bundled in one function - may be useful to move to a generic function to avoid duplication.

Same comment as the one for other registers.

We will look into it, but it's not a priority for now. Thanks for pointing it out.

Added this to the analysis document under restore checks with TODO.

Removed it; was a leftover from an older implementation.

Removed it; was a leftover from an older implementation.

Reworked the VM pause to block the threads in userspace.

I strongly disagree. Practically everywhere else, shared types declarations are pulled in via headers. What about this situation makes it so complicated that doing so isn't feasible?

Forward declarations are used extensively in FreeBSD when the header only needs to reference the pointer type. I guess I don’t understand why you strongly disagree with using them appropriately.

Have you quantified the overhead added by these mutex/condvar operations for exit-heavy workloads? VM exits which go all the way to userspace are relatively expensive, so the relative delta might be small, but it would be useful to know.

It might make sense to move this logic from libvmmapi into the bhyve process itself, where all of the other userspace vCPU state tracking is going on. If contention turned out to be high when lots of userspace exits are occurring (since there's only the one lock today) it would be possible, armed with knowledge of the number of active vCPU threads, to do something like a per-vCPU lock and pause state. Going that far might only make sense if the vCPU threads happen to contend in such a way.

Error logging to stderr is left up to the library consumer (read: the bhyve binary) today. It probably makes sense to continue that.

Here and the two functions below could just be sure to leave the error state (meta->dev_name or errno) in place to be logged by the consumer, should they so choose.

What is this for?

It looks like mevent.c has an extern declaration which refers to this which doesn't reflect the const change made here. It should be kept in sync?

Given the logic in spinup_vcpu() the UNRESTRICTED_GUEST capability is now a necessity for all bhyve users. If that is your intent, it probably needs to be properly documented and communicated to the user base.

Is there reason to believe that it's not necessary to allow the restoring host to query these parameters itself when bringing up the blockdev there? It could be that aspects of the device (particularly with respect to its capabilities) may change.

If you're taking a snapshot while the vCPUs are paused, won't copying of the virtqueues be redundant, as they'll be picked up in the copy of guest RAM?

The vCPU pause mechanism has been changed thanks to @jhb. It is now relying on the debug functionality and should no longer cause heavy contention on mutexes.

Will update this at a later time. The messages from the entire lib will have to be moved where functions are used.

Artifact from an older implementation. The code has since been refactored.

I have added the 'const' qualifier to the mevent.c variable. The vmname should not be modified at runtime, so it makes more sense to keep it as a constant. On a side note, since the name of the VM is also kept in libvmmapi's 'struct vmctx' , the name does not need to be kept as a global; a getter function may be better suited.

I have changed the initialization loop, and moved the BSP vCPU initialization outside it. Through it, the BSP vCPU will not have the UNRESTRICTED_GUEST capability set, and the other vCPUs will be initialized only when restoring the VM.

Since the changes added by @jhb keep track of the activated vCPUs, it may be worth keeping track of those when suspending/resuming, but I do not believe there will be any cases where this will actually be important, since they will be activated by the guest very early on.

If the information can be easily queried, the mechanism can be changed. However, the restore mechanism will likely become more complex, and relying on other saved data, such as guest memory, may not improve security as it is saved in a similar fashion (copied in/from a file on disk).

The GUEST2HOST_ADDR macros do not save memory, but rather guest memory mappings. The pointer parameter is converted to a guest address that can be used to locate the virtual queues in the new (restored) VM's address space.