Unify ACE and OpenSBI hart state to get rid of unnecessary memory load/store when delegating calls to OpenSBI (#84)

Signed-off-by: Wojciech Ozga <[email protected]>

Author: wojciechozga

security-monitor/src/confidential_flow/finite_state_machine.rs

@@ -216,14 +216,8 @@ impl<'a> ConfidentialFlow<'a> {
     pub fn broadcast_remote_command(
         &mut self, confidential_vm: &mut ConfidentialVm, confidential_hart_remote_command: ConfidentialHartRemoteCommand,
     ) -> Result<(), Error> {
-        // Hack: For the time-being, we rely on the OpenSBI's implementation of physical IPIs. To use OpenSBI functions we
-        // must set the mscratch register to the value expected by OpenSBI. We do it here, because we have access to the `HardwareHart`
-        // that knows the original value of the mscratch expected by OpenSBI.
-        self.hardware_hart.swap_mscratch();
-        let result = confidential_vm.broadcast_remote_command(confidential_hart_remote_command);
-        // We must revert the content of mscratch back to the value expected by our context switched.
-        self.hardware_hart.swap_mscratch();
-        result
+        // For the time-being, we rely on the OpenSBI's implementation of broadcasting IPIs to hardware harts.
+        self.hardware_hart.opensbi_context(|| confidential_vm.broadcast_remote_command(confidential_hart_remote_command))
     }
 
     /// Processes pending requests from other confidential harts by applying the corresponding state transformation to

security-monitor/src/core/architecture/riscv/control_status_registers.rs

@@ -11,14 +11,19 @@ use crate::core::control_data::{DigestType, MeasurementDigest};
 use core::arch::asm;
 
 /// Represents all control status registers (CSRs) accessible to modes less privileged than M-mode.
+#[repr(C)]
 pub struct ControlStatusRegisters {
     pub mepc: ReadWriteRiscvCsr<CSR_MEPC>,
+    pub mstatus: ReadWriteRiscvCsr<CSR_MSTATUS>,
+    // Safety: Architectural hart state must be bitwise equal to corresponding OpenSBI structure.
+    //         For that reason, this structure is layouted according to C convention and it starts
+    //         with mepc and mstatus. When updating to new version of OpenSBI, make sure that
+    //         CSRs' order in this structure is correct.
     pub mcause: ReadWriteRiscvCsr<CSR_MCAUSE>,
     pub medeleg: ReadWriteRiscvCsr<CSR_MEDELEG>,
     pub mideleg: ReadWriteRiscvCsr<CSR_MIDELEG>,
     pub mie: ReadWriteRiscvCsr<CSR_MIE>,
     pub mip: ReadRiscvCsr<CSR_MIP>,
-    pub mstatus: ReadWriteRiscvCsr<CSR_MSTATUS>,
     pub mtinst: ReadWriteRiscvCsr<CSR_MTINST>,
     pub mtval: ReadWriteRiscvCsr<CSR_MTVAL>,
     pub mtval2: ReadWriteRiscvCsr<CSR_MTVAL2>,
@@ -71,12 +76,12 @@ impl ControlStatusRegisters {
     pub fn empty() -> Self {
         let mut csrs = Self {
             mepc: ReadWriteRiscvCsr::new(),
+            mstatus: ReadWriteRiscvCsr::new(),
             mcause: ReadWriteRiscvCsr::new(),
             medeleg: ReadWriteRiscvCsr::new(),
             mideleg: ReadWriteRiscvCsr::new(),
             mie: ReadWriteRiscvCsr::new(),
             mip: ReadRiscvCsr::new(),
-            mstatus: ReadWriteRiscvCsr::new(),
             mtinst: ReadWriteRiscvCsr::new(),
             mtval: ReadWriteRiscvCsr::new(),
             mtval2: ReadWriteRiscvCsr::new(),
@@ -127,11 +132,11 @@ impl ControlStatusRegisters {
 
     pub fn save_in_main_memory(&mut self) {
         self.mepc.save_in_main_memory();
+        self.mstatus.save_in_main_memory();
         self.mcause.save_in_main_memory();
         self.medeleg.save_in_main_memory();
         self.mideleg.save_in_main_memory();
         self.mie.save_in_main_memory();
-        self.mstatus.save_in_main_memory();
         self.mtinst.save_in_main_memory();
         self.mtval.save_in_main_memory();
         self.mtval2.save_in_main_memory();
@@ -182,11 +187,11 @@ impl ControlStatusRegisters {
 
     pub fn restore_from_main_memory(&self) {
         self.mepc.restore_from_main_memory();
+        self.mstatus.restore_from_main_memory();
         self.mcause.restore_from_main_memory();
         self.medeleg.restore_from_main_memory();
         self.mideleg.restore_from_main_memory();
         self.mie.restore_from_main_memory();
-        self.mstatus.restore_from_main_memory();
         self.mtinst.restore_from_main_memory();
         self.mtval.restore_from_main_memory();
         self.mtval2.restore_from_main_memory();

security-monitor/src/core/control_data/hardware_hart.rs

@@ -56,6 +56,14 @@ impl HardwareHart {
         self.previous_mscratch = current_mscratch;
     }
 
+    pub fn opensbi_context<F>(&mut self, function: F) -> Result<(), crate::error::Error>
+    where F: FnOnce() -> Result<(), crate::error::Error> {
+        self.swap_mscratch();
+        let result = function();
+        self.swap_mscratch();
+        result
+    }
+
     pub fn confidential_hart(&self) -> &ConfidentialHart {
         &self.confidential_hart
     }

security-monitor/src/core/control_data/hypervisor_hart.rs

@@ -109,6 +109,10 @@ impl HypervisorHart {
         &self.hypervisor_hart_state
     }
 
+    pub fn hypervisor_hart_state_mut(&mut self) -> &mut HartArchitecturalState {
+        &mut self.hypervisor_hart_state
+    }
+
     pub fn shared_memory(&self) -> &NaclSharedMemory {
         &self.shared_memory
     }

security-monitor/src/non_confidential_flow/apply_to_hypervisor.rs

@@ -2,12 +2,10 @@
 // SPDX-FileContributor: Wojciech Ozga <[email protected]>, IBM Research - Zurich
 // SPDX-License-Identifier: Apache-2.0
 use crate::non_confidential_flow::handlers::nested_acceleration_extension::NaclSetupSharedMemory;
-use crate::non_confidential_flow::handlers::opensbi::DelegateToOpensbi;
 use crate::non_confidential_flow::handlers::supervisor_binary_interface::SbiResponse;
 
 /// Transformation of the hypervisor state created as a result of processing an SBI request from the hypervisor.
 pub enum ApplyToHypervisorHart {
     SbiResponse(SbiResponse),
-    OpenSbiResponse(DelegateToOpensbi),
     SetSharedMemory(NaclSetupSharedMemory),
 }

security-monitor/src/non_confidential_flow/finite_state_machine.rs

@@ -15,15 +15,19 @@ use crate::non_confidential_flow::handlers::cove_host_extension::{
     DestroyConfidentialVm, GetSecurityMonitorInfo, PromoteToConfidentialVm, RunConfidentialHart,
 };
 use crate::non_confidential_flow::handlers::nested_acceleration_extension::{NaclProbeFeature, NaclSetupSharedMemory};
-use crate::non_confidential_flow::handlers::opensbi::{DelegateToOpensbi, ProbeSbiExtension};
-use crate::non_confidential_flow::handlers::supervisor_binary_interface::InvalidCall;
+use crate::non_confidential_flow::handlers::supervisor_binary_interface::{InvalidCall, ProbeSbiExtension};
 use crate::non_confidential_flow::{ApplyToHypervisorHart, DeclassifyToHypervisor};
+use opensbi_sys::sbi_trap_regs;
 
 extern "C" {
     /// To ensure safety, specify all possible valid states that KVM expects to see and prove that security monitor
     /// never returns to KVM with other state. For example, only a subset of exceptions/interrupts can be handled by KVM.
     /// KVM kill the vcpu if it receives unexpected exception because it does not know what to do with it.
     fn exit_to_hypervisor_asm() -> !;
+
+    /// Currently, we rely on OpenSBI to handle some of the interrupts or exceptions. Below function is the entry point
+    /// to OpenSBI trap handler.
+    fn sbi_trap_handler(regs: *mut sbi_trap_regs) -> *mut sbi_trap_regs;
 }
 
 /// Represents the non-confidential part of the finite state machine (FSM), implementing router and exit nodes. It encapsulates the
@@ -55,15 +59,9 @@ impl<'a> NonConfidentialFlow<'a> {
         // hardware hart's dump area in main memory. This area in main memory is exclusively owned by the physical hart executing this code.
         // Specifically, every physical hart has its own are in the main memory and its `mscratch` register stores the address. See the
         // `initialization` procedure for more details.
-        let flow = unsafe { Self::create(hart_ptr.as_mut().expect(Self::CTX_SWITCH_ERROR_MSG)) };
+        let mut flow = unsafe { Self::create(hart_ptr.as_mut().expect(Self::CTX_SWITCH_ERROR_MSG)) };
         match TrapCause::from_hart_architectural_state(flow.hypervisor_hart().hypervisor_hart_state()) {
-            Interrupt => DelegateToOpensbi::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
-            IllegalInstruction => DelegateToOpensbi::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
-            LoadAddressMisaligned => DelegateToOpensbi::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
-            LoadAccessFault => DelegateToOpensbi::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
-            StoreAddressMisaligned => DelegateToOpensbi::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
-            StoreAccessFault => DelegateToOpensbi::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
-            HsEcall(Base(ProbeExtension)) => ProbeSbiExtension::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
+            HsEcall(Base(ProbeExtension)) => ProbeSbiExtension::from_hypervisor_hart(flow.hypervisor_hart_mut()).handle(flow),
             HsEcall(Covh(TsmGetInfo)) => GetSecurityMonitorInfo::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
             HsEcall(Covh(PromoteToTvm)) => PromoteToConfidentialVm::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
             HsEcall(Covh(TvmVcpuRun)) => RunConfidentialHart::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
@@ -72,12 +70,21 @@ impl<'a> NonConfidentialFlow<'a> {
             HsEcall(Nacl(ProbeFeature)) => NaclProbeFeature::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
             HsEcall(Nacl(SetupSharedMemory)) => NaclSetupSharedMemory::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
             HsEcall(Nacl(_)) => InvalidCall::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
-            HsEcall(_) => DelegateToOpensbi::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
-            MachineEcall => DelegateToOpensbi::from_hypervisor_hart(flow.hypervisor_hart()).handle(flow),
-            trap_reason => panic!("Bug: Incorrect interrupt delegation configuration: {:?}", trap_reason),
+            _ => flow.delegate_to_opensbi(),
         }
     }
 
+    pub fn delegate_to_opensbi(self) -> ! {
+        // Safety: We play with fire here. We must statically make sure that OpenSBI's input structure is bitwise same as ACE's hart state.
+        let trap_regs = self.hardware_hart.hypervisor_hart_mut().hypervisor_hart_state_mut() as *mut _ as *mut sbi_trap_regs;
+        let _ = self.hardware_hart.opensbi_context(|| {
+            Ok(unsafe {
+                sbi_trap_handler(trap_regs);
+            })
+        });
+        unsafe { exit_to_hypervisor_asm() }
+    }
+
     /// Tries to traverse to confidential flow of the finite state machine (FSM). Returns error if the identifier of a confidential VM or
     /// hart are incorrect or cannot be scheduled for execution.
     pub fn into_confidential_flow(
@@ -113,18 +120,11 @@ impl<'a> NonConfidentialFlow<'a> {
     pub(super) fn apply_and_exit_to_hypervisor(mut self, transformation: ApplyToHypervisorHart) -> ! {
         match transformation {
             ApplyToHypervisorHart::SbiResponse(v) => v.apply_to_hypervisor_hart(self.hypervisor_hart_mut()),
-            ApplyToHypervisorHart::OpenSbiResponse(v) => v.apply_to_hypervisor_hart(self.hypervisor_hart_mut()),
             ApplyToHypervisorHart::SetSharedMemory(v) => v.apply_to_hypervisor_hart(self.hypervisor_hart_mut()),
         }
         unsafe { exit_to_hypervisor_asm() }
     }
 
-    /// Swaps the mscratch register value with the original mascratch value used by OpenSBI. This function must be
-    /// called before executing any OpenSBI function. We can remove this once we get rid of the OpenSBI firmware.
-    pub fn swap_mscratch(&mut self) {
-        self.hardware_hart.swap_mscratch()
-    }
-
     pub fn shared_memory(&self) -> &NaclSharedMemory {
         self.hypervisor_hart().shared_memory()
     }

security-monitor/src/non_confidential_flow/handlers/mod.rs

@@ -3,5 +3,4 @@
 // SPDX-License-Identifier: Apache-2.0
 pub mod cove_host_extension;
 pub mod nested_acceleration_extension;
-pub mod opensbi;
 pub mod supervisor_binary_interface;

security-monitor/src/non_confidential_flow/handlers/opensbi/delegate_to_opensbi.rs

@@ -2,7 +2,9 @@
 // SPDX-FileContributor: Wojciech Ozga <[email protected]>, IBM Research - Zurich
 // SPDX-License-Identifier: Apache-2.0
 pub use invalid_call::InvalidCall;
+pub use probe_sbi_extension::ProbeSbiExtension;
 pub use response::SbiResponse;
 
 mod invalid_call;
+mod probe_sbi_extension;
 mod response;