From b8eff00fb79a64b7ec5a4baa6d86b36d5d7d230f Mon Sep 17 00:00:00 2001 From: Zhang Junyang Date: Fri, 18 Oct 2024 23:10:10 +0800 Subject: [PATCH] Remove the additional IRQ disable during EOI --- ostd/src/arch/x86/kernel/apic/mod.rs | 74 ++++++++++++++++--------- ostd/src/arch/x86/kernel/apic/x2apic.rs | 11 ++-- ostd/src/arch/x86/kernel/apic/xapic.rs | 24 ++++---- ostd/src/cpu/local/cpu_local.rs | 2 +- 4 files changed, 68 insertions(+), 43 deletions(-) diff --git a/ostd/src/arch/x86/kernel/apic/mod.rs b/ostd/src/arch/x86/kernel/apic/mod.rs index b8e0b3cde..06fefec33 100644 --- a/ostd/src/arch/x86/kernel/apic/mod.rs +++ b/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>> = RefCell::new(None); -} - static APIC_TYPE: Once = Once::new(); /// Do something over the APIC instance for the current CPU. @@ -37,16 +36,27 @@ static APIC_TYPE: Once = Once::new(); /// ticks /// }); /// ``` -pub fn with_borrow(f: impl FnOnce(&mut (dyn Apic + 'static)) -> R) -> R { - let irq_guard = crate::trap::disable_local(); - let apic_guard = APIC_INSTANCE.get_with(&irq_guard); - let mut apic_init_ref = apic_guard.borrow_mut(); +pub fn with_borrow(f: impl FnOnce(&(dyn Apic + 'static)) -> R) -> R { + cpu_local! { + static APIC_INSTANCE: UnsafeCell>> = UnsafeCell::new(None); + 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() { - apic_ref - } else { - *apic_init_ref = Some(match APIC_TYPE.get().unwrap() { + if IS_INITIALIZED + .get_on_cpu(preempt_guard.current_cpu()) + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Relaxed) + .is_ok() + { + let apic: Option> = 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(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(),)); - - ret + f.call_once((apic_ref,)) } -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 { diff --git a/ostd/src/arch/x86/kernel/apic/x2apic.rs b/ostd/src/arch/x86/kernel/apic/x2apic.rs index bf5f20964..9af7a9d47 100644 --- a/ostd/src/arch/x86/kernel/apic/x2apic.rs +++ b/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); } diff --git a/ostd/src/arch/x86/kernel/apic/xapic.rs b/ostd/src/arch/x86/kernel/apic/xapic.rs index b06a42a94..2655699fe 100644 --- a/ostd/src/arch/x86/kernel/apic/xapic.rs +++ b/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); } } diff --git a/ostd/src/cpu/local/cpu_local.rs b/ostd/src/cpu/local/cpu_local.rs index 50079b75d..f8548b451 100644 --- a/ostd/src/cpu/local/cpu_local.rs +++ b/ostd/src/cpu/local/cpu_local.rs @@ -108,7 +108,7 @@ impl CpuLocal { /// # 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();