In D40105#921197, @jhb wrote:

Can you provide more description of what this change is doing? From what I can tell it uses err directly to abort quicker if it fails to restore an in-kernel structure, it inlines vm_restore_kern_struct into the loop in vm_restore_kern_structs. It renames vm_snapshot_kern_structs which is somewhat gratuitous IMO as it now no longer matches the other function names like vm_snapshot_user_devs. But I think the big change is not using lookup_struct and instead using meta->dev_name instead of meta->dev_req as the main key for the JSON for a given kernel struct? I think this means you can avoid lookup_struct because now instead of a flat array of all kernel structures they are separate objects with unique names? Can you expand on that more perhaps maybe with some examples of before/after JSON snippets? I would also suggest perhaps only doing the JSON change in this commit and not mixing in the other changes that I think obscure the real change you are making (e.g. the function rename, or inlining the function).

Fixed note from @corvink

In D40108#913868, @corvink wrote:

Why don't we call pci_snapshot_pci_dev unconditionally even if pe_snapshot is NULL?

@rew Could you look again ?

Fixed notes from @rew.

In D40108#913426, @gusev.vitaliy_gmail.com wrote:

In D40108#913423, @rew wrote:

not sure that devices should define a snapshot handler when not supported by the device.

At least it saves pdi.pi_cfgdata.

Close as whole patch series D38886 is committed.

In D40108#913423, @rew wrote:

not sure that devices should define a snapshot handler when not supported by the device.

@corvink I've split this patch with several patches. Note, according to @rew comments about adding and usage "register_snapshot_dev", I replaced that code with pci_next() function to walk all available PCI devices and save/restore them. 'atkbdc` device is saved/restored separately in this case.

In D35447#910663, @corvink wrote:

Patch looks good but it doesn't apply any more. Could you send an updated version please?

Rebased on main.

@corvink Could you look at this ? While Patrick's changes requires (in comment) a lot of modifying generic code, this simple fix could solve issue related for suspend/resume.

In D35590#908783, @corvink wrote:

@gusev.vitaliy_gmail.com As said, please split this commit. You can also send me a link to a personal git repo where I can pull the changes from.

@corvink Can it be committed or I should improve/correct it ?

Correct mention of SDM : Section 29.2.1 --> Section 30.2.1

In D39620#905476, @markj wrote:

What exactly is the problematic scenario? How do we end up with a pending, undelivered interrupt after a vmexit? Presumably the guest must have enabled interrupts before executing HLT.

The change itself seems reasonable to me, but I'd like to understand it better.

up?

@jhb @markj Could you look at it? This fix potentially could be added to 13.2 .

@rew @emaste Can we go further with this review ? Note, that some of code will be re-written in "one file per snapshot" review. However it will be later, because format of image file requires discussion.

In D35590#887928, @rew wrote:

In D35590#887785, @gusev.vitaliy_gmail.com wrote:

That will work, but this is not easier than current review's approach.

This counter-argument lacks any technical substance.

@rew Of course if assume that only PCI and Keyboard devices are used, introducing

Rebased

In D38860#887537, @rew wrote:

In D38860#887381, @rew wrote:

https://reviews.freebsd.org/D38858 also needs to be addressed before this patch is committed.

I've dropped my request for changes in D38858 - there's nothing blocking this review from being landed.

Corrected fd_meta usage in case of error.

In D35454#887531, @emaste wrote:

FYI if you use just the D# notation Phabricator will add markup to show which reviews are open/closed e.g.

D38866 libvmm: Add missed ioctl to vm_ioctl_cmds
D38855 bhyve: [snapshot] Do not flush readonly device at blockif_pause()
D38856 bhyve: [snapshot] Add cap limits for ipc socket
D38872 bhyve: Exit with EX_OSERR if init checkpoint or restore time failed
D38857 bhyve: Init checkpoint before caph_enter()
D38858 bhyve: Use directory fd with checkpoint
D38860 bhyve: Enable Capsicum for snapshots

In D35590#886738, @rew wrote:

In D35590#886683, @gusev.vitaliy_gmail.com wrote:

The main idea of this review - introduce struct snapshot_dev and it will be used by the following nvlist changes.

Seems like it'd make more sense to generate this meta information from the in-core configuration at the time the snapshot is taken.

Have you considered this?

In D35590#886517, @rew wrote:

In D35590#886289, @gusev.vitaliy_gmail.com wrote:

I guess you think that I will upstream this changes and forgot about format work. But I am going to create nvlist (including format change) as soon as that review committed.

Before doing the work, I strongly encourage you to write an email to the virtualization mailing list with the proposed file format.

In D35590#886608, @corvink wrote:

Only the file format is controversal. So, if you split this review into smaller ones which are unrelated to the file format change, we can make some progress.

@markj Could you re-approve and commit ?

Fixed "_DP_9p= sbuf" indentation.

In D35590#885880, @rew wrote:

How is this a stopper for the file format changes?

In D38905#885863, @markj wrote:

Seems ok. Did you test 9pfs in the guest? I would suggest trying it if so. In particular, we have no other examples where a shared library depends on casper libraries. Up until now, it is always executables which depend on casper libraries. So there might be some subtle problem or interaction.

@rew This is stopper for following up nvlist changes. Please review and possibly approve. Thanks.

In D38858#885643, @rew wrote:

In D38858#885637, @gusev.vitaliy_gmail.com wrote:

Why do you think single format work (nvlist) should be *before* enabling Capsicum? nvlist work will have a lot of changes and we can stuck if discussing any useful thing eats a lot of time and risk is not to move forward for the months. Just note, multiple device review is 7-8 months old. It is crazy for the code that is under #ifdef BHYVE_SNAPSHOT. We need speedup this work. For now it is about 2 years when Snapshot/Resume was added and there no significant progress.

If you have questions or notes about the code, please write here. Otherwise please accept the reviews.

The only file descriptor that needs to be passed to bhyve is the snapshot file that is being written to, which won't require opening a file descriptor to a directory.

Rebase this review after the file format work has been merged into main.

Thanks.

In D38858#885635, @rew wrote:

In D38858#885606, @gusev.vitaliy_gmail.com wrote:

nvlist implementation will use single file for all:

• config
• vram
• kernel data
• devices data

So you are right, using multiple files is not reasonable and is hard to operate.

In which case, the file descriptor change needs to happen after bhyve starts using a single file for snapshots.

In D38858#885604, @rew wrote:

..

see the original commit message that brought the snapshot code in

The file format also does not currently support
    versioning of individual chunks of state.  As a result, the current
    file format is not a fixed binary format and future revisions to save
    and restore will break binary compatiblity of snapshot files.  The
    goal is to move to a more flexible format that adds versioning,
    etc. and at that point to commit to providing a reasonable level of
    compatibility.

If I understand right, all things could be achieved with nvlist implementation. Thanks.

Why does there need to be multiple files for a single snapshot?

In D38858#885508, @rew wrote:

! In D38858#884878, @gusev.vitaliy_gmail.com wrote:
Did you mean about format of the image files or something another? If format, I am going to resolve it with coming nvlist changes.

you plan on converting the snapshot file format to an nvlist?

Moved casper dependency to lib9p.

Added revert commit 966026246e62769f3bcd8247a47fe0f4f0433aba

Fixed @markj notes.

@markj @corvink Please re-accept.

@markj @corvink Please re-accept.

Fix note, use init_checkpoint_thread(ctx) != 0

Add empty line after #endif

Corrected according with @rew notes.

In D38858#884892, @rew wrote:

In D38858#884878, @gusev.vitaliy_gmail.com wrote:

Did you mean about format of the image files or something another? If format, I am going to resolve it with coming nvlist changes.

Have you sought out jhb's advice on this yet? I know he has expressed his opinion on it in the past.

In D38858#884827, @rew wrote:

I still don't understand why you haven't addressed my comments that I raised in the original review. Do you feel it's not worth addressing or is that I'm out of line?

@corvink As you suggested I split this patch to the several reviews:

Rebase on to review https://reviews.freebsd.org/D38872