cannon: Rework RMW ops for 64-bit compatibility

cannon/mipsevm/exec/mips_instructions.go

@@ -569,3 +569,47 @@ func HandleRd(cpu *mipsevm.CpuScalars, registers *[32]Word, storeReg Word, val W
 	cpu.NextPC = cpu.NextPC + 4
 	return nil
 }
+
+func LoadSubWord(memory *memory.Memory, addr Word, byteLength Word, signExtend bool, memoryTracker MemTracker) Word {
+	// Pull data from memory
+	effAddr := (addr) & arch.AddressMask
+	memoryTracker.TrackMemAccess(effAddr)
+	mem := memory.GetWord(effAddr)
+
+	// Extract a sub-word based on the low-order bits in addr
+	dataMask, bitOffset, bitLength := calculateSubWordMaskAndOffset(addr, byteLength)
+	retVal := (mem >> bitOffset) & dataMask
+	if signExtend {
+		retVal = SignExtend(retVal, bitLength)
+	}
+
+	return retVal
+}
+
+func StoreSubWord(memory *memory.Memory, addr Word, byteLength Word, value Word, memoryTracker MemTracker) {
+	// Pull data from memory
+	effAddr := (addr) & arch.AddressMask
+	memoryTracker.TrackMemAccess(effAddr)
+	mem := memory.GetWord(effAddr)
+
+	// Modify isolated sub-word within mem
+	dataMask, bitOffset, _ := calculateSubWordMaskAndOffset(addr, byteLength)
+	subWordValue := dataMask & value
+	memUpdateMask := dataMask << bitOffset
+	newMemVal := subWordValue<<bitOffset | (^memUpdateMask)&mem
+
+	memory.SetWord(effAddr, newMemVal)
+}
+
+func calculateSubWordMaskAndOffset(addr Word, byteLength Word) (dataMask, bitOffset, bitLength Word) {
+	bitLength = byteLength << 3
+	dataMask = ^Word(0) >> (arch.WordSize - bitLength)
+
+	// Figure out sub-word index based on the low-order bits in addr
+	byteIndexMask := addr & arch.ExtMask & ^(byteLength - 1)
+	maxByteShift := arch.WordSizeBytes - byteLength
+	byteIndex := addr & byteIndexMask
+	bitOffset = (maxByteShift - byteIndex) << 3
+
+	return dataMask, bitOffset, bitLength
+}

cannon/mipsevm/exec/mips_instructions32_test.go

