Merge 68a78854f3 into c770e7c276

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

@@ -205,14 +205,25 @@ void ExpansionInterfaceManager::ChangeDevice(Slot slot, EXIDeviceType device_typ
 void ExpansionInterfaceManager::ChangeDevice(u8 channel, u8 device_num, EXIDeviceType device_type,
                                              CoreTiming::FromThread from_thread)
 {
-  // Let the hardware see no device for 1 second
+  ChangeDevice(channel, device_num, device_type, 0, m_system.GetSystemTimers().GetTicksPerSecond(),
+               from_thread);
+}
+
+void ExpansionInterfaceManager::ChangeDevice(u8 channel, u8 device_num, EXIDeviceType device_type,
+                                             s64 cycles_delay_change, s64 cycles_no_device_visible,
+                                             CoreTiming::FromThread from_thread)
+{
+  // Let the hardware see no device for cycles_no_device_visible cycles after waiting for
+  // cycles_delay_change cycles
   auto& core_timing = m_system.GetCoreTiming();
-  core_timing.ScheduleEvent(0, m_event_type_change_device,
-                            ((u64)channel << 32) | ((u64)EXIDeviceType::None << 16) | device_num,
+  core_timing.ScheduleEvent(cycles_delay_change, m_event_type_change_device,
+                            (static_cast<u64>(channel) << 32) |
+                                (static_cast<u64>(EXIDeviceType::None) << 16) | device_num,
                             from_thread);
   core_timing.ScheduleEvent(
-      m_system.GetSystemTimers().GetTicksPerSecond(), m_event_type_change_device,
-      ((u64)channel << 32) | ((u64)device_type << 16) | device_num, from_thread);
+      cycles_delay_change + cycles_no_device_visible, m_event_type_change_device,
+      (static_cast<u64>(channel) << 32) | (static_cast<u64>(device_type) << 16) | device_num,
+      from_thread);
 }
 
 CEXIChannel* ExpansionInterfaceManager::GetChannel(u32 index)

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

@@ -10,6 +10,7 @@
 #include "Common/CommonTypes.h"
 #include "Common/EnumFormatter.h"
 #include "Core/CoreTiming.h"
+#include "Core/HW/SystemTimers.h"
 
 class PointerWrap;
 struct Sram;
@@ -82,6 +83,9 @@ public:
                     CoreTiming::FromThread from_thread = CoreTiming::FromThread::NON_CPU);
   void ChangeDevice(u8 channel, u8 device_num, EXIDeviceType device_type,
                     CoreTiming::FromThread from_thread = CoreTiming::FromThread::NON_CPU);
+  void ChangeDevice(u8 channel, u8 device_num, EXIDeviceType device_type, s64 cycles_delay_change,
+                    s64 cycles_no_device_visible,
+                    CoreTiming::FromThread from_thread = CoreTiming::FromThread::NON_CPU);
 
   CEXIChannel* GetChannel(u32 index);
   IEXIDevice* GetDevice(Slot slot);

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"
 #include "Core/System.h"
 
@@ -241,48 +243,94 @@ 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.Do(m_memcard_header_data);
 
   for (int device_index = 0; device_index < NUM_DEVICES; ++device_index)
   {
-    std::unique_ptr<IEXIDevice>& device = m_devices[device_index];
-    EXIDeviceType type = device->m_device_type;
+    EXIDeviceType type = m_devices[device_index]->m_device_type;
     p.Do(type);
 
-    if (type == device->m_device_type)
+    if (type != m_devices[device_index]->m_device_type)
     {
-      device->DoState(p);
-    }
-    else
-    {
-      std::unique_ptr<IEXIDevice> save_device =
-          EXIDevice_Create(m_system, type, m_channel_id, m_memcard_header_data);
-      save_device->DoState(p);
-      AddDevice(std::move(save_device), device_index, false);
+      AddDevice(EXIDevice_Create(m_system, type, m_channel_id, m_memcard_header_data), device_index,
+                false);
     }
 
-    if (type == EXIDeviceType::MemoryCardFolder && old_header_data != m_memcard_header_data &&
-        !m_system.GetMovie().IsMovieActive())
+    m_devices[device_index]->DoState(p);
+
+    const bool is_memcard =
+        (type == EXIDeviceType::MemoryCard || type == EXIDeviceType::MemoryCardFolder);
+    if (is_memcard && !m_system.GetMovie().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(EXIDeviceType::None, device_index);
-      m_system.GetExpansionInterface().ChangeDevice(
-          m_channel_id, device_index, EXIDeviceType::MemoryCardFolder, 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());
+      constexpr s32 file_system_data_size = 0xa000;
+
+      if (p.IsReadMode())
+      {
+        // When loading a state, we compare the 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, file_system_data_size> state_file_system_data;
+          p.Do(state_file_system_data);
+          std::array<u8, file_system_data_size> card_file_system_data;
+          const s32 bytes_read =
+              card_device->ReadFromMemcard(0, file_system_data_size, card_file_system_data.data());
+          bool read_success = bytes_read == file_system_data_size;
+          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.
+          const u32 cycles_per_second = m_system.GetSystemTimers().GetTicksPerSecond();
+          m_system.GetExpansionInterface().ChangeDevice(
+              m_channel_id, device_index, type, cycles_per_second / 60, cycles_per_second / 3,
+              CoreTiming::FromThread::CPU);
+        }
+      }
+      else
+      {
+        // When saving a state (or when we're measuring or verifiying the state data) we 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, file_system_data_size> card_file_system_data;
+        const s32 bytes_read =
+            card_device->ReadFromMemcard(0, file_system_data_size, card_file_system_data.data());
+        bool read_success = bytes_read == file_system_data_size;
+        p.Do(read_success);
+        if (read_success)
+          p.Do(card_file_system_data);
+      }
     }
   }
 }

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

