looks like this is value is stated in RFC8907 section 6.1 under the authen_type header

Thanks

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

At least it saves pdi.pi_cfgdata. Do you think it is waste doing and .pi_cfgdata can not be changed by guest OS or be changed between bhyve versions ?

small nits - other than that, looks good

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

In D39979#910118, @corvink wrote:

Do you think we should add this option now or should we wait until someone has some issues or a feature requires it?

It might be helpful for debugging purposes too.

In D39805#906206, @markj wrote:

Would anyone be willing to test this on an AMD system?

so I assume some ACPI devices will have different emulation than others?

the comments from D39315 apply to this review as well

Seems a bit odd making this public? as opposed to keeping a reference of the softc in gpu_passthru and passing that off to pci?

committed in rG2dfd9979d827b36a9ed6ef2d5911a755091216c7 or SVN r279342

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

Let's move forward and all useful things will be added :)

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

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

In D38860#887381, @rew wrote:

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

I withdraw my request for changes.

In D38860#887339, @markj wrote:

Looks like this patch needs to be rebased.

In D35590#887206, @gusev.vitaliy_gmail.com 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.

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

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

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

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

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.

I don't think so. It is not dependent thing. Current state of snapshot/resume is using multiple files. And this work is about adding support Capsicum. It is not related to the single file format at all.

So this review and https://reviews.freebsd.org/D38860 will have to wait till the above work is merged.

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.

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

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?

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 D38858#885557, @gusev.vitaliy_gmail.com wrote:

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?

Plain is:

1. Capsicum (has review)
2. Multiple devices support (has review)
3. nvlist review. Not yet created and waits for 1-2 finish. I will describe and propose file format with nvlist implementation.

Sorry, which advice did you mean? I don't see any comment from jhb in https://reviews.freebsd.org/D35454

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.

! 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.

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.

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?

I still hold the opinion that the snapshot data should be stored in a single file and the file descriptor for that file should be passed to bhyve.

spelling nit

Tested - works as expected for me,

https://github.com/openzfs/zfs/pull/14501

I don't see any issues with this offhand - be nice to have this fixed for 13.2

In D38231#868911, @emaste wrote:

would it be worth the effort to fix this upstream?

I prefer that if someone's willing to shepherd it in, but in the meantime adding the warning flag is sensible.

would it be worth the effort to fix this upstream? something like:

After a second look, perhaps md's devsw shouldn't even define dv_fmtdev or dv_parsedev and use the defaults instead.

it looked something like: https://reviews.freebsd.org/differential/diff/115789/

My local workaround to fix this was to add md_parsedev() and md_formatdev() and have it only understand md0.

In D38072#865036, @markj wrote:

Does the bug occur because the ppt module is unloaded after the vmm module when vmm.ko is unloaded?

previous diff was incorrect

In D37972#864919, @crowston_protonmail.com wrote:

I applied your patch to -CURRENT, but unfortunately I get a panic in vm_iommu_modify(): "vm mem_segs not locked "

That doesn't seem related to this change.

In D37972#864919, @crowston_protonmail.com wrote:

I applied your patch to -CURRENT, but unfortunately I get a panic in vm_iommu_modify(): "vm mem_segs not locked @ /usr/src/sys/amd64/vmm/vmm.c:1189"

In D37296#847263, @corvink wrote:

Disabling this warning will lead to even more unaligned accesses. If you're working on removing all unaligned accesses and this commit is inteded to increase the warn level early, it's fine but please mention it in the commit message.

In D37295#847262, @corvink wrote:

Even if it only finds very simple bugs, it's a good option. Simple bugs happen. Setting up a VM to find a bug requires some work and is error prone. So, it's much better if the compiler complains about your bug.
Note that if we disable the option, it'll get even worse.

Not sure if I missed a step while applying this stack of patches but, I got the following error while build testing. I've provided a diff for context that allowed the build to complete, https://reviews.freebsd.org/differential/diff/112707/

In D35454#830046, @emaste wrote:

I haven't fully reviewed in detail, but it seems that this is an incremental step forward, and it makes sense to go ahead with this change [

In D36279#824118, @kp wrote:

In D36279#824072, @kp wrote:

Hmm. This *looks* sane to me, but this patch breaks the if_vlan tests, so clearly I'm missing something. A ping over a vlan interface (on top of epair, in a vnet jail) results in ping: sendto: Invalid argument, and I'm not seeing why at the moment.

It looks like what's happening here is that we fail the arp lookup because we're trying to do a lookup on the underlying interface, not the vlan interface.

I've submitted a pull request upstream:

In D33400#818953, @kib wrote:

I meant something different. There is a very little need to add knlist_init_sx(9), when we only have single consumer. Put the code into ZFS OS-specific file.

In D33400#818876, @kib wrote:

Is it for single consumer?