Merge pull request #5925 from Sonicadvance1/233

Wine: Change behaviour around SMC and NtReadFile

Source/Windows/ARM64EC/Module.cpp

@@ -91,6 +91,7 @@ uint64_t WineNtProtectVirtualMemorySyscallId;
 NTSTATUS NtContinueNative(ARM64_NT_CONTEXT* NativeContext, BOOLEAN Alert);
 NTSTATUS NtAllocateVirtualMemoryNative(HANDLE, PVOID*, ULONG_PTR, SIZE_T*, ULONG, ULONG);
 NTSTATUS NtProtectVirtualMemoryNative(HANDLE, PVOID*, SIZE_T*, ULONG, ULONG*);
+static fextl::string AppConfigName {};
 
 [[noreturn]]
 void JumpSetStack(uintptr_t PC, uintptr_t SP);
@@ -583,12 +584,12 @@ NTSTATUS ProcessInit() {
   FEX::Windows::InitCRTProcess();
   FEX::Windows::SetupThreadHandlers();
   const auto ExecutablePath = FEX::Windows::GetExecutableFilePath();
-  const auto ExecutableName = FEX::Windows::BaseName(ExecutablePath);
-  FEX::Config::LoadConfig(fextl::string {ExecutableName}, _environ, FEX::ReadPortabilityInformation());
+  AppConfigName = FEX::Windows::BaseName(ExecutablePath);
+  FEX::Config::LoadConfig(AppConfigName, _environ, FEX::ReadPortabilityInformation());
   FEXCore::Config::ReloadMetaLayer();
   FEX::Windows::Logging::Init();
   FEXCore::Config::Set(FEXCore::Config::CONFIG_APP_FILENAME, ExecutablePath);
-  FEXCore::Config::Set(FEXCore::Config::CONFIG_APP_CONFIG_NAME, ExecutableName);
+  FEXCore::Config::Set(FEXCore::Config::CONFIG_APP_CONFIG_NAME, AppConfigName);
 
   FEXCore::Config::Set(FEXCore::Config::CONFIG_IS64BIT_MODE, "1");
 
@@ -644,8 +645,8 @@ NTSTATUS ProcessInit() {
     StatAllocHandler = fextl::make_unique<FEX::Windows::StatAlloc>(FEXCore::SHMStats::AppType::WIN_ARM64EC);
   }
 
-  if (StartupSleep() && (StartupSleepProcName().empty() || ExecutableName == StartupSleepProcName())) {
-    LogMan::Msg::IFmt("[{}][{}] Sleeping for {} seconds", GetCurrentProcessId(), ExecutableName, StartupSleep());
+  if (StartupSleep() && (StartupSleepProcName().empty() || AppConfigName == StartupSleepProcName())) {
+    LogMan::Msg::IFmt("[{}][{}] Sleeping for {} seconds", GetCurrentProcessId(), AppConfigName, StartupSleep());
     std::this_thread::sleep_for(std::chrono::seconds(StartupSleep()));
   }
 
@@ -894,22 +895,50 @@ void BTCpu64NotifyReadFile(HANDLE Handle, void* Address, SIZE_T Size, BOOL After
     return;
   }
 
-  auto& InLockedRWXRead = GetFrontendThreadData(ThreadState)->InLockedRWXRead;
-  if (!After) {
-    ThreadCreationMutex.lock();
-    CTX->GetCodeInvalidationMutex().lock();
-    if (InvalidationTracker->BeginUntrackedWriteLocked(reinterpret_cast<uint64_t>(Address), static_cast<uint64_t>(Size))) {
-      InLockedRWXRead = true;
+  // On file read this can cause JIT code invalidations. FEX previously used to hold locks on `CodeInvalidationMutex` and `ThreadCreationMutex` during
+  // this. This was a problem that file reads are blocking and may necessarily never return. A simple unit-test that holds a blocking read, then tries
+  // to create threads or JIT code would immediately hang.
+  //
+  // - PhysX msiexec installer (From Dishonored AppID 205100) did exactly this and hung forever.
+  //
+  // The alternative approach is to let the read progress as normal and only notify invalidation when the read actually returns. This introduces an
+  // unavoidable race which by my logic is fine.
+  //
+  // 1) If a thread is already jitting code from the address range then in a "real" x86 environment it may see old, new, or a combination of both.
+  //   - NtReadFile isn't guaranteed to return new data atomically, in order, or anything of the sort.
+  // 2) The calling thread only ever sees the before code, and the after code because `NtReadFile` never leaves "kernel" space before invalidation.
+  //
+  // The only "gotcha" is that FEX could have write-protected the memory region when a JIT executed the code, and once the `NtReadFile` tries
+  // returning data, it faults. This requires coordination between Wine and FEX in order to handle this edge case.
+  //
+  // Additionally, async reads are still completely untracked with invalidation, which never worked before anyway.
+
+  if (AppConfigName == "P5R.exe") {
+    // Use the locking mechanism for (Persona 5 Royal AppID 1687950) which does exactly the problem described above.
+    auto& InLockedRWXRead = GetFrontendThreadData(ThreadState)->InLockedRWXRead;
+    if (!After) {
+      ThreadCreationMutex.lock();
+      CTX->GetCodeInvalidationMutex().lock();
+      if (InvalidationTracker->BeginUntrackedWriteLocked(reinterpret_cast<uint64_t>(Address), static_cast<uint64_t>(Size))) {
+        InLockedRWXRead = true;
+      } else {
+        CTX->GetCodeInvalidationMutex().unlock();
+        ThreadCreationMutex.unlock();
+      }
     } else {
-      CTX->GetCodeInvalidationMutex().unlock();
-      ThreadCreationMutex.unlock();
-    }
-  } else {
-    if (InLockedRWXRead) {
-      InLockedRWXRead = false;
-      CTX->GetCodeInvalidationMutex().unlock();
-      ThreadCreationMutex.unlock();
+      if (InLockedRWXRead) {
+        InLockedRWXRead = false;
+        CTX->GetCodeInvalidationMutex().unlock();
+        ThreadCreationMutex.unlock();
+      }
     }
+
+    return;
+  }
+
+  if (After && Status == STATUS_SUCCESS) {
+    std::scoped_lock Lock(ThreadCreationMutex);
+    InvalidationTracker->InvalidateAlignedInterval(reinterpret_cast<uint64_t>(Address), static_cast<uint64_t>(Size), false);
   }
 }
 

Source/Windows/WOW64/Module.cpp

@@ -1034,22 +1034,15 @@ void BTCpuNotifyUnmapViewOfSection(void* Address, BOOL After, ULONG Status) {
 }
 
 void BTCpuNotifyReadFile(HANDLE Handle, void* Address, SIZE_T Size, BOOL After, NTSTATUS Status) {
-  auto& InLockedRWXRead = GetFrontendThreadData(GetTLS().ThreadState())->InLockedRWXRead;
-  if (!After) {
-    ThreadCreationMutex.lock();
-    CTX->GetCodeInvalidationMutex().lock();
-    if (InvalidationTracker->BeginUntrackedWriteLocked(reinterpret_cast<uint64_t>(Address), static_cast<uint64_t>(Size))) {
-      InLockedRWXRead = true;
-    } else {
-      CTX->GetCodeInvalidationMutex().unlock();
-      ThreadCreationMutex.unlock();
-    }
-  } else {
-    if (InLockedRWXRead) {
-      InLockedRWXRead = false;
-      CTX->GetCodeInvalidationMutex().unlock();
-      ThreadCreationMutex.unlock();
-    }
+  auto* ThreadState = GetTLS().ThreadState();
+  if (!InvalidationTracker || !ThreadState) {
+    return;
+  }
+
+  // See arm64ec `BTCpu64NotifyReadFile` for why this like this.
+  if (After && Status == STATUS_SUCCESS) {
+    std::scoped_lock Lock(ThreadCreationMutex);
+    InvalidationTracker->InvalidateAlignedInterval(reinterpret_cast<uint64_t>(Address), static_cast<uint64_t>(Size), false);
   }
 }