From 7398e1e401a085b7bae4e336c8d9b6e57202457a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Armejach?= Date: Wed, 7 Jun 2023 17:01:03 +0200 Subject: [PATCH] arch-riscv: fix load reserved store conditional * According to the manual, load reservations must be cleared on a failed or a successful SC attempt. * A load reservation can be arbitrarily large. The current implementation was reserving something different than cacheBlockSize which could lead to problems if snoop addresses are cache block aligned. This patch implementation assumes a cacheBlock granularity. * Load reservations should also be cleared on faults Change-Id: I64513534710b5f269260fcb204f717801913e2f5 Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/71558 Reviewed-by: Jason Lowe-Power Reviewed-by: Roger Chang Maintainer: Jason Lowe-Power Tested-by: kokoro --- src/arch/generic/isa.hh | 1 + src/arch/riscv/faults.cc | 3 +++ src/arch/riscv/isa.cc | 29 ++++++++++++++++++----------- src/arch/riscv/isa.hh | 12 ++++++++++++ 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/arch/generic/isa.hh b/src/arch/generic/isa.hh index e9e4d95d7b..58f66fc99b 100644 --- a/src/arch/generic/isa.hh +++ b/src/arch/generic/isa.hh @@ -70,6 +70,7 @@ class BaseISA : public SimObject public: virtual PCStateBase *newPCState(Addr new_inst_addr=0) const = 0; virtual void clear() {} + virtual void clearLoadReservation(ContextID cid) {} virtual RegVal readMiscRegNoEffect(RegIndex idx) const = 0; virtual RegVal readMiscReg(RegIndex idx) = 0; diff --git a/src/arch/riscv/faults.cc b/src/arch/riscv/faults.cc index 940f7107ba..8fb8f81261 100644 --- a/src/arch/riscv/faults.cc +++ b/src/arch/riscv/faults.cc @@ -153,6 +153,9 @@ RiscvFault::invoke(ThreadContext *tc, const StaticInstPtr &inst) tc->setMiscReg(MISCREG_NMIE, 0); } + // Clear load reservation address + tc->getIsaPtr()->clearLoadReservation(tc->contextId()); + // Set PC to fault handler address Addr addr = mbits(tc->readMiscReg(tvec), 63, 2); if (isInterrupt() && bits(tc->readMiscReg(tvec), 1, 0) == 1) diff --git a/src/arch/riscv/isa.cc b/src/arch/riscv/isa.cc index d744fe369b..94a8239bac 100644 --- a/src/arch/riscv/isa.cc +++ b/src/arch/riscv/isa.cc @@ -672,11 +672,6 @@ ISA::unserialize(CheckpointIn &cp) UNSERIALIZE_CONTAINER(miscRegFile); } -const int WARN_FAILURE = 10000; - -const Addr INVALID_RESERVATION_ADDR = (Addr) -1; -std::unordered_map load_reservation_addrs; - void ISA::handleLockedSnoop(PacketPtr pkt, Addr cacheBlockMask) { @@ -696,9 +691,9 @@ ISA::handleLockedRead(const RequestPtr &req) { Addr& load_reservation_addr = load_reservation_addrs[tc->contextId()]; - load_reservation_addr = req->getPaddr() & ~0xF; + load_reservation_addr = req->getPaddr(); DPRINTF(LLSC, "[cid:%d]: Reserved address %x.\n", - req->contextId(), req->getPaddr() & ~0xF); + req->contextId(), req->getPaddr()); } bool @@ -717,12 +712,13 @@ ISA::handleLockedWrite(const RequestPtr &req, Addr cacheBlockMask) lr_addr_empty ? "yes" : "no"); if (!lr_addr_empty) { DPRINTF(LLSC, "[cid:%d]: addr = %x.\n", req->contextId(), - req->getPaddr() & ~0xF); + req->getPaddr() & cacheBlockMask); DPRINTF(LLSC, "[cid:%d]: last locked addr = %x.\n", req->contextId(), - load_reservation_addr); + load_reservation_addr & cacheBlockMask); } - if (lr_addr_empty - || load_reservation_addr != ((req->getPaddr() & ~0xF))) { + if (lr_addr_empty || + (load_reservation_addr & cacheBlockMask) + != ((req->getPaddr() & cacheBlockMask))) { req->setExtraData(0); int stCondFailures = tc->readStCondFailures(); tc->setStCondFailures(++stCondFailures); @@ -730,12 +726,21 @@ ISA::handleLockedWrite(const RequestPtr &req, Addr cacheBlockMask) warn("%i: context %d: %d consecutive SC failures.\n", curTick(), tc->contextId(), stCondFailures); } + + // Must clear any reservations + load_reservation_addr = INVALID_RESERVATION_ADDR; + return false; } if (req->isUncacheable()) { req->setExtraData(2); } + // Must clear any reservations + load_reservation_addr = INVALID_RESERVATION_ADDR; + + DPRINTF(LLSC, "[cid:%d]: SC success! Current locked addr = %x.\n", + req->contextId(), load_reservation_addr & cacheBlockMask); return true; } @@ -743,6 +748,8 @@ void ISA::globalClearExclusive() { tc->getCpuPtr()->wakeup(tc->threadId()); + Addr& load_reservation_addr = load_reservation_addrs[tc->contextId()]; + load_reservation_addr = INVALID_RESERVATION_ADDR; } void diff --git a/src/arch/riscv/isa.hh b/src/arch/riscv/isa.hh index 5a2a610479..7ef5c526f5 100644 --- a/src/arch/riscv/isa.hh +++ b/src/arch/riscv/isa.hh @@ -76,6 +76,11 @@ class ISA : public BaseISA bool hpmCounterEnabled(int counter) const; + // Load reserve - store conditional monitor + const int WARN_FAILURE = 10000; + const Addr INVALID_RESERVATION_ADDR = (Addr)-1; + std::unordered_map load_reservation_addrs; + public: using Params = RiscvISAParams; @@ -87,6 +92,13 @@ class ISA : public BaseISA return new PCState(new_inst_addr, rv_type); } + void + clearLoadReservation(ContextID cid) override + { + Addr& load_reservation_addr = load_reservation_addrs[cid]; + load_reservation_addr = INVALID_RESERVATION_ADDR; + } + public: RegVal readMiscRegNoEffect(RegIndex idx) const override; RegVal readMiscReg(RegIndex idx) override;