hcl_compat_uefi_nvram_storage: load from storage if no saved state (#1697)

tjones60 · tjones60 · commit 0efdadf79c34 · 2025-07-17T16:32:55.000-07:00
Fix for a previous change that would have broken VMs that were serviced to a new version of OpenHCL with the UEFI NVRAM saved state. The new version would have skipped loading from storage since it was restoring, and the saved state would have contained an empty Vec. This change converts it to an Option<Vec<>> to distinguish between missing saved state and no NVRAM entries. If the saved state is missing, the HclCompatNvram will lazy load from storage like it did before. Original PR here: #1556 This is something that should have been caught by cross-version servicing tests, which don't exist currently. We should follow up to add that soon.
diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs
@@ -2074,7 +2074,6 @@ async fn new_underhill_vm(
                         Some(HclCompatNvramQuirks {
                             skip_corrupt_vars_with_missing_null_term: true,
                         }),
-                        is_restoring,
                     )
                     .await?,
                 ),
diff --git a/openvmm/hvlite_core/src/worker/dispatch.rs b/openvmm/hvlite_core/src/worker/dispatch.rs
@@ -1074,7 +1074,6 @@ impl InitializedVm {
                                             .context("failed to instantiate UEFI NVRAM store")?,
                                     ),
                                     None,
-                                    false,
                                 )
                                 .await?,
                             ),
diff --git a/vm/devices/firmware/hcl_compat_uefi_nvram_storage/src/lib.rs b/vm/devices/firmware/hcl_compat_uefi_nvram_storage/src/lib.rs
@@ -94,6 +94,9 @@ pub struct HclCompatNvram<S> {
     // reduced allocator pressure
     #[cfg_attr(feature = "inspect", inspect(skip))] // internal bookkeeping - not worth inspecting
     nvram_buf: Vec<u8>,
+
+    // whether the NVRAM has been loaded, either from storage or saved state
+    loaded: bool,
 }
 
 /// "Quirks" to take into account when loading/storing nvram blob data.
