From 48e64a205ff6a03ef6af22510959f33aea60057d Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Wed, 16 Oct 2024 17:39:08 +0000
Subject: [PATCH 01/11] lib: add facility to dump CPU regs on reset/triple
 fault

---
 bin/propolis-server/src/lib/vcpu_tasks.rs |  22 ++++
 lib/propolis/src/vcpu.rs                  | 134 ++++++++++++++++++++++
 2 files changed, 156 insertions(+)

diff --git a/bin/propolis-server/src/lib/vcpu_tasks.rs b/bin/propolis-server/src/lib/vcpu_tasks.rs
index be2d37f35..6a8be0545 100644
--- a/bin/propolis-server/src/lib/vcpu_tasks.rs
+++ b/bin/propolis-server/src/lib/vcpu_tasks.rs
@@ -159,14 +159,36 @@ impl VcpuTasks {
                         VmEntry::Run
                     }
                     VmExitKind::Suspended(SuspendDetail { kind, when }) => {
+                        use propolis::vcpu::Diagnostics;
                         match kind {
                             exits::Suspend::Halt => {
+                                slog::info!(
+                                    &log,
+                                    "halt event on vcpu {}",
+                                    vcpu.id;
+                                    "state" => %Diagnostics::capture(vcpu)
+                                );
+
                                 event_handler.suspend_halt_event(when);
                             }
                             exits::Suspend::Reset => {
+                                slog::info!(
+                                    &log,
+                                    "reset event on vcpu {}",
+                                    vcpu.id;
+                                    "state" => %Diagnostics::capture(vcpu)
+                                );
+
                                 event_handler.suspend_reset_event(when);
                             }
                             exits::Suspend::TripleFault(vcpuid) => {
+                                slog::info!(
+                                    &log,
+                                    "triple fault on vcpu {}",
+                                    vcpu.id;
+                                    "state" => %Diagnostics::capture(vcpu)
+                                );
+
                                 if vcpuid == -1 || vcpuid == vcpu.id {
                                     event_handler
                                         .suspend_triple_fault_event(vcpu.id, when);
diff --git a/lib/propolis/src/vcpu.rs b/lib/propolis/src/vcpu.rs
index 21fed44b5..885546fd1 100644
--- a/lib/propolis/src/vcpu.rs
+++ b/lib/propolis/src/vcpu.rs
@@ -1341,3 +1341,137 @@ mod bits {
     pub const MSR_DEBUGCTL: u32 = 0x1d9;
     pub const MSR_EFER: u32 = 0xc0000080;
 }
+
+/// Pretty-printable diagnostic information about the state of a vCPU.
+pub struct Diagnostics {
+    /// True if capturing vCPU diagnostics is disabled by the library's
+    /// build-time configuration. This is used to prevent vCPU data from
+    /// appearing in logs when building production Propolis binaries.
+    disabled_by_config: bool,
+    gp_regs: Result<migrate::VcpuGpRegsV1>,
+    seg_regs: Result<migrate::VcpuSegRegsV1>,
+    ctrl_regs: Result<migrate::VcpuCtrlRegsV1>,
+}
+
+impl Diagnostics {
+    #[cfg(not(feature = "omicron-build"))]
+    pub fn capture(vcpu: &Vcpu) -> Self {
+        Self {
+            disabled_by_config: false,
+            gp_regs: migrate::VcpuGpRegsV1::read(vcpu),
+            seg_regs: migrate::VcpuSegRegsV1::read(vcpu),
+            ctrl_regs: migrate::VcpuCtrlRegsV1::read(vcpu),
+        }
+    }
+
+    #[cfg(feature = "omicron-build")]
+    pub fn capture(_vcpu: &Vcpu) -> Self {
+        Self {
+            disabled_by_config: true,
+            gp_regs: Err(std::io::Error::from(std::io::ErrorKind::Other)),
+            seg_regs: Err(std::io::Error::from(std::io::ErrorKind::Other)),
+            ctrl_regs: Err(std::io::Error::from(std::io::ErrorKind::Other)),
+        }
+    }
+}
+
+impl std::fmt::Display for migrate::VcpuGpRegsV1 {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        writeln!(f, "%rax = {:#018x}\t%r9  = {:#018x}", self.rax, self.r9)?;
+        writeln!(f, "%rbx = {:#018x}\t%r10 = {:#018x}", self.rbx, self.r10)?;
+        writeln!(f, "%rcx = {:#018x}\t%r11 = {:#018x}", self.rcx, self.r11)?;
+        writeln!(f, "%rdx = {:#018x}\t%r12 = {:#018x}", self.rdx, self.r12)?;
+        writeln!(f, "%rsi = {:#018x}\t%r13 = {:#018x}", self.rsi, self.r13)?;
+        writeln!(f, "%rdi = {:#018x}\t%r14 = {:#018x}", self.rdi, self.r14)?;
+        writeln!(f, "%r8  = {:#018x}\t%r15 = {:#018x}", self.r8, self.r15)?;
+        writeln!(f)?;
+        writeln!(f, "%rip = {:#018x}", self.rip)?;
+        writeln!(f, "%rbp = {:#018x}", self.rbp)?;
+        writeln!(f, "%rsp = {:#018x}", self.rsp)?;
+        writeln!(f, "%rflags = {:#018x}", self.rflags)?;
+
+        Ok(())
+    }
+}
+
+impl std::fmt::Display for migrate::SegDesc {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "sel = {:#06x}\tbase = {:#018x}", self.selector, self.base)?;
+        write!(
+            f,
+            "\tlimit = {:#010x}\taccess = {:#010x}",
+            self.limit, self.access
+        )?;
+        Ok(())
+    }
+}
+
+impl std::fmt::Display for migrate::VcpuSegRegsV1 {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        writeln!(f, "%cs:   {}", self.cs)?;
+        writeln!(f, "%ds:   {}", self.ds)?;
+        writeln!(f, "%es:   {}", self.es)?;
+        writeln!(f, "%fs:   {}", self.fs)?;
+        writeln!(f, "%gs:   {}", self.gs)?;
+        writeln!(f, "%ss:   {}", self.ss)?;
+        writeln!(f, "%gdtr: {}", self.gdtr)?;
+        writeln!(f, "%idtr: {}", self.idtr)?;
+        writeln!(f, "%ldtr: {}", self.ldtr)?;
+        writeln!(f, "%tr:   {}", self.tr)?;
+        Ok(())
+    }
+}
+
+impl std::fmt::Display for migrate::VcpuCtrlRegsV1 {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        writeln!(f, "%cr0  = {:#018x}\t%cr2  = {:#018x}", self.cr0, self.cr2)?;
+        writeln!(f, "%cr3  = {:#018x}\t%cr4  = {:#018x}", self.cr3, self.cr4)?;
+        writeln!(
+            f,
+            "%xcr0 = {:#018x}\t%efer = {:#018x}",
+            self.xcr0, self.efer
+        )?;
+        Ok(())
+    }
+}
+
+impl std::fmt::Display for Diagnostics {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if self.disabled_by_config {
+            return writeln!(
+                f,
+                "vCPU diagnostics disabled by build configuration"
+            );
+        }
+
+        writeln!(f)?;
+        match &self.gp_regs {
+            Ok(regs) => {
+                writeln!(f, "{}", regs)?;
+            }
+            Err(e) => {
+                writeln!(f, "error reading general-purpose registers: {}", e)?;
+            }
+        }
+
+        match &self.seg_regs {
+            Ok(regs) => {
+                writeln!(f, "{}", regs)?;
+            }
+            Err(e) => {
+                writeln!(f, "error reading segment registers: {}", e)?;
+            }
+        }
+
+        match &self.ctrl_regs {
+            Ok(regs) => {
+                writeln!(f, "{}", regs)?;
+            }
+            Err(e) => {
+                writeln!(f, "error reading control registers: {}", e)?;
+            }
+        }
+
+        Ok(())
+    }
+}

From cd759ae6556d51cb6b357c98f6aea62b31149502 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Thu, 17 Oct 2024 00:23:43 +0000
Subject: [PATCH 02/11] server: add diagnostics for unhandled exits

---
 bin/propolis-server/src/lib/vcpu_tasks.rs | 65 ++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/bin/propolis-server/src/lib/vcpu_tasks.rs b/bin/propolis-server/src/lib/vcpu_tasks.rs
index 6a8be0545..7bf457a77 100644
--- a/bin/propolis-server/src/lib/vcpu_tasks.rs
+++ b/bin/propolis-server/src/lib/vcpu_tasks.rs
@@ -209,7 +209,70 @@ impl VcpuTasks {
                         task.force_hold();
                         VmEntry::Run
                     }
-                    _ => {
+                    VmExitKind::InstEmul(inst) => {
+                        // TODO(gjc) the context contains guest data!
+                        let diag = propolis::vcpu::Diagnostics::capture(vcpu);
+                        error!(log,
+                               "instruction emulation exit on vCPU {}",
+                               vcpu.id;
+                               "context" => ?inst,
+                               "vcpu_state" => %diag);
+
+                        event_handler.unhandled_vm_exit(vcpu.id, exit.kind);
+                        VmEntry::Run
+                    }
+                    VmExitKind::Unknown(code) => {
+                        error!(log,
+                               "unrecognized exit code on vCPU {}",
+                               vcpu.id;
+                               "code" => code);
+
+                        event_handler.unhandled_vm_exit(vcpu.id, exit.kind);
+                        VmEntry::Run
+                    }
+                    VmExitKind::Bogus => {
+                        error!(log,
+                               "lib returned bogus exit from vCPU {}",
+                               vcpu.id);
+
+                        event_handler.unhandled_vm_exit(vcpu.id, exit.kind);
+                        VmEntry::Run
+                    }
+                    VmExitKind::Debug => {
+                        error!(log,
+                               "lib returned debug exit from vCPU {}",
+                               vcpu.id);
+
+                        event_handler.unhandled_vm_exit(vcpu.id, exit.kind);
+                        VmEntry::Run
+                    }
+                    VmExitKind::VmxError(detail) => {
+                        error!(log,
+                               "unclassified VMX exit on vCPU {}",
+                               vcpu.id;
+                               "detail" => ?detail);
+
+                        event_handler.unhandled_vm_exit(vcpu.id, exit.kind);
+                        VmEntry::Run
+                    }
+                    VmExitKind::SvmError(detail) => {
+                        error!(log,
+                               "unclassified SVM exit on vCPU {}",
+                               vcpu.id;
+                               "detail" => ?detail);
+
+                        event_handler.unhandled_vm_exit(vcpu.id, exit.kind);
+                        VmEntry::Run
+                    }
+                    VmExitKind::Paging(gpa, fault_type) => {
+                        let diag = propolis::vcpu::Diagnostics::capture(vcpu);
+                        error!(log,
+                               "unhandled paging exit on vCPU {}",
+                               vcpu.id;
+                               "gpa" => gpa,
+                               "fault_type" => fault_type,
+                               "vcpu_state" => %diag);
+
                         event_handler.unhandled_vm_exit(vcpu.id, exit.kind);
                         VmEntry::Run
                     }

From f853b1db928d85076d0379cc46da25441ad7cf96 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Thu, 17 Oct 2024 20:49:34 +0000
Subject: [PATCH 03/11] don't dump instr bytes if diag is disabled

---
 lib/propolis/src/exits.rs | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/propolis/src/exits.rs b/lib/propolis/src/exits.rs
index 53df50e78..85dd578cb 100644
--- a/lib/propolis/src/exits.rs
+++ b/lib/propolis/src/exits.rs
@@ -93,11 +93,27 @@ impl From<&bhyve_api::vm_exit_vmx> for VmxDetail {
     }
 }
 
-#[derive(Copy, Clone, Debug)]
+#[derive(Copy, Clone)]
 pub struct InstEmul {
     pub inst_data: [u8; 15],
     pub len: u8,
 }
