Merge pull request #396 from msft-jlange/log_early

svsm: permit early initialization of console logging

kernel/src/console.rs

@@ -30,7 +30,9 @@ static CONSOLE_INITIALIZED: ImmutAfterInitCell<bool> = ImmutAfterInitCell::new(f
 
 pub fn init_console(writer: &'static dyn Terminal) -> ImmutAfterInitResult<()> {
     WRITER.lock().writer = writer;
-    CONSOLE_INITIALIZED.reinit(&true)
+    CONSOLE_INITIALIZED.reinit(&true)?;
+    log::info!("COCONUT Secure Virtual Machine Service Module");
+    Ok(())
 }
 
 #[doc(hidden)]

kernel/src/error.rs

@@ -73,6 +73,8 @@ pub enum SvsmError {
     InvalidBytes,
     /// Errors related to firmware parsing
     Firmware,
+    /// Errors related to console operation
+    Console,
     /// Errors related to firmware configuration contents
     FwCfg(FwCfgError),
     /// Errors related to ACPI parsing.

kernel/src/platform/mod.rs

@@ -44,11 +44,11 @@ pub enum PageStateChangeOp {
 /// underlying architectures.
 pub trait SvsmPlatform {
     /// Performs basic early initialization of the runtime environment.
-    fn env_setup(&mut self);
+    fn env_setup(&mut self, debug_serial_port: u16) -> Result<(), SvsmError>;
 
     /// Performs initialization of the platform runtime environment after
-    /// console logging has been initialized.
-    fn env_setup_late(&mut self);
+    /// the core system environment has been initialized.
+    fn env_setup_late(&mut self, debug_serial_port: u16) -> Result<(), SvsmError>;
 
     /// Completes initialization of a per-CPU object during construction.
     fn setup_percpu(&self, cpu: &PerCpu) -> Result<(), SvsmError>;
@@ -62,8 +62,9 @@ pub trait SvsmPlatform {
     /// Establishes state required for guest/host communication.
     fn setup_guest_host_comm(&mut self, cpu: &PerCpu, is_bsp: bool);
 
-    /// Obtains a console I/O port reference.
-    fn get_console_io_port(&self) -> &'static dyn IOPort;
+    /// Obtains a reference to an I/O port implemetation appropriate to the
+    /// platform.
+    fn get_io_port(&self) -> &'static dyn IOPort;
 
     /// Performs a page state change between private and shared states.
     fn page_state_change(

kernel/src/platform/native.rs

@@ -5,16 +5,21 @@
 // Author: Jon Lange <[email protected]>
 
 use crate::address::{PhysAddr, VirtAddr};
+use crate::console::init_console;
 use crate::cpu::cpuid::CpuidResult;
 use crate::cpu::msr::write_msr;
 use crate::cpu::percpu::PerCpu;
 use crate::error::SvsmError;
-use crate::platform::{IOPort, PageEncryptionMasks, PageStateChangeOp, SvsmPlatform};
+use crate::io::IOPort;
+use crate::platform::{PageEncryptionMasks, PageStateChangeOp, SvsmPlatform};
+use crate::serial::SerialPort;
 use crate::svsm_console::NativeIOPort;
 use crate::types::PageSize;
+use crate::utils::immut_after_init::ImmutAfterInitCell;
 use crate::utils::MemoryRegion;
 
 static CONSOLE_IO: NativeIOPort = NativeIOPort::new();
+static CONSOLE_SERIAL: ImmutAfterInitCell<SerialPort<'_>> = ImmutAfterInitCell::uninit();
 
 const APIC_MSR_ICR: u32 = 0x830;
 
@@ -34,8 +39,19 @@ impl Default for NativePlatform {
 }
 
 impl SvsmPlatform for NativePlatform {
-    fn env_setup(&mut self) {}
-    fn env_setup_late(&mut self) {}
+    fn env_setup(&mut self, debug_serial_port: u16) -> Result<(), SvsmError> {
+        // In the native platform, console output does not require the use of
+        // any platform services, so it can be initialized immediately.
+        CONSOLE_SERIAL
+            .init(&SerialPort::new(&CONSOLE_IO, debug_serial_port))
+            .map_err(|_| SvsmError::Console)?;
+        (*CONSOLE_SERIAL).init();
+        init_console(&*CONSOLE_SERIAL).map_err(|_| SvsmError::Console)
+    }
+
+    fn env_setup_late(&mut self, _debug_serial_port: u16) -> Result<(), SvsmError> {
+        Ok(())
+    }
 
     fn setup_percpu(&self, _cpu: &PerCpu) -> Result<(), SvsmError> {
         Ok(())
@@ -58,7 +74,7 @@ impl SvsmPlatform for NativePlatform {
 
     fn setup_guest_host_comm(&mut self, _cpu: &PerCpu, _is_bsp: bool) {}
 
-    fn get_console_io_port(&self) -> &'static dyn IOPort {
+    fn get_io_port(&self) -> &'static dyn IOPort {
         &CONSOLE_IO
     }
 

kernel/src/platform/snp.rs

@@ -5,12 +5,14 @@
 // Author: Jon Lange <[email protected]>
 
 use crate::address::{PhysAddr, VirtAddr};
+use crate::console::init_console;
 use crate::cpu::cpuid::cpuid_table;
 use crate::cpu::percpu::{current_ghcb, PerCpu};
 use crate::error::ApicError::Registration;
 use crate::error::SvsmError;
 use crate::io::IOPort;
 use crate::platform::{PageEncryptionMasks, PageStateChangeOp, SvsmPlatform};
+use crate::serial::SerialPort;
 use crate::sev::hv_doorbell::current_hv_doorbell;
 use crate::sev::msr_protocol::{hypervisor_ghcb_features, verify_ghcb_version, GHCBHvFeatures};
 use crate::sev::status::vtom_enabled;
@@ -19,11 +21,13 @@ use crate::sev::{
 };
 use crate::svsm_console::SVSMIOPort;
 use crate::types::PageSize;
+use crate::utils::immut_after_init::ImmutAfterInitCell;
 use crate::utils::MemoryRegion;
 
 use core::sync::atomic::{AtomicU32, Ordering};
 
 static CONSOLE_IO: SVSMIOPort = SVSMIOPort::new();
+static CONSOLE_SERIAL: ImmutAfterInitCell<SerialPort<'_>> = ImmutAfterInitCell::uninit();
 
 static APIC_EMULATION_REG_COUNT: AtomicU32 = AtomicU32::new(0);
 
@@ -43,13 +47,20 @@ impl Default for SnpPlatform {
 }
 
 impl SvsmPlatform for SnpPlatform {
-    fn env_setup(&mut self) {
+    fn env_setup(&mut self, _debug_serial_port: u16) -> Result<(), SvsmError> {
         sev_status_init();
+        Ok(())
     }
 
-    fn env_setup_late(&mut self) {
+    fn env_setup_late(&mut self, debug_serial_port: u16) -> Result<(), SvsmError> {
+        CONSOLE_SERIAL
+            .init(&SerialPort::new(&CONSOLE_IO, debug_serial_port))
+            .map_err(|_| SvsmError::Console)?;
+        (*CONSOLE_SERIAL).init();
+        init_console(&*CONSOLE_SERIAL).map_err(|_| SvsmError::Console)?;
         sev_status_verify();
-        init_hypervisor_ghcb_features().expect("Failed to obtain hypervisor GHCB features");
+        init_hypervisor_ghcb_features()?;
+        Ok(())
     }
 
     fn setup_percpu(&self, cpu: &PerCpu) -> Result<(), SvsmError> {
@@ -102,7 +113,7 @@ impl SvsmPlatform for SnpPlatform {
         cpu.register_ghcb().expect("Failed to register GHCB");
     }
 
-    fn get_console_io_port(&self) -> &'static dyn IOPort {
+    fn get_io_port(&self) -> &'static dyn IOPort {
         &CONSOLE_IO
     }
 

kernel/src/platform/tdp.rs

@@ -5,16 +5,22 @@
 // Author: Peter Fang <[email protected]>
 
 use crate::address::{PhysAddr, VirtAddr};
+use crate::console::init_console;
 use crate::cpu::cpuid::CpuidResult;
 use crate::cpu::percpu::PerCpu;
 use crate::error::SvsmError;
 use crate::io::IOPort;
 use crate::platform::{PageEncryptionMasks, PageStateChangeOp, SvsmPlatform};
+use crate::serial::SerialPort;
 use crate::svsm_console::SVSMIOPort;
 use crate::types::PageSize;
+use crate::utils::immut_after_init::ImmutAfterInitCell;
 use crate::utils::MemoryRegion;
 
+// FIXME - SVSMIOPort doesn't work on TDP, but the platform does not yet have
+// an alternative available.
 static CONSOLE_IO: SVSMIOPort = SVSMIOPort::new();
+static CONSOLE_SERIAL: ImmutAfterInitCell<SerialPort<'_>> = ImmutAfterInitCell::uninit();
 
 #[derive(Clone, Copy, Debug)]
 pub struct TdpPlatform {}
@@ -32,9 +38,17 @@ impl Default for TdpPlatform {
 }
 
 impl SvsmPlatform for TdpPlatform {
-    fn env_setup(&mut self) {}
+    fn env_setup(&mut self, _debug_serial_port: u16) -> Result<(), SvsmError> {
+        Ok(())
+    }
 
-    fn env_setup_late(&mut self) {}
+    fn env_setup_late(&mut self, debug_serial_port: u16) -> Result<(), SvsmError> {
+        CONSOLE_SERIAL
+            .init(&SerialPort::new(&CONSOLE_IO, debug_serial_port))
+            .map_err(|_| SvsmError::Console)?;
+        (*CONSOLE_SERIAL).init();
+        init_console(&*CONSOLE_SERIAL).map_err(|_| SvsmError::Console)
+    }
 
     fn setup_percpu(&self, _cpu: &PerCpu) -> Result<(), SvsmError> {
         Err(SvsmError::Tdx)
@@ -57,7 +71,7 @@ impl SvsmPlatform for TdpPlatform {
 
     fn setup_guest_host_comm(&mut self, _cpu: &PerCpu, _is_bsp: bool) {}
 
-    fn get_console_io_port(&self) -> &'static dyn IOPort {
+    fn get_io_port(&self) -> &'static dyn IOPort {
         &CONSOLE_IO
     }
 

kernel/src/stage2.rs

@@ -19,7 +19,7 @@ use cpuarch::snp_cpuid::SnpCpuidTable;
 use elf::ElfError;
 use svsm::address::{Address, PhysAddr, VirtAddr};
 use svsm::config::SvsmConfig;
-use svsm::console::{init_console, install_console_logger};
+use svsm::console::install_console_logger;
 use svsm::cpu::cpuid::{dump_cpuid_table, register_cpuid_table};
 use svsm::cpu::gdt;
 use svsm::cpu::idt::stage2::{early_idt_init, early_idt_init_no_ghcb};
@@ -37,9 +37,7 @@ use svsm::mm::validate::{
     init_valid_bitmap_alloc, valid_bitmap_addr, valid_bitmap_set_valid_range,
 };
 use svsm::platform::{PageStateChangeOp, SvsmPlatform, SvsmPlatformCell};
-use svsm::serial::SerialPort;
 use svsm::types::{PageSize, PAGE_SIZE, PAGE_SIZE_2M};
-use svsm::utils::immut_after_init::ImmutAfterInitCell;
 use svsm::utils::{halt, is_aligned, MemoryRegion};
 
 extern "C" {
@@ -74,18 +72,20 @@ fn shutdown_percpu() {
         .expect("Failed to shut down percpu data (including GHCB)");
 }
 
-static CONSOLE_SERIAL: ImmutAfterInitCell<SerialPort<'_>> = ImmutAfterInitCell::uninit();
-
 fn setup_env(
     config: &SvsmConfig<'_>,
     platform: &mut dyn SvsmPlatform,
     launch_info: &Stage2LaunchInfo,
 ) {
     gdt().load();
     early_idt_init_no_ghcb();
-    platform.env_setup();
 
+    let debug_serial_port = config.debug_serial_port();
     install_console_logger("Stage2").expect("Console logger already initialized");
+    platform
+        .env_setup(debug_serial_port)
+        .expect("Early environment setup failed");
+
     init_kernel_mapping_info(
         VirtAddr::null(),
         VirtAddr::from(640 * 1024usize),
@@ -101,19 +101,14 @@ fn setup_env(
     // Init IDT again with handlers requiring GHCB (eg. #VC handler)
     early_idt_init();
 
-    CONSOLE_SERIAL
-        .init(&SerialPort::new(
-            platform.get_console_io_port(),
-            config.debug_serial_port(),
-        ))
-        .expect("console serial output already configured");
-    (*CONSOLE_SERIAL).init();
-    init_console(&*CONSOLE_SERIAL).expect("Console writer already initialized");
-
-    // Console is fully working now and any unsupported configuration can be
-    // properly reported.
+    // Complete initializtion of the platform.  After that point, the console
+    // will be fully working and any unsupported configuration can be properly
+    // reported.
+    platform
+        .env_setup_late(debug_serial_port)
+        .expect("Late environment setup failed");
+
     dump_cpuid_table();
-    platform.env_setup_late();
 }
 
 /// Map and validate the specified virtual memory region at the given physical
@@ -158,7 +153,7 @@ fn get_svsm_config(
 ) -> Result<SvsmConfig<'static>, SvsmError> {
     if launch_info.igvm_params == 0 {
         return Ok(SvsmConfig::FirmwareConfig(FwCfg::new(
-            platform.get_console_io_port(),
+            platform.get_io_port(),
         )));
     }
 
@@ -335,8 +330,6 @@ pub extern "C" fn stage2_main(launch_info: &Stage2LaunchInfo) {
     let config = get_svsm_config(launch_info, platform).expect("Failed to get SVSM configuration");
     setup_env(&config, platform, launch_info);
 
-    log::info!("COCONUT Secure Virtual Machine Service Module (SVSM) Stage 2 Loader");
-
     // Get the available physical memory region for the kernel
     let kernel_region = config
         .find_kernel_region()

kernel/src/svsm.rs

@@ -18,7 +18,7 @@ use core::slice;
 use cpuarch::snp_cpuid::SnpCpuidTable;
 use svsm::address::{PhysAddr, VirtAddr};
 use svsm::config::SvsmConfig;
-use svsm::console::{init_console, install_console_logger};
+use svsm::console::install_console_logger;
 use svsm::cpu::control_regs::{cr0_init, cr4_init};
 use svsm::cpu::cpuid::{dump_cpuid_table, register_cpuid_table};
 use svsm::cpu::efer::efer_init;
@@ -43,10 +43,8 @@ use svsm::mm::virtualrange::virt_log_usage;
 use svsm::mm::{init_kernel_mapping_info, PerCPUPageMappingGuard};
 use svsm::platform::{SvsmPlatformCell, SVSM_PLATFORM};
 use svsm::requests::{request_loop, request_processing_main, update_mappings};
-use svsm::serial::SerialPort;
 use svsm::sev::utils::{rmp_adjust, RMPFlags};
 use svsm::sev::{secrets_page, secrets_page_mut};
-use svsm::svsm_console::SVSMIOPort;
 use svsm::svsm_paging::{init_page_table, invalidate_early_boot_memory};
 use svsm::task::exec_user;
 use svsm::task::{create_kernel_task, schedule_init};
@@ -236,9 +234,6 @@ pub fn memory_init(launch_info: &KernelLaunchInfo) {
     );
 }
 
-static CONSOLE_IO: SVSMIOPort = SVSMIOPort::new();
-static CONSOLE_SERIAL: ImmutAfterInitCell<SerialPort<'_>> = ImmutAfterInitCell::uninit();
-
 pub fn boot_stack_info() {
     // SAFETY: this is only unsafe because `bsp_stack_end` is an extern
     // static, but we're simply printing its address. We are not creating a
@@ -317,7 +312,10 @@ pub extern "C" fn svsm_start(li: &KernelLaunchInfo, vb_addr: usize) {
     cr0_init();
     cr4_init();
     efer_init();
-    platform.env_setup();
+    install_console_logger("SVSM").expect("Console logger already initialized");
+    platform
+        .env_setup(debug_serial_port)
+        .expect("Early environment setup failed");
 
     memory_init(&launch_info);
     migrate_valid_bitmap().expect("Failed to migrate valid-bitmap");
@@ -358,19 +356,11 @@ pub extern "C" fn svsm_start(li: &KernelLaunchInfo, vb_addr: usize) {
         .expect("Failed to allocate idle task for BSP");
 
     idt_init();
-
-    CONSOLE_SERIAL
-        .init(&SerialPort::new(&CONSOLE_IO, debug_serial_port))
-        .expect("console serial output already configured");
-    (*CONSOLE_SERIAL).init();
-
-    init_console(&*CONSOLE_SERIAL).expect("Console writer already initialized");
-    install_console_logger("SVSM").expect("Console logger already initialized");
-
-    log::info!("COCONUT Secure Virtual Machine Service Module (SVSM)");
+    platform
+        .env_setup_late(debug_serial_port)
+        .expect("Late environment setup failed");
 
     dump_cpuid_table();
-    platform.env_setup_late();
 
     let mem_info = memory_info();
     print_memory_info(&mem_info);
@@ -417,7 +407,7 @@ pub extern "C" fn svsm_main() {
         }
         SvsmConfig::IgvmConfig(igvm_params)
     } else {
-        SvsmConfig::FirmwareConfig(FwCfg::new(&CONSOLE_IO))
+        SvsmConfig::FirmwareConfig(FwCfg::new(SVSM_PLATFORM.as_dyn_ref().get_io_port()))
     };
 
     init_memory_map(&config, &LAUNCH_INFO).expect("Failed to init guest memory map");