-
Notifications
You must be signed in to change notification settings - Fork 419
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
devices: Add pvpanic support #5526
Conversation
d4d35e7
to
1b7e1de
Compare
Hi Rob, |
vmm/src/config.rs
Outdated
parser.add_valueless("pci"); | ||
parser.parse(device_type).map_err(Error::ParsePvPanic)?; | ||
|
||
if parser.is_set("pci") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we will ever support anything other than a PCI device so you don't need this.
bacc9cf
to
7394466
Compare
I think you need to update the integration tests. |
Yes, I am working on this. |
patches have been updated, Rob ;) |
tests/integration.rs
Outdated
// Wait a while for guest | ||
thread::sleep(std::time::Duration::new(5, 0)); | ||
|
||
// Ignore event check tempraly, as we need pvpanic-pci driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add the panic driver to the kernel config file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean resources/linux-config-x86_64?
Will the vmlinux used in CI workloads build from this config? Forgive me for not knowing this 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the CI builds from that config file - there is an aarch64 equivalent too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I will try to fix this.
Thx 😸
a8c4b05
to
d5fcc3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good - really close now!
vmm/src/config.rs
Outdated
/// Failed parsing pvpanic | ||
ParsePvPanic(OptionParserError), | ||
/// No device type given for pvpanic | ||
ParsePvPanicDeviceTypeGiven, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, thanks to point out.
vmm/src/config.rs
Outdated
@@ -336,6 +340,13 @@ impl fmt::Display for Error { | |||
ParseVdpaPathMissing => write!(f, "Error parsing --vdpa: path missing"), | |||
ParseTpm(o) => write!(f, "Error parsing --tpm: {o}"), | |||
ParseTpmPathMissing => write!(f, "Error parsing --tpm: path missing"), | |||
ParsePvPanic(o) => write!(f, "Error parsing --pvpanic: {}", o), | |||
ParsePvPanicDeviceTypeGiven => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be removed.
resources/linux-config-aarch64
Outdated
@@ -1402,7 +1402,9 @@ CONFIG_NVME_MULTIPATH=y | |||
# CONFIG_MISC_RTSX_PCI is not set | |||
# CONFIG_HABANA_AI is not set | |||
# CONFIG_UACCE is not set | |||
# CONFIG_PVPANIC is not set | |||
CONFIG_PVPANIC=y | |||
CONFIG_PVPANIC_MMIO=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this one? Does PCI depend on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No :)
We need pvpanic-pci only now, I will fix that immediately.
Hi all, |
That works, thanks so much. |
vmm/src/device_manager.rs
Outdated
|
||
self.device_tree.lock().unwrap().insert(id, node); | ||
|
||
info!("add pvpanic device"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :)
Introduce emulation of pvpanic device to allow cloud hypervisor to get the notify from guest's pvpanic driver when guest kernel crash. Signed-off-by: Yi Wang <foxywang@tencent.com>
Implemetion of BusDevice and PciDevice trait for pvpanic device. Pvpanic device can be implemented as an ISA device or as a PCI device, we choose PCI device for now. Signed-off-by: Yi Wang <foxywang@tencent.com>
Add PvPanicDeviceState to retore events of pvpanic from snapshot, and also PciConfigurationState. Signed-off-by: Yi Wang <foxywang@tencent.com>
Add method for DeviceManager to invoke. Signed-off-by: Yi Wang <foxywang@tencent.com> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
Add integration test for pvpanic, by two methods: - the vendor id and device id of pci device in guest - triggering a guest panic and check event-monitor. Also, to support pvpanic-pci driver, add pvpanic config in resources. Signed-off-by: Yi Wang <foxywang@tencent.com> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
When crash occurs in guest, we can use pvpanic device to notify through the event-monitor. This is useful to monitor the guest's running state and respond to the event.
The pvpanic device can be implemented as an ISA device or PCI device, we implemented the pvpanic-pci device for now.