+
+impl std::fmt::Debug for InstEmul {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        #[cfg(feature = "omicron-build")]
+        let inst_data = "<redacted>";
+
+        #[cfg(not(feature = "omicron-build"))]
+        let inst_data = &self.inst_data;
+
+        f.debug_struct("InstEmul")
+            .field("inst_data", &inst_data)
+            .field("len", &self.len)
+            .finish()
+    }
+}
+
 impl InstEmul {
     pub fn bytes(&self) -> &[u8] {
         &self.inst_data[..usize::min(self.inst_data.len(), self.len as usize)]

From 9c0333db4213848a10c04afa5ae1f5998bf297a6 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Thu, 17 Oct 2024 21:10:18 +0000
Subject: [PATCH 04/11] put diagnostics behind a default feature

---
 .github/buildomat/jobs/image.sh           | 10 +++++++---
 bin/propolis-server/Cargo.toml            |  2 +-
 bin/propolis-server/src/lib/vcpu_tasks.rs |  1 -
 lib/propolis/Cargo.toml                   |  1 +
 lib/propolis/src/exits.rs                 |  6 +++---
 lib/propolis/src/vcpu.rs                  | 24 ++++++-----------------
 6 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/.github/buildomat/jobs/image.sh b/.github/buildomat/jobs/image.sh
index d66ee29f0..29dae44b2 100755
--- a/.github/buildomat/jobs/image.sh
+++ b/.github/buildomat/jobs/image.sh
@@ -28,12 +28,16 @@ rustc --version
 
 banner build
 
-# Enable the "omicron-build" feature to indicate this is an artifact destined
-# for production use on an appropriately configured Oxide machine
+# This job produces the propolis-server zone image that the Omicron repository
+# imports to create full-blown Oxide software packages. Disable all default
+# features and opt into the "omicron-build" feature instead to build libraries
+# in their "production" configurations.
 #
 # The 'release' profile is configured for abort-on-panic, so we get an
 # immediate coredump rather than unwinding in the case of an error.
-ptime -m cargo build --release --verbose -p propolis-server --features omicron-build
+ptime -m cargo build --release --verbose -p propolis-server \
+    --no-default-features \
+    --features omicron-build
 
 banner image
 ptime -m cargo run -p propolis-package
diff --git a/bin/propolis-server/Cargo.toml b/bin/propolis-server/Cargo.toml
index 5132955fd..70dae94c8 100644
--- a/bin/propolis-server/Cargo.toml
+++ b/bin/propolis-server/Cargo.toml
@@ -76,7 +76,7 @@ expectorate.workspace = true
 mockall.workspace = true
 
 [features]
-default = []
+default = ["propolis/dump-guest-state"]
 
 # When building to be packaged for inclusion in the production ramdisk
 # (nominally an Omicron package), certain code is compiled in or out.
diff --git a/bin/propolis-server/src/lib/vcpu_tasks.rs b/bin/propolis-server/src/lib/vcpu_tasks.rs
index 7bf457a77..10f704056 100644
--- a/bin/propolis-server/src/lib/vcpu_tasks.rs
+++ b/bin/propolis-server/src/lib/vcpu_tasks.rs
@@ -210,7 +210,6 @@ impl VcpuTasks {
                         VmEntry::Run
                     }
                     VmExitKind::InstEmul(inst) => {
-                        // TODO(gjc) the context contains guest data!
                         let diag = propolis::vcpu::Diagnostics::capture(vcpu);
                         error!(log,
                                "instruction emulation exit on vCPU {}",
diff --git a/lib/propolis/Cargo.toml b/lib/propolis/Cargo.toml
index dc403b885..2f0678bcc 100644
--- a/lib/propolis/Cargo.toml
+++ b/lib/propolis/Cargo.toml
@@ -56,6 +56,7 @@ rand.workspace = true
 [features]
 default = []
 crucible-full = ["crucible", "crucible-client-types", "oximeter", "nexus-client"]
+dump-guest-state = []
 falcon = ["libloading", "p9ds", "dlpi", "ispf", "rand", "softnpu", "viona_api/falcon"]
 
 # TODO until crucible#1280 is addressed, enabling Nexus notifications is done
diff --git a/lib/propolis/src/exits.rs b/lib/propolis/src/exits.rs
index 85dd578cb..90fecf7c1 100644
--- a/lib/propolis/src/exits.rs
+++ b/lib/propolis/src/exits.rs
@@ -101,10 +101,10 @@ pub struct InstEmul {
 
 impl std::fmt::Debug for InstEmul {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        #[cfg(feature = "omicron-build")]
-        let inst_data = "<redacted>";
+        #[cfg(not(feature = "dump-guest-state"))]
+        let inst_data = "<dump-guest-state disabled>";
 
-        #[cfg(not(feature = "omicron-build"))]
+        #[cfg(feature = "dump-guest-state")]
         let inst_data = &self.inst_data;
 
         f.debug_struct("InstEmul")
diff --git a/lib/propolis/src/vcpu.rs b/lib/propolis/src/vcpu.rs
index 885546fd1..d6ee537d7 100644
--- a/lib/propolis/src/vcpu.rs
+++ b/lib/propolis/src/vcpu.rs
@@ -1344,33 +1344,28 @@ mod bits {
 
 /// Pretty-printable diagnostic information about the state of a vCPU.
 pub struct Diagnostics {
-    /// True if capturing vCPU diagnostics is disabled by the library's
-    /// build-time configuration. This is used to prevent vCPU data from
-    /// appearing in logs when building production Propolis binaries.
-    disabled_by_config: bool,
     gp_regs: Result<migrate::VcpuGpRegsV1>,
     seg_regs: Result<migrate::VcpuSegRegsV1>,
     ctrl_regs: Result<migrate::VcpuCtrlRegsV1>,
 }
 
 impl Diagnostics {
-    #[cfg(not(feature = "omicron-build"))]
+    #[cfg(feature = "dump-guest-state")]
     pub fn capture(vcpu: &Vcpu) -> Self {
         Self {
-            disabled_by_config: false,
             gp_regs: migrate::VcpuGpRegsV1::read(vcpu),
             seg_regs: migrate::VcpuSegRegsV1::read(vcpu),
             ctrl_regs: migrate::VcpuCtrlRegsV1::read(vcpu),
         }
     }
 
-    #[cfg(feature = "omicron-build")]
+    #[cfg(not(feature = "dump-guest-state"))]
     pub fn capture(_vcpu: &Vcpu) -> Self {
+        let msg = "dump-guest-state feature disabled";
         Self {
-            disabled_by_config: true,
-            gp_regs: Err(std::io::Error::from(std::io::ErrorKind::Other)),
-            seg_regs: Err(std::io::Error::from(std::io::ErrorKind::Other)),
-            ctrl_regs: Err(std::io::Error::from(std::io::ErrorKind::Other)),
+            gp_regs: Err(std::io::Error::new(std::io::ErrorKind::Other, msg)),
+            seg_regs: Err(std::io::Error::new(std::io::ErrorKind::Other, msg)),
+            ctrl_regs: Err(std::io::Error::new(std::io::ErrorKind::Other, msg)),
         }
     }
 }
@@ -1437,13 +1432,6 @@ impl std::fmt::Display for migrate::VcpuCtrlRegsV1 {
 
 impl std::fmt::Display for Diagnostics {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        if self.disabled_by_config {
-            return writeln!(
-                f,
-                "vCPU diagnostics disabled by build configuration"
-            );
-        }
-
         writeln!(f)?;
         match &self.gp_regs {
             Ok(regs) => {

From 99e1785658ee608222623b4207bd962179e18bbe Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Thu, 17 Oct 2024 21:51:57 +0000
Subject: [PATCH 05/11] don't bother logging for halt/reset

---
 bin/propolis-server/src/lib/vcpu_tasks.rs | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/bin/propolis-server/src/lib/vcpu_tasks.rs b/bin/propolis-server/src/lib/vcpu_tasks.rs
index 10f704056..e5394b4eb 100644
--- a/bin/propolis-server/src/lib/vcpu_tasks.rs
+++ b/bin/propolis-server/src/lib/vcpu_tasks.rs
@@ -162,23 +162,9 @@ impl VcpuTasks {
                         use propolis::vcpu::Diagnostics;
                         match kind {
                             exits::Suspend::Halt => {
-                                slog::info!(
-                                    &log,
-                                    "halt event on vcpu {}",
-                                    vcpu.id;
-                                    "state" => %Diagnostics::capture(vcpu)
-                                );
-
                                 event_handler.suspend_halt_event(when);
                             }
                             exits::Suspend::Reset => {
-                                slog::info!(
-                                    &log,
-                                    "reset event on vcpu {}",
-                                    vcpu.id;
-                                    "state" => %Diagnostics::capture(vcpu)
-                                );
-
                                 event_handler.suspend_reset_event(when);
                             }
                             exits::Suspend::TripleFault(vcpuid) => {

From 3fb9025d98473186066e13b41465c0df3606e396 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Thu, 17 Oct 2024 22:13:22 +0000
Subject: [PATCH 06/11] opt standalone into dump-guest-state too

---
 bin/propolis-standalone/Cargo.toml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bin/propolis-standalone/Cargo.toml b/bin/propolis-standalone/Cargo.toml
index 8f131d618..d5e1a6d35 100644
--- a/bin/propolis-standalone/Cargo.toml
+++ b/bin/propolis-standalone/Cargo.toml
@@ -40,5 +40,5 @@ tar.workspace = true
 uuid.workspace = true
 
 [features]
-default = []
+default = ["propolis/dump-guest-state"]
 crucible = ["propolis/crucible-full", "propolis/oximeter", "crucible-client-types"]

From e4f891fa751b6d2ce4500b36b54355bc7266cdb9 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Thu, 17 Oct 2024 22:14:04 +0000
Subject: [PATCH 07/11] bogus exits should never be returned to the server

---
 bin/propolis-server/src/lib/vcpu_tasks.rs | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/bin/propolis-server/src/lib/vcpu_tasks.rs b/bin/propolis-server/src/lib/vcpu_tasks.rs
index e5394b4eb..e74074e3e 100644
--- a/bin/propolis-server/src/lib/vcpu_tasks.rs
+++ b/bin/propolis-server/src/lib/vcpu_tasks.rs
@@ -215,13 +215,20 @@ impl VcpuTasks {
                         event_handler.unhandled_vm_exit(vcpu.id, exit.kind);
                         VmEntry::Run
                     }
+                    // Bhyve emits the `Bogus` exit kind when there is no actual
+                    // guest exit for user space to handle, but circumstances
+                    // nevertheless dictate that the kernel VMM should exit to
+                    // user space (e.g. a caller requested that all vCPUs be
+                    // forced to exit to user space so their threads can
+                    // rendezvous there).
+                    //
+                    // `process_vmexit` should always successfully handle this
+                    // exit, since it never entails any work that could fail to
+                    // be completed.
                     VmExitKind::Bogus => {
-                        error!(log,
-                               "lib returned bogus exit from vCPU {}",
-                               vcpu.id);
-
-                        event_handler.unhandled_vm_exit(vcpu.id, exit.kind);
-                        VmEntry::Run
+                        unreachable!(
+                            "propolis-lib always handles VmExitKind::Bogus"
+                        );
                     }
                     VmExitKind::Debug => {
                         error!(log,

From 645545df7c77d364afc32a9129465d65bf4bf8bc Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Fri, 18 Oct 2024 21:28:44 +0000
Subject: [PATCH 08/11] make guest data display in logs nominally
 runtime-configurable

---
 .github/buildomat/jobs/image.sh     | 10 ++--
 bin/propolis-server/Cargo.toml      |  2 +-
 bin/propolis-server/src/main.rs     |  7 +++
 bin/propolis-standalone/Cargo.toml  |  2 +-
 bin/propolis-standalone/src/main.rs |  3 ++
 lib/propolis/Cargo.toml             |  1 -
 lib/propolis/src/common.rs          | 39 +++++++++++++++
 lib/propolis/src/exits.rs           | 31 ++++--------
 lib/propolis/src/vcpu.rs            | 75 ++++++++++++-----------------
 9 files changed, 95 insertions(+), 75 deletions(-)

diff --git a/.github/buildomat/jobs/image.sh b/.github/buildomat/jobs/image.sh
index 29dae44b2..d66ee29f0 100755
--- a/.github/buildomat/jobs/image.sh
+++ b/.github/buildomat/jobs/image.sh
@@ -28,16 +28,12 @@ rustc --version
 
 banner build
 
-# This job produces the propolis-server zone image that the Omicron repository
-# imports to create full-blown Oxide software packages. Disable all default
-# features and opt into the "omicron-build" feature instead to build libraries
-# in their "production" configurations.
+# Enable the "omicron-build" feature to indicate this is an artifact destined
+# for production use on an appropriately configured Oxide machine
 #
 # The 'release' profile is configured for abort-on-panic, so we get an
 # immediate coredump rather than unwinding in the case of an error.
-ptime -m cargo build --release --verbose -p propolis-server \
-    --no-default-features \
-    --features omicron-build
+ptime -m cargo build --release --verbose -p propolis-server --features omicron-build
 
 banner image
 ptime -m cargo run -p propolis-package
diff --git a/bin/propolis-server/Cargo.toml b/bin/propolis-server/Cargo.toml
index 70dae94c8..5132955fd 100644
--- a/bin/propolis-server/Cargo.toml
+++ b/bin/propolis-server/Cargo.toml
@@ -76,7 +76,7 @@ expectorate.workspace = true
 mockall.workspace = true
 
 [features]
-default = ["propolis/dump-guest-state"]
+default = []
 
 # When building to be packaged for inclusion in the production ramdisk
 # (nominally an Omicron package), certain code is compiled in or out.
diff --git a/bin/propolis-server/src/main.rs b/bin/propolis-server/src/main.rs
index 5b5054e92..b4127ec31 100644
--- a/bin/propolis-server/src/main.rs
+++ b/bin/propolis-server/src/main.rs
@@ -136,6 +136,13 @@ fn run_server(
         Err(e).context("API version checks")?;
     }
 
+    // If this is a development image being run outside of an Omicron zone,
+    // enable the display (in logs, panic messages, and the like) of diagnostic
+    // data that may have originated in the guest.
+    #[cfg(not(feature = "omicron-build"))]
+    propolis::common::DISPLAY_GUEST_DATA
+        .store(true, std::sync::atomic::Ordering::SeqCst);
+
     let use_reservoir = config::reservoir_decide(&log);
 
     let context = server::DropshotEndpointContext::new(
diff --git a/bin/propolis-standalone/Cargo.toml b/bin/propolis-standalone/Cargo.toml
index d5e1a6d35..8f131d618 100644
--- a/bin/propolis-standalone/Cargo.toml
+++ b/bin/propolis-standalone/Cargo.toml
@@ -40,5 +40,5 @@ tar.workspace = true
 uuid.workspace = true
 
 [features]
-default = ["propolis/dump-guest-state"]
+default = []
 crucible = ["propolis/crucible-full", "propolis/oximeter", "crucible-client-types"]
diff --git a/bin/propolis-standalone/src/main.rs b/bin/propolis-standalone/src/main.rs
index 10fa8968e..1a0b23b8f 100644
--- a/bin/propolis-standalone/src/main.rs
+++ b/bin/propolis-standalone/src/main.rs
@@ -1421,6 +1421,9 @@ fn main() -> anyhow::Result<ExitCode> {
     // Check that vmm and viona device version match what we expect
     api_version_checks(&log).context("API version checks")?;
 
+    propolis::common::DISPLAY_GUEST_DATA
+        .store(true, std::sync::atomic::Ordering::SeqCst);
+
     // Load/parse the config first, since it's required to size the tokio runtime
     // used to run the instance.
     let config = if restore {
diff --git a/lib/propolis/Cargo.toml b/lib/propolis/Cargo.toml
index 2f0678bcc..dc403b885 100644
--- a/lib/propolis/Cargo.toml
+++ b/lib/propolis/Cargo.toml
@@ -56,7 +56,6 @@ rand.workspace = true
 [features]
 default = []
 crucible-full = ["crucible", "crucible-client-types", "oximeter", "nexus-client"]
-dump-guest-state = []
 falcon = ["libloading", "p9ds", "dlpi", "ispf", "rand", "softnpu", "viona_api/falcon"]
 
 # TODO until crucible#1280 is addressed, enabling Nexus notifications is done
diff --git a/lib/propolis/src/common.rs b/lib/propolis/src/common.rs
index df22322c3..e0eb098c3 100644
--- a/lib/propolis/src/common.rs
+++ b/lib/propolis/src/common.rs
@@ -5,9 +5,48 @@
 use std::ops::{Add, BitAnd};
 use std::ops::{Bound::*, RangeBounds};
 use std::slice::SliceIndex;
+use std::sync::atomic::{AtomicBool, Ordering};
 
 use crate::vmm::SubMapping;
 
+/// Controls whether items wrapped in a [`GuestData`] are displayed or redacted
+/// when the wrappers are printed via their `Display` or `Debug` impls.
+pub static DISPLAY_GUEST_DATA: AtomicBool = AtomicBool::new(false);
+
+/// A wrapper type denoting that the contained `T` was obtained from the guest
+/// (e.g. by reading the guest's memory).
+///
+/// This type has custom `Display` and `Debug` impls that redact the wrapped `T`
+/// if [`DISPLAY_GUEST_DATA`] is `false`.
+///
+/// WARNING: This type only suppresses display of its contents. It does not do
+/// anything else to secure those contents: the inner `T` can be moved from the
+/// wrapper, the memory that held it is not zeroed when the wrapper is dropped,
+/// etc.
+#[derive(Clone, Copy)]
+#[repr(transparent)]
+pub struct GuestData<T>(pub T);
+
+impl<T: std::fmt::Display> std::fmt::Display for GuestData<T> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if DISPLAY_GUEST_DATA.load(Ordering::Relaxed) {
+            write!(f, "{}", self.0)
+        } else {
+            write!(f, "<guest data redacted>")
+        }
+    }
+}
+
+impl<T: std::fmt::Debug> std::fmt::Debug for GuestData<T> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if DISPLAY_GUEST_DATA.load(Ordering::Relaxed) {
+            write!(f, "{:?}", self.0)
+        } else {
+            write!(f, "<guest data redacted>")
+        }
+    }
+}
+
 fn numeric_bounds(
     bound: impl RangeBounds<usize>,
     len: usize,
diff --git a/lib/propolis/src/exits.rs b/lib/propolis/src/exits.rs
index 90fecf7c1..a112b8472 100644
--- a/lib/propolis/src/exits.rs
+++ b/lib/propolis/src/exits.rs
@@ -12,6 +12,8 @@ use bhyve_api::{
     vm_suspend_how,
 };
 
+use crate::common::GuestData;
+
 /// Describes the reason for exiting execution of a vCPU.
 pub struct VmExit {
     /// The instruction pointer of the guest at the time of exit.
@@ -93,37 +95,24 @@ impl From<&bhyve_api::vm_exit_vmx> for VmxDetail {
     }
 }
 
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, Debug)]
 pub struct InstEmul {
-    pub inst_data: [u8; 15],
+    pub inst_data: GuestData<[u8; 15]>,
     pub len: u8,
 }
 
-impl std::fmt::Debug for InstEmul {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        #[cfg(not(feature = "dump-guest-state"))]
-        let inst_data = "<dump-guest-state disabled>";
-
-        #[cfg(feature = "dump-guest-state")]
-        let inst_data = &self.inst_data;
-
-        f.debug_struct("InstEmul")
-            .field("inst_data", &inst_data)
-            .field("len", &self.len)
-            .finish()
-    }
-}
-
 impl InstEmul {
     pub fn bytes(&self) -> &[u8] {
-        &self.inst_data[..usize::min(self.inst_data.len(), self.len as usize)]
+        &self.inst_data.0
+            [..usize::min(self.inst_data.0.len(), self.len as usize)]
     }
 }
 impl From<&bhyve_api::vm_inst_emul> for InstEmul {
     fn from(raw: &bhyve_api::vm_inst_emul) -> Self {
-        let mut res = Self { inst_data: [0u8; 15], len: raw.num_valid };
-        assert!(res.len as usize <= res.inst_data.len());
-        res.inst_data.copy_from_slice(&raw.inst[..]);
+        let mut res =
+            Self { inst_data: GuestData([0u8; 15]), len: raw.num_valid };
+        assert!(res.len as usize <= res.inst_data.0.len());
+        res.inst_data.0.copy_from_slice(&raw.inst[..]);
 
         res
     }
diff --git a/lib/propolis/src/vcpu.rs b/lib/propolis/src/vcpu.rs
index d6ee537d7..9de940bc1 100644
--- a/lib/propolis/src/vcpu.rs
+++ b/lib/propolis/src/vcpu.rs
@@ -7,6 +7,7 @@
 use std::io::Result;
 use std::sync::Arc;
 
+use crate::common::GuestData;
 use crate::common::Lifecycle;
 use crate::cpuid;
 use crate::exits::*;
@@ -1344,28 +1345,17 @@ mod bits {
 
 /// Pretty-printable diagnostic information about the state of a vCPU.
 pub struct Diagnostics {
-    gp_regs: Result<migrate::VcpuGpRegsV1>,
-    seg_regs: Result<migrate::VcpuSegRegsV1>,
-    ctrl_regs: Result<migrate::VcpuCtrlRegsV1>,
+    gp_regs: Result<GuestData<migrate::VcpuGpRegsV1>>,
+    seg_regs: Result<GuestData<migrate::VcpuSegRegsV1>>,
+    ctrl_regs: Result<GuestData<migrate::VcpuCtrlRegsV1>>,
 }
 
 impl Diagnostics {
-    #[cfg(feature = "dump-guest-state")]
     pub fn capture(vcpu: &Vcpu) -> Self {
         Self {
-            gp_regs: migrate::VcpuGpRegsV1::read(vcpu),
-            seg_regs: migrate::VcpuSegRegsV1::read(vcpu),
-            ctrl_regs: migrate::VcpuCtrlRegsV1::read(vcpu),
-        }
-    }
-
-    #[cfg(not(feature = "dump-guest-state"))]
-    pub fn capture(_vcpu: &Vcpu) -> Self {
-        let msg = "dump-guest-state feature disabled";
-        Self {
-            gp_regs: Err(std::io::Error::new(std::io::ErrorKind::Other, msg)),
-            seg_regs: Err(std::io::Error::new(std::io::ErrorKind::Other, msg)),
-            ctrl_regs: Err(std::io::Error::new(std::io::ErrorKind::Other, msg)),
+            gp_regs: migrate::VcpuGpRegsV1::read(vcpu).map(GuestData),
+            seg_regs: migrate::VcpuSegRegsV1::read(vcpu).map(GuestData),
+            ctrl_regs: migrate::VcpuCtrlRegsV1::read(vcpu).map(GuestData),
         }
     }
 }
@@ -1433,33 +1423,30 @@ impl std::fmt::Display for migrate::VcpuCtrlRegsV1 {
 impl std::fmt::Display for Diagnostics {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         writeln!(f)?;
-        match &self.gp_regs {
-            Ok(regs) => {
-                writeln!(f, "{}", regs)?;
-            }
-            Err(e) => {
-                writeln!(f, "error reading general-purpose registers: {}", e)?;
-            }
-        }
-
-        match &self.seg_regs {
-            Ok(regs) => {
-                writeln!(f, "{}", regs)?;
-            }
-            Err(e) => {
-                writeln!(f, "error reading segment registers: {}", e)?;
-            }
-        }
-
-        match &self.ctrl_regs {
-            Ok(regs) => {
-                writeln!(f, "{}", regs)?;
-            }
-            Err(e) => {
-                writeln!(f, "error reading control registers: {}", e)?;
-            }
-        }
-
+        writeln!(
+            f,
+            "{}",
+            self.gp_regs.as_ref().map(|regs| regs.to_string()).unwrap_or_else(
+                |e| format!("error reading general-purpose registers: {e}")
+            )
+        )?;
+        writeln!(
+            f,
+            "{}",
+            self.seg_regs.as_ref().map(|regs| regs.to_string()).unwrap_or_else(
+                |e| format!("error reading segment registers: {e}")
+            )
+        )?;
+        writeln!(
+            f,
+            "{}",
+            self.ctrl_regs
+                .as_ref()
+                .map(|regs| regs.to_string())
+                .unwrap_or_else(|e| format!(
+                    "error reading control registers: {e}"
+                ))
+        )?;
         Ok(())
     }
 }

From 941b4cf13e1be24dcde300260c1924a4551077e5 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Fri, 18 Oct 2024 23:57:32 +0000
Subject: [PATCH 09/11] make MemCtx reads produce GuestData

---
 bin/propolis-server/src/lib/migrate/source.rs | 13 +++---
 lib/propolis/src/common.rs                    | 33 ++++++++++++---
 lib/propolis/src/exits.rs                     |  9 ++--
 lib/propolis/src/hw/nvme/cmds.rs              | 24 ++++++-----
 lib/propolis/src/hw/nvme/queue.rs             |  2 +-
 lib/propolis/src/hw/qemu/fwcfg.rs             | 12 +++---
 lib/propolis/src/hw/virtio/p9fs.rs            |  6 +--
 lib/propolis/src/hw/virtio/queue.rs           | 12 +++---
 lib/propolis/src/hw/virtio/softnpu.rs         |  4 +-
 lib/propolis/src/vcpu.rs                      |  6 +--
 lib/propolis/src/vmm/mem.rs                   | 41 ++++++++++++-------
 11 files changed, 101 insertions(+), 61 deletions(-)

diff --git a/bin/propolis-server/src/lib/migrate/source.rs b/bin/propolis-server/src/lib/migrate/source.rs
index 5abc41aa1..1b2900500 100644
--- a/bin/propolis-server/src/lib/migrate/source.rs
+++ b/bin/propolis-server/src/lib/migrate/source.rs
@@ -4,7 +4,7 @@
 
 use bitvec::prelude::{BitSlice, Lsb0};
 use futures::{SinkExt, StreamExt};
-use propolis::common::{GuestAddr, PAGE_SIZE};
+use propolis::common::{GuestAddr, GuestData, PAGE_SIZE};
 use propolis::migrate::{
     MigrateCtx, MigrateStateError, Migrator, PayloadOutputs,
 };
@@ -657,9 +657,12 @@ impl<'vm, T: MigrateConn> RonV0Runner<'vm, T> {
         info!(self.log(), "ram_push: xfer RAM between {start:#x} and {end:#x}",);
         self.send_msg(memx::make_mem_xfer(start, end, bits)).await?;
         for addr in PageIter::new(start, end, bits) {
-            let mut bytes = [0u8; PAGE_SIZE];
-            self.read_guest_mem(GuestAddr(addr), &mut bytes).await?;
-            self.send_msg(codec::Message::Page(bytes.into())).await?;
+            let mut byte_buffer = [0u8; PAGE_SIZE];
+            {
+                let mut bytes = GuestData::from(byte_buffer.as_mut_slice());
+                self.read_guest_mem(GuestAddr(addr), &mut bytes).await?;
+            }
+            self.send_msg(codec::Message::Page(byte_buffer.into())).await?;
             probes::migrate_xfer_ram_page!(|| (addr, PAGE_SIZE as u64));
         }
         Ok(())
@@ -919,7 +922,7 @@ impl<'vm, T: MigrateConn> RonV0Runner<'vm, T> {
     async fn read_guest_mem(
         &mut self,
         addr: GuestAddr,
-        buf: &mut [u8],
+        buf: &mut GuestData<&mut [u8]>,
     ) -> Result<(), MigrateError> {
         let objects = self.vm.lock_shared().await;
         let memctx = objects.access_mem().unwrap();
diff --git a/lib/propolis/src/common.rs b/lib/propolis/src/common.rs
index e0eb098c3..65028f864 100644
--- a/lib/propolis/src/common.rs
+++ b/lib/propolis/src/common.rs
@@ -18,14 +18,9 @@ pub static DISPLAY_GUEST_DATA: AtomicBool = AtomicBool::new(false);
 ///
 /// This type has custom `Display` and `Debug` impls that redact the wrapped `T`
 /// if [`DISPLAY_GUEST_DATA`] is `false`.
-///
-/// WARNING: This type only suppresses display of its contents. It does not do
-/// anything else to secure those contents: the inner `T` can be moved from the
-/// wrapper, the memory that held it is not zeroed when the wrapper is dropped,
-/// etc.
 #[derive(Clone, Copy)]
 #[repr(transparent)]
-pub struct GuestData<T>(pub T);
+pub struct GuestData<T: ?Sized>(T);
 
 impl<T: std::fmt::Display> std::fmt::Display for GuestData<T> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
@@ -47,6 +42,32 @@ impl<T: std::fmt::Debug> std::fmt::Debug for GuestData<T> {
     }
 }
 
+impl<T> std::ops::Deref for GuestData<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl<T> std::ops::DerefMut for GuestData<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
+impl<T> From<T> for GuestData<T> {
+    fn from(value: T) -> Self {
+        Self(value)
+    }
+}
+
+impl<T> std::borrow::Borrow<T> for GuestData<T> {
+    fn borrow(&self) -> &T {
+        &self.0
+    }
+}
+
 fn numeric_bounds(
     bound: impl RangeBounds<usize>,
     len: usize,
diff --git a/lib/propolis/src/exits.rs b/lib/propolis/src/exits.rs
index a112b8472..21da5ac4f 100644
--- a/lib/propolis/src/exits.rs
+++ b/lib/propolis/src/exits.rs
@@ -103,16 +103,15 @@ pub struct InstEmul {
 
 impl InstEmul {
     pub fn bytes(&self) -> &[u8] {
-        &self.inst_data.0
-            [..usize::min(self.inst_data.0.len(), self.len as usize)]
+        &self.inst_data[..usize::min(self.inst_data.len(), self.len as usize)]
     }
 }
 impl From<&bhyve_api::vm_inst_emul> for InstEmul {
     fn from(raw: &bhyve_api::vm_inst_emul) -> Self {
         let mut res =
-            Self { inst_data: GuestData([0u8; 15]), len: raw.num_valid };
-        assert!(res.len as usize <= res.inst_data.0.len());
-        res.inst_data.0.copy_from_slice(&raw.inst[..]);
+            Self { inst_data: GuestData::from([0u8; 15]), len: raw.num_valid };
+        assert!(res.len as usize <= res.inst_data.len());
+        res.inst_data.copy_from_slice(&raw.inst[..]);
 
         res
     }
diff --git a/lib/propolis/src/hw/nvme/cmds.rs b/lib/propolis/src/hw/nvme/cmds.rs
index 7b3ffd9db..c4b9bf23b 100644
--- a/lib/propolis/src/hw/nvme/cmds.rs
+++ b/lib/propolis/src/hw/nvme/cmds.rs
@@ -54,12 +54,14 @@ pub enum AdminCmd {
     /// Asynchronous Event Request Command
     AsyncEventReq,
     /// An unknown admin command
-    Unknown(#[allow(dead_code)] SubmissionQueueEntry),
+    Unknown(#[allow(dead_code)] GuestData<SubmissionQueueEntry>),
 }
 
 impl AdminCmd {
     /// Try to parse an `AdminCmd` out of a raw Submission Entry.
-    pub fn parse(raw: SubmissionQueueEntry) -> Result<Self, ParseErr> {
+    pub fn parse(
+        raw: GuestData<SubmissionQueueEntry>,
+    ) -> Result<Self, ParseErr> {
         let cmd = match raw.opcode() {
             bits::ADMIN_OPC_DELETE_IO_SQ => {
                 AdminCmd::DeleteIOSubQ(raw.cdw10 as u16)
@@ -645,12 +647,14 @@ pub enum NvmCmd {
     /// Read data and metadata
     Read(ReadCmd),
     /// An unknown NVM command
-    Unknown(SubmissionQueueEntry),
+    Unknown(GuestData<SubmissionQueueEntry>),
 }
 
 impl NvmCmd {
     /// Try to parse an `NvmCmd` out of a raw Submission Entry.
-    pub fn parse(raw: SubmissionQueueEntry) -> Result<Self, ParseErr> {
+    pub fn parse(
+        raw: GuestData<SubmissionQueueEntry>,
+    ) -> Result<Self, ParseErr> {
         let _fuse = match (raw.cdw0 >> 8) & 0b11 {
             0b00 => Ok(()),               // Normal (non-fused) operation
             0b01 => Err(ParseErr::Fused), // First fused op
@@ -882,7 +886,7 @@ impl PrpIter<'_> {
             PrpNext::List(base, idx) => {
                 assert!(idx <= PRP_LIST_MAX);
                 let entry_addr = base + u64::from(idx) * 8;
-                let entry: u64 = self
+                let entry: GuestData<u64> = self
                     .mem
                     .read(GuestAddr(entry_addr))
                     .ok_or_else(|| "Unable to read PRP list entry")?;
@@ -890,21 +894,21 @@ impl PrpIter<'_> {
                     || (self as *const Self as u64, entry,)
                 );
 
-                if entry & PAGE_OFFSET as u64 != 0 {
+                if *entry & PAGE_OFFSET as u64 != 0 {
                     return Err("Inappropriate PRP list entry offset");
                 }
 
                 if self.remain <= PAGE_SIZE as u64 {
-                    (entry, self.remain, PrpNext::Done)
+                    (*entry, self.remain, PrpNext::Done)
                 } else if idx != PRP_LIST_MAX {
-                    (entry, PAGE_SIZE as u64, PrpNext::List(base, idx + 1))
+                    (*entry, PAGE_SIZE as u64, PrpNext::List(base, idx + 1))
                 } else {
                     // The last PRP in this list chains to another
                     // (page-aligned) list with the next PRP.
-                    self.next = PrpNext::List(entry, 0);
+                    self.next = PrpNext::List(*entry, 0);
                     probes::nvme_prp_list!(|| (
                         self as *const Self as u64,
-                        entry,
+                        *entry,
                         0,
                     ));
                     return self.get_next();
diff --git a/lib/propolis/src/hw/nvme/queue.rs b/lib/propolis/src/hw/nvme/queue.rs
index eb06efd52..8cb9d74e8 100644
--- a/lib/propolis/src/hw/nvme/queue.rs
+++ b/lib/propolis/src/hw/nvme/queue.rs
@@ -429,7 +429,7 @@ impl SubQueue {
     pub fn pop(
         self: &Arc<SubQueue>,
         mem: &MemCtx,
-    ) -> Option<(SubmissionQueueEntry, Permit, u16)> {
+    ) -> Option<(GuestData<SubmissionQueueEntry>, Permit, u16)> {
         // Attempt to reserve an entry on the Completion Queue
         let permit = self.cq.reserve_entry(&self)?;
         let mut state = self.state.lock();
diff --git a/lib/propolis/src/hw/qemu/fwcfg.rs b/lib/propolis/src/hw/qemu/fwcfg.rs
index 2fb8b635f..9327cf811 100644
--- a/lib/propolis/src/hw/qemu/fwcfg.rs
+++ b/lib/propolis/src/hw/qemu/fwcfg.rs
@@ -599,7 +599,7 @@ impl FwCfg {
 
         let mem_guard = self.acc_mem.access().expect("usable mem accessor");
         let mem = mem_guard.deref();
-        let req: FwCfgDmaAccess =
+        let req: GuestData<FwCfgDmaAccess> =
             mem.read(GuestAddr(addr)).ok_or(FwCfgErr::BadAddr)?;
 
         fn dma_write_result(
@@ -632,7 +632,7 @@ impl FwCfg {
     fn dma_operation(
         &self,
         state: &mut MutexGuard<State>,
-        req: FwCfgDmaAccess,
+        req: GuestData<FwCfgDmaAccess>,
         mem: &MemCtx,
     ) -> Result<(), FwCfgErr> {
         let opts = FwCfgDmaCtrl::from_bits_truncate(req.ctrl.get());
@@ -995,9 +995,9 @@ mod test {
         submit_dma_req(&dev, req_addr);
 
         // DMA should have successfully completed now
-        assert_eq!(mem.read::<u32>(GuestAddr(req_addr)).unwrap(), 0);
+        assert_eq!(*mem.read::<u32>(GuestAddr(req_addr)).unwrap(), 0);
         let data = mem.read::<[u8; 4]>(GuestAddr(dma_addr)).unwrap();
-        assert_eq!(&data, "QEMU".as_bytes());
+        assert_eq!(&*data, "QEMU".as_bytes());
     }
 
     #[test]
@@ -1019,9 +1019,9 @@ mod test {
         submit_dma_req(&dev, req_addr);
 
         // DMA should have successfully completed now
-        assert_eq!(mem.read::<u32>(GuestAddr(req_addr)).unwrap(), 0);
+        assert_eq!(*mem.read::<u32>(GuestAddr(req_addr)).unwrap(), 0);
         let data = mem.read::<[u8; 4]>(GuestAddr(dma_addr)).unwrap();
-        assert_eq!(data, [0u8; 4]);
+        assert_eq!(*data, [0u8; 4]);
     }
 
     #[test]
diff --git a/lib/propolis/src/hw/virtio/p9fs.rs b/lib/propolis/src/hw/virtio/p9fs.rs
index 807f63fd3..8cfa8cf65 100644
--- a/lib/propolis/src/hw/virtio/p9fs.rs
+++ b/lib/propolis/src/hw/virtio/p9fs.rs
@@ -223,15 +223,15 @@ pub trait P9Handler: Sync + Send + 'static {
         let mut data = Vec::new();
         let msize = self.msize();
         data.resize(msize as usize, 0);
-        let buf = data.as_mut_slice();
+        let mut buf = GuestData::from(data.as_mut_slice());
 
         // TODO copy pasta from tail end of Chain::read function. Seemingly
         // cannot use Chain::read as-is because it expects a statically sized
         // type.
         let mut done = 0;
         let _total = chain.for_remaining_type(true, |addr, len| {
-            let remain = &mut buf[done..];
-            if let Some(copied) = mem.read_into(addr, remain, len) {
+            let mut remain = GuestData::from(&mut buf[done..]);
+            if let Some(copied) = mem.read_into(addr, &mut remain, len) {
                 let need_more = copied != remain.len();
                 done += copied;
                 (copied, need_more)
diff --git a/lib/propolis/src/hw/virtio/queue.rs b/lib/propolis/src/hw/virtio/queue.rs
index fdaa88d57..ef2ea78fb 100644
--- a/lib/propolis/src/hw/virtio/queue.rs
+++ b/lib/propolis/src/hw/virtio/queue.rs
@@ -59,7 +59,7 @@ impl VqAvail {
             return None;
         }
         if let Some(idx) = mem.read::<u16>(self.gpa_idx) {
-            let ndesc = Wrapping(idx) - self.cur_avail_idx;
+            let ndesc = Wrapping(*idx) - self.cur_avail_idx;
             if ndesc.0 != 0 && ndesc.0 < rsize {
                 let avail_idx = self.cur_avail_idx.0 & (rsize - 1);
                 self.cur_avail_idx += Wrapping(1);
@@ -68,7 +68,7 @@ impl VqAvail {
                 let addr = self.gpa_ring.offset::<u16>(avail_idx as usize);
                 return mem
                     .read(addr)
-                    .map(|desc_idx| VqReq { desc_idx, avail_idx });
+                    .map(|desc_idx| VqReq { desc_idx: *desc_idx, avail_idx });
             }
         }
         None
@@ -78,7 +78,7 @@ impl VqAvail {
         id: u16,
         rsize: u16,
         mem: &MemCtx,
-    ) -> Option<VqdDesc> {
+    ) -> Option<GuestData<VqdDesc>> {
         assert!(id < rsize);
         let addr = self.gpa_desc.offset::<VqdDesc>(id as usize);
         mem.read::<VqdDesc>(addr)
@@ -127,7 +127,7 @@ impl VqUsed {
         mem.write(self.gpa_idx, &self.used_idx.0);
     }
     fn intr_supressed(&self, mem: &MemCtx) -> bool {
-        let flags: u16 = mem.read(self.gpa_flags).unwrap();
+        let flags: u16 = *mem.read(self.gpa_flags).unwrap();
         flags & VRING_AVAIL_F_NO_INTERRUPT != 0
     }
     fn reset(&mut self) {
@@ -504,8 +504,8 @@ impl Chain {
         };
         let mut done = 0;
         let total = self.for_remaining_type(true, |addr, len| {
-            let remain = &mut raw[done..];
-            if let Some(copied) = mem.read_into(addr, remain, len) {
+            let mut remain = GuestData::from(&mut raw[done..]);
+            if let Some(copied) = mem.read_into(addr, &mut remain, len) {
                 let need_more = copied != remain.len();
 
                 done += copied;
diff --git a/lib/propolis/src/hw/virtio/softnpu.rs b/lib/propolis/src/hw/virtio/softnpu.rs
index cf1740fae..3c1152013 100644
--- a/lib/propolis/src/hw/virtio/softnpu.rs
+++ b/lib/propolis/src/hw/virtio/softnpu.rs
@@ -652,8 +652,8 @@ use bits::*;
 fn read_buf(mem: &MemCtx, chain: &mut Chain, buf: &mut [u8]) -> usize {
     let mut done = 0;
     chain.for_remaining_type(true, |addr, len| {
-        let remain = &mut buf[done..];
-        if let Some(copied) = mem.read_into(addr, remain, len) {
+        let mut remain = GuestData::from(&mut buf[done..]);
+        if let Some(copied) = mem.read_into(addr, &mut remain, len) {
             let need_more = copied != remain.len();
             done += copied;
             (copied, need_more)
diff --git a/lib/propolis/src/vcpu.rs b/lib/propolis/src/vcpu.rs
index 9de940bc1..5001952ef 100644
--- a/lib/propolis/src/vcpu.rs
+++ b/lib/propolis/src/vcpu.rs
@@ -1353,9 +1353,9 @@ pub struct Diagnostics {
 impl Diagnostics {
     pub fn capture(vcpu: &Vcpu) -> Self {
         Self {
-            gp_regs: migrate::VcpuGpRegsV1::read(vcpu).map(GuestData),
-            seg_regs: migrate::VcpuSegRegsV1::read(vcpu).map(GuestData),
-            ctrl_regs: migrate::VcpuCtrlRegsV1::read(vcpu).map(GuestData),
+            gp_regs: migrate::VcpuGpRegsV1::read(vcpu).map(GuestData::from),
+            seg_regs: migrate::VcpuSegRegsV1::read(vcpu).map(GuestData::from),
+            ctrl_regs: migrate::VcpuCtrlRegsV1::read(vcpu).map(GuestData::from),
         }
     }
 }
diff --git a/lib/propolis/src/vmm/mem.rs b/lib/propolis/src/vmm/mem.rs
index c54245e20..054f9e500 100644
--- a/lib/propolis/src/vmm/mem.rs
+++ b/lib/propolis/src/vmm/mem.rs
@@ -14,8 +14,7 @@ use std::sync::{Arc, Mutex};
 
 use libc::iovec;
 
-use crate::common::PAGE_SIZE;
-use crate::common::{GuestAddr, GuestRegion};
+use crate::common::{GuestAddr, GuestData, GuestRegion, PAGE_SIZE};
 use crate::util::aspace::ASpace;
 use crate::vmm::VmmHdl;
 
@@ -798,11 +797,14 @@ pub struct MemCtx {
 }
 impl MemCtx {
     /// Reads a generic value from a specified guest address.
-    pub fn read<T: Copy + FromBytes>(&self, addr: GuestAddr) -> Option<T> {
+    pub fn read<T: Copy + FromBytes>(
+        &self,
+        addr: GuestAddr,
+    ) -> Option<GuestData<T>> {
         if let Some(mapping) =
             self.region_covered(addr, size_of::<T>(), Prot::READ)
         {
-            mapping.read().ok()
+            mapping.read::<T>().ok().map(GuestData::from)
         } else {
             None
         }
@@ -813,7 +815,7 @@ impl MemCtx {
     pub fn read_into(
         &self,
         addr: GuestAddr,
-        buf: &mut [u8],
+        buf: &mut GuestData<&mut [u8]>,
         len: usize,
     ) -> Option<usize> {
         let len = usize::min(buf.len(), len);
@@ -828,7 +830,7 @@ impl MemCtx {
     pub fn direct_read_into(
         &self,
         addr: GuestAddr,
-        buf: &mut [u8],
+        buf: &mut GuestData<&mut [u8]>,
         len: usize,
     ) -> Option<usize> {
         let len = usize::min(buf.len(), len);
@@ -842,9 +844,16 @@ impl MemCtx {
         &self,
         base: GuestAddr,
         count: usize,
-    ) -> Option<MemMany<T>> {
+    ) -> Option<GuestData<MemMany<T>>> {
         self.region_covered(base, size_of::<T>() * count, Prot::READ).map(
-            |mapping| MemMany { mapping, pos: 0, count, phantom: PhantomData },
+            |mapping| {
+                GuestData::from(MemMany {
+                    mapping,
+                    pos: 0,
+                    count,
+                    phantom: PhantomData,
+                })
+            },
         )
     }
     /// Writes a value to guest memory.
@@ -1025,26 +1034,30 @@ pub struct MemMany<'a, T: Copy> {
     pos: usize,
     phantom: PhantomData<T>,
 }
-impl<'a, T: Copy + FromBytes> MemMany<'a, T> {
+impl<'a, T: Copy + FromBytes> GuestData<MemMany<'a, T>> {
     /// Gets the object at position `pos` within the memory region.
     ///
     /// Returns [`Option::None`] if out of range.
-    pub fn get(&self, pos: usize) -> Option<T> {
+    pub fn get(&self, pos: usize) -> Option<GuestData<T>> {
         if pos < self.count {
             let sz = std::mem::size_of::<T>();
-            self.mapping.subregion(pos * sz, sz)?.read().ok()
+            self.mapping
+                .subregion(pos * sz, sz)?
+                .read::<T>()
+                .ok()
+                .map(GuestData::from)
         } else {
             None
         }
     }
 }
-impl<'a, T: Copy + FromBytes> Iterator for MemMany<'a, T> {
-    type Item = T;
+impl<'a, T: Copy + FromBytes> Iterator for GuestData<MemMany<'a, T>> {
+    type Item = GuestData<T>;
 
     fn next(&mut self) -> Option<Self::Item> {
         let res = self.get(self.pos);
         self.pos += 1;
-        res
+        res.map(GuestData::from)
     }
 }
 

From bf130b893152c94e4fb8187774d0a4a03bd1e100 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Sat, 19 Oct 2024 03:36:50 +0000
Subject: [PATCH 10/11] add some comments

---
 lib/propolis/src/common.rs | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/lib/propolis/src/common.rs b/lib/propolis/src/common.rs
index 65028f864..210c06b05 100644
--- a/lib/propolis/src/common.rs
+++ b/lib/propolis/src/common.rs
@@ -11,13 +11,25 @@ use crate::vmm::SubMapping;
 
 /// Controls whether items wrapped in a [`GuestData`] are displayed or redacted
 /// when the wrappers are printed via their `Display` or `Debug` impls.
+#[no_mangle]
 pub static DISPLAY_GUEST_DATA: AtomicBool = AtomicBool::new(false);
 
 /// A wrapper type denoting that the contained `T` was obtained from the guest
-/// (e.g. by reading the guest's memory).
+/// (e.g. by reading the guest's memory). This type implements various traits
+/// (`Deref`, `DerefMut`, and `Borrow`) that allow it to be treated in most
+/// cases as just another instance of a `T`. The main difference is that this
+/// wrapper has custom `Display` and `Debug` implementations that redact the
+/// wrapped value unless the program has set the [`DISPLAY_GUEST_DATA`] flag.
 ///
-/// This type has custom `Display` and `Debug` impls that redact the wrapped `T`
-/// if [`DISPLAY_GUEST_DATA`] is `false`.
+/// NOTE: This wrapper type is not airtight: owners of a wrapper can always
+/// dereference it and invoke the Display/Debug impls directly on the resulting
+/// reference to the wrapped value. If `T` is `Clone`, they can also clone the
+/// dereferenced value and display the clone. (This comes with the territory
+/// here: users need to be able to get at the wrapped value to be able to do
+/// anything useful with it!)
+///
+/// NOTE: This type does not provide any other security guarantees (e.g. it does
+/// not ensure that the wrapped memory will be zeroed on drop).
 #[derive(Clone, Copy)]
 #[repr(transparent)]
 pub struct GuestData<T: ?Sized>(T);

From 7334b8d2593da4fa6e41123bf4076b00c2487034 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Mon, 21 Oct 2024 16:47:55 +0000
Subject: [PATCH 11/11] explain use of no_mangle

---
 lib/propolis/src/common.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/propolis/src/common.rs b/lib/propolis/src/common.rs
index 210c06b05..4256db730 100644
--- a/lib/propolis/src/common.rs
+++ b/lib/propolis/src/common.rs
@@ -11,6 +11,13 @@ use crate::vmm::SubMapping;
 
 /// Controls whether items wrapped in a [`GuestData`] are displayed or redacted
 /// when the wrappers are printed via their `Display` or `Debug` impls.
+//
+// The Propolis server binary should only link the Propolis lib once (any
+// structure that links the lib multiple times means something is very odd about
+// its dependency graph), so there should never be any ambiguity about what
+// `DISPLAY_GUEST_DATA` refers to when linking. But to be maximally cautious,
+// label this static as `no_mangle` so that pulling in multiple Propolis
+// libraries will break the build instead of possibly resolving ambiguously.
 #[no_mangle]
 pub static DISPLAY_GUEST_DATA: AtomicBool = AtomicBool::new(false);