feedback: flip builder direction

Author: sluck-msft

openhcl/underhill_mem/src/init.rs

@@ -6,7 +6,7 @@
 use crate::HardwareIsolatedMemoryProtector;
 use crate::MemoryAcceptor;
 use crate::mapping::GuestMemoryMapping;
-use crate::mapping::GuestPartitionMemoryBuilder;
+use crate::mapping::GuestPartitionMemoryView;
 use anyhow::Context;
 use futures::future::try_join_all;
 use guestmem::GuestMemory;
@@ -210,19 +210,17 @@ pub async fn init(params: &Init<'_>) -> anyhow::Result<MemoryMappings> {
         // kernel-registered RAM.
 
         tracing::debug!("Building valid encrypted memory view");
-        let encrypted_memory_builder = {
+        let encrypted_memory_view = {
             let _span = tracing::info_span!("create encrypted memory view").entered();
-            GuestPartitionMemoryBuilder::new(params.mem_layout, Some(true))?
+            GuestPartitionMemoryView::new(params.mem_layout, true)?
         };
 
         tracing::debug!("Building VTL0 memory map");
         let vtl0_mapping = Arc::new({
             let _span = tracing::info_span!("map_vtl0_memory").entered();
-            encrypted_memory_builder
-                .build_guest_memory_mapping(
-                    &gpa_fd,
-                    GuestMemoryMapping::builder(0).dma_base_address(None),
-                )
+            GuestMemoryMapping::builder(0)
+                .dma_base_address(None)
+                .build_with_bitmap(&gpa_fd, &encrypted_memory_view)
                 .context("failed to map vtl0 memory")?
         });
 
@@ -292,34 +290,28 @@ pub async fn init(params: &Init<'_>) -> anyhow::Result<MemoryMappings> {
 
         tracing::debug!("Building shared memory map");
 
-        let shared_memory_builder = {
+        let shared_memory_view = {
             let _span = tracing::info_span!("create shared memory view").entered();
-            GuestPartitionMemoryBuilder::new(params.complete_memory_layout, Some(false))?
+            GuestPartitionMemoryView::new(params.complete_memory_layout, false)?
         };
 
-        let valid_shared_memory = shared_memory_builder.partition_valid_memory();
+        let valid_shared_memory = shared_memory_view.partition_valid_memory();
 
         // Update the shared mapping bitmap for pages used by the shared
         // visibility pool to be marked as shared, since by default pages are
         // marked as no-access in the bitmap.
         tracing::debug!("Updating shared mapping bitmaps");
         for range in params.shared_pool {
-            valid_shared_memory
-                .as_ref()
-                .unwrap()
-                .update_valid(range.range, true);
+            valid_shared_memory.as_ref().update_valid(range.range, true);
         }
 
         let shared_mapping = Arc::new({
             let _span = tracing::info_span!("map_shared_memory").entered();
-            shared_memory_builder
-                .build_guest_memory_mapping(
-                    &gpa_fd,
-                    GuestMemoryMapping::builder(shared_offset)
-                        .shared(true)
-                        .ignore_registration_failure(params.boot_init.is_none())
-                        .dma_base_address(Some(dma_base_address)),
-                )
+            GuestMemoryMapping::builder(shared_offset)
+                .shared(true)
+                .ignore_registration_failure(params.boot_init.is_none())
+                .dma_base_address(Some(dma_base_address))
+                .build_with_bitmap(&gpa_fd, &shared_memory_view)
                 .context("failed to map shared memory")?
         });
 
@@ -373,11 +365,8 @@ pub async fn init(params: &Init<'_>) -> anyhow::Result<MemoryMappings> {
         let private_vtl0_memory = GuestMemory::new("trusted", vtl0_mapping.clone());
 
         let protector = Arc::new(HardwareIsolatedMemoryProtector::new(
-            encrypted_memory_builder
-                .partition_valid_memory()
-                .unwrap()
-                .clone(),
-            valid_shared_memory.unwrap().clone(),
+            encrypted_memory_view.partition_valid_memory().clone(),
+            valid_shared_memory.clone(),
             vtl0_mapping.clone(),
             params.mem_layout.clone(),
             acceptor.as_ref().unwrap().clone(),
@@ -396,22 +385,17 @@ pub async fn init(params: &Init<'_>) -> anyhow::Result<MemoryMappings> {
             }),
         }
     } else {
-        let partition_memory_builder = GuestPartitionMemoryBuilder::new(params.mem_layout, None)?;
-
         tracing::debug!("Creating VTL0 guest memory");
         let vtl0_mapping = {
             let _span = tracing::info_span!("map_vtl0_memory").entered();
             let base_address = params.vtl0_alias_map_bit.unwrap_or(0);
 
             Arc::new(
-                partition_memory_builder
-                    .build_guest_memory_mapping(
-                        &gpa_fd,
-                        GuestMemoryMapping::builder(base_address)
-                            .for_kernel_access(true)
-                            .dma_base_address(Some(base_address))
-                            .ignore_registration_failure(params.boot_init.is_none()),
-                    )
+                GuestMemoryMapping::builder(base_address)
+                    .for_kernel_access(true)
+                    .dma_base_address(Some(base_address))
+                    .ignore_registration_failure(params.boot_init.is_none())
+                    .build_without_bitmap(&gpa_fd, params.mem_layout)
                     .context("failed to map vtl0 memory")?,
             )
         };
@@ -442,14 +426,11 @@ pub async fn init(params: &Init<'_>) -> anyhow::Result<MemoryMappings> {
 
                 let _span = tracing::info_span!("map_vtl1_memory").entered();
                 Some(Arc::new(
-                    partition_memory_builder
-                        .build_guest_memory_mapping(
-                            &gpa_fd,
-                            GuestMemoryMapping::builder(0)
-                                .for_kernel_access(true)
-                                .dma_base_address(Some(0))
-                                .ignore_registration_failure(params.boot_init.is_none()),
-                        )
+                    GuestMemoryMapping::builder(0)
+                        .for_kernel_access(true)
+                        .dma_base_address(Some(0))
+                        .ignore_registration_failure(params.boot_init.is_none())
+                        .build_without_bitmap(&gpa_fd, params.mem_layout)
                         .context("failed to map vtl1 memory")?,
                 ))
             }

openhcl/underhill_mem/src/mapping.rs

@@ -21,45 +21,43 @@ use std::sync::Arc;
 use thiserror::Error;
 use vm_topology::memory::MemoryLayout;
 
-pub struct GuestPartitionMemoryBuilder<'a> {
+pub struct GuestPartitionMemoryView<'a> {
     memory_layout: &'a MemoryLayout,
-    valid_memory: Option<Arc<GuestValidMemory>>,
+    valid_memory: Arc<GuestValidMemory>,
 }
 
-impl<'a> GuestPartitionMemoryBuilder<'a> {
-    /// If `valid_bitmap_state` is `Some`, a bitmap is created to track the
-    /// accessibility state of each page in the lower VTL memory. The bitmap is
-    /// initialized to the provided state.
+impl<'a> GuestPartitionMemoryView<'a> {
+    /// A bitmap is created to track the accessibility state of each page in the
+    /// lower VTL memory. The bitmap is initialized to valid_bitmap_state.
     ///
     /// This is used to support tracking the shared/encrypted state of each
     /// page.
     pub fn new(
         memory_layout: &'a MemoryLayout,
-        valid_bitmap_state: Option<bool>,
+        valid_bitmap_state: bool,
     ) -> Result<Self, MappingError> {
-        let valid_memory = valid_bitmap_state
-            .map(|state| GuestValidMemory::new(memory_layout, state).map(Arc::new))
-            .transpose()?;
+        let valid_memory =
+            GuestValidMemory::new(memory_layout, valid_bitmap_state).map(Arc::new)?;
         Ok(Self {
             memory_layout,
             valid_memory,
         })
     }
 
     /// Returns the built partition-wide valid memory.
-    pub fn partition_valid_memory(&self) -> Option<Arc<GuestValidMemory>> {
+    pub fn partition_valid_memory(&self) -> Arc<GuestValidMemory> {
         self.valid_memory.clone()
     }
 
     /// Build a [`GuestMemoryMapping`], feeding in any related partition-wide
     /// state.
-    pub fn build_guest_memory_mapping(
+    fn build_guest_memory_mapping(
         &self,
         mshv_vtl_low: &MshvVtlLow,
         memory_mapping_builder: &mut GuestMemoryMappingBuilder,
     ) -> Result<GuestMemoryMapping, MappingError> {
         memory_mapping_builder
-            .use_partition_valid_memory(self.valid_memory.clone())
+            .use_partition_valid_memory(Some(self.valid_memory.clone()))
             .build(mshv_vtl_low, self.memory_layout)
     }
 }
@@ -307,6 +305,24 @@ impl GuestMemoryMappingBuilder {
         self
     }
 
+    /// Mapping should leverage the bitmap used to track the accessibility state
+    /// of each page in the lower VTL memory.
+    pub fn build_with_bitmap(
+        &mut self,
+        mshv_vtl_low: &MshvVtlLow,
+        partition_builder: &GuestPartitionMemoryView<'_>,
+    ) -> Result<GuestMemoryMapping, MappingError> {
+        partition_builder.build_guest_memory_mapping(mshv_vtl_low, self)
+    }
+
+    pub fn build_without_bitmap(
+        &self,
+        mshv_vtl_low: &MshvVtlLow,
+        memory_layout: &MemoryLayout,
+    ) -> Result<GuestMemoryMapping, MappingError> {
+        self.build(mshv_vtl_low, memory_layout)
+    }
+
     /// Map the lower VTL address space.
     ///
     /// If `is_shared`, then map the kernel mapping as shared memory.
@@ -318,10 +334,6 @@ impl GuestMemoryMappingBuilder {
     /// When handing out IOVAs for device DMA, add `iova_offset`. This can be
     /// VTOM for SNP-isolated VMs, or it can be the VTL0 alias map start for
     /// non-isolated VMs.
-    ///
-    /// If `bitmap_state` is `Some`, a bitmap is created to track the
-    /// accessibility state of each page in the lower VTL memory. The bitmap is
-    /// initialized to the provided state.
     fn build(
         &self,
         mshv_vtl_low: &MshvVtlLow,