Merge pull request #613 from msft-jlange/req_task

requests: refactor SVSM request loop

kernel/src/cpu/percpu.rs

@@ -42,17 +42,21 @@ use crate::mm::{
     SVSM_PERCPU_VMSA_BASE, SVSM_SHADOW_STACKS_INIT_TASK, SVSM_SHADOW_STACK_ISST_DF_BASE,
     SVSM_STACK_IST_DF_BASE,
 };
-use crate::platform::{SvsmPlatform, SVSM_PLATFORM};
+use crate::platform::{halt, SvsmPlatform, SVSM_PLATFORM};
+use crate::requests::{request_loop_main, request_processing_main};
 use crate::sev::ghcb::{GhcbPage, GHCB};
 use crate::sev::hv_doorbell::{allocate_hv_doorbell_page, HVDoorbell};
 use crate::sev::msr_protocol::{hypervisor_ghcb_features, GHCBHvFeatures};
 use crate::sev::utils::RMPFlags;
 use crate::sev::vmsa::{VMSAControl, VmsaPage};
-use crate::task::{schedule, schedule_task, RunQueue, Task, TaskPointer, WaitQueue};
+use crate::task::{
+    schedule, schedule_task, start_kernel_task, RunQueue, Task, TaskPointer, WaitQueue,
+};
 use crate::types::{
     PAGE_SHIFT, PAGE_SHIFT_2M, PAGE_SIZE, PAGE_SIZE_2M, SVSM_TR_ATTRIBUTES, SVSM_TSS,
 };
 use crate::utils::MemoryRegion;
+use alloc::format;
 use alloc::string::String;
 use alloc::sync::Arc;
 use alloc::vec::Vec;
