From a7b4add27c95591a175aa0b6485a47ccae2d6c96 Mon Sep 17 00:00:00 2001 From: Stefan Kalkowski Date: Thu, 4 Jul 2024 14:50:14 +0200 Subject: [PATCH] hw: move cpu kernel object into cpu local area Fix genodelabs/genode#5310 --- repos/base-hw/src/core/board/pc/board.h | 11 +- repos/base-hw/src/core/kernel/cpu.cc | 40 ++++--- repos/base-hw/src/core/kernel/cpu.h | 71 +++--------- repos/base-hw/src/core/kernel/main.cc | 101 +++++++----------- repos/base-hw/src/core/kernel/thread.cc | 5 +- .../src/core/spec/x86_64/kernel/thread.cc | 5 +- .../src/include/hw/spec/x86_64/pc_board.h | 6 +- 7 files changed, 91 insertions(+), 148 deletions(-) diff --git a/repos/base-hw/src/core/board/pc/board.h b/repos/base-hw/src/core/board/pc/board.h index 32d4ebed23..f07a971d08 100644 --- a/repos/base-hw/src/core/board/pc/board.h +++ b/repos/base-hw/src/core/board/pc/board.h @@ -19,20 +19,15 @@ /* PC virtualization */ #include -/* - * As long as Board::NR_OF_CPUS is used as constant within pic.h - * we sadly have to include Hw::Pc_board into the namespace - * before including pic.h. This will change as soon as the number - * O CPUs is only define per function -*/ -namespace Board { using namespace Hw::Pc_board; }; - /* base-hw core includes */ #include #include #include namespace Board { + + using namespace Hw::Pc_board; + class Pic : public Local_interrupt_controller { using Local_interrupt_controller::Local_interrupt_controller; }; diff --git a/repos/base-hw/src/core/kernel/cpu.cc b/repos/base-hw/src/core/kernel/cpu.cc index 97d6d6edda..0e0c686984 100644 --- a/repos/base-hw/src/core/kernel/cpu.cc +++ b/repos/base-hw/src/core/kernel/cpu.cc @@ -22,6 +22,7 @@ #include #include #include +#include using namespace Kernel; @@ -154,7 +155,7 @@ Cpu_job & Cpu::schedule() _timer.process_timeouts(); _scheduler.update(_timer.time()); time_t t = _scheduler.current_time_left(); - _timer.set_timeout(this, t); + _timer.set_timeout(&_timeout, t); time_t duration = _timer.schedule_timeout(); old_job.update_execution_time(duration); } @@ -188,6 +189,17 @@ Cpu::Cpu(unsigned const id, _global_work_list { cpu_pool.work_list() } { _arch_init(); + + /* + * We insert the cpu objects in order into the cpu_pool's list + * to ensure that the cpu with the lowest given id is the first + * one. + */ + Cpu * cpu = cpu_pool._cpus.first(); + while (cpu && cpu->next() && (cpu->next()->id() < _id)) + cpu = cpu->next(); + cpu = (cpu && cpu->id() < _id) ? cpu : nullptr; + cpu_pool._cpus.insert(this, cpu); } @@ -195,6 +207,15 @@ Cpu::Cpu(unsigned const id, ** Cpu_pool ** **************/ +template +static inline T* cpu_object_by_id(unsigned const id) +{ + using namespace Hw::Mm; + addr_t base = CPU_LOCAL_MEMORY_AREA_START + id*CPU_LOCAL_MEMORY_SLOT_SIZE; + return (T*)(base + CPU_LOCAL_MEMORY_SLOT_OBJECT_OFFSET); +} + + void Cpu_pool:: initialize_executing_cpu(Board::Address_space_id_allocator &addr_space_id_alloc, @@ -203,22 +224,13 @@ initialize_executing_cpu(Board::Address_space_id_allocator &addr_space_id_alloc Board::Global_interrupt_controller &global_irq_ctrl) { unsigned id = Cpu::executing_id(); - _cpus[id].construct( - id, addr_space_id_alloc, user_irq_pool, *this, core_pd, global_irq_ctrl); + Genode::construct_at(cpu_object_by_id(id), id, + addr_space_id_alloc, user_irq_pool, + *this, core_pd, global_irq_ctrl); } Cpu & Cpu_pool::cpu(unsigned const id) { - assert(id < _nr_of_cpus && _cpus[id].constructed()); - return *_cpus[id]; + return *cpu_object_by_id(id); } - - -using Boot_info = Hw::Boot_info; - - -Cpu_pool::Cpu_pool(unsigned nr_of_cpus) -: - _nr_of_cpus(nr_of_cpus) -{ } diff --git a/repos/base-hw/src/core/kernel/cpu.h b/repos/base-hw/src/core/kernel/cpu.h index d0c6d1c034..83f2d5b7cc 100644 --- a/repos/base-hw/src/core/kernel/cpu.h +++ b/repos/base-hw/src/core/kernel/cpu.h @@ -36,35 +36,8 @@ namespace Kernel { } -/* - * The 'Cpu' class violates the "Effective C++" practices because it publicly - * inherits the 'Genode::Cpu' base class, which does not have a virtual - * destructor. Since 'Cpu' implements the 'Timeout' interface, however, it has - * a vtable. - * - * Adding a virtual destructor in the base class would be unnatural as the base - * class hierarchy does not represent an abstract interface. - * - * Inheriting the 'Genode::Cpu' class privately is not an option because the - * user of 'Cpu' class expects architecture-specific register definitions to be - * provided by 'Cpu'. Hence, all those architecture- specific definitions would - * end up as 'using' clauses in the generic class. - * - * XXX Remove the disabled warning, e.g., by one of the following approaches: - * - * * Prevent 'Cpu' to have virtual methods by making 'Timeout' a member instead - * of a base class. - * - * * Change the class hierarchy behind 'Genode::Cpu' such that - * architecture-specific bits do no longer need to implicitly become part - * of the public interface of 'Cpu'. For example, register-definition types - * could all be embedded in an 'Arch_regs' type, which the 'Cpu' class could - * publicly provide via a 'typedef Genode::Cpu::Arch_regs Arch_regs'. - * Then, the 'Genode::Cpu' could be inherited privately. - */ -#pragma GCC diagnostic ignored "-Wnon-virtual-dtor" - -class Kernel::Cpu : public Core::Cpu, private Irq::Pool, private Timeout +class Kernel::Cpu : public Core::Cpu, private Irq::Pool, + public Genode::List::Element { private: @@ -126,6 +99,7 @@ class Kernel::Cpu : public Core::Cpu, private Irq::Pool, private Timeout State _state { RUN }; unsigned const _id; Board::Pic _pic; + Timeout _timeout {}; Timer _timer; Scheduler _scheduler; Idle_thread _idle; @@ -155,8 +129,6 @@ class Kernel::Cpu : public Core::Cpu, private Irq::Pool, private Timeout Pd &core_pd, Board::Global_interrupt_controller &global_irq_ctrl); - static inline unsigned primary_id() { return 0; } - /** * Raise the IPI of the CPU */ @@ -212,60 +184,41 @@ class Kernel::Cpu : public Core::Cpu, private Irq::Pool, private Timeout }; -/* - * See the comment above the 'Cpu' class definition. - */ -#pragma GCC diagnostic pop - - class Kernel::Cpu_pool { private: Inter_processor_work_list _global_work_list {}; - unsigned _nr_of_cpus; - Genode::Constructible _cpus[Board::NR_OF_CPUS]; + Genode::List _cpus {}; + + friend class Cpu; public: - Cpu_pool(unsigned nr_of_cpus); - void initialize_executing_cpu(Board::Address_space_id_allocator &addr_space_id_alloc, Irq::Pool &user_irq_pool, Pd &core_pd, Board::Global_interrupt_controller &global_irq_ctrl); - /** - * Return whether CPU object is valid and is constructed. - */ - bool cpu_valid(unsigned const id) const { - return id < _nr_of_cpus && _cpus[id].constructed(); } - - /** - * Return object of CPU 'id' - */ Cpu & cpu(unsigned const id); /** * Return object of primary CPU */ - Cpu & primary_cpu() { return cpu(Cpu::primary_id()); } - - /** - * Return object of current CPU - */ - Cpu & executing_cpu() { return cpu(Cpu::executing_id()); } + Cpu & primary_cpu() { return *_cpus.first(); } void for_each_cpu(auto const &fn) { - for (unsigned i = 0; i < _nr_of_cpus; i++) fn(cpu(i)); + Cpu * c = _cpus.first(); + while (c) { + fn(*c); + c = c->next(); + } } Inter_processor_work_list & work_list() { return _global_work_list; } - - unsigned nr_of_cpus() { return _nr_of_cpus; } }; #endif /* _CORE__KERNEL__CPU_H_ */ diff --git a/repos/base-hw/src/core/kernel/main.cc b/repos/base-hw/src/core/kernel/main.cc index 6849895966..e14b17f1c5 100644 --- a/repos/base-hw/src/core/kernel/main.cc +++ b/repos/base-hw/src/core/kernel/main.cc @@ -40,7 +40,7 @@ class Kernel::Main static Main *_instance; Lock _data_lock { }; - Cpu_pool _cpu_pool; + Cpu_pool _cpu_pool { }; Irq::Pool _user_irq_pool { }; Board::Address_space_id_allocator _addr_space_id_alloc { }; Core::Core_platform_pd _core_platform_pd { _addr_space_id_alloc }; @@ -52,8 +52,6 @@ class Kernel::Main void _handle_kernel_entry(); - Main(unsigned nr_of_cpus); - public: static Core::Platform_pd &core_platform_pd(); @@ -63,12 +61,6 @@ class Kernel::Main Kernel::Main *Kernel::Main::_instance; -Kernel::Main::Main(unsigned nr_of_cpus) -: - _cpu_pool { nr_of_cpus } -{ } - - void Kernel::Main::_handle_kernel_entry() { Cpu &cpu = _cpu_pool.cpu(Cpu::executing_id()); @@ -94,7 +86,7 @@ void Kernel::main_initialize_and_handle_kernel_entry() { using Boot_info = Hw::Boot_info; - static volatile bool instance_initialized { false }; + static Lock init_lock; static volatile unsigned nr_of_initialized_cpus { 0 }; static volatile bool kernel_initialized { false }; @@ -102,34 +94,27 @@ void Kernel::main_initialize_and_handle_kernel_entry() *reinterpret_cast(Hw::Mm::boot_info().base) }; unsigned const nr_of_cpus { boot_info.cpus }; - bool const primary_cpu { Cpu::executing_id() == Cpu::primary_id() }; - if (primary_cpu) { + /** + * Let the first CPU create a Main object and initialize the static + * reference to it. + */ + { + Lock::Guard guard(init_lock); - /** - * Let the primary CPU create a Main object and initialize the static - * reference to it. - */ - static Main instance { nr_of_cpus }; + static Main instance; Main::_instance = &instance; - } else { - - /** - * Let secondary CPUs block until the primary CPU has managed to set - * up the Main instance. - */ - while (!instance_initialized) { } } - if (Main::_instance->_cpu_pool.cpu_valid(Cpu::executing_id())) { - /* the CPU resumed since the cpu object is already valid */ + /* the CPU resumed if the kernel is already initialized */ + if (kernel_initialized) { + { Lock::Guard guard(Main::_instance->_data_lock); - if (kernel_initialized) { + if (nr_of_initialized_cpus == nr_of_cpus) { nr_of_initialized_cpus = 0; - kernel_initialized = false; Main::_instance->_serial.init(); Main::_instance->_global_irq_ctrl.init(); @@ -140,12 +125,11 @@ void Kernel::main_initialize_and_handle_kernel_entry() Main::_instance->_cpu_pool.cpu(Cpu::executing_id()).reinit_cpu(); if (nr_of_initialized_cpus == nr_of_cpus) { - kernel_initialized = true; Genode::raw("kernel resumed"); } } - while (!kernel_initialized) { } + while (nr_of_initialized_cpus < nr_of_cpus) { } Main::_instance->_handle_kernel_entry(); /* never reached */ @@ -158,7 +142,6 @@ void Kernel::main_initialize_and_handle_kernel_entry() * CPU pool. */ Lock::Guard guard(Main::_instance->_data_lock); - instance_initialized = true; Main::_instance->_cpu_pool.initialize_executing_cpu( Main::_instance->_addr_space_id_alloc, Main::_instance->_user_irq_pool, @@ -174,42 +157,40 @@ void Kernel::main_initialize_and_handle_kernel_entry() */ while (nr_of_initialized_cpus < nr_of_cpus) { } - if (primary_cpu) { - - /** - * Let the primary CPU initialize the core main thread and finish - * initialization of the boot info. - */ - + /** + * Let the primary CPU initialize the core main thread and finish + * initialization of the boot info. + */ + { Lock::Guard guard(Main::_instance->_data_lock); - Main::_instance->_cpu_pool.for_each_cpu([&] (Kernel::Cpu &cpu) { - boot_info.kernel_irqs.add(cpu.timer().interrupt_id()); - }); - boot_info.kernel_irqs.add((unsigned)Board::Pic::IPI); + if (Cpu::executing_id() == Main::_instance->_cpu_pool.primary_cpu().id()) { + Main::_instance->_cpu_pool.for_each_cpu([&] (Kernel::Cpu &cpu) { + boot_info.kernel_irqs.add(cpu.timer().interrupt_id()); + }); + boot_info.kernel_irqs.add((unsigned)Board::Pic::IPI); - Main::_instance->_core_main_thread.construct( - Main::_instance->_addr_space_id_alloc, - Main::_instance->_user_irq_pool, - Main::_instance->_cpu_pool, - Main::_instance->_core_platform_pd.kernel_pd()); + Main::_instance->_core_main_thread.construct( + Main::_instance->_addr_space_id_alloc, + Main::_instance->_user_irq_pool, + Main::_instance->_cpu_pool, + Main::_instance->_core_platform_pd.kernel_pd()); - boot_info.core_main_thread_utcb = - (addr_t)Main::_instance->_core_main_thread->utcb(); + boot_info.core_main_thread_utcb = + (addr_t)Main::_instance->_core_main_thread->utcb(); - Genode::log(""); - Genode::log("kernel initialized"); - kernel_initialized = true; - - } else { - - /** - * Let secondary CPUs block until the primary CPU has initialized the - * core main thread and finished initialization of the boot info. - */ - while (!kernel_initialized) {;} + Genode::log(""); + Genode::log("kernel initialized"); + kernel_initialized = true; + } } + /** + * Let secondary CPUs block until the primary CPU has initialized the + * core main thread and finished initialization of the boot info. + */ + while (!kernel_initialized) {;} + Main::_instance->_handle_kernel_entry(); } diff --git a/repos/base-hw/src/core/kernel/thread.cc b/repos/base-hw/src/core/kernel/thread.cc index f794d6f86a..b4febf8070 100644 --- a/repos/base-hw/src/core/kernel/thread.cc +++ b/repos/base-hw/src/core/kernel/thread.cc @@ -718,9 +718,8 @@ void Thread::_call_new_irq() Genode::Irq_session::Polarity polarity = (Genode::Irq_session::Polarity) (user_arg_3() & 0b11); - _call_new( - (unsigned)user_arg_2(), trigger, polarity, *c, - _cpu_pool.executing_cpu().pic(), _user_irq_pool); + _call_new((unsigned)user_arg_2(), trigger, polarity, *c, + _cpu->pic(), _user_irq_pool); } diff --git a/repos/base-hw/src/core/spec/x86_64/kernel/thread.cc b/repos/base-hw/src/core/spec/x86_64/kernel/thread.cc index 361246bd5a..89d87ad049 100644 --- a/repos/base-hw/src/core/spec/x86_64/kernel/thread.cc +++ b/repos/base-hw/src/core/spec/x86_64/kernel/thread.cc @@ -142,9 +142,8 @@ void Kernel::Thread::_call_suspend() /* single core CPU case */ if (cpu_count == 1) { - auto &cpu = _cpu_pool.executing_cpu(); - /* this CPU triggers final ACPI suspend outside kernel lock */ - cpu.next_state_suspend(); + /* current CPU triggers final ACPI suspend outside kernel lock */ + _cpu->next_state_suspend(); return; } diff --git a/repos/base-hw/src/include/hw/spec/x86_64/pc_board.h b/repos/base-hw/src/include/hw/spec/x86_64/pc_board.h index 68fe21d18a..be21f64a89 100644 --- a/repos/base-hw/src/include/hw/spec/x86_64/pc_board.h +++ b/repos/base-hw/src/include/hw/spec/x86_64/pc_board.h @@ -24,7 +24,11 @@ namespace Hw::Pc_board { struct Serial; enum Dummies { UART_BASE, UART_CLOCK }; - static constexpr Genode::size_t NR_OF_CPUS = 32; + /** + * The constant 'NR_OF_CPUS' defines the _maximum_ of cpus currently + * supported on x86. The actual number is detected at booting. + */ + static constexpr Genode::size_t NR_OF_CPUS = 256; }