@@ -0,0 +1,110 @@
+// These tests target architectures that are 32-bit or larger
+package exec
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/ethereum-optimism/optimism/cannon/mipsevm/arch"
+	"github.com/ethereum-optimism/optimism/cannon/mipsevm/memory"
+)
+
+// TestLoadSubWord_32bits validates LoadSubWord with 32-bit offsets (up to 3 bytes)
+func TestLoadSubWord_32bits(t *testing.T) {
+	cases := []struct {
+		name             string
+		byteLength       Word
+		addr             uint32
+		memVal           uint32
+		signExtend       bool
+		shouldSignExtend bool
+		expectedValue    uint32
+	}{
+		{name: "32-bit", byteLength: 4, addr: 0xFF00_0000, memVal: 0x1234_5678, expectedValue: 0x1234_5678},
+		{name: "32-bit, extra bits", byteLength: 4, addr: 0xFF00_0001, memVal: 0x1234_5678, expectedValue: 0x1234_5678},
+		{name: "32-bit, extra bits", byteLength: 4, addr: 0xFF00_0002, memVal: 0x1234_5678, expectedValue: 0x1234_5678},
+		{name: "32-bit, extra bits", byteLength: 4, addr: 0xFF00_0003, memVal: 0x1234_5678, expectedValue: 0x1234_5678},
+		{name: "16-bit, offset=0", byteLength: 2, addr: 0x00, memVal: 0x1234_5678, expectedValue: 0x1234},
+		{name: "16-bit, offset=0, extra bit set", byteLength: 2, addr: 0x01, memVal: 0x1234_5678, expectedValue: 0x1234},
+		{name: "16-bit, offset=2", byteLength: 2, addr: 0x02, memVal: 0x1234_5678, expectedValue: 0x5678},
+		{name: "16-bit, offset=2, extra bit set", byteLength: 2, addr: 0x03, memVal: 0x1234_5678, expectedValue: 0x5678},
+		{name: "16-bit, sign extend positive val", byteLength: 2, addr: 0x02, memVal: 0x1234_5678, expectedValue: 0x5678, signExtend: true, shouldSignExtend: false},
+		{name: "16-bit, sign extend negative val", byteLength: 2, addr: 0x02, memVal: 0x1234_F678, expectedValue: 0xFFFF_F678, signExtend: true, shouldSignExtend: true},
+		{name: "16-bit, do not sign extend negative val", byteLength: 2, addr: 0x02, memVal: 0x1234_F678, expectedValue: 0xF678, signExtend: false},
+		{name: "8-bit, offset=0", byteLength: 1, addr: 0x1230, memVal: 0x1234_5678, expectedValue: 0x12},
+		{name: "8-bit, offset=1", byteLength: 1, addr: 0x1231, memVal: 0x1234_5678, expectedValue: 0x34},
+		{name: "8-bit, offset=2", byteLength: 1, addr: 0x1232, memVal: 0x1234_5678, expectedValue: 0x56},
+		{name: "8-bit, offset=3", byteLength: 1, addr: 0x1233, memVal: 0x1234_5678, expectedValue: 0x78},
+		{name: "8-bit, sign extend positive", byteLength: 1, addr: 0x1233, memVal: 0x1234_5678, expectedValue: 0x78, signExtend: true, shouldSignExtend: false},
+		{name: "8-bit, sign extend negative", byteLength: 1, addr: 0x1233, memVal: 0x1234_5688, expectedValue: 0xFFFF_FF88, signExtend: true, shouldSignExtend: true},
+		{name: "8-bit, do not sign extend neg value", byteLength: 1, addr: 0x1233, memVal: 0x1234_5688, expectedValue: 0x88, signExtend: false},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			mem := memory.NewMemory()
+			memTracker := NewMemoryTracker(mem)
+
+			effAddr := Word(c.addr) & arch.AddressMask
+			// Shift memval for consistency across architectures
+			memVal := Word(c.memVal) << (arch.WordSize - 32)
+			mem.SetWord(effAddr, memVal)
+
+			retVal := LoadSubWord(mem, Word(c.addr), c.byteLength, c.signExtend, memTracker)
+
+			// If sign extending, make sure retVal is consistent across architectures
+			expected := Word(c.expectedValue)
+			if c.shouldSignExtend {
+				signedBits := ^Word(0xFFFF_FFFF)
+				expected = expected | signedBits
+			}
+			require.Equal(t, expected, retVal)
+		})
+	}
+}
+
+// TestStoreSubWord_32bits validates LoadSubWord with 32-bit offsets (up to 3 bytes)
+func TestStoreSubWord_32bits(t *testing.T) {
+	memVal := 0xFFFF_FFFF
+	value := 0x1234_5678
+
+	cases := []struct {
+		name          string
+		byteLength    Word
+		addr          uint32
+		expectedValue uint32
+	}{
+		{name: "32-bit", byteLength: 4, addr: 0xFF00_0000, expectedValue: 0x1234_5678},
+		{name: "32-bit, extra bits", byteLength: 4, addr: 0xFF00_0001, expectedValue: 0x1234_5678},
+		{name: "32-bit, extra bits", byteLength: 4, addr: 0xFF00_0002, expectedValue: 0x1234_5678},
+		{name: "32-bit, extra bits", byteLength: 4, addr: 0xFF00_0003, expectedValue: 0x1234_5678},
+		{name: "16-bit, subword offset=0", byteLength: 2, addr: 0x00, expectedValue: 0x5678_FFFF},
+		{name: "16-bit, subword offset=0, extra bit set", byteLength: 2, addr: 0x01, expectedValue: 0x5678_FFFF},
+		{name: "16-bit, subword offset=2", byteLength: 2, addr: 0x02, expectedValue: 0xFFFF_5678},
+		{name: "16-bit, subword offset=2, extra bit set", byteLength: 2, addr: 0x03, expectedValue: 0xFFFF_5678},
+		{name: "8-bit, offset=0", byteLength: 1, addr: 0x1230, expectedValue: 0x78FF_FFFF},
+		{name: "8-bit, offset=1", byteLength: 1, addr: 0x1231, expectedValue: 0xFF78_FFFF},
+		{name: "8-bit, offset=2", byteLength: 1, addr: 0x1232, expectedValue: 0xFFFF_78FF},
+		{name: "8-bit, offset=3", byteLength: 1, addr: 0x1233, expectedValue: 0xFFFF_FF78},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			mem := memory.NewMemory()
+			memTracker := NewMemoryTracker(mem)
+
+			effAddr := Word(c.addr) & arch.AddressMask
+			// Shift memval for consistency across architectures
+			memVal := Word(memVal) << (arch.WordSize - 32)
+			mem.SetWord(effAddr, memVal)
+
+			StoreSubWord(mem, Word(c.addr), c.byteLength, Word(value), memTracker)
+			newMemVal := mem.GetWord(effAddr)
+
+			// Make sure expectation is consistent across architectures
+			expected := Word(c.expectedValue) << (arch.WordSize - 32)
+			require.Equal(t, expected, newMemVal)
+		})
+	}
+}

