@samitouri / QOSamiQemu / commits / 784b7f28e2

accel/tcg: Allow overlapping reads in record_save

record_save() assumed that a target reads the bytes of an insn as a strictly ascending sequence of adjacent chunks, and asserted that each read begins exactly where the previous one ended. That assumption no longer holds for riscv. Since f9eaa1542b ("target/riscv: support atomic instruction fetch (Ziccif)"), decode_opc() loads a full aligned word whenever pc is 4-byte aligned, even when the insn turns out to be a 2-byte compressed one, so the record may already hold bytes past the end of the insn being translated. When such a compressed insn sits at page offset 0xffc, pc_next becomes 0xffe, which is within MAX_INSN_LEN of the end of the page, and riscv_tr_translate_insn() probes the next insn to decide whether it would cross the page boundary. That probe reads at offset 2 while the record already covers [0,4), and the assert fires: qemu-system-riscv32: accel/tcg/translator.c:395: record_save: Assertion `offset == db->record_start + db->record_len' failed. record_save() is only reached when the insn is fetched from MMIO, so this is visible on boards that execute code from a region created with memory_region_init_io(), such as an XIP flash window mapped over a serial flash controller. Both sides of the collision are correct: the wide fetch is required for Ziccif atomicity, and the probe is required for correct fault reporting at a page boundary, per 00c07344fa ("target/riscv: Make translator stop before the end of a page"). Unlike a86d3352ab ("target/riscv: do not use translator_ldl in opcode_at"), where a non-translation caller had no business using translator_ld*, the probe here is a genuine translation read whose bytes must be recorded. Relax the invariant instead. Keep requiring that a read neither moves backwards nor leaves a gap, but let a read overlapping the recorded range extend it only by the bytes past its end. Cc: qemu-stable@nongnu.org Fixes: f9eaa1542b ("target/riscv: support atomic instruction fetch (Ziccif)") Signed-off-by: Ilya Chichkov <ilya.chichkov.dev@gmail.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Message-ID: <20260814142159.3800744-1-ilya.chichkov.dev@gmail.com>

Ilya Chichkov committed Aug 14, 2026 at 17:21 UTC 784b7f28e2761fbd3c753883be949eb3d1ffe228

1 file changed +11 -3

accel/tcg/translator.c

+11 -3

@@ -387,14 +387,22 @@ static void record_save(DisasContextBase *db, vaddr pc,

387 * Either the first or second page may be I/O. If it is the second,

388 * then the first byte we need to record will be at a non-zero offset.

389 * In either case, we should not need to record but a single insn.

390 + *

391 + * A read may re-read bytes that are already recorded: a target may

392 + * fetch a whole aligned word to decode an insn (e.g. riscv Ziccif),

393 + * then probe the following insn, which lies within that same word.

394 + * Such a read extends the record only by the bytes past its end.

395 */

396 if (db->record_len == 0) {

397 db->record_start = offset;

398 db->record_len = size;

399 } else {

395 - assert(offset == db->record_start + db->record_len);

396 - assert(db->record_len + size <= sizeof(db->record));

397 - db->record_len += size;

400 + int end = offset - db->record_start + size;

401 +

402 + assert(offset >= db->record_start);

403 + assert(offset <= db->record_start + db->record_len);

404 + assert(end <= sizeof(db->record));

405 + db->record_len = MAX(db->record_len, end);

406 }

407

408 memcpy(db->record + (offset - db->record_start), from, size);