EXI: Improve behavior of savestates made while game is assuming the s…

…tate of the inserted memory card.

Source/Core/Core/HW/EXI/EXI.cpp

@@ -152,15 +152,15 @@ static void ChangeDeviceCallback(u64 userdata, s64 cyclesLate)
 }
 
 void ChangeDevice(const u8 channel, const TEXIDevices device_type, const u8 device_num,
-                  CoreTiming::FromThread from_thread)
+                  CoreTiming::FromThread from_thread, u32 delay)
 {
   // Let the hardware see no device for 1 second
-  CoreTiming::ScheduleEvent(0, changeDevice,
+  CoreTiming::ScheduleEvent(delay, changeDevice,
                             ((u64)channel << 32) | ((u64)EXIDEVICE_NONE << 16) | device_num,
                             from_thread);
-  CoreTiming::ScheduleEvent(SystemTimers::GetTicksPerSecond(), changeDevice,
-                            ((u64)channel << 32) | ((u64)device_type << 16) | device_num,
-                            from_thread);
+  CoreTiming::ScheduleEvent(
+      SystemTimers::GetTicksPerSecond() + static_cast<s64>(delay), changeDevice,
+      ((u64)channel << 32) | ((u64)device_type << 16) | device_num, from_thread);
 }
 
 CEXIChannel* GetChannel(u32 index)

Source/Core/Core/HW/EXI/EXI.h

@@ -41,7 +41,8 @@ void UpdateInterrupts();
 void ScheduleUpdateInterrupts(CoreTiming::FromThread from, int cycles_late);
 
 void ChangeDevice(const u8 channel, const TEXIDevices device_type, const u8 device_num,
-                  CoreTiming::FromThread from_thread = CoreTiming::FromThread::NON_CPU);
+                  CoreTiming::FromThread from_thread = CoreTiming::FromThread::NON_CPU,
+                  u32 delay = 0);
 
 CEXIChannel* GetChannel(u32 index);
 

Source/Core/Core/HW/EXI/EXI_Channel.cpp

@@ -13,7 +13,9 @@
 #include "Core/CoreTiming.h"
 #include "Core/HW/EXI/EXI.h"
 #include "Core/HW/EXI/EXI_Device.h"
+#include "Core/HW/EXI/EXI_DeviceMemoryCard.h"
 #include "Core/HW/MMIO.h"
+#include "Core/HW/SystemTimers.h"
 #include "Core/Movie.h"
 
 namespace ExpansionInterface
@@ -238,48 +240,86 @@ void CEXIChannel::DoState(PointerWrap& p)
   p.Do(m_dma_length);
   p.Do(m_control);
   p.Do(m_imm_data);