cannon/mipsevm/exec/mips_instructions64_test.go

@@ -0,0 +1,111 @@
+//go:build cannon64
+// +build cannon64
+
+// These tests target architectures that are 64-bit or larger
+package exec
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/ethereum-optimism/optimism/cannon/mipsevm/arch"
+	"github.com/ethereum-optimism/optimism/cannon/mipsevm/memory"
+)
+
+// TestLoadSubWord_64bits extends TestLoadSubWord_32bits by testing up to 64-bits (7 byte) offsets
+func TestLoadSubWord_64bits(t *testing.T) {
+	memVal := uint64(0x1234_5678_9876_5432)
+	cases := []struct {
+		name          string
+		byteLength    Word
+		addr          uint64
+		memVal        uint64
+		signExtend    bool
+		expectedValue uint64
+	}{
+		{name: "64-bit", byteLength: 8, addr: 0xFF00_0000, memVal: 0x8234_5678_9876_5432, expectedValue: 0x8234_5678_9876_5432},
+		{name: "64-bit w sign extension", byteLength: 8, addr: 0xFF00_0000, memVal: 0x8234_5678_9876_5432, expectedValue: 0x8234_5678_9876_5432, signExtend: true},
+		{name: "32-bit, offset=0", byteLength: 4, addr: 0xFF00_0000, memVal: memVal, expectedValue: 0x1234_5678},
+		{name: "32-bit, offset=0, extra bits", byteLength: 4, addr: 0xFF00_0001, memVal: memVal, expectedValue: 0x1234_5678},
+		{name: "32-bit, offset=0, extra bits", byteLength: 4, addr: 0xFF00_0002, memVal: memVal, expectedValue: 0x1234_5678},
+		{name: "32-bit, offset=0, extra bits", byteLength: 4, addr: 0xFF00_0003, memVal: memVal, expectedValue: 0x1234_5678},
+		{name: "32-bit, offset=4", byteLength: 4, addr: 0xFF00_0004, memVal: memVal, expectedValue: 0x9876_5432},
+		{name: "32-bit, offset=4, extra bits", byteLength: 4, addr: 0xFF00_0005, memVal: memVal, expectedValue: 0x9876_5432},
+		{name: "32-bit, offset=4, extra bits", byteLength: 4, addr: 0xFF00_0006, memVal: memVal, expectedValue: 0x9876_5432},
+		{name: "32-bit, offset=4, extra bits", byteLength: 4, addr: 0xFF00_0007, memVal: memVal, expectedValue: 0x9876_5432},
+		{name: "32-bit, sign extend negative", byteLength: 4, addr: 0xFF00_0006, memVal: 0x1234_5678_F1E2_A1B1, expectedValue: 0xFFFF_FFFF_F1E2_A1B1, signExtend: true},
+		{name: "32-bit, sign extend positive", byteLength: 4, addr: 0xFF00_0007, memVal: 0x1234_5678_7876_5432, expectedValue: 0x7876_5432, signExtend: true},
+		{name: "16-bit, subword offset=4", byteLength: 2, addr: 0x04, memVal: memVal, expectedValue: 0x9876},
+		{name: "16-bit, subword offset=4, extra bit set", byteLength: 2, addr: 0x05, memVal: memVal, expectedValue: 0x9876},
+		{name: "16-bit, subword offset=6", byteLength: 2, addr: 0x06, memVal: memVal, expectedValue: 0x5432},
+		{name: "16-bit, subword offset=6, extra bit set", byteLength: 2, addr: 0x07, memVal: memVal, expectedValue: 0x5432},
+		{name: "16-bit, sign extend negative val", byteLength: 2, addr: 0x04, memVal: 0x1234_5678_8BEE_CCDD, expectedValue: 0xFFFF_FFFF_FFFF_8BEE, signExtend: true},
+		{name: "16-bit, sign extend positive val", byteLength: 2, addr: 0x04, memVal: 0x1234_5678_7876_5432, expectedValue: 0x7876, signExtend: true},
+		{name: "8-bit, offset=4", byteLength: 1, addr: 0x1234, memVal: memVal, expectedValue: 0x98},
+		{name: "8-bit, offset=5", byteLength: 1, addr: 0x1235, memVal: memVal, expectedValue: 0x76},
+		{name: "8-bit, offset=6", byteLength: 1, addr: 0x1236, memVal: memVal, expectedValue: 0x54},
+		{name: "8-bit, offset=7", byteLength: 1, addr: 0x1237, memVal: memVal, expectedValue: 0x32},
+		{name: "8-bit, sign extend positive", byteLength: 1, addr: 0x1237, memVal: memVal, expectedValue: 0x32, signExtend: true},
+		{name: "8-bit, sign extend negative", byteLength: 1, addr: 0x1237, memVal: 0x1234_5678_8764_4381, expectedValue: 0xFFFF_FFFF_FFFF_FF81, signExtend: true},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			mem := memory.NewMemory()
+			memTracker := NewMemoryTracker(mem)
+
+			effAddr := Word(c.addr) & arch.AddressMask
+			mem.SetWord(effAddr, c.memVal)
+
+			retVal := LoadSubWord(mem, Word(c.addr), c.byteLength, c.signExtend, memTracker)
+			require.Equal(t, c.expectedValue, retVal)
+		})
+	}
+}
+
+// TestStoreSubWord_64bits extends TestStoreSubWord_32bits by testing up to 64-bits (7 byte) offsets
+func TestStoreSubWord_64bits(t *testing.T) {
+	memVal := uint64(0xFFFF_FFFF_FFFF_FFFF)
+	value := uint64(0x1234_5678_9876_5432)
+
+	cases := []struct {
+		name          string
+		byteLength    Word
+		addr          uint64
+		expectedValue uint64
+	}{
+		{name: "64-bit", byteLength: 8, addr: 0xFF00_0000, expectedValue: value},
+		{name: "32-bit, offset 0", byteLength: 4, addr: 0xFF00_0000, expectedValue: 0x9876_5432_FFFF_FFFF},
+		{name: "32-bit, offset 0, extra addr bits", byteLength: 4, addr: 0xFF00_0001, expectedValue: 0x9876_5432_FFFF_FFFF},
+		{name: "32-bit, offset 0, extra addr bits", byteLength: 4, addr: 0xFF00_0002, expectedValue: 0x9876_5432_FFFF_FFFF},
+		{name: "32-bit, offset 0, extra addr bits", byteLength: 4, addr: 0xFF00_0003, expectedValue: 0x9876_5432_FFFF_FFFF},
+		{name: "32-bit, offset 4", byteLength: 4, addr: 0xFF00_0004, expectedValue: 0xFFFF_FFFF_9876_5432},
+		{name: "32-bit, offset 4, extra addr bits", byteLength: 4, addr: 0xFF00_0005, expectedValue: 0xFFFF_FFFF_9876_5432},
+		{name: "32-bit, offset 4, extra addr bits", byteLength: 4, addr: 0xFF00_0006, expectedValue: 0xFFFF_FFFF_9876_5432},
+		{name: "32-bit, offset 4, extra addr bits", byteLength: 4, addr: 0xFF00_0007, expectedValue: 0xFFFF_FFFF_9876_5432},
+		{name: "16-bit, offset=4", byteLength: 2, addr: 0x04, expectedValue: 0xFFFF_FFFF_5432_FFFF},
+		{name: "16-bit, offset=4, extra bit set", byteLength: 2, addr: 0x05, expectedValue: 0xFFFF_FFFF_5432_FFFF},
+		{name: "16-bit, offset=6", byteLength: 2, addr: 0x06, expectedValue: 0xFFFF_FFFF_FFFF_5432},
+		{name: "16-bit, offset=6, extra bit set", byteLength: 2, addr: 0x07, expectedValue: 0xFFFF_FFFF_FFFF_5432},
+		{name: "8-bit, offset=4", byteLength: 1, addr: 0x1234, expectedValue: 0xFFFF_FFFF_32FF_FFFF},
+		{name: "8-bit, offset=5", byteLength: 1, addr: 0x1235, expectedValue: 0xFFFF_FFFF_FF32_FFFF},
+		{name: "8-bit, offset=6", byteLength: 1, addr: 0x1236, expectedValue: 0xFFFF_FFFF_FFFF_32FF},
+		{name: "8-bit, offset=7", byteLength: 1, addr: 0x1237, expectedValue: 0xFFFF_FFFF_FFFF_FF32},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			mem := memory.NewMemory()
+			memTracker := NewMemoryTracker(mem)
+
+			effAddr := Word(c.addr) & arch.AddressMask
+			mem.SetWord(effAddr, memVal)
+
+			StoreSubWord(mem, Word(c.addr), c.byteLength, Word(value), memTracker)
+			newMemVal := mem.GetWord(effAddr)
+
+			require.Equal(t, c.expectedValue, newMemVal)
+		})
+	}
+}