@@ -121,9 +124,8 @@ impl<S: StorageBackend> HclCompatNvram<S> {
     pub async fn new(
         storage: S,
         quirks: Option<HclCompatNvramQuirks>,
-        is_restoring: bool,
     ) -> Result<Self, NvramStorageError> {
-        let mut nvram = Self {
+        let nvram = Self {
             quirks: quirks.unwrap_or(HclCompatNvramQuirks {
                 skip_corrupt_vars_with_missing_null_term: false,
             }),
@@ -133,16 +135,14 @@ impl<S: StorageBackend> HclCompatNvram<S> {
             in_memory: in_memory::InMemoryNvram::new(),
 
             nvram_buf: Vec::new(),
+
+            loaded: false,
         };
-        if !is_restoring {
-            nvram.load_from_storage().await?;
-        }
         Ok(nvram)
     }
 
-    async fn load_from_storage(&mut self) -> Result<(), NvramStorageError> {
-        tracing::info!("loading uefi nvram from storage");
-        let res = self.load_from_storage_inner().await;
+    async fn lazy_load_from_storage(&mut self) -> Result<(), NvramStorageError> {
+        let res = self.lazy_load_from_storage_inner().await;
         if let Err(e) = &res {
             tracing::error!(CVM_ALLOWED, "storage contains corrupt nvram state");
             tracing::error!(
@@ -154,7 +154,13 @@ impl<S: StorageBackend> HclCompatNvram<S> {
         res
     }
 
-    async fn load_from_storage_inner(&mut self) -> Result<(), NvramStorageError> {
+    async fn lazy_load_from_storage_inner(&mut self) -> Result<(), NvramStorageError> {
+        if self.loaded {
+            return Ok(());
+        }
+
+        tracing::info!("loading uefi nvram from storage");
+
         let nvram_buf = self
             .storage
             .restore()
@@ -293,6 +299,7 @@ impl<S: StorageBackend> HclCompatNvram<S> {
             ));
         }
 
+        self.loaded = true;
         Ok(())
     }
 
@@ -343,8 +350,11 @@ impl<S: StorageBackend> HclCompatNvram<S> {
 
     /// Iterate over the NVRAM entries. This function asynchronously loads the
     /// NVRAM contents into memory from the backing storage if necessary.
-    pub fn iter(&mut self) -> impl Iterator<Item = in_memory::VariableEntry<'_>> {
-        self.in_memory.iter()
+    pub async fn iter(
+        &mut self,
+    ) -> Result<impl Iterator<Item = in_memory::VariableEntry<'_>>, NvramStorageError> {
+        self.lazy_load_from_storage().await?;
+        Ok(self.in_memory.iter())
     }
 }
 
@@ -355,6 +365,8 @@ impl<S: StorageBackend> NvramStorage for HclCompatNvram<S> {
         name: &Ucs2LeSlice,
         vendor: Guid,
     ) -> Result<Option<(u32, Vec<u8>, EFI_TIME)>, NvramStorageError> {
+        self.lazy_load_from_storage().await?;
+
         if name.as_bytes().len() > EFI_MAX_VARIABLE_NAME_SIZE {
             return Err(NvramStorageError::VariableNameTooLong);
         }
@@ -370,6 +382,8 @@ impl<S: StorageBackend> NvramStorage for HclCompatNvram<S> {
         data: Vec<u8>,
         timestamp: EFI_TIME,
     ) -> Result<(), NvramStorageError> {
+        self.lazy_load_from_storage().await?;
+
         if name.as_bytes().len() > EFI_MAX_VARIABLE_NAME_SIZE {
             return Err(NvramStorageError::VariableNameTooLong);
         }
@@ -404,14 +418,15 @@ impl<S: StorageBackend> NvramStorage for HclCompatNvram<S> {
 
         Ok(())
     }
-
     async fn append_variable(
         &mut self,
         name: &Ucs2LeSlice,
         vendor: Guid,
         data: Vec<u8>,
         timestamp: EFI_TIME,
     ) -> Result<bool, NvramStorageError> {
+        self.lazy_load_from_storage().await?;
+
         if name.as_bytes().len() > EFI_MAX_VARIABLE_NAME_SIZE {
             return Err(NvramStorageError::VariableNameTooLong);
         }
@@ -442,6 +457,8 @@ impl<S: StorageBackend> NvramStorage for HclCompatNvram<S> {
         name: &Ucs2LeSlice,
         vendor: Guid,
     ) -> Result<bool, NvramStorageError> {
+        self.lazy_load_from_storage().await?;
+
         if name.as_bytes().len() > EFI_MAX_VARIABLE_NAME_SIZE {
             return Err(NvramStorageError::VariableNameTooLong);
         }
@@ -456,6 +473,8 @@ impl<S: StorageBackend> NvramStorage for HclCompatNvram<S> {
         &mut self,
         name_vendor: Option<(&Ucs2LeSlice, Guid)>,
     ) -> Result<NextVariable, NvramStorageError> {
+        self.lazy_load_from_storage().await?;
+
         if let Some((name, _)) = name_vendor {
             if name.as_bytes().len() > EFI_MAX_VARIABLE_NAME_SIZE {
                 return Err(NvramStorageError::VariableNameTooLong);
@@ -481,7 +500,11 @@ mod save_restore {
         }
 
         fn restore(&mut self, state: Self::SavedState) -> Result<(), RestoreError> {
-            self.in_memory.restore(state)
+            if state.nvram.is_some() {
+                self.in_memory.restore(state)?;
+                self.loaded = true;
+            }
+            Ok(())
         }
     }
 }
@@ -516,36 +539,28 @@ mod test {
     #[async_test]
     async fn test_single_variable() {
         let mut storage = EphemeralStorageBackend::default();
-        let mut nvram = HclCompatNvram::new(&mut storage, None, false)
-            .await
-            .unwrap();
+        let mut nvram = HclCompatNvram::new(&mut storage, None).await.unwrap();
         impl_agnostic_tests::test_single_variable(&mut nvram).await;
     }
 
     #[async_test]
     async fn test_multiple_variable() {
         let mut storage = EphemeralStorageBackend::default();
-        let mut nvram = HclCompatNvram::new(&mut storage, None, false)
-            .await
-            .unwrap();
+        let mut nvram = HclCompatNvram::new(&mut storage, None).await.unwrap();
         impl_agnostic_tests::test_multiple_variable(&mut nvram).await;
     }
 
     #[async_test]
     async fn test_next() {
         let mut storage = EphemeralStorageBackend::default();
-        let mut nvram = HclCompatNvram::new(&mut storage, None, false)
-            .await
-            .unwrap();
+        let mut nvram = HclCompatNvram::new(&mut storage, None).await.unwrap();
         impl_agnostic_tests::test_next(&mut nvram).await;
     }
 
     #[async_test]
     async fn boundary_conditions() {
         let mut storage = EphemeralStorageBackend::default();
-        let mut nvram = HclCompatNvram::new(&mut storage, None, false)
-            .await
-            .unwrap();
+        let mut nvram = HclCompatNvram::new(&mut storage, None).await.unwrap();
 
         let vendor = Guid::new_random();
         let attr = 0x1234;
@@ -633,9 +648,7 @@ mod test {
         let data = vec![0x1, 0x2, 0x3, 0x4, 0x5];
         let timestamp = EFI_TIME::default();
 
-        let mut nvram = HclCompatNvram::new(&mut storage, None, false)
-            .await
-            .unwrap();
+        let mut nvram = HclCompatNvram::new(&mut storage, None).await.unwrap();
         nvram
             .set_variable(name1, vendor1, attr, data.clone(), timestamp)
             .await
@@ -652,9 +665,7 @@ mod test {
         drop(nvram);
 
         // reload
-        let mut nvram = HclCompatNvram::new(&mut storage, None, false)
-            .await
-            .unwrap();
+        let mut nvram = HclCompatNvram::new(&mut storage, None).await.unwrap();
 
         let (result_attr, result_data, result_timestamp) =
             nvram.get_variable(name1, vendor1).await.unwrap().unwrap();
diff --git a/vm/devices/firmware/uefi_nvram_storage/src/in_memory.rs b/vm/devices/firmware/uefi_nvram_storage/src/in_memory.rs
@@ -216,7 +216,7 @@ mod save_restore {
         #[mesh(package = "firmware.in_memory_nvram")]
         pub struct SavedState {
             #[mesh(1)]
-            pub nvram: Vec<NvramEntry>,
+            pub nvram: Option<Vec<NvramEntry>>,
         }
     }
 
@@ -225,39 +225,41 @@ mod save_restore {
 
         fn save(&mut self) -> Result<Self::SavedState, SaveError> {
             Ok(state::SavedState {
-                nvram: self
-                    .nvram
-                    .iter()
-                    .map(|(k, v)| state::NvramEntry {
-                        vendor: k.vendor,
-                        name: k.name.clone().into_inner(),
-                        data: v.data.clone(),
-                        timestamp: v.timestamp,
-                        attr: v.attr,
-                    })
-                    .collect(),
+                nvram: Some(
+                    self.nvram
+                        .iter()
+                        .map(|(k, v)| state::NvramEntry {
+                            vendor: k.vendor,
+                            name: k.name.clone().into_inner(),
+                            data: v.data.clone(),
+                            timestamp: v.timestamp,
+                            attr: v.attr,
+                        })
+                        .collect(),
+                ),
             })
         }
 
         fn restore(&mut self, state: Self::SavedState) -> Result<(), RestoreError> {
-            let state::SavedState { nvram } = state;
-            self.nvram = nvram
-                .into_iter()
-                .map::<Result<(VariableKey, Variable), RestoreError>, _>(|e| {
-                    Ok((
-                        VariableKey {
-                            vendor: e.vendor,
-                            name: Ucs2LeVec::from_vec_with_nul(e.name)
-                                .map_err(|e| RestoreError::InvalidSavedState(e.into()))?,
-                        },
-                        Variable {
-                            data: e.data,
-                            timestamp: e.timestamp,
-                            attr: e.attr,
-                        },
-                    ))
-                })
-                .collect::<Result<BTreeMap<_, _>, _>>()?;
+            if let state::SavedState { nvram: Some(nvram) } = state {
+                self.nvram = nvram
+                    .into_iter()
+                    .map::<Result<(VariableKey, Variable), RestoreError>, _>(|e| {
+                        Ok((
+                            VariableKey {
+                                vendor: e.vendor,
+                                name: Ucs2LeVec::from_vec_with_nul(e.name)
+                                    .map_err(|e| RestoreError::InvalidSavedState(e.into()))?,
+                            },
+                            Variable {
+                                data: e.data,
+                                timestamp: e.timestamp,
+                                attr: e.attr,
+                            },
+                        ))
+                    })
+                    .collect::<Result<BTreeMap<_, _>, _>>()?;
+            }
             Ok(())
         }
     }
diff --git a/vm/vmgs/vmgstool/src/uefi_nvram.rs b/vm/vmgs/vmgstool/src/uefi_nvram.rs
@@ -144,7 +144,7 @@ async fn dump_nvram(
     truncate: bool,
 ) -> Result<(), Error> {
     let mut count = 0;
-    for entry in nvram_storage.iter() {
+    for entry in nvram_storage.iter().await? {
         let meta = NvramEntryMetadata {
             vendor: entry.vendor.to_string(),
             name: entry.name.to_string(),
@@ -354,7 +354,6 @@ async fn open_nvram(
         VmgsStorageBackend::new(vmgs, vmgs::FileId::BIOS_NVRAM, encrypted)
             .map_err(Error::VmgsStorageBackend)?,
         None,
-        false,
     )
     .await?)
 }

Original file line number	Diff line number	Diff line change
`@@ -2074,7 +2074,6 @@ async fn new_underhill_vm(`
`2074`	`2074`	`Some(HclCompatNvramQuirks {`
`2075`	`2075`	`skip_corrupt_vars_with_missing_null_term: true,`
`2076`	`2076`	`}),`
`2077`		`- is_restoring,`
`2078`	`2077`	`)`
`2079`	`2078`	`.await?,`
`2080`	`2079`	`),`
Original file line number	Diff line number	Diff line change
`@@ -1074,7 +1074,6 @@ impl InitializedVm {`
`1074`	`1074`	`.context("failed to instantiate UEFI NVRAM store")?,`
`1075`	`1075`	`),`
`1076`	`1076`	`None,`
`1077`		`- false,`
`1078`	`1077`	`)`
`1079`	`1078`	`.await?,`
`1080`	`1079`	`),`