cleanup/exclusive-ranges (#620)

- Switch to half-open / usual range for platform addresses. - Expose the `Range` type (#621). - Introduce methods `iter_addresses` which is not the same as `Range::iter`. (This became easy to fix after recent changes and useful for upcoming changes.) --------- Co-authored-by: Aurélien Nicolas <[email protected]>
scroll-tech · Nov 22, 2024 · ea9fa2d · ea9fa2d
1 parent 0efb9d1
commit ea9fa2d
Show file tree

Hide file tree

Showing 10 changed files with 83 additions and 84 deletions.
diff --git a/ceno_emul/src/addr.rs b/ceno_emul/src/addr.rs
@@ -14,7 +14,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::{fmt, ops};
+use std::{
+    fmt,
+    ops::{self, Range},
+};
 
 pub const WORD_SIZE: usize = 4;
 pub const PC_WORD_SIZE: usize = 4;
@@ -192,3 +195,29 @@ impl ops::AddAssign<u32> for ByteAddr {
         self.0 += rhs;
     }
 }
+
+pub trait IterAddresses {
+    fn iter_addresses(&self) -> impl Iterator<Item = Addr>;
+}
+
+impl IterAddresses for Range<Addr> {
+    fn iter_addresses(&self) -> impl Iterator<Item = Addr> {
+        self.clone().step_by(WORD_SIZE)
+    }
+}
+
+impl<'a, T: GetAddr> IterAddresses for &'a [T] {
+    fn iter_addresses(&self) -> impl Iterator<Item = Addr> {
+        self.iter().map(T::get_addr)
+    }
+}
+
+pub trait GetAddr {
+    fn get_addr(&self) -> Addr;
+}
+
+impl GetAddr for Addr {
+    fn get_addr(&self) -> Addr {
+        *self
+    }
+}
diff --git a/ceno_emul/src/platform.rs b/ceno_emul/src/platform.rs
@@ -1,3 +1,5 @@
+use std::ops::Range;
+
 use crate::addr::{Addr, RegIdx};
 
 /// The Platform struct holds the parameters of the VM.
@@ -7,74 +9,35 @@ use crate::addr::{Addr, RegIdx};
 /// - codes of environment calls.
 #[derive(Clone, Debug)]
 pub struct Platform {
-    pub rom_start: Addr,
-    pub rom_end: Addr,
-    pub ram_start: Addr,
-    pub ram_end: Addr,
+    pub rom: Range<Addr>,
+    pub ram: Range<Addr>,
+    pub public_io: Range<Addr>,
     pub stack_top: Addr,
     /// If true, ecall instructions are no-op instead of trap. Testing only.
     pub unsafe_ecall_nop: bool,
 }
 
 pub const CENO_PLATFORM: Platform = Platform {
-    rom_start: 0x2000_0000,
-    rom_end: 0x3000_0000 - 1,
-    ram_start: 0x8000_0000,
-    ram_end: 0xFFFF_0000 - 1,
+    rom: 0x2000_0000..0x3000_0000,
+    ram: 0x8000_0000..0xFFFF_0000,
+    public_io: 0x3000_1000..0x3000_2000,
     stack_top: 0xC0000000,
     unsafe_ecall_nop: false,
 };
 
 impl Platform {
     // Virtual memory layout.
 
-    pub const fn rom_start(&self) -> Addr {
-        self.rom_start
-    }
-
-    pub const fn rom_end(&self) -> Addr {
-        self.rom_end
-    }
-
     pub fn is_rom(&self, addr: Addr) -> bool {
-        (self.rom_start()..=self.rom_end()).contains(&addr)
-    }
-
-    // TODO figure out a proper region for public io
-    pub const fn public_io_start(&self) -> Addr {
-        0x3000_1000
-    }
-
-    pub const fn public_io_end(&self) -> Addr {
-        0x3000_2000 - 1
-    }
-
-    pub const fn ram_start(&self) -> Addr {
-        if cfg!(feature = "forbid_overflow") {
-            // -1<<11 == 0x800 is the smallest negative 'immediate'
-            // offset we can have in memory instructions.
-            // So if we stay away from it, we are safe.
-            assert!(self.ram_start >= 0x800);
-        }
-        self.ram_start
-    }
-
-    pub const fn ram_end(&self) -> Addr {
-        if cfg!(feature = "forbid_overflow") {
-            // (1<<11) - 1 == 0x7ff is the largest positive 'immediate'
-            // offset we can have in memory instructions.
-            // So if we stay away from it, we are safe.
-            assert!(self.ram_end < -(1_i32 << 11) as u32)
-        }
-        self.ram_end
+        self.rom.contains(&addr)
     }
 
     pub fn is_ram(&self, addr: Addr) -> bool {
-        (self.ram_start()..=self.ram_end()).contains(&addr)
+        self.ram.contains(&addr)
     }
 
     pub fn is_pub_io(&self, addr: Addr) -> bool {
-        (self.public_io_start()..=self.public_io_end()).contains(&addr)
+        self.public_io.contains(&addr)
     }
 
     /// Virtual address of a register.
@@ -91,7 +54,7 @@ impl Platform {
     // Startup.
 
     pub const fn pc_base(&self) -> Addr {
-        self.rom_start()
+        self.rom.start
     }
 
     // Permissions.
@@ -139,17 +102,17 @@ impl Platform {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::VMState;
+    use crate::{VMState, WORD_SIZE};
 
     #[test]
     fn test_no_overlap() {
         let p = CENO_PLATFORM;
         assert!(p.can_execute(p.pc_base()));
         // ROM and RAM do not overlap.
-        assert!(!p.is_rom(p.ram_start()));
-        assert!(!p.is_rom(p.ram_end()));
-        assert!(!p.is_ram(p.rom_start()));
-        assert!(!p.is_ram(p.rom_end()));
+        assert!(!p.is_rom(p.ram.start));
+        assert!(!p.is_rom(p.ram.end - WORD_SIZE as Addr));
+        assert!(!p.is_ram(p.rom.start));
+        assert!(!p.is_ram(p.rom.end - WORD_SIZE as Addr));
         // Registers do not overlap with ROM or RAM.
         for reg in [
             Platform::register_vma(0),

diff --git a/ceno_emul/src/tracer.rs b/ceno_emul/src/tracer.rs
@@ -199,7 +199,7 @@ impl StepRecord {
             Some(value),
             Some(Change::new(value, value)),
             Some(WriteOp {
-                addr: CENO_PLATFORM.ram_start().into(),
+                addr: CENO_PLATFORM.ram.start.into(),
                 value: Change {
                     before: value,
                     after: value,

diff --git a/ceno_emul/src/vm_state.rs b/ceno_emul/src/vm_state.rs
@@ -123,7 +123,7 @@ impl EmuContext for VMState {
             // Read two registers, write one register, write one memory word, and branch.
             tracing::warn!("ecall ignored: syscall_id={}", function);
             self.store_register(DecodedInstruction::RD_NULL as RegIdx, 0)?;
-            let addr = self.platform.ram_start().into();
+            let addr = self.platform.ram.start.into();
             self.store_memory(addr, self.peek_memory(addr))?;
             self.set_pc(ByteAddr(self.pc) + PC_STEP_SIZE);
             Ok(true)

diff --git a/ceno_emul/tests/test_elf.rs b/ceno_emul/tests/test_elf.rs
@@ -27,7 +27,7 @@ fn test_ceno_rt_mem() -> Result<()> {
     let mut state = VMState::new_from_elf(CENO_PLATFORM, program_elf)?;
     let _steps = run(&mut state)?;
 
-    let value = state.peek_memory(CENO_PLATFORM.ram_start().into());
+    let value = state.peek_memory(CENO_PLATFORM.ram.start.into());
     assert_eq!(value, 6765, "Expected Fibonacci 20, got {}", value);
     Ok(())
 }

diff --git a/ceno_zkvm/examples/riscv_opcodes.rs b/ceno_zkvm/examples/riscv_opcodes.rs
@@ -42,10 +42,10 @@ const PROGRAM_CODE: [u32; PROGRAM_SIZE] = {
     let mut program: [u32; PROGRAM_SIZE] = [ECALL_HALT; PROGRAM_SIZE];
     declare_program!(
         program,
-        encode_rv32(LUI, 0, 0, 10, CENO_PLATFORM.public_io_start()), // lui x10, public_io
-        encode_rv32(LW, 10, 0, 1, 0),                                // lw x1, 0(x10)
-        encode_rv32(LW, 10, 0, 2, 4),                                // lw x2, 4(x10)
-        encode_rv32(LW, 10, 0, 3, 8),                                // lw x3, 8(x10)
+        encode_rv32(LUI, 0, 0, 10, CENO_PLATFORM.public_io.start), // lui x10, public_io
+        encode_rv32(LW, 10, 0, 1, 0),                              // lw x1, 0(x10)
+        encode_rv32(LW, 10, 0, 2, 4),                              // lw x2, 4(x10)
+        encode_rv32(LW, 10, 0, 3, 8),                              // lw x3, 8(x10)
         // Main loop.
         encode_rv32(ADD, 1, 4, 4, 0),              // add x4, x1, x4
         encode_rv32(ADD, 2, 3, 3, 0),              // add x3, x2, x3
@@ -90,8 +90,8 @@ fn main() {
             })
             .collect(),
     );
-    let mem_addresses = CENO_PLATFORM.ram_start()..=CENO_PLATFORM.ram_end();
-    let io_addresses = CENO_PLATFORM.public_io_start()..=CENO_PLATFORM.public_io_end();
+    let mem_addresses = CENO_PLATFORM.ram.clone();
+    let io_addresses = CENO_PLATFORM.public_io.clone();
 
     let (flame_layer, _guard) = FlameLayer::with_file("./tracing.folded").unwrap();
     let mut fmt_layer = fmt::layer()

diff --git a/ceno_zkvm/src/bin/e2e.rs b/ceno_zkvm/src/bin/e2e.rs
@@ -79,17 +79,16 @@ fn main() {
         Preset::Sp1 => Platform {
             // The stack section is not mentioned in ELF headers, so we repeat the constant STACK_TOP here.
             stack_top: 0x0020_0400,
-            rom_start: 0x0020_0800,
-            rom_end: 0x003f_ffff,
-            ram_start: 0x0020_0000,
-            ram_end: 0xFFFF_0000 - 1,
+            rom: 0x0020_0800..0x0040_0000,
+            ram: 0x0020_0000..0xFFFF_0000,
             unsafe_ecall_nop: true,
+            ..CENO_PLATFORM
         },
     };
     tracing::info!("Running on platform {:?}", args.platform);
 
     const STACK_SIZE: u32 = 256;
-    let mut mem_padder = MemPadder::new(platform.ram_start()..=platform.ram_end());
+    let mut mem_padder = MemPadder::new(platform.ram.clone());
 
     tracing::info!("Loading ELF file: {}", args.elf);
     let elf_bytes = fs::read(&args.elf).expect("read elf file");

diff --git a/ceno_zkvm/src/instructions/riscv/rv32im/mmu.rs b/ceno_zkvm/src/instructions/riscv/rv32im/mmu.rs
@@ -1,6 +1,6 @@
-use std::{collections::HashSet, iter::zip, ops::RangeInclusive};
+use std::{collections::HashSet, iter::zip, ops::Range};
 
-use ceno_emul::{Addr, Cycle, WORD_SIZE, Word};
+use ceno_emul::{Addr, Cycle, IterAddresses, WORD_SIZE, Word};
 use ff_ext::ExtensionField;
 use itertools::{Itertools, chain};
 
@@ -48,11 +48,7 @@ impl<E: ExtensionField> MmuConfig<E> {
         io_addrs: &[Addr],
     ) {
         assert!(
-            chain(
-                static_mem_init.iter().map(|record| record.addr),
-                io_addrs.iter().copied(),
-            )
-            .all_unique(),
+            chain!(static_mem_init.iter_addresses(), io_addrs.iter_addresses()).all_unique(),
             "memory addresses must be unique"
         );
 
@@ -107,7 +103,7 @@ impl<E: ExtensionField> MmuConfig<E> {
 }
 
 pub struct MemPadder {
-    valid_addresses: RangeInclusive<Addr>,
+    valid_addresses: Range<Addr>,
     used_addresses: HashSet<Addr>,
 }
 
@@ -118,7 +114,7 @@ impl MemPadder {
     ///
     /// Require: `values.len() <= padded_len <= address_range.len()`
     pub fn init_mem(
-        address_range: RangeInclusive<Addr>,
+        address_range: Range<Addr>,
         padded_len: usize,
         values: &[Word],
     ) -> Vec<MemInitRecord> {
@@ -129,7 +125,7 @@ impl MemPadder {
         records
     }
 
-    pub fn new(valid_addresses: RangeInclusive<Addr>) -> Self {
+    pub fn new(valid_addresses: Range<Addr>) -> Self {
         Self {
             valid_addresses,
             used_addresses: HashSet::new(),

diff --git a/ceno_zkvm/src/tables/ram.rs b/ceno_zkvm/src/tables/ram.rs
@@ -19,11 +19,11 @@ impl DynVolatileRamTable for DynMemTable {
     const ZERO_INIT: bool = true;
 
     fn offset_addr(params: &ProgramParams) -> Addr {
-        params.platform.ram_start()
+        params.platform.ram.start
     }
 
     fn end_addr(params: &ProgramParams) -> Addr {
-        params.platform.ram_end()
+        params.platform.ram.end
     }
 
     fn name() -> &'static str {
@@ -41,11 +41,11 @@ impl DynVolatileRamTable for PrivateMemTable {
     const ZERO_INIT: bool = false;
 
     fn offset_addr(params: &ProgramParams) -> Addr {
-        params.platform.ram_start()
+        params.platform.ram.start
     }
 
     fn end_addr(params: &ProgramParams) -> Addr {
-        params.platform.ram_end()
+        params.platform.ram.end
     }
 
     fn name() -> &'static str {

diff --git a/ceno_zkvm/src/tables/ram/ram_circuit.rs b/ceno_zkvm/src/tables/ram/ram_circuit.rs
@@ -1,6 +1,6 @@
 use std::{collections::HashMap, marker::PhantomData};
 
-use ceno_emul::{Addr, Cycle, WORD_SIZE, Word};
+use ceno_emul::{Addr, Cycle, GetAddr, WORD_SIZE, Word};
 use ff_ext::ExtensionField;
 
 use crate::{
@@ -26,6 +26,18 @@ pub struct MemFinalRecord {
     pub value: Word,
 }
 
+impl GetAddr for MemInitRecord {
+    fn get_addr(&self) -> Addr {
+        self.addr
+    }
+}
+
+impl GetAddr for MemFinalRecord {
+    fn get_addr(&self) -> Addr {
+        self.addr
+    }
+}
+
 /// - **Non-Volatile**: The initial values can be set to any arbitrary value.
 ///
 /// **Special Note**: