Merge "Handle ramdump in microdroid_manager" am: 14762fa3b9
Original change: https://android-review.googlesource.com/c/platform/packages/modules/Virtualization/+/2505576
Change-Id: If36c3f39bb21f3809251441e4a10866a3e385a22
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/microdroid_manager/src/main.rs b/microdroid_manager/src/main.rs
index ffa2e45..8732be1 100644
--- a/microdroid_manager/src/main.rs
+++ b/microdroid_manager/src/main.rs
@@ -55,8 +55,8 @@
use std::convert::TryInto;
use std::env;
use std::ffi::CString;
-use std::fs::{self, create_dir, OpenOptions};
-use std::io::Write;
+use std::fs::{self, create_dir, OpenOptions, File};
+use std::io::{Read, Write};
use std::os::unix::process::CommandExt;
use std::os::unix::process::ExitStatusExt;
use std::path::Path;
@@ -73,6 +73,7 @@
const DM_MOUNTED_APK_PATH: &str = "/dev/block/mapper/microdroid-apk";
const AVF_STRICT_BOOT: &str = "/sys/firmware/devicetree/base/chosen/avf,strict-boot";
const AVF_NEW_INSTANCE: &str = "/sys/firmware/devicetree/base/chosen/avf,new-instance";
+const AVF_DEBUG_POLICY_RAMDUMP: &str = "/sys/firmware/devicetree/base/avf/guest/common/ramdump";
const DEBUG_MICRODROID_NO_VERIFIED_BOOT: &str =
"/sys/firmware/devicetree/base/virtualization/guest/debug-microdroid,no-verified-boot";
@@ -314,6 +315,21 @@
}
}
+/// Get debug policy value in bool. It's true iff the value is explicitly set to <1>.
+fn get_debug_policy_bool(path: &'static str) -> Result<Option<bool>> {
+ let mut file = match File::open(path) {
+ Ok(dp) => dp,
+ Err(e) => {
+ info!("{e:?}. Assumes <0>");
+ return Ok(Some(false));
+ }
+ };
+ let mut log: [u8; 4] = Default::default();
+ file.read_exact(&mut log).context("Malformed data in {path}")?;
+ // DT spec uses big endian although Android is always little endian.
+ Ok(Some(u32::from_be_bytes(log) == 1))
+}
+
fn try_run_payload(service: &Strong<dyn IVirtualMachineService>) -> Result<i32> {
let metadata = load_metadata().context("Failed to load payload metadata")?;
let dice = DiceDriver::new(Path::new("/dev/open-dice0")).context("Failed to load DICE")?;
@@ -781,16 +797,27 @@
}
}
-/// Loads the crashkernel into memory using kexec if the VM is loaded with `crashkernel=' parameter
-/// in the cmdline.
+/// Loads the crashkernel into memory using kexec if debuggable or debug policy says so.
+/// The VM should be loaded with `crashkernel=' parameter in the cmdline to allocate memory
+/// for crashkernel.
fn load_crashkernel_if_supported() -> Result<()> {
let supported = std::fs::read_to_string("/proc/cmdline")?.contains(" crashkernel=");
info!("ramdump supported: {}", supported);
- if supported {
+
+ if !supported {
+ return Ok(());
+ }
+
+ let debuggable = system_properties::read_bool(DEBUGGABLE_PROP, true)?;
+ let ramdump = get_debug_policy_bool(AVF_DEBUG_POLICY_RAMDUMP)?.unwrap_or_default();
+ let requested = debuggable | ramdump;
+
+ if requested {
let status = Command::new("/system/bin/kexec_load").status()?;
if !status.success() {
return Err(anyhow!("Failed to load crashkernel: {:?}", status));
}
+ info!("ramdump is loaded: debuggable={debuggable}, ramdump={ramdump}");
}
Ok(())
}
diff --git a/pvmfw/src/debug_policy.rs b/pvmfw/src/debug_policy.rs
index 15efa1c..f4b99a6 100644
--- a/pvmfw/src/debug_policy.rs
+++ b/pvmfw/src/debug_policy.rs
@@ -15,7 +15,7 @@
//! Support for the debug policy overlay in pvmfw
use crate::cstr;
-use alloc::{vec, vec::Vec};
+use alloc::vec::Vec;
use core::ffi::CStr;
use core::fmt;
use libfdt::FdtError;
@@ -64,65 +64,6 @@
fdt.pack().map_err(|e| DebugPolicyError::OverlaidFdt("Failed to re-pack", e))
}
-/// Disables ramdump by removing crashkernel from bootargs in /chosen.
-fn disable_ramdump(fdt: &mut libfdt::Fdt) -> Result<(), DebugPolicyError> {
- let chosen = match fdt
- .node(cstr!("/chosen"))
- .map_err(|e| DebugPolicyError::Fdt("Failed to find /chosen", e))?
- {
- Some(node) => node,
- None => return Ok(()),
- };
-
- let bootargs = match chosen
- .getprop_str(cstr!("bootargs"))
- .map_err(|e| DebugPolicyError::Fdt("Failed to find bootargs prop", e))?
- {
- Some(value) if !value.to_bytes().is_empty() => value,
- _ => return Ok(()),
- };
-
- // TODO: Improve add 'crashkernel=17MB' only when it's unnecessary.
- // Currently 'crashkernel=17MB' in virtualizationservice and passed by
- // chosen node, because it's not exactly a debug policy but a
- // configuration. However, it's actually microdroid specific
- // so we need a way to generalize it.
- let mut args = vec![];
- for arg in bootargs.to_bytes().split(|byte| byte.is_ascii_whitespace()) {
- if arg.is_empty() || arg.starts_with(b"crashkernel=") {
- continue;
- }
- args.push(arg);
- }
- let mut new_bootargs = args.as_slice().join(&b" "[..]);
- new_bootargs.push(b'\0');
-
- // We've checked existence of /chosen node at the beginning.
- let mut chosen_mut = fdt.node_mut(cstr!("/chosen")).unwrap().unwrap();
- chosen_mut.setprop(cstr!("bootargs"), new_bootargs.as_slice()).map_err(|e| {
- DebugPolicyError::OverlaidFdt("Failed to remove crashkernel. FDT might be corrupted", e)
- })
-}
-
-/// Returns true only if fdt has ramdump prop in the /avf/guest/common node with value <1>
-fn is_ramdump_enabled(fdt: &libfdt::Fdt) -> Result<bool, DebugPolicyError> {
- let common = match fdt
- .node(cstr!("/avf/guest/common"))
- .map_err(|e| DebugPolicyError::DebugPolicyFdt("Failed to find /avf/guest/common node", e))?
- {
- Some(node) => node,
- None => return Ok(false),
- };
-
- match common
- .getprop_u32(cstr!("ramdump"))
- .map_err(|e| DebugPolicyError::DebugPolicyFdt("Failed to find ramdump prop", e))?
- {
- Some(1) => Ok(true),
- _ => Ok(false),
- }
-}
-
/// Enables console output by adding kernel.printk.devkmsg and kernel.console to bootargs.
/// This uses hardcoded console name 'hvc0' and it should be match with microdroid's bootconfig.debuggable.
fn enable_console_output(fdt: &mut libfdt::Fdt) -> Result<(), DebugPolicyError> {
@@ -191,13 +132,6 @@
apply_debug_policy(fdt, dp)?;
}
- // Handles ramdump in the debug policy
- if is_ramdump_enabled(fdt)? {
- info!("ramdump is enabled by debug policy");
- } else {
- disable_ramdump(fdt)?;
- }
-
// Handles console output in the debug policy
if is_console_output_enabled(fdt)? {
enable_console_output(fdt)?;
diff --git a/tests/hostside/Android.bp b/tests/hostside/Android.bp
index 6330b20..4b5cbda 100644
--- a/tests/hostside/Android.bp
+++ b/tests/hostside/Android.bp
@@ -9,20 +9,6 @@
}
genrule {
- name: "test_avf_debug_policy_with_ramdump",
- defaults: ["test_avf_debug_policy_overlay"],
- srcs: ["assets/avf_debug_policy_with_ramdump.dts"],
- out: ["avf_debug_policy_with_ramdump.dtbo"],
-}
-
-genrule {
- name: "test_avf_debug_policy_without_ramdump",
- defaults: ["test_avf_debug_policy_overlay"],
- srcs: ["assets/avf_debug_policy_without_ramdump.dts"],
- out: ["avf_debug_policy_without_ramdump.dtbo"],
-}
-
-genrule {
name: "test_avf_debug_policy_with_log.dtbo",
defaults: ["test_avf_debug_policy_overlay"],
srcs: ["assets/avf_debug_policy_with_log.dts"],
@@ -74,8 +60,6 @@
":test.com.android.virt.pem",
":test2.com.android.virt.pem",
":pvmfw_test",
- ":test_avf_debug_policy_with_ramdump",
- ":test_avf_debug_policy_without_ramdump",
":test_avf_debug_policy_with_log.dtbo",
":test_avf_debug_policy_without_log.dtbo",
":test_avf_debug_policy_with_adb",
diff --git a/tests/hostside/assets/avf_debug_policy_with_ramdump.dts b/tests/hostside/assets/avf_debug_policy_with_ramdump.dts
deleted file mode 100644
index 26db7be..0000000
--- a/tests/hostside/assets/avf_debug_policy_with_ramdump.dts
+++ /dev/null
@@ -1,21 +0,0 @@
-/dts-v1/;
-/plugin/;
-
-/ {
- fragment@avf {
- target-path = "/";
-
- __overlay__ {
- avf {
- guest {
- common {
- ramdump = <1>;
- };
- microdroid {
- adb = <1>; // adb is required to check VM's bootargs.
- };
- };
- };
- };
- };
-};
\ No newline at end of file
diff --git a/tests/hostside/assets/avf_debug_policy_without_ramdump.dts b/tests/hostside/assets/avf_debug_policy_without_ramdump.dts
deleted file mode 100644
index 194e314..0000000
--- a/tests/hostside/assets/avf_debug_policy_without_ramdump.dts
+++ /dev/null
@@ -1,21 +0,0 @@
-/dts-v1/;
-/plugin/;
-
-/ {
- fragment@avf {
- target-path = "/";
-
- __overlay__ {
- avf {
- guest {
- common {
- ramdump = <0>;
- };
- microdroid {
- adb = <1>; // adb is required to check VM's bootargs.
- };
- };
- };
- };
- };
-};
\ No newline at end of file
diff --git a/tests/hostside/java/com/android/microdroid/test/PvmfwDebugPolicyHostTests.java b/tests/hostside/java/com/android/microdroid/test/PvmfwDebugPolicyHostTests.java
index 0d64442..18aa273 100644
--- a/tests/hostside/java/com/android/microdroid/test/PvmfwDebugPolicyHostTests.java
+++ b/tests/hostside/java/com/android/microdroid/test/PvmfwDebugPolicyHostTests.java
@@ -151,33 +151,6 @@
}
@Test
- public void testRamdump() throws Exception {
- Pvmfw pvmfw = createPvmfw("avf_debug_policy_with_ramdump.dtbo");
- pvmfw.serialize(mCustomPvmfwBinFileOnHost);
- mMicrodroidDevice = launchProtectedVmAndWaitForBootCompleted(MICRODROID_DEBUG_FULL);
-
- assertThat(readMicrodroidFileAsString(MICRODROID_CMDLINE_PATH)).contains("crashkernel=");
- assertThat(readMicrodroidFileAsString(MICRODROID_DT_BOOTARGS_PATH))
- .contains("crashkernel=");
- assertThat(readMicrodroidFileAsHexString(MICRODROID_DT_RAMDUMP_PATH))
- .isEqualTo(HEX_STRING_ONE);
- }
-
- @Test
- public void testNoRamdump() throws Exception {
- Pvmfw pvmfw = createPvmfw("avf_debug_policy_without_ramdump.dtbo");
- pvmfw.serialize(mCustomPvmfwBinFileOnHost);
- mMicrodroidDevice = launchProtectedVmAndWaitForBootCompleted(MICRODROID_DEBUG_FULL);
-
- assertThat(readMicrodroidFileAsString(MICRODROID_CMDLINE_PATH))
- .doesNotContain("crashkernel=");
- assertThat(readMicrodroidFileAsString(MICRODROID_DT_BOOTARGS_PATH))
- .doesNotContain("crashkernel=");
- assertThat(readMicrodroidFileAsHexString(MICRODROID_DT_RAMDUMP_PATH))
- .isEqualTo(HEX_STRING_ZERO);
- }
-
- @Test
public void testLog_consoleOutput() throws Exception {
Pvmfw pvmfw = createPvmfw("avf_debug_policy_with_log.dtbo");
pvmfw.serialize(mCustomPvmfwBinFileOnHost);
diff --git a/virtualizationmanager/src/crosvm.rs b/virtualizationmanager/src/crosvm.rs
index 19f5f01..745d4f6 100644
--- a/virtualizationmanager/src/crosvm.rs
+++ b/virtualizationmanager/src/crosvm.rs
@@ -723,8 +723,7 @@
command.arg("--unmap-guest-memory-on-fork");
if config.ramdump.is_some() {
- // Protected VM needs to reserve memory for ramdump here. pvmfw will drop This
- // if ramdump should be disabled (via debug policy). Note that we reserve more
+ // Protected VM needs to reserve memory for ramdump here. Note that we reserve more
// memory for the restricted dma pool.
let ramdump_reserve = RAMDUMP_RESERVED_MIB + swiotlb_size_mib;
command.arg("--params").arg(format!("crashkernel={ramdump_reserve}M"));