firmware_uefi: EfiDiagnostics switch to polling instead of Arc<Mutex<>> (microsoft#1693)

This PR aims to take a different approach to flushing EfiDiagnostics on
Watchdog timeout.

- Removes Arc<Mutex<>>, which was originally used to share the
diagnostics service between UefiDevice and its WatchdogPlatform
- Creates a mesh::channel before creating the UefiDevice. The
watchdog_callback that gets added to the UefiDevice has its `on_timeout`
method modified to invoke the sender to send a notification.
- The mesh::receiver end gets sent down the UefiDevice, and it is used
in `poll_device()` to freely respond to watchdog events

Author: maheeraeron

Cargo.lock

@@ -1721,7 +1721,6 @@ dependencies = [
  "open_enum",
  "openssl",
  "pal_async",
- "parking_lot",
  "test_with_tracing",
  "thiserror 2.0.12",
  "time",

openhcl/underhill_core/src/worker.rs

@@ -2060,6 +2060,7 @@ async fn new_underhill_vm(
                 },
             };
 
+            let (watchdog_send, watchdog_recv) = mesh::channel();
             deps_hyperv_firmware_uefi = Some(dev::HyperVFirmwareUefi {
                 config,
                 logger: Box::new(UnderhillLogger {
@@ -2091,19 +2092,22 @@ async fn new_underhill_vm(
                     #[cfg(guest_arch = "x86_64")]
                     let watchdog_callback = WatchdogTimeoutNmi {
                         partition: partition.clone(),
+                        watchdog_send: Some(watchdog_send),
                     };
 
                     // ARM64 does not have NMI support yet, so halt instead
                     #[cfg(guest_arch = "aarch64")]
                     let watchdog_callback = WatchdogTimeoutReset {
                         halt_vps: halt_vps.clone(),
+                        watchdog_send: Some(watchdog_send),
                     };
 
                     // Add the callback
                     underhill_watchdog_platform.add_callback(Box::new(watchdog_callback));
 
                     Box::new(underhill_watchdog_platform)
                 },
+                watchdog_recv,
                 vsm_config: Some(Box::new(UnderhillVsmConfig {
                     partition: Arc::downgrade(&partition),
                 })),
@@ -2422,13 +2426,15 @@ async fn new_underhill_vm(
                     .as_non_volatile_store(vmgs::FileId::GUEST_WATCHDOG, false)
                     .context("failed to instantiate guest watchdog store")?;
 
-                let trigger_reset = WatchdogTimeoutReset {
+                let watchdog_callback = WatchdogTimeoutReset {
                     halt_vps: halt_vps.clone(),
+                    watchdog_send: None, // This is not the UEFI watchdog, so no need to send
+                                         // watchdog notifications.
                 };
 
                 let mut underhill_watchdog_platform =
                     UnderhillWatchdogPlatform::new(store, get_client.clone()).await?;
-                underhill_watchdog_platform.add_callback(Box::new(trigger_reset));
+                underhill_watchdog_platform.add_callback(Box::new(watchdog_callback));
 
                 Box::new(underhill_watchdog_platform)
             },
@@ -3441,6 +3447,7 @@ impl ChipsetDevice for FallbackMmioDevice {
 #[cfg(guest_arch = "x86_64")]
 struct WatchdogTimeoutNmi {
     partition: Arc<UhPartition>,
+    watchdog_send: Option<mesh::Sender<()>>,
 }
 
 #[cfg(guest_arch = "x86_64")]
@@ -3454,18 +3461,27 @@ impl WatchdogCallback for WatchdogTimeoutNmi {
             Vtl::Vtl0,
             MsiRequest::new_x86(virt::irqcon::DeliveryMode::NMI, 0, false, 0, false),
         );
+
+        if let Some(watchdog_send) = &self.watchdog_send {
+            watchdog_send.send(());
+        }
     }
 }
 
 struct WatchdogTimeoutReset {
     halt_vps: Arc<Halt>,
+    watchdog_send: Option<mesh::Sender<()>>,
 }
 
 #[async_trait::async_trait]
 impl WatchdogCallback for WatchdogTimeoutReset {
     async fn on_timeout(&mut self) {
         crate::livedump::livedump().await;
 
-        self.halt_vps.halt(HaltReason::Reset)
+        self.halt_vps.halt(HaltReason::Reset);
+
+        if let Some(watchdog_send) = &self.watchdog_send {
+            watchdog_send.send(());
+        }
     }
 }

openvmm/hvlite_core/src/worker/dispatch.rs

@@ -1047,6 +1047,7 @@ impl InitializedVm {
         let mut deps_hyperv_firmware_uefi = None;
         match &cfg.load_mode {
             LoadMode::Uefi { .. } => {
+                let (watchdog_send, watchdog_recv) = mesh::channel();
                 deps_hyperv_firmware_uefi = Some(dev::HyperVFirmwareUefi {
                     config: firmware_uefi::UefiConfig {
                         custom_uefi_vars: cfg.custom_uefi_vars,
@@ -1094,19 +1095,22 @@ impl InitializedVm {
                         #[cfg(guest_arch = "x86_64")]
                         let watchdog_callback = WatchdogTimeoutNmi {
                             partition: partition.clone(),
+                            watchdog_send: Some(watchdog_send),
                         };
 
                         // ARM64 does not have NMI support yet, so halt instead
                         #[cfg(guest_arch = "aarch64")]
                         let watchdog_callback = WatchdogTimeoutReset {
                             halt_vps: halt_vps.clone(),
+                            watchdog_send: Some(watchdog_send),
                         };
 
                         // Add callbacks
                         base_watchdog_platform.add_callback(Box::new(watchdog_callback));
 
                         Box::new(base_watchdog_platform)
                     },
+                    watchdog_recv,
                     vsm_config: None,
                     // TODO: persist SystemTimeClock time across reboots.
                     time_source: Box::new(local_clock::SystemTimeClock::new(
@@ -1326,6 +1330,8 @@ impl InitializedVm {
                     // Create callback to reset on watchdog timeout
                     let watchdog_callback = WatchdogTimeoutReset {
                         halt_vps: halt_vps.clone(),
+                        watchdog_send: None, // This is not the UEFI watchdog, so no need to send
+                                             // watchdog notifications
                     };
 
                     // Add callbacks
@@ -2987,6 +2993,7 @@ fn add_devices_to_dsdt(
 #[cfg(guest_arch = "x86_64")]
 struct WatchdogTimeoutNmi {
     partition: Arc<dyn HvlitePartition>,
+    watchdog_send: Option<mesh::Sender<()>>,
 }
 
 #[cfg(guest_arch = "x86_64")]
@@ -2998,16 +3005,25 @@ impl WatchdogCallback for WatchdogTimeoutNmi {
             Vtl::Vtl0,
             virt::irqcon::MsiRequest::new_x86(virt::irqcon::DeliveryMode::NMI, 0, false, 0, false),
         );
+
+        if let Some(watchdog_send) = &self.watchdog_send {
+            watchdog_send.send(());
+        }
     }
 }
 
 struct WatchdogTimeoutReset {
     halt_vps: Arc<Halt>,
+    watchdog_send: Option<mesh::Sender<()>>,
 }
 
 #[async_trait::async_trait]
 impl WatchdogCallback for WatchdogTimeoutReset {
     async fn on_timeout(&mut self) {
-        self.halt_vps.halt(HaltReason::Reset)
+        self.halt_vps.halt(HaltReason::Reset);
+
+        if let Some(watchdog_send) = &self.watchdog_send {
+            watchdog_send.send(());
+        }
     }
 }

vm/devices/firmware/firmware_uefi/Cargo.toml

@@ -45,7 +45,6 @@ bitfield-struct.workspace = true
 der = { workspace = true, features = ["derive", "alloc", "oid"], optional = true }
 getrandom.workspace = true
 openssl = { optional = true, workspace = true }
-parking_lot.workspace = true
 thiserror.workspace = true
 time = { workspace = true, features = ["local-offset"] }
 tracelimit.workspace = true

vm/devices/firmware/firmware_uefi/src/lib.rs

@@ -68,13 +68,12 @@ use inspect::Inspect;
 use inspect::InspectMut;
 use local_clock::InspectableLocalClock;
 use pal_async::local::block_on;
-use parking_lot::Mutex;
 use platform::logger::UefiLogger;
 use platform::nvram::VsmConfig;
 use std::convert::TryInto;
 use std::ops::RangeInclusive;
-use std::sync::Arc;
 use std::task::Context;
+use std::task::Poll;
 use thiserror::Error;
 use uefi_nvram_storage::InspectableNvramStorage;
 use vmcore::device_state::ChangeDeviceState;
@@ -106,7 +105,7 @@ struct UefiDeviceServices {
     generation_id: service::generation_id::GenerationIdServices,
     #[inspect(mut)]
     time: service::time::TimeServices,
-    diagnostics: Arc<Mutex<service::diagnostics::DiagnosticsServices>>,
+    diagnostics: service::diagnostics::DiagnosticsServices,
 }
 
 // Begin and end range are inclusive.
@@ -135,6 +134,7 @@ pub struct UefiRuntimeDeps<'a> {
     pub logger: Box<dyn UefiLogger>,
     pub vmtime: &'a VmTimeSource,
     pub watchdog_platform: Box<dyn WatchdogPlatform>,
+    pub watchdog_recv: mesh::Receiver<()>,
     pub generation_id_deps: generation_id::GenerationIdRuntimeDeps,
     pub vsm_config: Option<Box<dyn VsmConfig>>,
     pub time_source: Box<dyn InspectableLocalClock>,
@@ -158,6 +158,10 @@ pub struct UefiDevice {
     // Volatile state
     #[inspect(hex)]
     address: u32,
+
+    // Receiver for watchdog timeout events
+    #[inspect(skip)]
+    watchdog_recv: mesh::Receiver<()>,
 }
 
 impl UefiDevice {
@@ -171,26 +175,20 @@ impl UefiDevice {
             nvram_storage,
             logger,
             vmtime,
-            mut watchdog_platform,
+            watchdog_platform,
+            watchdog_recv,
             generation_id_deps,
             vsm_config,
             time_source,
         } = runtime_deps;
 
-        // Create diagnostics serparately since it will be shared.
-        let diagnostics = Arc::new(Mutex::new(service::diagnostics::DiagnosticsServices::new()));
-
-        // Add a watchdog callback to process diagnostics on timeout.
-        watchdog_platform.add_callback(Box::new(
-            service::diagnostics::DiagnosticsWatchdogCallback::new(diagnostics.clone(), gm.clone()),
-        ));
-
         // Create the UEFI device with the rest of the services.
         let uefi = UefiDevice {
             use_mmio: cfg.use_mmio,
             command_set: cfg.command_set,
             address: 0,
             gm,
+            watchdog_recv,
             service: UefiDeviceServices {
                 nvram: service::nvram::NvramServices::new(
                     nvram_storage,
@@ -212,7 +210,7 @@ impl UefiDevice {
                     generation_id_deps,
                 ),
                 time: service::time::TimeServices::new(time_source),
-                diagnostics,
+                diagnostics: service::diagnostics::DiagnosticsServices::new(),
             },
         };
 
@@ -269,15 +267,15 @@ impl UefiDevice {
             }
             UefiCommand::SET_EFI_DIAGNOSTICS_GPA => {
                 tracelimit::info_ratelimited!(?addr, data, "set gpa for diagnostics");
-                self.service.diagnostics.lock().set_gpa(data)
+                self.service.diagnostics.set_gpa(data)
             }
-            UefiCommand::PROCESS_EFI_DIAGNOSTICS => self.ratelimited_process_diagnostics(),
+            UefiCommand::PROCESS_EFI_DIAGNOSTICS => self.process_diagnostics(false, "guest"),
             _ => tracelimit::warn_ratelimited!(addr, data, "unknown uefi write"),
         }
     }
 
     fn inspect_extra(&mut self, _resp: &mut inspect::Response<'_>) {
-        self.force_process_diagnostics("inspect");
+        self.process_diagnostics(true, "inspect_extra");
     }
 }
 
@@ -293,7 +291,7 @@ impl ChangeDeviceState for UefiDevice {
         self.service.event_log.reset();
         self.service.uefi_watchdog.watchdog.reset();
         self.service.generation_id.reset();
-        self.service.diagnostics.lock().reset();
+        self.service.diagnostics.reset();
     }
 }
 
@@ -313,8 +311,15 @@ impl ChipsetDevice for UefiDevice {
 
 impl PollDevice for UefiDevice {
     fn poll_device(&mut self, cx: &mut Context<'_>) {
+        // Poll services
         self.service.uefi_watchdog.watchdog.poll(cx);
         self.service.generation_id.poll(cx);
+
+        // Poll watchdog timeout events
+        if let Poll::Ready(Ok(())) = self.watchdog_recv.poll_recv(cx) {
+            tracing::info!("watchdog timeout received");
+            self.process_diagnostics(true, "watchdog timeout");
+        }
     }
 }
 
@@ -500,6 +505,7 @@ mod save_restore {
                 use_mmio: _,
                 command_set: _,
                 gm: _,
+                watchdog_recv: _,
                 service:
                     UefiDeviceServices {
                         nvram,
@@ -520,7 +526,7 @@ mod save_restore {
                 watchdog: uefi_watchdog.save()?,
                 generation_id: generation_id.save()?,
                 time: time.save()?,
-                diagnostics: diagnostics.lock().save()?,
+                diagnostics: diagnostics.save()?,
             })
         }
 
@@ -543,7 +549,7 @@ mod save_restore {
             self.service.uefi_watchdog.restore(watchdog)?;
             self.service.generation_id.restore(generation_id)?;
             self.service.time.restore(time)?;
-            self.service.diagnostics.lock().restore(diagnostics)?;
+            self.service.diagnostics.restore(diagnostics)?;
 
             Ok(())
         }