From 0209de123b0e8dfd793d23c6a9cb825ea6da5b8e Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 7 Mar 2019 18:34:22 -0500 Subject: [PATCH 1/3] kernel/svc: Move address arbiter waiting behind a unified API function Rather than let the service call itself work out which function is the proper one to call, we can make that a behavior of the arbiter itself, so we don't need to directly expose those implementation details. --- src/core/hle/kernel/address_arbiter.cpp | 20 +++++++++++++++++--- src/core/hle/kernel/address_arbiter.h | 7 +++++-- src/core/hle/kernel/svc.cpp | 16 ++-------------- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/core/hle/kernel/address_arbiter.cpp b/src/core/hle/kernel/address_arbiter.cpp index 9780a78491..b6269c7085 100644 --- a/src/core/hle/kernel/address_arbiter.cpp +++ b/src/core/hle/kernel/address_arbiter.cpp @@ -92,6 +92,20 @@ ResultCode AddressArbiter::ModifyByWaitingCountAndSignalToAddressIfEqual(VAddr a return RESULT_SUCCESS; } +ResultCode AddressArbiter::WaitForAddress(VAddr address, ArbitrationType type, s32 value, + s64 timeout_ns) { + switch (type) { + case ArbitrationType::WaitIfLessThan: + return WaitForAddressIfLessThan(address, value, timeout_ns, false); + case ArbitrationType::DecrementAndWaitIfLessThan: + return WaitForAddressIfLessThan(address, value, timeout_ns, true); + case ArbitrationType::WaitIfEqual: + return WaitForAddressIfEqual(address, value, timeout_ns); + default: + return ERR_INVALID_ENUM_VALUE; + } +} + ResultCode AddressArbiter::WaitForAddressIfLessThan(VAddr address, s32 value, s64 timeout, bool should_decrement) { // Ensure that we can read the address. @@ -113,7 +127,7 @@ ResultCode AddressArbiter::WaitForAddressIfLessThan(VAddr address, s32 value, s6 return RESULT_TIMEOUT; } - return WaitForAddress(address, timeout); + return WaitForAddressImpl(address, timeout); } ResultCode AddressArbiter::WaitForAddressIfEqual(VAddr address, s32 value, s64 timeout) { @@ -130,10 +144,10 @@ ResultCode AddressArbiter::WaitForAddressIfEqual(VAddr address, s32 value, s64 t return RESULT_TIMEOUT; } - return WaitForAddress(address, timeout); + return WaitForAddressImpl(address, timeout); } -ResultCode AddressArbiter::WaitForAddress(VAddr address, s64 timeout) { +ResultCode AddressArbiter::WaitForAddressImpl(VAddr address, s64 timeout) { SharedPtr current_thread = system.CurrentScheduler().GetCurrentThread(); current_thread->SetArbiterWaitAddress(address); current_thread->SetStatus(ThreadStatus::WaitArb); diff --git a/src/core/hle/kernel/address_arbiter.h b/src/core/hle/kernel/address_arbiter.h index e0c36f2e3e..ebda75b2a7 100644 --- a/src/core/hle/kernel/address_arbiter.h +++ b/src/core/hle/kernel/address_arbiter.h @@ -51,6 +51,10 @@ public: ResultCode ModifyByWaitingCountAndSignalToAddressIfEqual(VAddr address, s32 value, s32 num_to_wake); + /// Waits on an address with a particular arbitration type. + ResultCode WaitForAddress(VAddr address, ArbitrationType type, s32 value, s64 timeout_ns); + +private: /// Waits on an address if the value passed is less than the argument value, /// optionally decrementing. ResultCode WaitForAddressIfLessThan(VAddr address, s32 value, s64 timeout, @@ -59,9 +63,8 @@ public: /// Waits on an address if the value passed is equal to the argument value. ResultCode WaitForAddressIfEqual(VAddr address, s32 value, s64 timeout); -private: // Waits on the given address with a timeout in nanoseconds - ResultCode WaitForAddress(VAddr address, s64 timeout); + ResultCode WaitForAddressImpl(VAddr address, s64 timeout); // Gets the threads waiting on an address. std::vector> GetThreadsWaitingOnAddress(VAddr address) const; diff --git a/src/core/hle/kernel/svc.cpp b/src/core/hle/kernel/svc.cpp index 7f5c0cc862..82ceb235cd 100644 --- a/src/core/hle/kernel/svc.cpp +++ b/src/core/hle/kernel/svc.cpp @@ -1479,21 +1479,9 @@ static ResultCode WaitForAddress(VAddr address, u32 type, s32 value, s64 timeout return ERR_INVALID_ADDRESS; } + const auto arbitration_type = static_cast(type); auto& address_arbiter = Core::System::GetInstance().Kernel().AddressArbiter(); - switch (static_cast(type)) { - case AddressArbiter::ArbitrationType::WaitIfLessThan: - return address_arbiter.WaitForAddressIfLessThan(address, value, timeout, false); - case AddressArbiter::ArbitrationType::DecrementAndWaitIfLessThan: - return address_arbiter.WaitForAddressIfLessThan(address, value, timeout, true); - case AddressArbiter::ArbitrationType::WaitIfEqual: - return address_arbiter.WaitForAddressIfEqual(address, value, timeout); - default: - LOG_ERROR(Kernel_SVC, - "Invalid arbitration type, expected WaitIfLessThan, DecrementAndWaitIfLessThan " - "or WaitIfEqual but got {}", - type); - return ERR_INVALID_ENUM_VALUE; - } + return address_arbiter.WaitForAddress(address, arbitration_type, value, timeout); } // Signals to an address (via Address Arbiter) From b7f331afa3db235db39eca041fef720873f3091a Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 7 Mar 2019 18:42:44 -0500 Subject: [PATCH 2/3] kernel/svc: Move address arbiter signaling behind a unified API function Similar to how WaitForAddress was isolated to its own function, we can also move the necessary conditional checking into the address arbiter class itself, allowing us to hide the implementation details of it from public use. --- src/core/hle/kernel/address_arbiter.cpp | 18 ++++++++++++++++-- src/core/hle/kernel/address_arbiter.h | 13 ++++++++----- src/core/hle/kernel/svc.cpp | 17 ++--------------- 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/core/hle/kernel/address_arbiter.cpp b/src/core/hle/kernel/address_arbiter.cpp index b6269c7085..352190da80 100644 --- a/src/core/hle/kernel/address_arbiter.cpp +++ b/src/core/hle/kernel/address_arbiter.cpp @@ -42,7 +42,21 @@ void WakeThreads(const std::vector>& waiting_threads, s32 num_ AddressArbiter::AddressArbiter(Core::System& system) : system{system} {} AddressArbiter::~AddressArbiter() = default; -ResultCode AddressArbiter::SignalToAddress(VAddr address, s32 num_to_wake) { +ResultCode AddressArbiter::SignalToAddress(VAddr address, SignalType type, s32 value, + s32 num_to_wake) { + switch (type) { + case SignalType::Signal: + return SignalToAddressOnly(address, num_to_wake); + case SignalType::IncrementAndSignalIfEqual: + return IncrementAndSignalToAddressIfEqual(address, value, num_to_wake); + case SignalType::ModifyByWaitingCountAndSignalIfEqual: + return ModifyByWaitingCountAndSignalToAddressIfEqual(address, value, num_to_wake); + default: + return ERR_INVALID_ENUM_VALUE; + } +} + +ResultCode AddressArbiter::SignalToAddressOnly(VAddr address, s32 num_to_wake) { const std::vector> waiting_threads = GetThreadsWaitingOnAddress(address); WakeThreads(waiting_threads, num_to_wake); return RESULT_SUCCESS; @@ -60,7 +74,7 @@ ResultCode AddressArbiter::IncrementAndSignalToAddressIfEqual(VAddr address, s32 } Memory::Write32(address, static_cast(value + 1)); - return SignalToAddress(address, num_to_wake); + return SignalToAddressOnly(address, num_to_wake); } ResultCode AddressArbiter::ModifyByWaitingCountAndSignalToAddressIfEqual(VAddr address, s32 value, diff --git a/src/core/hle/kernel/address_arbiter.h b/src/core/hle/kernel/address_arbiter.h index ebda75b2a7..801ab6dab8 100644 --- a/src/core/hle/kernel/address_arbiter.h +++ b/src/core/hle/kernel/address_arbiter.h @@ -40,8 +40,15 @@ public: AddressArbiter(AddressArbiter&&) = default; AddressArbiter& operator=(AddressArbiter&&) = delete; + /// Signals an address being waited on with a particular signaling type. + ResultCode SignalToAddress(VAddr address, SignalType type, s32 value, s32 num_to_wake); + + /// Waits on an address with a particular arbitration type. + ResultCode WaitForAddress(VAddr address, ArbitrationType type, s32 value, s64 timeout_ns); + +private: /// Signals an address being waited on. - ResultCode SignalToAddress(VAddr address, s32 num_to_wake); + ResultCode SignalToAddressOnly(VAddr address, s32 num_to_wake); /// Signals an address being waited on and increments its value if equal to the value argument. ResultCode IncrementAndSignalToAddressIfEqual(VAddr address, s32 value, s32 num_to_wake); @@ -51,10 +58,6 @@ public: ResultCode ModifyByWaitingCountAndSignalToAddressIfEqual(VAddr address, s32 value, s32 num_to_wake); - /// Waits on an address with a particular arbitration type. - ResultCode WaitForAddress(VAddr address, ArbitrationType type, s32 value, s64 timeout_ns); - -private: /// Waits on an address if the value passed is less than the argument value, /// optionally decrementing. ResultCode WaitForAddressIfLessThan(VAddr address, s32 value, s64 timeout, diff --git a/src/core/hle/kernel/svc.cpp b/src/core/hle/kernel/svc.cpp index 82ceb235cd..d44def6583 100644 --- a/src/core/hle/kernel/svc.cpp +++ b/src/core/hle/kernel/svc.cpp @@ -1499,22 +1499,9 @@ static ResultCode SignalToAddress(VAddr address, u32 type, s32 value, s32 num_to return ERR_INVALID_ADDRESS; } + const auto signal_type = static_cast(type); auto& address_arbiter = Core::System::GetInstance().Kernel().AddressArbiter(); - switch (static_cast(type)) { - case AddressArbiter::SignalType::Signal: - return address_arbiter.SignalToAddress(address, num_to_wake); - case AddressArbiter::SignalType::IncrementAndSignalIfEqual: - return address_arbiter.IncrementAndSignalToAddressIfEqual(address, value, num_to_wake); - case AddressArbiter::SignalType::ModifyByWaitingCountAndSignalIfEqual: - return address_arbiter.ModifyByWaitingCountAndSignalToAddressIfEqual(address, value, - num_to_wake); - default: - LOG_ERROR(Kernel_SVC, - "Invalid signal type, expected Signal, IncrementAndSignalIfEqual " - "or ModifyByWaitingCountAndSignalIfEqual but got {}", - type); - return ERR_INVALID_ENUM_VALUE; - } + return address_arbiter.SignalToAddress(address, signal_type, value, num_to_wake); } /// This returns the total CPU ticks elapsed since the CPU was powered-on From 8e510d5afa71de2ac5745f461ae3d6156aae803a Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 7 Mar 2019 18:48:14 -0500 Subject: [PATCH 3/3] kernel: Make the address arbiter instance per-process Now that we have the address arbiter extracted to its own class, we can fix an innaccuracy with the kernel. Said inaccuracy being that there isn't only one address arbiter. Each process instance contains its own AddressArbiter instance in the actual kernel. This fixes that and gets rid of another long-standing issue that could arise when attempting to create more than one process. --- src/core/core.cpp | 2 +- src/core/hle/kernel/address_arbiter.h | 4 +++- src/core/hle/kernel/kernel.cpp | 12 +----------- src/core/hle/kernel/kernel.h | 6 ------ src/core/hle/kernel/process.cpp | 9 +++++---- src/core/hle/kernel/process.h | 22 ++++++++++++++++++++-- src/core/hle/kernel/svc.cpp | 6 ++++-- src/tests/core/arm/arm_test_common.cpp | 2 +- 8 files changed, 35 insertions(+), 28 deletions(-) diff --git a/src/core/core.cpp b/src/core/core.cpp index eba2177d12..89b3fb4189 100644 --- a/src/core/core.cpp +++ b/src/core/core.cpp @@ -116,7 +116,7 @@ struct System::Impl { if (web_browser == nullptr) web_browser = std::make_unique(); - auto main_process = Kernel::Process::Create(kernel, "main"); + auto main_process = Kernel::Process::Create(system, "main"); kernel.MakeCurrentProcess(main_process.get()); telemetry_session = std::make_unique(); diff --git a/src/core/hle/kernel/address_arbiter.h b/src/core/hle/kernel/address_arbiter.h index 801ab6dab8..ed0d0e69ff 100644 --- a/src/core/hle/kernel/address_arbiter.h +++ b/src/core/hle/kernel/address_arbiter.h @@ -4,8 +4,10 @@ #pragma once +#include + #include "common/common_types.h" -#include "core/hle/kernel/address_arbiter.h" +#include "core/hle/kernel/object.h" union ResultCode; diff --git a/src/core/hle/kernel/kernel.cpp b/src/core/hle/kernel/kernel.cpp index 04ea9349ee..4d224d01dd 100644 --- a/src/core/hle/kernel/kernel.cpp +++ b/src/core/hle/kernel/kernel.cpp @@ -87,7 +87,7 @@ static void ThreadWakeupCallback(u64 thread_handle, [[maybe_unused]] int cycles_ } struct KernelCore::Impl { - explicit Impl(Core::System& system) : address_arbiter{system}, system{system} {} + explicit Impl(Core::System& system) : system{system} {} void Initialize(KernelCore& kernel) { Shutdown(); @@ -138,8 +138,6 @@ struct KernelCore::Impl { std::vector> process_list; Process* current_process = nullptr; - Kernel::AddressArbiter address_arbiter; - SharedPtr system_resource_limit; Core::Timing::EventType* thread_wakeup_event_type = nullptr; @@ -192,14 +190,6 @@ const Process* KernelCore::CurrentProcess() const { return impl->current_process; } -AddressArbiter& KernelCore::AddressArbiter() { - return impl->address_arbiter; -} - -const AddressArbiter& KernelCore::AddressArbiter() const { - return impl->address_arbiter; -} - void KernelCore::AddNamedPort(std::string name, SharedPtr port) { impl->named_ports.emplace(std::move(name), std::move(port)); } diff --git a/src/core/hle/kernel/kernel.h b/src/core/hle/kernel/kernel.h index 4d292aca9c..ff17ff8654 100644 --- a/src/core/hle/kernel/kernel.h +++ b/src/core/hle/kernel/kernel.h @@ -75,12 +75,6 @@ public: /// Retrieves a const pointer to the current process. const Process* CurrentProcess() const; - /// Provides a reference to the kernel's address arbiter. - Kernel::AddressArbiter& AddressArbiter(); - - /// Provides a const reference to the kernel's address arbiter. - const Kernel::AddressArbiter& AddressArbiter() const; - /// Adds a port to the named port table void AddNamedPort(std::string name, SharedPtr port); diff --git a/src/core/hle/kernel/process.cpp b/src/core/hle/kernel/process.cpp index 8009150e06..7e8ba978c8 100644 --- a/src/core/hle/kernel/process.cpp +++ b/src/core/hle/kernel/process.cpp @@ -53,9 +53,10 @@ void SetupMainThread(Process& owner_process, KernelCore& kernel, VAddr entry_poi CodeSet::CodeSet() = default; CodeSet::~CodeSet() = default; -SharedPtr Process::Create(KernelCore& kernel, std::string&& name) { - SharedPtr process(new Process(kernel)); +SharedPtr Process::Create(Core::System& system, std::string&& name) { + auto& kernel = system.Kernel(); + SharedPtr process(new Process(system)); process->name = std::move(name); process->resource_limit = kernel.GetSystemResourceLimit(); process->status = ProcessStatus::Created; @@ -233,8 +234,8 @@ void Process::LoadModule(CodeSet module_, VAddr base_addr) { Core::System::GetInstance().ArmInterface(3).ClearInstructionCache(); } -Kernel::Process::Process(KernelCore& kernel) : WaitObject{kernel} {} -Kernel::Process::~Process() {} +Process::Process(Core::System& system) : WaitObject{system.Kernel()}, address_arbiter{system} {} +Process::~Process() = default; void Process::Acquire(Thread* thread) { ASSERT_MSG(!ShouldWait(thread), "Object unavailable!"); diff --git a/src/core/hle/kernel/process.h b/src/core/hle/kernel/process.h index dcc57ae9f2..2a132c894b 100644 --- a/src/core/hle/kernel/process.h +++ b/src/core/hle/kernel/process.h @@ -12,12 +12,17 @@ #include #include #include "common/common_types.h" +#include "core/hle/kernel/address_arbiter.h" #include "core/hle/kernel/handle_table.h" #include "core/hle/kernel/process_capability.h" #include "core/hle/kernel/vm_manager.h" #include "core/hle/kernel/wait_object.h" #include "core/hle/result.h" +namespace Core { +class System; +} + namespace FileSys { class ProgramMetadata; } @@ -116,7 +121,7 @@ public: static constexpr std::size_t RANDOM_ENTROPY_SIZE = 4; - static SharedPtr Create(KernelCore& kernel, std::string&& name); + static SharedPtr Create(Core::System& system, std::string&& name); std::string GetTypeName() const override { return "Process"; @@ -150,6 +155,16 @@ public: return handle_table; } + /// Gets a reference to the process' address arbiter. + AddressArbiter& GetAddressArbiter() { + return address_arbiter; + } + + /// Gets a const reference to the process' address arbiter. + const AddressArbiter& GetAddressArbiter() const { + return address_arbiter; + } + /// Gets the current status of the process ProcessStatus GetStatus() const { return status; @@ -251,7 +266,7 @@ public: void FreeTLSSlot(VAddr tls_address); private: - explicit Process(KernelCore& kernel); + explicit Process(Core::System& kernel); ~Process() override; /// Checks if the specified thread should wait until this process is available. @@ -309,6 +324,9 @@ private: /// Per-process handle table for storing created object handles in. HandleTable handle_table; + /// Per-process address arbiter. + AddressArbiter address_arbiter; + /// Random values for svcGetInfo RandomEntropy std::array random_entropy; diff --git a/src/core/hle/kernel/svc.cpp b/src/core/hle/kernel/svc.cpp index d44def6583..77d0e3d961 100644 --- a/src/core/hle/kernel/svc.cpp +++ b/src/core/hle/kernel/svc.cpp @@ -1480,7 +1480,8 @@ static ResultCode WaitForAddress(VAddr address, u32 type, s32 value, s64 timeout } const auto arbitration_type = static_cast(type); - auto& address_arbiter = Core::System::GetInstance().Kernel().AddressArbiter(); + auto& address_arbiter = + Core::System::GetInstance().Kernel().CurrentProcess()->GetAddressArbiter(); return address_arbiter.WaitForAddress(address, arbitration_type, value, timeout); } @@ -1500,7 +1501,8 @@ static ResultCode SignalToAddress(VAddr address, u32 type, s32 value, s32 num_to } const auto signal_type = static_cast(type); - auto& address_arbiter = Core::System::GetInstance().Kernel().AddressArbiter(); + auto& address_arbiter = + Core::System::GetInstance().Kernel().CurrentProcess()->GetAddressArbiter(); return address_arbiter.SignalToAddress(address, signal_type, value, num_to_wake); } diff --git a/src/tests/core/arm/arm_test_common.cpp b/src/tests/core/arm/arm_test_common.cpp index ea27ef90da..6fe56833df 100644 --- a/src/tests/core/arm/arm_test_common.cpp +++ b/src/tests/core/arm/arm_test_common.cpp @@ -15,7 +15,7 @@ namespace ArmTests { TestEnvironment::TestEnvironment(bool mutable_memory_) : mutable_memory(mutable_memory_), test_memory(std::make_shared(this)), kernel{Core::System::GetInstance()} { - auto process = Kernel::Process::Create(kernel, ""); + auto process = Kernel::Process::Create(Core::System::GetInstance(), ""); kernel.MakeCurrentProcess(process.get()); page_table = &process->VMManager().page_table;