@@ -1358,3 +1362,34 @@ pub fn process_requests() {
 pub fn current_task() -> TaskPointer {
     this_cpu().runqueue.lock_read().current_task()
 }
+
+#[no_mangle]
+pub extern "C" fn cpu_idle_loop() {
+    // Start request processing on this CPU if required.
+    if SVSM_PLATFORM.start_svsm_request_loop() {
+        // Start request processing on this CPU.
+        let apic_id = this_cpu().get_apic_id();
+        let processing_name = format!("request-processing on CPU {}", apic_id);
+        start_kernel_task(request_processing_main, processing_name)
+            .expect("Failed to launch request processing task");
+        let loop_name = format!("request-loop on CPU {}", apic_id);
+        start_kernel_task(request_loop_main, loop_name)
+            .expect("Failed to launch request loop task");
+    }
+
+    loop {
+        // Go idle
+        halt();
+
+        // If idle was explicitly requested by another task, then schedule that
+        // task to execute again in case it wants to perform processing as a
+        // result of the wake from idle.
+        let maybe_task = this_cpu().runqueue().lock_write().wake_from_idle();
+        if let Some(task) = maybe_task {
+            schedule_task(task);
+        }
+
+        // Execute any tasks that are currently runnable.
+        schedule();
+    }
+}

kernel/src/cpu/smp.rs

@@ -4,12 +4,10 @@
 //
 // Author: Joerg Roedel <jroedel@suse.de>
 
-extern crate alloc;
-
 use crate::acpi::tables::ACPICPUInfo;
 use crate::address::{Address, VirtAddr};
 use crate::cpu::idt::idt;
-use crate::cpu::percpu::{this_cpu, this_cpu_shared, PerCpu};
+use crate::cpu::percpu::{cpu_idle_loop, this_cpu, this_cpu_shared, PerCpu};
 use crate::cpu::shadow_stack::{is_cet_ss_supported, SCetFlags, MODE_64BIT, S_CET};
 use crate::cpu::sse::sse_init;
 use crate::cpu::tlb::set_tlb_flush_smp;
@@ -18,11 +16,9 @@ use crate::error::SvsmError;
 use crate::hyperv;
 use crate::mm::STACK_SIZE;
 use crate::platform::{SvsmPlatform, SVSM_PLATFORM};
-use crate::requests::{request_loop, request_processing_main};
-use crate::task::{schedule_init, start_kernel_task};
+use crate::task::schedule_init;
 use crate::utils::MemoryRegion;
 
-use alloc::string::String;
 use bootlib::kernel_launch::ApStartContext;
 use core::arch::global_asm;
 use core::mem;
@@ -152,8 +148,8 @@ extern "C" fn start_ap() -> ! {
         .expect("setup_on_cpu() failed");
 
     percpu
-        .setup_idle_task(ap_request_loop)
-        .expect("Failed to allocated idle task for AP");
+        .setup_idle_task(cpu_idle_loop)
+        .expect("Failed to allocate idle task for AP");
 
     // Send a life-sign
     log::info!("AP with APIC-ID {} is online", this_cpu().get_apic_id());
@@ -171,11 +167,3 @@ extern "C" fn start_ap() -> ! {
 
     unreachable!("Road ends here!");
 }
-
-#[no_mangle]
-pub extern "C" fn ap_request_loop() {
-    start_kernel_task(request_processing_main, String::from("request-processing"))
-        .expect("Failed to launch request processing task");
-    request_loop();
-    panic!("Returned from request_loop!");
-}

kernel/src/platform/mod.rs

@@ -16,6 +16,7 @@ use native::NativePlatform;
 use snp::SnpPlatform;
 use tdp::TdpPlatform;
 
+use core::arch::asm;
 use core::ops::{Deref, DerefMut};
 
 use crate::address::{PhysAddr, VirtAddr};
@@ -27,7 +28,6 @@ use crate::error::SvsmError;
 use crate::hyperv;
 use crate::io::IOPort;
 use crate::types::PageSize;
-use crate::utils;
 use crate::utils::immut_after_init::ImmutAfterInitCell;
 use crate::utils::MemoryRegion;
 
@@ -69,7 +69,10 @@ pub trait SvsmPlatform {
     where
         Self: Sized,
     {
-        utils::halt();
+        // SAFETY: executing HLT in assembly is always safe.
+        unsafe {
+            asm!("hlt");
+        }
     }
 
     /// Performs basic early initialization of the runtime environment.
@@ -175,6 +178,11 @@ pub trait SvsmPlatform {
         cpu: &PerCpu,
         context: &hyperv::HvInitialVpContext,
     ) -> Result<(), SvsmError>;
+
+    /// Indicates whether this platform should invoke the SVSM request loop.
+    fn start_svsm_request_loop(&self) -> bool {
+        false
+    }
 }
 
 //FIXME - remove Copy trait

kernel/src/platform/snp.rs

@@ -330,6 +330,10 @@ impl SvsmPlatform for SnpPlatform {
 
         current_ghcb().ap_create(vmsa_pa, cpu.get_apic_id().into(), 0, sev_features)
     }
+
+    fn start_svsm_request_loop(&self) -> bool {
+        true
+    }
 }
 
 #[derive(Clone, Copy, Debug, Default)]

kernel/src/requests.rs

@@ -12,13 +12,13 @@ use crate::protocols::apic::apic_protocol_request;
 use crate::protocols::core::core_protocol_request;
 use crate::protocols::errors::{SvsmReqError, SvsmResultCode};
 use crate::sev::ghcb::switch_to_vmpl;
+use crate::task::go_idle;
 
 #[cfg(all(feature = "vtpm", not(test)))]
 use crate::protocols::{vtpm::vtpm_protocol_request, SVSM_VTPM_PROTOCOL};
 use crate::protocols::{RequestParams, SVSM_APIC_PROTOCOL, SVSM_CORE_PROTOCOL};
 use crate::sev::vmsa::VMSAControl;
 use crate::types::GUEST_VMPL;
-use crate::utils::halt;
 use cpuarch::vmsa::GuestVMExit;
 
 /// The SVSM Calling Area (CAA)
@@ -144,7 +144,11 @@ fn check_requests() -> Result<bool, SvsmReqError> {
     }
 }
 