@@ -172,6 +172,19 @@ CEXIMemoryCard::GetGCIFolderPath(Slot card_slot, AllowMovieFolder allow_movie_fo
   return {Config::GetGCIFolderPath(card_slot, region), true};
 }
 
+s32 CEXIMemoryCard::ReadFromMemcard(u32 memcard_offset, s32 length, u8* dest_address) const
+{
+  if (!m_memory_card)
+    return 0;
+
+  return m_memory_card->Read(memcard_offset, length, dest_address);
+}
+
+void CEXIMemoryCard::DisableWrites()
+{
+  m_allow_writes = false;
+}
+
 void CEXIMemoryCard::SetupGciFolder(const Memcard::HeaderData& header_data)
 {
   const std::string& game_id = SConfig::GetInstance().GetGameID();
@@ -289,7 +302,8 @@ void CEXIMemoryCard::SetCS(int cs)
     case Command::SectorErase:
       if (m_position > 2)
       {
-        m_memory_card->ClearBlock(m_address & (m_memory_card_size - 1));
+        if (m_allow_writes)
+          m_memory_card->ClearBlock(m_address & (m_memory_card_size - 1));
         m_status |= MC_STATUS_BUSY;
         m_status &= ~MC_STATUS_READY;
 
@@ -302,7 +316,8 @@ void CEXIMemoryCard::SetCS(int cs)
     case Command::ChipErase:
       if (m_position > 2)
       {
-        m_memory_card->ClearAll();
+        if (m_allow_writes)
+          m_memory_card->ClearAll();
         m_status &= ~MC_STATUS_BUSY;
       }
       break;
@@ -316,7 +331,8 @@ void CEXIMemoryCard::SetCS(int cs)
 
         while (count--)
         {
-          m_memory_card->Write(m_address, 1, &(m_programming_buffer[i++]));
+          if (m_allow_writes)
+            m_memory_card->Write(m_address, 1, &(m_programming_buffer[i++]));
           i &= 127;
           m_address = (m_address & ~0x1FF) | ((m_address + 1) & 0x1FF);
         }
@@ -542,12 +558,16 @@ void CEXIMemoryCard::DMARead(u32 addr, u32 size)
 // write all at once instead of single byte at a time as done by IEXIDevice::DMAWrite
 void CEXIMemoryCard::DMAWrite(u32 addr, u32 size)
 {
-  auto& memory = m_system.GetMemory();
-  m_memory_card->Write(m_address, size, memory.GetPointerForRange(addr, size));
+  if (m_allow_writes)
+  {
+    auto& memory = m_system.GetMemory();
+    m_memory_card->Write(m_address, size, memory.GetPointerForRange(addr, size));
+  }
 
   if (((m_address + size) % Memcard::BLOCK_SIZE) == 0)
   {
-    INFO_LOG_FMT(EXPANSIONINTERFACE, "writing to block: {:x}", m_address / Memcard::BLOCK_SIZE);
+    INFO_LOG_FMT(EXPANSIONINTERFACE, "{}writing to block: {:x}", m_allow_writes ? "" : "fake ",
+                 m_address / Memcard::BLOCK_SIZE);
   }
 
   // Schedule transfer complete later based on write speed

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

@@ -64,6 +64,15 @@ public:
   static std::pair<std::string /* path */, bool /* migrate */>
   GetGCIFolderPath(Slot card_slot, AllowMovieFolder allow_movie_folder, Movie::MovieManager& movie);
 
+  s32 ReadFromMemcard(u32 memcard_offset, s32 length, u8* dest_address) const;
+
+  // After this call all writes to the card are disabled.
+  // This is used to have a 'grace period' after loading a savestate where the emulated software
+  // still sees the memory card but can't write to it anymore.
+  // It is expected that this device is destroyed and possibly recreated soon (within a few emulated
+  // frames) after this method has been called.
+  void DisableWrites();
+
 private:
   void SetupGciFolder(const Memcard::HeaderData& header_data);
   void SetupRawMemcard(u16 size_mb);
@@ -119,6 +128,7 @@ private:
   unsigned int m_address;
   u32 m_memory_card_size;
   std::unique_ptr<MemoryCardBase> m_memory_card;
+  bool m_allow_writes = true;
 
 protected:
   void TransferByte(u8& byte) override;

Source/Core/Core/State.cpp

@@ -99,7 +99,7 @@ static size_t s_state_writes_in_queue;
 static std::condition_variable s_state_write_queue_is_empty;
 
 // Don't forget to increase this after doing changes on the savestate system
-constexpr u32 STATE_VERSION = 169;  // Last changed in PR 13074
+constexpr u32 STATE_VERSION = 170;  // Last changed in PR 9027
 
 // Increase this if the StateExtendedHeader definition changes
 constexpr u32 EXTENDED_HEADER_VERSION = 1;  // Last changed in PR 12217