-
-  Memcard::HeaderData old_header_data = m_memcard_header_data;
   p.DoPOD(m_memcard_header_data);
 
   for (int device_index = 0; device_index < NUM_DEVICES; ++device_index)
   {
-    std::unique_ptr<IEXIDevice>& device = m_devices[device_index];
-    TEXIDevices type = device->m_device_type;
+    TEXIDevices type = m_devices[device_index]->m_device_type;
     p.Do(type);
 
-    if (type == device->m_device_type)
-    {
-      device->DoState(p);
-    }
-    else
-    {
-      std::unique_ptr<IEXIDevice> save_device =
-          EXIDevice_Create(type, m_channel_id, m_memcard_header_data);
-      save_device->DoState(p);
-      AddDevice(std::move(save_device), device_index, false);
-    }
+    if (type != m_devices[device_index]->m_device_type)
+      AddDevice(EXIDevice_Create(type, m_channel_id, m_memcard_header_data), device_index, false);
+
+    m_devices[device_index]->DoState(p);
 
-    if (type == EXIDEVICE_MEMORYCARDFOLDER && old_header_data != m_memcard_header_data &&
-        !Movie::IsMovieActive())
+    bool is_memcard = type == EXIDEVICE_MEMORYCARD || type == EXIDEVICE_MEMORYCARDFOLDER;
+    if (is_memcard && !Movie::IsMovieActive())
     {
-      // We have loaded a savestate that has a GCI folder memcard that is different to the virtual
-      // card that is currently active. In order to prevent the game from recognizing this card as a
-      // 'different' memory card and preventing saving on it, we need to reinitialize the GCI folder
-      // card here with the loaded header data.
-      // We're intentionally calling ExpansionInterface::ChangeDevice() here instead of changing it
-      // directly so we don't switch immediately but after a delay, as if changed in the GUI. This
-      // should prevent games from assuming any stale data about the memory card, such as location
-      // of the individual save blocks, which may be different on the reinitialized card.
-      // Additionally, we immediately force the memory card to None so that any 'in-flight' writes
-      // (if someone managed to savestate while saving...) don't happen to hit the card.
-      // TODO: It might actually be enough to just switch to the card with the
-      // notify_presence_changed flag set to true? Not sure how software behaves if the previous and
-      // the new device type are identical in this case. I assume there is a reason we have this
-      // grace period when switching in the GUI.
-      AddDevice(EXIDEVICE_NONE, device_index);
-      ExpansionInterface::ChangeDevice(m_channel_id, EXIDEVICE_MEMORYCARDFOLDER, device_index,
-                                       CoreTiming::FromThread::CPU);
+      // We are processing a savestate that has a memory card plugged into the system, but don't
+      // want to actually store the memory card in the savestate, so loading older savestates
+      // doesn't confusingly revert in-game saves done since then.
+
+      // Handling this properly is somewhat complex. When loading a state, the game still needs to
+      // see the memory card device as it was (or at least close enough) when the state was made,
+      // but at the same time we need to tell the game that the data on the card may have been
+      // changed and it should not assume that the data (especially the file system blocks) it may
+      // or may not have read before is still valid.
+
+      // To accomplish this we do the following:
+
+      CEXIMemoryCard* card_device = static_cast<CEXIMemoryCard*>(m_devices[device_index].get());
+
+      if (p.GetMode() != PointerWrap::MODE_READ)
+      {
+        // In any of the non-modifying modes, we just read the file system blocks from the currently
+        // active memory card and push them into p, so we have a reference of how the card file
+        // system looked at the time of savestate creation.
+        std::array<u8, 0xa000> card_file_system_data;
+        bool read_success =
+            card_device->ReadFromMemcard(0, 0xa000, card_file_system_data.data()) == 0xa000;
+        p.Do(read_success);
+        if (read_success)
+          p.DoPOD(card_file_system_data);
+      }
+      else
+      {
+        // When loading a state, we compare this previously written file system block data with the
+        // data currently in the card. If it mismatches in any way, we make sure to notify the game.
+        bool should_notify_game = false;
+        bool has_card_file_system_data;
+        p.Do(has_card_file_system_data);
+        if (has_card_file_system_data)
+        {
+          std::array<u8, 0xa000> state_file_system_data;
+          p.DoPOD(state_file_system_data);
+          std::array<u8, 0xa000> card_file_system_data;
+          bool read_success =
+              card_device->ReadFromMemcard(0, 0xa000, card_file_system_data.data()) == 0xa000;
+          should_notify_game = !(read_success && state_file_system_data == card_file_system_data);
+        }
+        else
+        {
+          should_notify_game = true;
+        }
+
+        if (should_notify_game)
+        {
+          // We want to tell the game that the card is different, but don't want to immediately
+          // destroy the memory card device, as there may be pending operations running on the CPU
+          // side (at the very least, I've seen memory accesses to 0 when switching the device
+          // immediately in F-Zero GX). In order to ensure data on the card isn't corrupted by
+          // these, we disable writes to the memory card device.
+          card_device->DisableWrites();
+
+          // And then we force a re-load of the memory card by switching to a null device a frame
+          // from now, then back to the memory card a few frames later. This also makes sure that
+          // the GCI folder header matches what the game expects, so the game never recognizes it as
+          // a 'different' memory card and prevents saving on it.
+          ExpansionInterface::ChangeDevice(m_channel_id, type, device_index,
+                                           CoreTiming::FromThread::CPU,
+                                           SystemTimers::GetTicksPerSecond() / 60);
+        }
+      }
     }
   }
 }

Source/Core/Core/State.cpp

@@ -74,7 +74,7 @@ static Common::Event g_compressAndDumpStateSyncEvent;
 static std::thread g_save_thread;
 
 // Don't forget to increase this after doing changes on the savestate system
-constexpr u32 STATE_VERSION = 122;  // Last changed in PR 8571
+constexpr u32 STATE_VERSION = 123;  // Last changed in PR 9027
 
 // Maps savestate versions to Dolphin versions.
 // Versions after 42 don't need to be added to this list,