Remove the additional IRQ disable during EOI

ostd/src/arch/x86/kernel/apic/mod.rs

@@ -1,21 +1,20 @@
 // SPDX-License-Identifier: MPL-2.0
 
 use alloc::boxed::Box;
-use core::cell::RefCell;
+use core::{
+    cell::UnsafeCell,
+    sync::atomic::{AtomicBool, Ordering},
+};
 
 use bit_field::BitField;
 use spin::Once;
 
-use crate::cpu_local;
+use crate::{cpu::PinCurrentCpu, cpu_local};
 
 pub mod ioapic;
 pub mod x2apic;
 pub mod xapic;
 
-cpu_local! {
-    static APIC_INSTANCE: RefCell<Option<Box<dyn Apic + 'static>>> = RefCell::new(None);
-}
-
 static APIC_TYPE: Once<ApicType> = Once::new();
 
 /// Do something over the APIC instance for the current CPU.
@@ -37,16 +36,27 @@ static APIC_TYPE: Once<ApicType> = Once::new();
 ///     ticks
 /// });
 /// ```
-pub fn with_borrow<R>(f: impl FnOnce(&mut (dyn Apic + 'static)) -> R) -> R {
+pub fn with_borrow<R>(f: impl FnOnce(&(dyn Apic + 'static)) -> R) -> R {
-    let irq_guard = crate::trap::disable_local();
+    cpu_local! {
-    let apic_guard = APIC_INSTANCE.get_with(&irq_guard);
+        static APIC_INSTANCE: UnsafeCell<Option<Box<dyn Apic + 'static>>> = UnsafeCell::new(None);
-    let mut apic_init_ref = apic_guard.borrow_mut();
+        static IS_INITIALIZED: AtomicBool = AtomicBool::new(false);
+    }
+
+    let preempt_guard = crate::task::disable_preempt();
+
+    // SAFETY: Preemption is disabled, so we can safely access the CPU-local variable.
+    let apic_ptr = unsafe {
+        let ptr = APIC_INSTANCE.as_ptr();
+        (*ptr).get()
+    };
 
     // If it is not initialized, lazily initialize it.
-    let apic_ref = if let Some(apic_ref) = apic_init_ref.as_mut() {
+    if IS_INITIALIZED
-        apic_ref
+        .get_on_cpu(preempt_guard.current_cpu())
-    } else {
+        .compare_exchange(false, true, Ordering::AcqRel, Ordering::Relaxed)
-        *apic_init_ref = Some(match APIC_TYPE.get().unwrap() {
+        .is_ok()
+    {
+        let apic: Option<Box<dyn Apic>> = Some(match APIC_TYPE.get().unwrap() {
             ApicType::XApic => {
                 let mut xapic = xapic::XApic::new().unwrap();
                 xapic.enable();
@@ -72,30 +82,42 @@ pub fn with_borrow<R>(f: impl FnOnce(&mut (dyn Apic + 'static)) -> R) -> R {
                 Box::new(x2apic)
             }
         });
+        // SAFETY: It will be only written once and no other read or write can
+        // happen concurrently for the synchronization with `IS_INITIALIZED`.
+        //
+        // If interrupts happen during the initialization, it can be written
+        // twice. But it should not happen since we must call this function
+        // when interrupts are disabled during the initialization of OSTD.
+        unsafe {
+            debug_assert!(apic_ptr.read().is_none());
+            apic_ptr.write(apic);
+        }
+    }
 
-        apic_init_ref.as_mut().unwrap()
+    // SAFETY: The APIC pointer is not accessed by other CPUs. It should point
-    };
+    // to initialized data and there will not be any write-during-read problem
+    // since there would be no write after `IS_INITIALIZED` is set to true.
+    let apic_ref = unsafe { &*apic_ptr };
+    let apic_ref = apic_ref.as_ref().unwrap().as_ref();
 
-    let ret = f.call_once((apic_ref.as_mut(),));
+    f.call_once((apic_ref,))
-
-    ret
 }
 
-pub trait Apic: ApicTimer + Sync + Send {
+pub trait Apic: ApicTimer {
     fn id(&self) -> u32;
 
     fn version(&self) -> u32;
 
     /// End of Interrupt, this function will inform APIC that this interrupt has been processed.
-    fn eoi(&mut self);
+    fn eoi(&self);
 
     /// Send a general inter-processor interrupt.
-    unsafe fn send_ipi(&mut self, icr: Icr);
+    unsafe fn send_ipi(&self, icr: Icr);
 }
 
-pub trait ApicTimer: Sync + Send {
+pub trait ApicTimer {
     /// Sets the initial timer count, the APIC timer will count down from this value.
-    fn set_timer_init_count(&mut self, value: u64);
+    fn set_timer_init_count(&self, value: u64);
 
     /// Gets the current count of the timer.
     /// The interval can be expressed by the expression: `init_count` - `current_count`.
@@ -106,10 +128,10 @@ pub trait ApicTimer: Sync + Send {
     /// Bit 12:    Delivery Status, 0 for Idle, 1 for Send Pending.
     /// Bit 16:    Mask bit.
     /// Bit 17-18: Timer Mode, 0 for One-shot, 1 for Periodic, 2 for TSC-Deadline.
-    fn set_lvt_timer(&mut self, value: u64);
+    fn set_lvt_timer(&self, value: u64);
 
     /// Sets timer divide config register.
-    fn set_timer_div_config(&mut self, div_config: DivideConfig);
+    fn set_timer_div_config(&self, div_config: DivideConfig);
 }
 
 enum ApicType {

ostd/src/arch/x86/kernel/apic/x2apic.rs

@@ -61,13 +61,14 @@ impl super::Apic for X2Apic {
         unsafe { rdmsr(IA32_X2APIC_VERSION) as u32 }
     }
 
-    fn eoi(&mut self) {
+    fn eoi(&self) {
         unsafe {
             wrmsr(IA32_X2APIC_EOI, 0);
         }
     }
 
-    unsafe fn send_ipi(&mut self, icr: super::Icr) {
+    unsafe fn send_ipi(&self, icr: super::Icr) {
+        let _guard = crate::trap::disable_local();
         wrmsr(IA32_X2APIC_ESR, 0);
         wrmsr(IA32_X2APIC_ICR, icr.0);
         loop {
@@ -83,7 +84,7 @@ impl super::Apic for X2Apic {
 }
 
 impl ApicTimer for X2Apic {
-    fn set_timer_init_count(&mut self, value: u64) {
+    fn set_timer_init_count(&self, value: u64) {
         unsafe {
             wrmsr(IA32_X2APIC_INIT_COUNT, value);
         }
@@ -93,13 +94,13 @@ impl ApicTimer for X2Apic {
         unsafe { rdmsr(IA32_X2APIC_CUR_COUNT) }
     }
 
-    fn set_lvt_timer(&mut self, value: u64) {
+    fn set_lvt_timer(&self, value: u64) {
         unsafe {
             wrmsr(IA32_X2APIC_LVT_TIMER, value);
         }
     }
 
-    fn set_timer_div_config(&mut self, div_config: super::DivideConfig) {
+    fn set_timer_div_config(&self, div_config: super::DivideConfig) {
         unsafe {
             wrmsr(IA32_X2APIC_DIV_CONF, div_config as u64);
         }

ostd/src/arch/x86/kernel/apic/xapic.rs

@@ -15,7 +15,7 @@ const APIC_LVT_MASK_BITS: u32 = 1 << 16;
 
 #[derive(Debug)]
 pub struct XApic {
-    mmio_region: &'static mut [u32],
+    mmio_start: *mut u32,
 }
 
 impl XApic {
@@ -24,9 +24,8 @@ impl XApic {
             return None;
         }
         let address = mm::paddr_to_vaddr(get_apic_base_address());
-        let region: &'static mut [u32] = unsafe { &mut *(address as *mut [u32; 256]) };
         Some(Self {
-            mmio_region: region,
+            mmio_start: address as *mut u32,
         })
     }
 
@@ -34,14 +33,16 @@ impl XApic {
     fn read(&self, offset: u32) -> u32 {
         assert!(offset as usize % 4 == 0);
         let index = offset as usize / 4;
-        unsafe { core::ptr::read_volatile(&self.mmio_region[index]) }
+        debug_assert!(index < 256);
+        unsafe { core::ptr::read_volatile(self.mmio_start.add(index)) }
     }
 
     /// Writes a register in the MMIO region.
-    fn write(&mut self, offset: u32, val: u32) {
+    fn write(&self, offset: u32, val: u32) {
         assert!(offset as usize % 4 == 0);
         let index = offset as usize / 4;
-        unsafe { core::ptr::write_volatile(&mut self.mmio_region[index], val) }
+        debug_assert!(index < 256);
+        unsafe { core::ptr::write_volatile(self.mmio_start.add(index), val) }
     }
 
     pub fn enable(&mut self) {
@@ -68,11 +69,12 @@ impl super::Apic for XApic {
         self.read(xapic::XAPIC_VERSION)
     }
 
-    fn eoi(&mut self) {
+    fn eoi(&self) {
         self.write(xapic::XAPIC_EOI, 0);
     }
 
-    unsafe fn send_ipi(&mut self, icr: super::Icr) {
+    unsafe fn send_ipi(&self, icr: super::Icr) {
+        let _guard = crate::trap::disable_local();
         self.write(xapic::XAPIC_ESR, 0);
         // The upper 32 bits of ICR must be written into XAPIC_ICR1 first,
         // because writing into XAPIC_ICR0 will trigger the action of
@@ -92,7 +94,7 @@ impl super::Apic for XApic {
 }
 
 impl ApicTimer for XApic {
-    fn set_timer_init_count(&mut self, value: u64) {
+    fn set_timer_init_count(&self, value: u64) {
         self.write(xapic::XAPIC_TIMER_INIT_COUNT, value as u32);
     }
 
@@ -100,11 +102,11 @@ impl ApicTimer for XApic {
         self.read(xapic::XAPIC_TIMER_CURRENT_COUNT) as u64
     }
 
-    fn set_lvt_timer(&mut self, value: u64) {
+    fn set_lvt_timer(&self, value: u64) {
         self.write(xapic::XAPIC_LVT_TIMER, value as u32);
     }
 
-    fn set_timer_div_config(&mut self, div_config: super::DivideConfig) {
+    fn set_timer_div_config(&self, div_config: super::DivideConfig) {
         self.write(xapic::XAPIC_TIMER_DIV_CONF, div_config as u32);
     }
 }

ostd/src/cpu/local/cpu_local.rs

@@ -108,7 +108,7 @@ impl<T: 'static> CpuLocal<T> {
     /// # Safety
     ///
     /// The caller must ensure that the reference to `self` is static.
-    unsafe fn as_ptr(&'static self) -> *const T {
+    pub(crate) unsafe fn as_ptr(&'static self) -> *const T {
         super::has_init::assert_true();
 
         let offset = self.get_offset();