Wine: Change behaviour around SMC and NtReadFile

JIT invalidation around syscalls and kernel calls is mighty fickle.
This is why the previous code path was born. In order to properly handle
this case we need strong coordination between Wine and FEX around
invalidating code and memory protections. This doesn't quite exist today
so we're kind of stuck with a kludge solution. Instead of forcing the
Persona 5 code invalidation on to every process, only do it on P5R.

This fixes a hang in msiexec with PhysX trying to do a blocking read and
jitting code or creating threads, while also maintaining the P5R
approach. The full comment is in the file about the reasoning.

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);
   }
 }