From 00fbb0f8b0b01edb0ccc67be588e1ed7eef8450f Mon Sep 17 00:00:00 2001 From: Zexin Fu Date: Mon, 24 Aug 2026 20:52:30 +0200 Subject: [PATCH] amo: fix LR/SC atomicity and forward progress Three defects in spatz_cache_amo, which sits per L1 cache controller and is therefore shared by every core of every tile. 1. Any core's LR overwrote a live reservation, so symmetric LR/SC retry loops could clobber each other in a ring and starve. Keep the incumbent reservation, and age it out (ResvTimeoutCycles) so the LR-without-SC case that made this guard unusable stays bounded. 2. core_id is 2 bits and only unique within a tile, but the reservation owner, the foreign-write invalidation and the SC response match all compared it alone: one tile's core could satisfy or take delivery of another tile's SC. Key on the full hart id {tile_id, core_id}. 3. The SC outcome was looked up in single registers holding "the" outstanding SC. A response that missed that entry fell through with the raw memory word as its data, and sc.w reads rd == 0 as success, so any zero word there told a hart its store-conditional had succeeded when it never wrote. Carry the outcome with the transaction instead: tcdm_user_t gains is_sc/sc_fail, stamped at issue and echoed back by the memory system, so a response decodes its own status. Removes the single-entry tracking entirely. New test lrsc-forward-progress: 16 cores, symmetric CAS-increment retry loop on one word. Before: 22/32, and 8 iterations/core did not finish in 900 s. After: 256/256 with 16 iterations/core in 25,778 cycles. RLC M1_N1350_K100 unchanged at 241,339 cycles, 32/32 scoreboards; spin-lock, byte-enable, cache-coverage-min, v12-race pass. Reported by Johannes Pfau (defect 1). --- hardware/src/cachepool_pkg.sv | 10 ++ hardware/src/cachepool_tile.sv | 1 + hardware/src/spatz_cache_amo.sv | 151 +++++++++++++++----- software/tests/CMakeLists.txt | 1 + software/tests/lrsc-forward-progress/main.c | 88 ++++++++++++ 5 files changed, 212 insertions(+), 39 deletions(-) create mode 100644 software/tests/lrsc-forward-progress/main.c diff --git a/hardware/src/cachepool_pkg.sv b/hardware/src/cachepool_pkg.sv index a7b38cd4..6c046d58 100644 --- a/hardware/src/cachepool_pkg.sv +++ b/hardware/src/cachepool_pkg.sv @@ -419,6 +419,16 @@ package cachepool_pkg; logic is_amo; reqid_t req_id; logic is_fpu; + /// Store-conditional status, carried with the transaction. + /// `is_sc` marks the request as an SC; `sc_fail` is its outcome, decided at + /// the AMO unit when the request is issued. The memory system echoes `user` + /// back unmodified, so the response carries its own SC status and the AMO + /// unit does not have to look it up. That lookup used a single-entry + /// register, which could not disambiguate concurrent SCs: a response that + /// missed the tracked entry returned raw memory data, and a zero word there + /// reads as "SC succeeded" (rd == 0), silently losing an update. + logic is_sc; + logic sc_fail; } tcdm_user_t; typedef struct packed { diff --git a/hardware/src/cachepool_tile.sv b/hardware/src/cachepool_tile.sv index 9defdd60..52ea73ca 100644 --- a/hardware/src/cachepool_tile.sv +++ b/hardware/src/cachepool_tile.sv @@ -754,6 +754,7 @@ module cachepool_tile spatz_cache_amo #( .DataWidth ( DataWidth ), .CoreIDWidth ( CoreIDWidth ), + .TileIDWidth ( TileIDWidth ), .tcdm_req_t ( tcdm_req_t ), .tcdm_rsp_t ( tcdm_rsp_t ), .tcdm_req_chan_t ( tcdm_req_chan_t ), diff --git a/hardware/src/spatz_cache_amo.sv b/hardware/src/spatz_cache_amo.sv index bf8c8a8e..75f2cf44 100644 --- a/hardware/src/spatz_cache_amo.sv +++ b/hardware/src/spatz_cache_amo.sv @@ -26,6 +26,16 @@ module spatz_cache_amo parameter int unsigned DataWidth = 64, /// Core ID type. parameter int unsigned CoreIDWidth = 1, + /// Tile ID width. `core_id` is only unique inside a tile, but this AMO unit + /// sits at a cache controller and serves every tile, so the reservation owner + /// must be identified by {tile_id, core_id} to be a hart id. + parameter int unsigned TileIDWidth = 1, + /// Grace period, in cycles, that an LR reservation is held without its paired + /// SC before it is dropped. Must exceed the worst-case LR->SC round trip of a + /// constrained sequence, otherwise every sequence expires and no SC ever + /// succeeds. It only bounds how long a hart that never issues its SC can + /// block other harts; it is not a performance knob. + parameter int unsigned ResvTimeoutCycles = 1024, /// Port type of the data request ports. parameter type tcdm_req_t = logic, /// Port type of the data response ports. @@ -61,7 +71,7 @@ module spatz_cache_amo logic [AddrMemWidth-1:0] addr_q; amo_op_e amo_op_q; logic load_amo; - logic sc_successful, sc_successful_q; + logic sc_successful; tcdm_user_t amo_user, amo_user_q; typedef enum logic [1:0] { @@ -80,21 +90,50 @@ module spatz_cache_amo /// Which core made this reservation. Important to /// track the reservations from different cores and /// to prevent any live-locking. + /// `core` alone is not a hart id: it is only unique within a tile, and this + /// unit serves all tiles. Both fields together identify the owner. logic [CoreIDWidth-1:0] core; + logic [TileIDWidth-1:0] tile; } reservation_t; reservation_t reservation_d, reservation_q; + /// Reservation aging. Restoring the "do not steal a valid reservation" rule + /// (below) is what removes the starvation ring, but on its own it reintroduces + /// the hazard the rule was originally disabled for: a hart may legally execute + /// LR and never the paired SC, and would then hold the reservation forever. + /// The timer bounds that: a reservation that is not consumed within + /// ResvTimeoutCycles is dropped, so blocking is bounded and some hart always + /// makes progress. + localparam int unsigned ResvTimerWidth = $clog2(ResvTimeoutCycles + 1); + logic [ResvTimerWidth-1:0] resv_timer_d, resv_timer_q; + logic resv_expired; + + assign resv_expired = reservation_q.valid & (resv_timer_q == '0); + logic core_ready; + /// The request handshake as the core actually sees it. `amo_req_ready` is only + /// the memory's ready; the unit can additionally stall (FSM busy, or an SC + /// already in flight), and the reservation/SC bookkeeping must only advance on + /// requests that were really accepted. + logic amo_req_accepted; tcdm_req_chan_t amo_req; tcdm_rsp_chan_t amo_rsp; logic amo_req_valid, amo_req_ready, amo_rsp_valid, amo_rsp_ready; amo_op_e amo_insn; logic [CoreIDWidth-1:0] amo_cid; + logic [TileIDWidth-1:0] amo_tid; + /// True when the request comes from the hart that owns the reservation. + logic amo_is_owner; + + assign amo_req_accepted = amo_req_valid & core_ready; assign amo_insn = amo_req.amo; assign amo_cid = amo_req.user.core_id; + assign amo_tid = amo_req.user.tile_id; + assign amo_is_owner = (reservation_q.core == amo_cid) && + (reservation_q.tile == amo_tid); assign amo_user = amo_req.user; always_comb begin : amo_req_comb @@ -115,59 +154,70 @@ module spatz_cache_amo // ----- // LR/SC // ----- - logic sc_req_valid, sc_req_ready; + /// The SC outcome is decided here, at issue, and then travels with the + /// transaction in `user.is_sc` / `user.sc_fail`; the memory system echoes + /// `user` back untouched, so a response carries its own status. + /// + /// It used to be looked up instead, from single registers holding "the" + /// outstanding SC. With one AMO unit shared by every core of every tile there + /// is rarely just one: a response that did not match the tracked entry fell + /// through with the raw memory word as its data, and because `sc.w` reads + /// rd == 0 as success, any zero word there told a hart its store-conditional + /// had succeeded when it never wrote. Silent lost update. logic sc_rsp_valid; - logic sc_q, sc_d; - logic sc_set, sc_clr, sc_en; - tcdm_user_t sc_user_d, sc_user_q; tcdm_rsp_chan_t sc_rsp; - logic is_sc_rsp; - - assign sc_req_valid = core_req_i.q_valid & (core_req_i.q.amo inside {AMOSC}); - assign sc_req_ready = mem_rsp_i.q_ready; - assign sc_rsp_valid = is_sc_rsp; - - assign sc_user_d = core_req_i.q.user; - assign sc_en = sc_set | sc_clr; - assign sc_set = amo_req_valid & amo_req_ready & (amo_insn == AMOSC); + assign sc_rsp_valid = amo_rsp_valid & amo_rsp.user.is_sc; - assign is_sc_rsp = amo_rsp_valid & sc_q & - (sc_user_q.tile_id == amo_rsp.user.tile_id) & - (sc_user_q.core_id == amo_rsp.user.core_id) & - (sc_user_q.req_id == amo_rsp.user.req_id); - - assign sc_clr = is_sc_rsp & amo_rsp_ready; - - assign sc_d = sc_set & ~sc_clr; - - `FFL(sc_successful_q, sc_successful, sc_set, 1'b0) - `FFL(sc_q, sc_d, sc_en, 1'b0) - `FFL(sc_user_q, sc_user_d, sc_set, '0) `FF(reservation_q, reservation_d, '0) + `FF(resv_timer_q, resv_timer_d, '0) always_comb begin : sc_rsp_comb - sc_rsp = mem_rsp_i.p; - sc_rsp.data = sc_q ? {DataWidth/32{31'h0,~sc_successful_q}} : mem_rsp_i.p.data; + sc_rsp = mem_rsp_i.p; + // rd = 0 on success, 1 on failure, per the ISA. + sc_rsp.data = {DataWidth/32{31'h0, mem_rsp_i.p.user.sc_fail}}; end always_comb begin reservation_d = reservation_q; + resv_timer_d = resv_timer_q; sc_successful = 1'b0; - // new valid transaction - if (amo_req_valid & amo_req_ready) begin - // An SC can only pair with the most recent LR in program order. - // Place a reservation on the address if there isn't already a valid reservation. - // We prevent a live-lock by don't throwing away the reservation of a hart unless - // it makes a new reservation in program order or issues any SC. + // Age an outstanding reservation and drop it once the grace period is over. + // This runs every cycle, not only on a transaction: the point is to bound a + // reservation whose owner never comes back with its SC. + if (reservation_q.valid && (resv_timer_q != '0)) begin + resv_timer_d = resv_timer_q - 1'b1; + end + if (resv_expired) begin + reservation_d.valid = 1'b0; + end + + // new accepted transaction + if (amo_req_accepted) begin - // But it is legal to only run the lr but never run the paired sc, - // so this live lock method would cause another live lock - if (amo_req.amo == AMOLR /* && (!reservation_q.valid || reservation_q.core == amo_cid) */) begin + // An SC can only pair with the most recent LR in program order. + // Place a reservation only if none is currently held, if the held one has + // expired, or if the holder is re-issuing its own LR. + // + // Letting an LR steal a live reservation from another hart is what broke + // forward progress: with several harts in symmetric LR/SC retry loops on + // words homed at this AMO unit, each LR invalidated the previous hart's + // reservation, so every SC could fail indefinitely (RISC-V requires that + // a constrained LR/SC sequence eventually succeeds). Keeping the incumbent + // makes one hart win the race; the aging above keeps that bounded. + if (amo_req.amo == AMOLR && (!reservation_q.valid || resv_expired || + amo_is_owner)) begin reservation_d.valid = 1'b1; reservation_d.addr = amo_req.addr; reservation_d.core = amo_cid; + reservation_d.tile = amo_tid; + // Reload the grace period only when the reservation is actually + // (re)acquired. A hart that loops on LR alone must not be able to keep + // refreshing its own reservation and lock everyone else out. + if (!reservation_q.valid || resv_expired) begin + resv_timer_d = ResvTimeoutCycles[ResvTimerWidth-1:0]; + end end // An SC may succeed only if no store from another hart (or other device) to @@ -176,20 +226,39 @@ module spatz_cache_amo // LR and itself in program order. // check whether another core has made a write attempt - if ((amo_cid != reservation_q.core) && + if (!amo_is_owner && (amo_req.addr == reservation_q.addr) && (!(amo_insn inside {AMONone, AMOLR, AMOSC}) || amo_req.write)) begin reservation_d.valid = 1'b0; end // An SC from the same hart clears any pending reservation. - if (reservation_q.valid && amo_insn == AMOSC && reservation_q.core == amo_cid) begin + if (reservation_q.valid && amo_insn == AMOSC && amo_is_owner) begin reservation_d.valid = 1'b0; sc_successful = reservation_q.addr == amo_req.addr; end end end +`ifndef TARGET_SYNTHESIS +`ifdef AMO_DEBUG + // Trace every accepted LR/SC and every SC response match. Debug only. + always_ff @(posedge clk_i) begin + if (rst_ni && amo_req_accepted && (amo_insn inside {AMOLR, AMOSC})) begin + $display("[AMOREQ] t=%0t %m insn=%0d hart={t%0d,c%0d} rid=%0d addr=%0h | resv v=%b {t%0d,c%0d} a=%0h tmr=%0d | sc_ok=%b", + $time, amo_insn, amo_tid, amo_cid, amo_req.user.req_id, amo_req.addr, + reservation_q.valid, reservation_q.tile, reservation_q.core, + reservation_q.addr, resv_timer_q, sc_successful); + end + if (rst_ni && amo_rsp_valid && amo_rsp.user.is_sc) begin + $display("[AMORSP] t=%0t %m SC rsp={t%0d,c%0d,r%0d} sc_fail=%b data=%0h", + $time, amo_rsp.user.tile_id, amo_rsp.user.core_id, amo_rsp.user.req_id, + amo_rsp.user.sc_fail, core_rsp_o.p.data); + end + end +`endif +`endif + // ------- // Atomics // ------- @@ -218,6 +287,10 @@ module spatz_cache_amo mem_req_o.q = amo_req; mem_req_o.q_valid = amo_req_valid; core_ready = amo_req_ready; + // Carry the SC outcome with the request so the response can be decoded on + // its own, without any per-unit bookkeeping. + mem_req_o.q.user.is_sc = (amo_insn == AMOSC); + mem_req_o.q.user.sc_fail = (amo_insn == AMOSC) & ~sc_successful; mem_req_o.q.write = amo_req.write | (sc_successful & (amo_insn == AMOSC)); mem_req_o.q.amo = AMONone; mem_req_o.q.data = amo_req.data; diff --git a/software/tests/CMakeLists.txt b/software/tests/CMakeLists.txt index d82b0494..86d9a23b 100644 --- a/software/tests/CMakeLists.txt +++ b/software/tests/CMakeLists.txt @@ -92,6 +92,7 @@ add_spatz_test_zeroParam(mcs-lock mcs-lock/main.c) add_spatz_test_zeroParam(byte-enable byte-enable/main.c) add_spatz_test_zeroParam(cache-line-rw-smoke cache-line-rw-smoke/main.c) add_spatz_test_zeroParam(minimal-tile0-repro minimal-tile0-repro/main.c) +add_spatz_test_zeroParam(lrsc-forward-progress lrsc-forward-progress/main.c) add_spatz_test_zeroParam(cache-test-scalar cache-test-scalar/main.c) add_spatz_test_zeroParam(cache-test-vector cache-test-vector/main.c) add_spatz_test_zeroParam(cache-mix-smoke cache-mix-smoke/main.c) diff --git a/software/tests/lrsc-forward-progress/main.c b/software/tests/lrsc-forward-progress/main.c new file mode 100644 index 00000000..89e3f97a --- /dev/null +++ b/software/tests/lrsc-forward-progress/main.c @@ -0,0 +1,88 @@ +// Copyright 2025 ETH Zurich and University of Bologna. +// +// SPDX-License-Identifier: Apache-2.0 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// LR/SC forward-progress stress test. +// +// Every core runs a symmetric compare-and-swap retry loop (LR/SC on RV32A) +// against ONE shared word, so all reservations are served by the same AMO unit +// -- the case the RISC-V spec's forward-progress guarantee for constrained +// LR/SC sequences must cover. +// +// Before the reservation fix in spatz_cache_amo.sv, an LR from any core +// invalidated whatever reservation was held, so the cores could clobber each +// other's reservations in a ring and no SC would ever succeed: the test hangs. +// With the fix (incumbent reservation is kept, aged out after a grace period) +// one core always wins each round and the loop terminates. +// +// Author: Zexin Fu + +#include +#include +#include +#include + +#ifndef CAS_ITERATIONS +#define CAS_ITERATIONS 16 +#endif + +// Single contended word: one address => one AMO unit => one reservation. +static _Atomic uint32_t counter __attribute__((section(".data"))); +// Per-core retry census, to show the contention actually happened. +static _Atomic uint32_t total_retries __attribute__((section(".data"))); + +int main() { + const unsigned int num_cores = snrt_cluster_core_num(); + const unsigned int cid = snrt_cluster_core_idx(); + + if (cid == 0) { + atomic_store_explicit(&counter, 0, memory_order_relaxed); + atomic_store_explicit(&total_retries, 0, memory_order_relaxed); + } + + snrt_cluster_hw_barrier(); + + uint32_t retries = 0; + for (unsigned int i = 0; i < CAS_ITERATIONS; i++) { + uint32_t expected = atomic_load_explicit(&counter, memory_order_relaxed); + // Symmetric CAS retry loop: this is the constrained LR/SC sequence. + while (!atomic_compare_exchange_strong_explicit( + &counter, &expected, expected + 1, + memory_order_relaxed, memory_order_relaxed)) { + retries++; + } + } + atomic_fetch_add_explicit(&total_retries, retries, memory_order_relaxed); + + snrt_cluster_hw_barrier(); + + int ret = 0; + if (cid == 0) { + const uint32_t expect = (uint32_t)num_cores * CAS_ITERATIONS; + const uint32_t got = atomic_load_explicit(&counter, memory_order_relaxed); + const uint32_t rt = atomic_load_explicit(&total_retries, memory_order_relaxed); + if (got == expect) { + printf("[PASS] lrsc-forward-progress: %u cores x %u CAS = %u, retries=%u\n", + num_cores, (unsigned)CAS_ITERATIONS, got, rt); + } else { + printf("[FAIL] lrsc-forward-progress: expected %u, got %u, retries=%u\n", + expect, got, rt); + ret = 1; + } + } + + snrt_cluster_hw_barrier(); + return ret; +}