cannon/mipsevm/memory/memory.go

@@ -9,9 +9,10 @@ import (
 	"slices"
 	"sort"
 
-	"github.com/ethereum-optimism/optimism/cannon/mipsevm/arch"
 	"github.com/ethereum/go-ethereum/crypto"
 	"golang.org/x/exp/maps"
+
+	"github.com/ethereum-optimism/optimism/cannon/mipsevm/arch"
 )
 
 // Note: 2**12 = 4 KiB, the min phys page size in the Go runtime.
@@ -193,8 +194,8 @@ func (m *Memory) pageLookup(pageIndex Word) (*CachedPage, bool) {
 }
 
 func (m *Memory) SetUint32(addr Word, v uint32) {
-	// addr must be aligned to WordSizeBytes bytes
-	if addr&arch.ExtMask != 0 {
+	// addr must be aligned to 4-byte (32-bit) boundaries
+	if addr&3 != 0 {
 		panic(fmt.Errorf("unaligned memory access: %x", addr))
 	}
 

cannon/mipsevm/multithreaded/mips.go

@@ -325,14 +325,14 @@ func (m *InstrumentedState) mipsStep() error {
 }
 
 func (m *InstrumentedState) handleMemoryUpdate(memAddr Word) {
-	if memAddr == m.state.LLAddress {
+	if memAddr == (arch.AddressMask & m.state.LLAddress) {
 		// Reserved address was modified, clear the reservation
 		m.clearLLMemoryReservation()
 	}
 }
 
 func (m *InstrumentedState) clearLLMemoryReservation() {
-	m.state.LLReservationActive = false
+	m.state.LLReservationStatus = LLStatusNone
 	m.state.LLAddress = 0
 	m.state.LLOwnerThread = 0
 }
@@ -343,36 +343,40 @@ func (m *InstrumentedState) handleRMWOps(insn, opcode uint32) error {
 	base := m.state.GetRegistersRef()[baseReg]
 	rtReg := Word((insn >> 16) & 0x1F)
 	offset := exec.SignExtendImmediate(insn)
+	addr := base + offset
 
-	effAddr := (base + offset) & arch.AddressMask
-	m.memoryTracker.TrackMemAccess(effAddr)
-	mem := m.state.Memory.GetWord(effAddr)
+	// Determine some opcode-specific parameters
+	targetStatus := LLStatusActive32bit
+	byteLength := Word(4)
+	if opcode == exec.OpLoadLinked64 || opcode == exec.OpStoreConditional64 {
+		// Use 64-bit params
+		targetStatus = LLStatusActive64bit
+		byteLength = Word(8)
+	}
 
 	var retVal Word
 	threadId := m.state.GetCurrentThread().ThreadId
-	if opcode == exec.OpLoadLinked || opcode == exec.OpLoadLinked64 {
-		retVal = mem
-		m.state.LLReservationActive = true
-		m.state.LLAddress = effAddr
+	switch opcode {
+	case exec.OpLoadLinked, exec.OpLoadLinked64:
+		retVal = exec.LoadSubWord(m.state.GetMemory(), addr, byteLength, true, m.memoryTracker)
+
+		m.state.LLReservationStatus = targetStatus
+		m.state.LLAddress = addr
 		m.state.LLOwnerThread = threadId
-	} else if opcode == exec.OpStoreConditional || opcode == exec.OpStoreConditional64 {
-		// TODO(#12205): Determine bits affected by coherence stores on 64-bits
-		// Check if our memory reservation is still intact
-		if m.state.LLReservationActive && m.state.LLOwnerThread == threadId && m.state.LLAddress == effAddr {
+	case exec.OpStoreConditional, exec.OpStoreConditional64:
+		if m.state.LLReservationStatus == targetStatus && m.state.LLOwnerThread == threadId && m.state.LLAddress == addr {
 			// Complete atomic update: set memory and return 1 for success
 			m.clearLLMemoryReservation()
-			rt := m.state.GetRegistersRef()[rtReg]
-			if opcode == exec.OpStoreConditional {
-				m.state.Memory.SetUint32(effAddr, uint32(rt))
-			} else {
-				m.state.Memory.SetWord(effAddr, rt)
-			}
+
+			val := m.state.GetRegistersRef()[rtReg]
+			exec.StoreSubWord(m.state.GetMemory(), addr, byteLength, val, m.memoryTracker)
+
 			retVal = 1
 		} else {
 			// Atomic update failed, return 0 for failure
 			retVal = 0
 		}
-	} else {
+	default:
 		panic(fmt.Sprintf("Invalid instruction passed to handleRMWOps (opcode %08x)", opcode))
 	}