bhyve: snapshot virtio queues can fail
ClosedPublic
Actions

Authored by gusev.vitaliy_gmail.com on Sep 1 2020, 11:23 AM.

Details

Reviewers

jhb
darius.mihaim_gmail.com

Group Reviewers

bhyve

Commits

rGd079fc197aca: bhyve: Only snapshot initialized VirtIO queues

Summary

If virtio device is not fully initialized, then suspend fails with:

    
vi_pci_snapshot_queues: invalid address: vq->vq_desc
Failed to snapshot virtio-rnd; ret=14

Test Plan

Suspend and resume VM. VM should suspend w/o error and keep working after resume.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

gusev.vitaliy_gmail.com created this revision.Sep 1 2020, 11:23 AM

Herald added a reviewer: bhyve. · View Herald TranscriptSep 1 2020, 11:23 AM

Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript

gusev.vitaliy_gmail.com requested review of this revision.Sep 1 2020, 11:23 AM

mgrooms_shrew.net edited reviewers, added: darius.mihaim_gmail.com; removed: mgrooms_shrew.net.Sep 3 2020, 5:06 PM

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

elenamihailescu22_gmail.com added a subscriber: elenamihailescu22_gmail.com.Sep 7 2020, 7:11 AM

In D26268#584978, @darius.mihaim_gmail.com wrote:

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

I saw the same message also for virtio-net:

vi_pci_snapshot_queues: invalid address: vq->vq_desc
Failed to snapshot virtio-net; ret=14

To reproduce virtio-net fail, you need to suspend VM before vi_vq_init() is called, i.e. before even grub is started. To reproduce for virtio-rnd, you need run suspend exactly after grub and before guest Linux OS initialise virtio random device.

By the code you can see that this patch is required if device is not initialised by guest OS.

evgueni.gavrilov_itglobal.com added a subscriber: evgueni.gavrilov_itglobal.com.Sep 8 2020, 5:34 AM

Another wording might be: "Only snapshot initialized VirtIO queues."?

This revision is now accepted and ready to land.Nov 28 2020, 4:33 AM

In D26268#612118, @jhb wrote:

Another wording might be: "Only snapshot initialized VirtIO queues."?

Yes, sure.

Should this be committed?

In D26268#756453, @markj wrote:

Should this be committed?

I think yes. Of course if @jhb doesn't mind.

In D26268#756453, @markj wrote:

Should this be committed?

I think yes. Can you do it?
Just a side note. Vitaliy is my colleague, and we have been thoroughly testing his patches for several months in our test lab (vstack.com or Serverspace).
We think that these patches are quite stable, and in the near future we are thinking of releasing them into production.

It would be nice if these patches goes to the upstream.

In D26268#757216, @afedorov wrote:

In D26268#756453, @markj wrote:

Should this be committed?

I think yes. Can you do it?
Just a side note. Vitaliy is my colleague, and we have been thoroughly testing his patches for several months in our test lab (vstack.com or Serverspace).
We think that these patches are quite stable, and in the near future we are thinking of releasing them into production.

It would be nice if these patches goes to the upstream.