amo: fix LR/SC atomicity and forward progress

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 {

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  ),

hardware/src/spatz_cache_amo.sv

@@ -26,6 +26,16 @@
   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 @@
   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 @@
     /// 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 @@
   // -----
   // 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 @@
       // 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 @@
     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;

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)

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 <zexifu@iis.ee.ethz.ch>
+
+#include <benchmark.h>
+#include <snrt.h>
+#include <stdatomic.h>
+#include <stdio.h>
+
+#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;
+}