Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vmm: Correctly suspend and resume the vmm driver. #1419

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MegaManSec
Copy link
Contributor

Previously, VMXON would be executed on a resume, contrary to proper initalization. The MSR lock may be lost on suspension, therefore must be taken again. Likewise, the VMX Enable bit may be cleared upon suspend, requiring to be re-set.

Concretely disable VMX on suspend, and re-enable it on resume.

Note: any IOMMU context will remain lost for any enabled vmm devices.

@bsdimp bsdimp self-assigned this Oct 4, 2024
Copy link
Member

@bsdjhb bsdjhb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be good to be explicit that the contents of MSR_IA32_FEATURE_CONTROL need to be restored in resume (I think that's what "MSR lock" is referring to?) in the log message.

@emaste
Copy link
Member

emaste commented Oct 4, 2024

Note: any IOMMU context will remain lost for any enabled vmm devices.

Even if this isn't a new issue from your change, should we have this noted somewhere?

@MegaManSec
Copy link
Contributor Author

MegaManSec commented Oct 4, 2024

I think it might be good to be explicit that the contents of MSR_IA32_FEATURE_CONTROL need to be restored in resume (I think that's what "MSR lock" is referring to?) in the log message.

Yes, MSR lock refers to MSR_IA32_FEATURE_CONTROL and specifically the IA32_FEATURE_CONTROL_LOCK bit. I will change the commit message to state that the contents MSR_IA32_FEATURE_CONTROL needs to be restored.

@MegaManSec
Copy link
Contributor Author

MegaManSec commented Oct 4, 2024

Note: any IOMMU context will remain lost for any enabled vmm devices.

Even if this isn't a new issue from your change, should we have this noted somewhere?

Probably (and I may have used the incorrect terminology about "IOMMU context"). FWIW, using vmm with a pci device on my system stalls the system completely upon its use after a resume (more details: https://joshua.hu/brcmfmac-bcm43602-suspension-shutdown-hanging-freeze-linux-freebsd-wifi-bug-pci-passthru) -- when I'm passthru'ing the device to a Linux VM, I have to "remove" the device inside the VM before a suspension, otherwise the system crashes (just completely freezes/stalls) after resume and the device is used again.

As far as I could tell when debugging a bit more lately, the irq (at least) of the device is lost on resume, which results in the system freezing when the device is interacted with.

Previously, VMXON would be executed on a resume, contrary to proper
initalization. The contents of MSR_IA32_FEATURE_CONTROL may be lost on
suspension, therefore must be restored. Likewise, the VMX Enable bit may be
cleared upon suspend, requiring it to be re-set.

Concretely disable VMX on suspend, and re-enable it on resume.

Note: any IOMMU context will remain lost for any enabled vmm devices.

Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants