In D34718#925619, @gusev.vitaliy_gmail.com wrote:

In D34718#925608, @elenamihailescu22_gmail.com wrote:

We consider that the migration part is a must-have feature for a mature hypervisor and we really want to have it for bhyve. The end goal is to have a live migration feature for bhyve. It means inspecting the guest memory pages to check for changes between rounds. It also modifies a dirty bit. The inspection (and changes) should be done from within the bhyve process. I'm not sure how this part should be tackled in your suggestion. However, please keep in mind that this approach was discussed with the bhyve's maintainers 4 years ago. We asked for advice and came up with this idea of having a custom dirty bit for migration.

I don't see any problem with that. I will open review with possibility to get diff pages.

So instead of having a lot code in bhyve, I would suggest to have portion of primitives that can help to implement warm, live migration outside of bhyve process.

In D34718#925599, @gusev.vitaliy_gmail.com wrote:

In D34718#925598, @darius.mihaim_gmail.com wrote:

The *live* migration must be part of bhyve, as the migration process must transfer the virtual machine's memory from one host to the other while the virtual machine runs on the original host.

Of course, some changes to the migration code are unavoidable in case the snapshot process changes in some particular ways, but they should remain minimal in my expectations.

I believe that migration code can be outside of bhyve process even with warm or live migration. Really, suppose to have bmigrate process written in C, shell, python (high level language):

a. bhyve dumps diff-pages to a file1.
b. bmigrate sends data to host2 or just places on shared storage
c. bmigrate calls bhyve to suspend without memory.
d. bmigrate sends suspended data to host2. and restore memory 1-st iteration on host2.
e. bmigrate calls vmm to dump diff-pages to file2.
f. bmigrate sends diff-pages to host2 and restore 2-st iteration.
g. bmigrate restore process on host2.

This is rough idea, but it should work w/o adding a lot of code to bhyve.

The same is for warm migration. Why this code should be in bhyve? All states, cpus, etc. could be tracked outside of bhyve.

In D34718#925593, @gusev.vitaliy_gmail.com wrote:

In D34718#925583, @darius.mihaim_gmail.com wrote:

In D34718#925207, @afedorov wrote:

I think we should bring support for snapshots first. And enable BHYVE_SNAPSHOT by default.

To do this, it remains to agree on the snapshot format: https://lists.freebsd.org/archives/freebsd-virtualization/2023-May/001295.html

I'm not sure whether to tie the transfer of the memory dump to the TCP.

Or add an opportunity similar to 'zfs send' to be independent of transport. For example, share a memory image using SSH or NFS.

Not sure how the transfer would be *tied* to the TCP protocol just by having the implementation use TCP by default. Many tools like curl, socat and browsers have figured out that you can use URIs to specify protocols and even authentication when connecting to a service. The migration parameters can be extended to use ssh://, file:/// or even the basic - for piping the binary data to standard output.

I think asking this question more than one year after this review has been open, and more than two after the original review (https://reviews.freebsd.org/D28270) was created is simply meant to delay accepting the changes. I understand that having a good and secure code base is a must, but this change simply asks for "why don't we over engineer this part which has very little to do with the actual virtual machine migration process?", without actually providing a good case for "this is not up to the BSD standards".

I had review (D35590) that was opened about a year. This is fix for current snapshot implementation. We are talking about two things:

1. Why this feature cannot be done outside of bhyve?
2. Who will do fixing if this migration review(s) becomes committed?

I suppose, both questions should answered before committing those changes. Because even without this migration additions, current snapshot implementation still has a lot of bugs and, I believe, they should be fixed first before add new functionality for snapshot/resume.

In D34718#925207, @afedorov wrote:

I think we should bring support for snapshots first. And enable BHYVE_SNAPSHOT by default.

To do this, it remains to agree on the snapshot format: https://lists.freebsd.org/archives/freebsd-virtualization/2023-May/001295.html

I'm not sure whether to tie the transfer of the memory dump to the TCP.

Or add an opportunity similar to 'zfs send' to be independent of transport. For example, share a memory image using SSH or NFS.

In D26264#596960, @gusev.vitaliy_gmail.com wrote:

@darius.mihaim_gmail.com Is this review good for you ?

Is the issue specific to virtio-rnd, or why does it appear? I'm wondering why it was not caught earlier.

This one seems pretty straight-forward.

Rebase the code with SVN R359133 / git 217fa09bf639260f4fe7c9415d8f42b141637d51.

Add license and copyrights to snapshot specific files (snapshot.[ch], vmm_snapshot[ch]).

• Added compilation flag(s) to use as experimental functionality (BHYVE_SNAPSHOT) thanks to @jhb
• Rebase with freebsd/freebsd updated master branch, on 4th of October 2019. Refs: git: e96b4170d024f41e9f178394a77006f39aaf6610 SVN: r353039

• integrate updated vCPU pause mechanism from @jhb that uses the VM debug components.
• separate BSP vCPU initialization from other vCPUs (do not initialize all vCPUs before starting the VM when not restoring; do not automatically grant UNRESTRICTED_GUEST capabilities to BSP)
• fix various coding errors, such as removing unused macros, using proper buffer size for snprintf and coding style

• kick the guest threads out of vmrun when starting the pausing vCPUs.
• remove host registers from intel/vmx.c snapshot process (AMD registers have not been removed yet).

Remove some irrelevant comments and unused functions

I have applied the feedback @jhb gave, except for the #ifdef guards. Will work on it after rebasing with a newer master.

• rework vCPU pausing and resuming mechanism
• rewrite VM suspend procedure - do not resume VM and devices before exit
• add missing break clause in vmm_dev switch-case
• various minor coding style fixes pointed by @jhb
• remove unnecessary critical section nesting level check

Remove debug printfs from pci_ahci.c

Remove vLAPIC fields from snapshot procedure, if they are not required after a restore / can be computed from other fields.
Save vLAPIC's LAPIC page as part of vLAPIC snapshot procedure, instead of a separate data structure.
Rename variables / functions / macros used to snapshot guest to host memory mappings to make them more intuitive.

Add minor fixes to snapshot logic:

• correct variable used as parameter for 'sizeof' in vatpic.c (the size was correct before, by coincidence)
• remove a comment that became irrelevant in the past in vhpet.c
• fix vLAPIC timer reset logic in the off-chance the timer would be periodic and a value of 0 is saved for the counter

In D19495#426019, @novel wrote:

This is a very useful feature, thanks for working on that!

Remove some unused code and fix style of comments

• remove guest memory mapping device that is currently unused. The device was used to make snapshots faster by creating a separate Copy-on-Write mapping of the guest memory. This approach is not useful for guest suspends, and we may be able to avoid exporting it through a device even for snapshots (will be investigating that approach).
• remove some unused code

Fix issues pointed by inline comments