-pub fn request_loop() {
+#[no_mangle]
+pub extern "C" fn request_loop_main() {
+    let apic_id = this_cpu().get_apic_id();
+    log::info!("Launching request loop task on CPU {}", apic_id);
+
     loop {
         // Determine whether the guest is runnable.  If not, halt and wait for
         // the guest to execute.  When halting, assume that the hypervisor
@@ -181,7 +185,7 @@ pub fn request_loop() {
             drop(guard);
         } else {
             log::debug!("No VMSA or CAA! Halting");
-            halt();
+            go_idle();
         }
 
         // Update mappings again on return from the guest VMPL or halt. If this

kernel/src/sev/msr_protocol.rs

@@ -9,7 +9,7 @@ use crate::cpu::irq_state::raw_irqs_disable;
 use crate::cpu::msr::{read_msr, write_msr, SEV_GHCB};
 use crate::cpu::{irqs_enabled, IrqGuard};
 use crate::error::SvsmError;
-use crate::utils::halt;
+use crate::platform::halt;
 use crate::utils::immut_after_init::ImmutAfterInitCell;
 
 use super::utils::raw_vmgexit;

kernel/src/svsm.rs

@@ -7,8 +7,6 @@
 #![cfg_attr(not(test), no_std)]
 #![cfg_attr(not(test), no_main)]
 
-extern crate alloc;
-
 use bootlib::kernel_launch::KernelLaunchInfo;
 use core::arch::global_asm;
 use core::panic::PanicInfo;
@@ -21,7 +19,7 @@ use svsm::cpu::control_regs::{cr0_init, cr4_init};
 use svsm::cpu::cpuid::{dump_cpuid_table, register_cpuid_table};
 use svsm::cpu::gdt::GLOBAL_GDT;
 use svsm::cpu::idt::svsm::{early_idt_init, idt_init};
-use svsm::cpu::percpu::{this_cpu, PerCpu};
+use svsm::cpu::percpu::{cpu_idle_loop, this_cpu, PerCpu};
 use svsm::cpu::shadow_stack::{
     determine_cet_support, is_cet_ss_supported, SCetFlags, MODE_64BIT, S_CET,
 };
@@ -41,11 +39,10 @@ use svsm::mm::virtualrange::virt_log_usage;
 use svsm::mm::{init_kernel_mapping_info, FixedAddressMappingRange};
 use svsm::platform;
 use svsm::platform::{init_platform_type, SvsmPlatformCell, SVSM_PLATFORM};
-use svsm::requests::{request_loop, request_processing_main};
 use svsm::sev::secrets_page_mut;
 use svsm::svsm_paging::{init_page_table, invalidate_early_boot_memory};
 use svsm::task::exec_user;
-use svsm::task::{schedule_init, start_kernel_task};
+use svsm::task::schedule_init;
 use svsm::types::PAGE_SIZE;
 use svsm::utils::{immut_after_init::ImmutAfterInitCell, zero_mem_region, MemoryRegion};
 #[cfg(all(feature = "vtpm", not(test)))]
@@ -55,8 +52,6 @@ use svsm::mm::validate::{init_valid_bitmap_ptr, migrate_valid_bitmap};
 
 use release::COCONUT_VERSION;
 
-use alloc::string::String;
-
 extern "C" {
     pub static bsp_stack: u8;
     pub static bsp_stack_end: u8;
@@ -346,9 +341,6 @@ pub extern "C" fn svsm_main() {
         panic!("Failed to launch FW: {e:#?}");
     }
 
-    start_kernel_task(request_processing_main, String::from("request-processing"))
-        .expect("Failed to launch request processing task");
-
     #[cfg(test)]
     {
         if config.is_qemu() {
@@ -362,9 +354,7 @@ pub extern "C" fn svsm_main() {
         Err(e) => log::info!("Failed to launch /init: {e:#?}"),
     }
 
-    request_loop();
-
-    panic!("Road ends here!");
+    cpu_idle_loop();
 }
 
 #[panic_handler]

kernel/src/task/mod.rs

@@ -10,8 +10,9 @@ mod tasks;
 mod waiting;
 
 pub use schedule::{
-    create_user_task, current_task, current_task_terminated, finish_user_task, is_current_task,
-    schedule, schedule_init, schedule_task, start_kernel_task, terminate, RunQueue, TASKLIST,
+    create_user_task, current_task, current_task_terminated, finish_user_task, go_idle,
+    is_current_task, schedule, schedule_init, schedule_task, start_kernel_task, terminate,
+    RunQueue, TASKLIST,
 };
 
 pub use tasks::{

kernel/src/task/schedule.rs

@@ -67,6 +67,9 @@ pub struct RunQueue {
 
     /// Temporary storage for tasks which are about to be terminated
     terminated_task: Option<TaskPointer>,
+
+    /// Pointer to a task that should be woken when returning from idle
+    wake_from_idle: Option<TaskPointer>,
 }
 
 impl RunQueue {
@@ -79,6 +82,7 @@ impl RunQueue {
             current_task: None,
             idle_task: OnceCell::new(),
             terminated_task: None,
+            wake_from_idle: None,
         }
     }
 
@@ -191,6 +195,16 @@ impl RunQueue {
     pub fn current_task(&self) -> TaskPointer {
         self.current_task.as_ref().unwrap().clone()
     }
+
+    /// Wakes a task from idle if required.
+    ///
+    /// # Returns
+    ///
+    /// An `Option<TaskPointer>` indicating which task should be woken, if
+    /// any.
+    pub fn wake_from_idle(&mut self) -> Option<TaskPointer> {
+        self.wake_from_idle.take()
+    }
 }
 
 /// Global task list
@@ -326,6 +340,27 @@ pub fn terminate() {
     schedule();
 }
 
+pub fn go_idle() {
+    // Mark this task as blocked and indicate that it is waiting for wake after
+    // idle.  Only one task on each CPU can be in the wake-from-idle state at
+    // one time.
+    let task = this_cpu().current_task();
+    task.set_task_blocked();
+    let mut runqueue = this_cpu().runqueue().lock_write();
+    assert!(runqueue.wake_from_idle.is_none());
+    runqueue.wake_from_idle = Some(task);
+    drop(runqueue);
+
+    // Find another task to run.  If no other task is runnable, then the idle
+    // thread will execute.
+    schedule();
+}
+
+// SAFETY: This function returns a raw pointer to a task. It is safe
+// because this function is only used in the task switch code, which also only
+// takes a single reference to the next and previous tasks. Also, this
+// function works on an Arc, which ensures that only a single mutable reference
+// can exist.
 fn task_pointer(taskptr: TaskPointer) -> *const Task {
     Arc::as_ptr(&taskptr)
 }

kernel/src/utils/mod.rs

@@ -14,5 +14,5 @@ pub mod util;
 pub use memory_region::MemoryRegion;
 pub use scoped::{ScopedMut, ScopedRef};
 pub use util::{
-    align_down, align_up, halt, is_aligned, overlap, page_align_up, page_offset, zero_mem_region,
+    align_down, align_up, is_aligned, overlap, page_align_up, page_offset, zero_mem_region,
 };

kernel/src/utils/util.rs

@@ -6,7 +6,6 @@
 
 use crate::address::{Address, VirtAddr};
 use crate::types::PAGE_SIZE;
-use core::arch::asm;
 use core::ops::{Add, BitAnd, Not, Sub};
 
 pub fn align_up<T>(addr: T, align: T) -> T
@@ -31,14 +30,6 @@ where
     (addr & (align - T::from(1u8))) == T::from(0u8)
 }
 
-pub fn halt() {
-    // SAFETY: Inline assembly to call HLT instruction, which does not change
-    // any state related to memory safety.
-    unsafe {
-        asm!("hlt", options(att_syntax));
-    }
-}
-
 pub fn page_align_up(x: usize) -> usize {
     align_up(x, PAGE_SIZE)
 }