diff --git a/Makefile b/Makefile index 55f390a0..e4450093 100644 --- a/Makefile +++ b/Makefile @@ -1777,6 +1777,16 @@ test: test/test_dram_timing test/test_cheat test/test_event_queue test/test_jlin else \ bash scripts/test-skip.sh record "Doom (savestate determinism, enhancement path)" "no ROM matching 'Doom*' in the private corpus"; \ fi + @# Issue #787: the HLE CD BIOS streams CD_read data at drive rate from + @# state that used to live outside the blob, so every HLE disc diverged + @# under run-ahead while a read was in flight. Warmup 400 lands + @# mid-stream on Hover Strike's intro (failed all four checks pre-fix). + @rom=$$(bash scripts/find-rom.sh 'Hover Strike - Unconquered Lands (USA).cue'); \ + if [ -n "$$rom" ]; then \ + ./test/tools/test_runahead_determinism ./$(TARGET) "$$rom" --warmup 400 --frames 120 --quiet; \ + else \ + bash scripts/test-skip.sh record "Hover Strike CD (HLE savestate determinism, #787)" "no disc matching 'Hover Strike - Unconquered Lands (USA).cue' in the private corpus"; \ + fi ./test/test_butch_cd ./test/test_cd_hle_idempotent ./test/test_cd_pregap diff --git a/libretro.c b/libretro.c index 6ef26eca..127d8e6d 100644 --- a/libretro.c +++ b/libretro.c @@ -4646,6 +4646,21 @@ bool retro_serialize(void *data, size_t size) STATE_SAVE_VAR(buf, disc_index); } + /* v15 (extended in place, #787): HLE CD streaming state -- the + * in-flight CD_read the HLE BIOS delivers at drive rate, plus its + * CD_poll bookkeeping. Outside the blob, a rollback replayed frames + * against a transfer that had already advanced (run-ahead divergence + * on every HLE CD title measured mid-stream). + * + * STRICTLY LAST, after the disc identity: appending keeps every older + * blob loadable, and the chunk's own magic word tells a pre-#787 v15 + * blob (zero-filled tail) from one that carries it. */ + /* Check room BEFORE writing (Kimi review on #803): the chunk is ~2.4 KB + * and the size check below only runs after the bytes are already out. */ + if ((size_t)(buf - start) + JaguarCDHLEStateSize() > STATE_SIZE) + return false; + buf += JaguarCDHLEStateSave(buf); + written = (size_t)(buf - start); if (written > STATE_SIZE) return false; @@ -4878,6 +4893,16 @@ bool retro_unserialize(const void *data, size_t size) disk_index = disc_index; } + /* v15 (extended in place, #787): HLE CD streaming state -- see the + * matching save comment. Older layouts end before it; a v15 blob + * written before #787 ends with a zero-filled tail, which the chunk's + * magic word rejects. Either way the in-flight transfer is dropped + * rather than inherited from whatever this session was doing. */ + if (version >= STATE_VERSION_HLE_CD_STREAM) + buf += JaguarCDHLEStateLoad(buf); + else + JaguarCDHLEStateReset(); + /* tomRam8 was restored raw above; recompute the DRAM/refresh timing * that bus_arbiter derives from MEMCON1/MEMCON2 so it matches the * loaded state (dram_row_miss/rom_clocks/dram_refresh_clks from diff --git a/src/cd/jagcd_hle.c b/src/cd/jagcd_hle.c index 38145aa8..2e466fad 100644 --- a/src/cd/jagcd_hle.c +++ b/src/cd/jagcd_hle.c @@ -26,6 +26,7 @@ #include "dsp.h" #include "jaguar.h" #include "m68000/m68kinterface.h" +#include "state.h" /* There is NO DSP-RAM "CD transfer done" flag. An earlier HLE wrote * $00000000 / $FFFFFFFF to $F1B4C8 around every CD_read, citing a @@ -1414,6 +1415,175 @@ uint32_t JaguarCDHLEStreamArmCount(void) return hle_stream_arm_count; } +/* ------------------------------------------------------------------ */ +/* Savestate chunk (#787) */ +/* */ +/* Field by field, never the raw struct: padding bytes and the size of */ +/* `bool` are compiler properties, and a rollback-netplay peer may run */ +/* a different build. hle_active is NOT saved -- it is set by the boot */ +/* strategy (HLE vs real BIOS), which is session configuration like */ +/* NTSC/PAL. hle_stream_arm_count is a probe counter, not machine */ +/* state. */ +/* ------------------------------------------------------------------ */ + +#define HLE_STATE_MAGIC 0x31454C48u /* "HLE1" little-endian */ +#define HLE_STATE_RAM_SIZE 0x200000u /* 2 MB main RAM */ + +/* A GPU data-area base is either unset (0) or a range that holds its + * +0/+4/+8 longs: main RAM, or GPU local RAM ($F03000-$F03FFF, where the + * BIOS default $F03B00 and Battle Morph's $F03158 live). */ +static bool hle_state_base_ok(uint32_t base) +{ + if (base == 0) + return true; + if ((base & 3u) != 0) + return false; + if (base <= HLE_STATE_RAM_SIZE - 12u) + return true; + return base >= 0xF03000u && base <= 0xF04000u - 12u; +} + +/* Bytes JaguarCDHLEStateSave writes, so the caller can check room first. */ +size_t JaguarCDHLEStateSize(void) +{ + return 4u + 1u + 7u * 4u /* magic, pending, 7 bookkeeping words */ + + 2u + 15u * 4u /* active, bufValid, 15 stream words */ + + sizeof(hleStream.buf); +} + +size_t JaguarCDHLEStateSave(uint8_t *buf) +{ + uint8_t *start = buf; + uint32_t magic = HLE_STATE_MAGIC; + uint8_t b; + + STATE_SAVE_VAR(buf, magic); + + b = hle_read_pending ? 1 : 0; + STATE_SAVE_VAR(buf, b); + STATE_SAVE_VAR(buf, hle_read_dest); + STATE_SAVE_VAR(buf, hle_read_end_addr); + STATE_SAVE_VAR(buf, hle_read_progress); + STATE_SAVE_VAR(buf, hle_gpu_data_base); + STATE_SAVE_VAR(buf, hle_post_read_lba); + STATE_SAVE_VAR(buf, hle_align_phase_track); + STATE_SAVE_VAR(buf, hle_align_phase); + + b = hleStream.active ? 1 : 0; + STATE_SAVE_VAR(buf, b); + b = hleStream.bufValid ? 1 : 0; + STATE_SAVE_VAR(buf, b); + STATE_SAVE_VAR(buf, hleStream.lba); + STATE_SAVE_VAR(buf, hleStream.bufOff); + STATE_SAVE_VAR(buf, hleStream.dest); + STATE_SAVE_VAR(buf, hleStream.total); + STATE_SAVE_VAR(buf, hleStream.reqTotal); + STATE_SAVE_VAR(buf, hleStream.written); + STATE_SAVE_VAR(buf, hleStream.accFrac); + STATE_SAVE_VAR(buf, hleStream.d1); + STATE_SAVE_VAR(buf, hleStream.statusBase); + STATE_SAVE_VAR(buf, hleStream.sigD0); + STATE_SAVE_VAR(buf, hleStream.sigD1); + STATE_SAVE_VAR(buf, hleStream.sigA0); + STATE_SAVE_VAR(buf, hleStream.sigA1); + STATE_SAVE_VAR(buf, hleStream.speedMult); + STATE_SAVE_VAR(buf, hleStream.startDelay); + /* The sector buffer holds the un-swapped bytes of the sector being + * delivered; re-reading it from the disc on load would work too, but + * only for a valid buffer, and this keeps the load path trivial. */ + STATE_SAVE_BUF(buf, hleStream.buf, sizeof(hleStream.buf)); + + return (size_t)(buf - start); +} + +void JaguarCDHLEStateReset(void) +{ + hleStream.active = false; + hleStream.bufValid = false; + hle_read_pending = false; + hle_read_progress = 0; +} + +size_t JaguarCDHLEStateLoad(const uint8_t *buf) +{ + const uint8_t *start = buf; + uint32_t magic; + uint8_t pending, active, bufValid; + + STATE_LOAD_VAR(buf, magic); + if (magic != HLE_STATE_MAGIC) + { + /* No chunk: a v15 blob written before #787 (its zero-filled tail + * reads as magic 0). Nothing past this point is ours. */ + JaguarCDHLEStateReset(); + return (size_t)(buf - start); + } + + STATE_LOAD_VAR(buf, pending); + STATE_LOAD_VAR(buf, hle_read_dest); + STATE_LOAD_VAR(buf, hle_read_end_addr); + STATE_LOAD_VAR(buf, hle_read_progress); + STATE_LOAD_VAR(buf, hle_gpu_data_base); + STATE_LOAD_VAR(buf, hle_post_read_lba); + STATE_LOAD_VAR(buf, hle_align_phase_track); + STATE_LOAD_VAR(buf, hle_align_phase); + hle_read_pending = pending != 0; + + STATE_LOAD_VAR(buf, active); + STATE_LOAD_VAR(buf, bufValid); + STATE_LOAD_VAR(buf, hleStream.lba); + STATE_LOAD_VAR(buf, hleStream.bufOff); + STATE_LOAD_VAR(buf, hleStream.dest); + STATE_LOAD_VAR(buf, hleStream.total); + STATE_LOAD_VAR(buf, hleStream.reqTotal); + STATE_LOAD_VAR(buf, hleStream.written); + STATE_LOAD_VAR(buf, hleStream.accFrac); + STATE_LOAD_VAR(buf, hleStream.d1); + STATE_LOAD_VAR(buf, hleStream.statusBase); + STATE_LOAD_VAR(buf, hleStream.sigD0); + STATE_LOAD_VAR(buf, hleStream.sigD1); + STATE_LOAD_VAR(buf, hleStream.sigA0); + STATE_LOAD_VAR(buf, hleStream.sigA1); + STATE_LOAD_VAR(buf, hleStream.speedMult); + STATE_LOAD_VAR(buf, hleStream.startDelay); + STATE_LOAD_BUF(buf, hleStream.buf, sizeof(hleStream.buf)); + hleStream.active = active != 0; + hleStream.bufValid = bufValid != 0; + + /* Everything below came from a file. The arm path only ever builds + * these from a bounded CD_read, so a state that a real session wrote + * always passes; a damaged or crafted one must not turn into an + * out-of-bounds access (Copilot review on #803): + * - StreamTick indexes buf[bufOff..2351] and copies total - written; + * - HLEStreamFinish writes the pad/ATRI block at dest + total, with a + * bound check that a dest near $FFFFFFFF would wrap past; + * - CD_poll and the status writes touch base +0/+4/+8. + * On any failure drop the transfer AND the pointers it would use. */ + if (!hle_state_base_ok(hle_gpu_data_base) + || !hle_state_base_ok(hleStream.statusBase)) + { + hle_gpu_data_base = 0; + hleStream.statusBase = 0; + JaguarCDHLEStateReset(); + } + if (hleStream.bufOff > sizeof(hleStream.buf) + || hleStream.written > hleStream.total + || hleStream.reqTotal > hleStream.total + || hleStream.dest >= HLE_STATE_RAM_SIZE + || hleStream.total > HLE_STATE_RAM_SIZE - hleStream.dest) + JaguarCDHLEStateReset(); + if (hle_read_dest >= HLE_STATE_RAM_SIZE + || hle_read_end_addr < hle_read_dest + || hle_read_end_addr > HLE_STATE_RAM_SIZE) + { + hle_read_dest = 0; + hle_read_end_addr = 0; + JaguarCDHLEStateReset(); + } + + return (size_t)(buf - start); +} + void JaguarCDHLEStreamTick(void) { uint32_t budget; diff --git a/src/cd/jagcd_hle.h b/src/cd/jagcd_hle.h index 2beb42e6..80e4db22 100644 --- a/src/cd/jagcd_hle.h +++ b/src/cd/jagcd_hle.h @@ -2,6 +2,7 @@ #define __JAGCD_HLE_H__ #include +#include #include #ifdef __cplusplus @@ -58,6 +59,22 @@ uint32_t JaguarCDHLEStreamArmCount(void); /* Force HLE active state (for unit testing without a disc image). */ void JaguarCDHLESetActive(bool active); +/* Savestate chunk (#787): the in-flight streamed CD_read and the CD_read / + * CD_poll bookkeeping, which used to live outside the state blob -- so a + * rollback (run-ahead, netplay) or a load into a fresh core lost or + * leaked the transfer. Appended strictly last to the blob and tagged + * with a magic word, so any blob without it (v14 and older, and v15 + * written before this chunk existed) is detected and takes + * JaguarCDHLEStateReset() instead. Save/Load return the bytes consumed. */ +size_t JaguarCDHLEStateSave(uint8_t *buf); +size_t JaguarCDHLEStateSize(void); +size_t JaguarCDHLEStateLoad(const uint8_t *buf); +/* For a state that carries no HLE chunk: drop any in-flight transfer + * (the state's RAM and CPU registers describe a machine that is not + * mid-read as far as we can know) and leave the session-stable setup -- + * GPU data area base, align phase, post-read LBA -- as it is. */ +void JaguarCDHLEStateReset(void); + #ifdef __cplusplus } #endif diff --git a/src/core/state.h b/src/core/state.h index 587f1000..fd626400 100644 --- a/src/core/state.h +++ b/src/core/state.h @@ -83,7 +83,10 @@ extern "C" { * v15: CDROM chunk gained the last $03nn/$14nn TOC word handed out * (dsaLastMultiWord). develop only — first bump of the post-v3.6.1 * cycle, so any other in-flight state change this cycle extends v15 - * in place rather than bumping again. */ + * in place rather than bumping again. + * EXTENDED IN PLACE (still v15): #787 appended the HLE CD streaming + * chunk strictly last, behind a magic word, so a v15 blob written + * before it (zero-filled tail) loads with the transfer dropped. */ #define STATE_MAGIC 0x564A5353 /* "VJSS" */ #define STATE_VERSION 15 /* Oldest layout retro_unserialize still accepts. States between @@ -194,6 +197,9 @@ extern "C" { * loaded into a fresh core would otherwise read 0, and run-ahead/netplay * would diverge (the #400 class). Older states load it as 0. */ #define STATE_VERSION_CDROM_DSA_LASTWORD 15 +/* First version that may carry the trailing HLE CD streaming chunk + * (#787). "May": the chunk is tagged, see JaguarCDHLEStateLoad. */ +#define STATE_VERSION_HLE_CD_STREAM 15 /* Header flags */ #define STATE_FLAG_MEMTRACK 0x01 diff --git a/test/test_cd_synth_read.c b/test/test_cd_synth_read.c index 1e8a14bb..e0b8391f 100644 --- a/test/test_cd_synth_read.c +++ b/test/test_cd_synth_read.c @@ -82,6 +82,11 @@ static uint32_t (*p_cd_total)(void); static bool (*p_load_game)(const struct retro_game_info *); static void (*p_unload_game)(void); static void (*p_run)(void); +static size_t (*p_ser_size)(void); +static bool (*p_serialize)(void *, size_t); +static bool (*p_unserialize)(const void *, size_t); +static size_t (*p_hle_state_save)(uint8_t *); +static size_t (*p_hle_state_size)(void); static uint8_t *ram; @@ -577,6 +582,85 @@ TEST(delivery_is_streamed_not_instant) bad, ram[DEST_A + bad], expect_ram(bad)); } +/* ------------------------------------------------------------------ */ +/* 3b. An in-flight stream survives a savestate round trip (#787) */ +/* ------------------------------------------------------------------ */ + +/* The HLE stream lives in jagcd_hle.c statics. Before #787 none of it was + * in the state blob, so restoring a state saved mid-transfer left the + * stream wherever the live session had taken it -- here, already finished + * -- and the bytes still owed after the save point never arrived: exactly + * what run-ahead's rollback does every frame. Save part-way through, + * finish the transfer, restore, and the SAME transfer must be in flight + * again and complete byte-exact. + * + * Negative control (verified): on the pre-#787 core the stream is inactive + * right after the restore and this test goes red. */ +TEST(inflight_stream_survives_savestate_round_trip) +{ + uint32_t size = 0x8000u; + uint32_t totalAtSave, ticks, bad, i; + size_t ssz; + uint8_t *state; + + /* The room check in retro_serialize trusts JaguarCDHLEStateSize(). */ + { + static uint8_t chunk[8192]; + ASSERT_EQ_U32((uint32_t)p_hle_state_save(chunk), + (uint32_t)p_hle_state_size()); + } + + ssz = p_ser_size(); + state = (uint8_t *)malloc(ssz); + ASSERT_TRUE(state != NULL); + + fill_dest(DEST_A, size + 128u); + arm_read(payloadLBA, DEST_A, size); + for (i = 0; i < 20u; i++) + p_hle_tick(); + ASSERT_TRUE(p_hle_active()); + if (ram[DEST_A + size - 1u] != FILLER) + FAIL("stream finished before the save point; shorten the warmup"); + totalAtSave = p_hle_bytes(); + + if (!p_serialize(state, ssz)) + { + free(state); + FAIL("retro_serialize failed mid-stream"); + } + + /* Let the live session finish the transfer. */ + drive_stream(200000u); + if (p_hle_active()) + { + free(state); + FAIL("stream never completed before the restore"); + } + + /* Roll back to the save point. */ + if (!p_unserialize(state, ssz)) + { + free(state); + FAIL("retro_unserialize failed"); + } + free(state); + + if (!p_hle_active()) + FAIL("restored state has no stream in flight -- the transfer was lost"); + ASSERT_EQ_U32(p_hle_dest(), DEST_A); + ASSERT_EQ_U32(p_hle_bytes(), totalAtSave); + if (ram[DEST_A + size - 1u] != FILLER) + FAIL("restored RAM already holds the tail -- RAM was not rolled back"); + + ticks = drive_stream(200000u); + if (p_hle_active()) + FAIL("restored stream never completed (%u ticks)", ticks); + bad = first_bad(DEST_A, size); + if (bad != size) + FAIL("after restore, byte %u: got $%02X, expected $%02X", + bad, ram[DEST_A + bad], expect_ram(bad)); +} + /* ------------------------------------------------------------------ */ /* 4. Arming over an in-flight stream */ /* ------------------------------------------------------------------ */ @@ -725,10 +809,17 @@ static bool resolve_symbols(void) p_load_game = (bool (*)(const struct retro_game_info *))dlsym(C.handle, "retro_load_game"); p_unload_game = (void (*)(void))dlsym(C.handle, "retro_unload_game"); p_run = (void (*)(void))dlsym(C.handle, "retro_run"); + p_ser_size = (size_t (*)(void))dlsym(C.handle, "retro_serialize_size"); + p_serialize = (bool (*)(void *, size_t))dlsym(C.handle, "retro_serialize"); + p_unserialize = (bool (*)(const void *, size_t))dlsym(C.handle, "retro_unserialize"); + p_hle_state_save = (size_t (*)(uint8_t *))dlsym(C.handle, "JaguarCDHLEStateSave"); + p_hle_state_size = (size_t (*)(void))dlsym(C.handle, "JaguarCDHLEStateSize"); return p_hle_hook && p_hle_tick && p_hle_active && p_hle_dest && p_hle_bytes && p_hle_arms && p_cd_read_block && p_cd_s2_first && p_cd_total && p_load_game && p_unload_game && p_run && + p_ser_size && p_serialize && p_unserialize && + p_hle_state_save && p_hle_state_size && C.m68k_set_reg && C.m68k_get_reg && C.GetRamPtr; } @@ -830,6 +921,7 @@ int main(int argc, char *argv[]) RUN_TEST(synth_disc_lba_addressing); RUN_TEST(long_rounded_tail_holds_real_disc_bytes); RUN_TEST(delivery_is_streamed_not_instant); + RUN_TEST(inflight_stream_survives_savestate_round_trip); RUN_TEST(arming_over_inflight_drops_tail_and_completion); /* Fresh boot for the end-to-end run so the 68K starts at the stub. */