PoC: Control access to FW RAM

Allow FW RAM access only in the following execution contexts: - Firmware mode - IRQ_SYSCALL_HI Input port `system_mode` of the `fw_ram` module is replaced with an enable port. Since access to FW RAM not longer depend only on system_mode
tillitis · Dec 11, 2024 · 3097cfe · 3097cfe
1 parent a171a10
commit 3097cfe
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 37 deletions.
diff --git a/hw/application_fpga/README.md b/hw/application_fpga/README.md
@@ -138,16 +138,18 @@ Interrupts can be enabled/disabled using the PicoRV32 specific
 The following table shows resource availablility for each execution
 context:
 
-| *Execution Context* | *ROM*  |
-|---------------------|--------|
-| Firmware mode       | r/x    |
-| App mode            | r      |
-| IRQ_SYSCALL_LO      | r/x    |
-| IRQ_SYSCALL_HI      | r/x    |
+| *Execution Context* | *ROM*  | *FW RAM* |
+|---------------------|--------|----------|
+| Firmware mode       | r/x    | r/w      |
+| App mode            | r      | i        |
+| IRQ_SYSCALL_LO      | r/x    | i        |
+| IRQ_SYSCALL_HI      | r/x    | r/w      |
 
 Legend:
 r = readable
+w = writeable
 x = executable
+i = invisible
 
 ## `tk1`
 

diff --git a/hw/application_fpga/core/fw_ram/rtl/fw_ram.v b/hw/application_fpga/core/fw_ram/rtl/fw_ram.v
@@ -17,8 +17,7 @@ module fw_ram (
     input wire clk,
     input wire reset_n,
 
-    input wire system_mode,
-
+    input  wire          en,
     input  wire          cs,
     input  wire [ 3 : 0] we,
     input  wire [ 8 : 0] address,
@@ -31,21 +30,19 @@ module fw_ram (
   //----------------------------------------------------------------
   // Registers and wires.
   //----------------------------------------------------------------
-  reg  [31 : 0] tmp_read_data;
-  reg  [31 : 0] mem_read_data0;
-  reg  [31 : 0] mem_read_data1;
-  reg           ready_reg;
-  wire          system_mode_cs;
-  reg           bank0;
-  reg           bank1;
+  reg [31 : 0] tmp_read_data;
+  reg [31 : 0] mem_read_data0;
+  reg [31 : 0] mem_read_data1;
+  reg          ready_reg;
+  reg          bank0;
+  reg          bank1;
 
 
   //----------------------------------------------------------------
   // Concurrent assignment of ports.
   //----------------------------------------------------------------
-  assign read_data      = tmp_read_data;
-  assign ready          = ready_reg;
-  assign system_mode_cs = cs && ~system_mode;
+  assign read_data = tmp_read_data;
+  assign ready     = ready_reg;
 
 
   //----------------------------------------------------------------
@@ -56,12 +53,12 @@ module fw_ram (
       .RADDR({3'h0, address[7 : 0]}),
       .RCLK(clk),
       .RCLKE(1'h1),
-      .RE(system_mode_cs & bank0),
+      .RE(en & cs & bank0),
       .WADDR({3'h0, address[7 : 0]}),
       .WCLK(clk),
       .WCLKE(1'h1),
       .WDATA(write_data[15 : 0]),
-      .WE((|we & system_mode_cs & bank0)),
+      .WE((|we & en & cs & bank0)),
       .MASK({{8{~we[1]}}, {8{~we[0]}}})
   );
 
@@ -70,12 +67,12 @@ module fw_ram (
       .RADDR({3'h0, address[7 : 0]}),
       .RCLK(clk),
       .RCLKE(1'h1),
-      .RE(system_mode_cs & bank0),
+      .RE(en & cs & bank0),
       .WADDR({3'h0, address[7 : 0]}),
       .WCLK(clk),
       .WCLKE(1'h1),
       .WDATA(write_data[31 : 16]),
-      .WE((|we & system_mode_cs & bank0)),
+      .WE((|we & en & cs & bank0)),
       .MASK({{8{~we[3]}}, {8{~we[2]}}})
   );
 
@@ -85,12 +82,12 @@ module fw_ram (
       .RADDR({3'h0, address[7 : 0]}),
       .RCLK(clk),
       .RCLKE(1'h1),
-      .RE(system_mode_cs & bank1),
+      .RE(en & cs & bank1),
       .WADDR({3'h0, address[7 : 0]}),
       .WCLK(clk),
       .WCLKE(1'h1),
       .WDATA(write_data[15 : 0]),
-      .WE((|we & system_mode_cs & bank1)),
+      .WE((|we & en & cs & bank1)),
       .MASK({{8{~we[1]}}, {8{~we[0]}}})
   );
 
@@ -99,12 +96,12 @@ module fw_ram (
       .RADDR({3'h0, address[7 : 0]}),
       .RCLK(clk),
       .RCLKE(1'h1),
-      .RE(system_mode_cs & bank1),
+      .RE(en & cs & bank1),
       .WADDR({3'h0, address[7 : 0]}),
       .WCLK(clk),
       .WCLKE(1'h1),
       .WDATA(write_data[31 : 16]),
-      .WE((|we & system_mode_cs & bank1)),
+      .WE((|we & en & cs & bank1)),
       .MASK({{8{~we[3]}}, {8{~we[2]}}})
   );
 
@@ -129,7 +126,7 @@ module fw_ram (
     bank1         = 1'h0;
     tmp_read_data = 32'h0;
 
-    if (system_mode_cs) begin
+    if (en & cs) begin
       if (address[8]) begin
         bank1 = 1'h1;
         tmp_read_data = mem_read_data1;

diff --git a/hw/application_fpga/core/tk1/rtl/tk1.v b/hw/application_fpga/core/tk1/rtl/tk1.v
@@ -48,6 +48,8 @@ module tk1 #(
     input wire access_level_hi,
     input wire access_level_med,
 
+    output wire fw_ram_en,
+
     input  wire          cs,
     input  wire          we,
     input  wire [ 7 : 0] address,
@@ -202,6 +204,7 @@ module tk1 #(
   assign system_reset  = system_reset_reg;
 
   assign rom_exec_en   = !system_mode | access_level_med | access_level_hi;
+  assign fw_ram_en     = !system_mode | access_level_hi;
 
   //----------------------------------------------------------------
   // Module instance.

diff --git a/hw/application_fpga/fw/irqpoc_with_app/start.S b/hw/application_fpga/fw/irqpoc_with_app/start.S
@@ -11,6 +11,8 @@
 
 #include "custom_ops.S"  // PicoRV32 custom instructions
 
+#define illegal_insn() .word 0
+
 	.section ".text.init"
 	.globl _start
 _start:
@@ -24,18 +26,38 @@ irq_handler:
 	// PicoRV32 stores the IRQ bitmask in x4.
 	// If bit 31 is 1: IRQ31 was triggered.
 	// If bit 30 is 1: IRQ30 was triggered.
-
-	nop // NOPs are not necessary. Only added to make it easier to find
-	nop // when simulating.
-	nop
+irq_syscall_lo_check:
+	li t4, (1 << 30)
+	bne x4, t4, irq_syscall_hi_check
+	// Firmware RAM should not be readable from IRQ_SYSCALL_LO
+	call check_cannot_read_test_val_from_fw_ram
+	j irq_source_check_done
+irq_syscall_hi_check:
+	li t4, (1 << 31)
+	bne x4, t4, unexpected_irq
+	// Firmware RAM should be readable from IRQ_SYSCALL_HI
+	call check_can_read_test_val_from_fw_ram
+	j irq_source_check_done
+unexpected_irq:
+	illegal_insn()
+irq_source_check_done:
 	picorv32_retirq_insn() // Return from interrupt
 
 //
 // Init
 //
-	.=0x20 // Setting location of init to 0x20. Makes it easier to find when
-	       // simulating.
+	.=0x100
 init:
+	// Save test value in firmware RAM
+	li t0, 0xd0000000
+	li t1, 0x5555aaaa
+	sw t1, 0(t0)
+
+	// Firmware RAM should be readable from firmware mode
+	call check_can_read_test_val_from_fw_ram
+
+
+	// Enable IRQs
 	li t0, 0x3fffffff // IRQ31 & IRQ30 mask
 	picorv32_maskirq_insn(zero, t0) // Enable IRQs
 
@@ -59,17 +81,43 @@ copy_app:
 //
 	.align 4
 app_start:
+	// Firmware RAM should not be readable from app mode
+	call check_cannot_read_test_val_from_fw_ram
+
+	// Raise IRQ_SYSCALL_HI
 	li t0, 0xe1000000 // IRQ_SYSCALL_HI (IRQ31) trigger address
 	sw zero, 0(t0) // Raise IRQ by writing to interrupt trigger address.
 	               // Writing any data triggers an interrupt.
 
+	// Raise IRQ_SYSCALL_LO
 	li t0, 0xe0000000 // IRQ_SYSCALL_LO (IRQ30) trigger address
 	sw zero, 0(t0) // Raise IRQ by writing to interrupt trigger address.
 	               // Writing any data triggers an interrupt.
 
 	jalr zero, 0(zero) // Jumping to firmware. Expecting trap
 app_loop:
 	j app_loop
+
+
+check_cannot_read_test_val_from_fw_ram:
+	li t0, 0xd0000000
+	lw t1, 0(t0)
+	li t2, 0
+	bne t1, t2, cannot_read_test_val_from_fw_ram_fail
+	ret
+cannot_read_test_val_from_fw_ram_fail:
+	illegal_insn()
+
+check_can_read_test_val_from_fw_ram:
+	// Check that saved test value can not be read while in app mode
+	li t0, 0xd0000000
+	lw t1, 0(t0)
+	li t2, 0x5555aaaa
+	bne t1, t2, can_read_test_val_from_fw_ram_fail
+	ret
+can_read_test_val_from_fw_ram_fail:
+	illegal_insn()
+
 	.align 4
 app_end:
 
diff --git a/hw/application_fpga/rtl/application_fpga.v b/hw/application_fpga/rtl/application_fpga.v
@@ -135,6 +135,7 @@ module application_fpga (
   reg  [31 : 0] fw_ram_write_data;
   wire [31 : 0] fw_ram_read_data;
   wire          fw_ram_ready;
+  wire          fw_ram_en;
 
   reg           touch_sense_cs;
   reg           touch_sense_we;
@@ -257,7 +258,7 @@ module application_fpga (
       .clk(clk),
       .reset_n(reset_n),
 
-      .system_mode(system_mode),
+      .en(fw_ram_en),
 
       .cs(fw_ram_cs),
       .we(fw_ram_we),
@@ -370,6 +371,8 @@ module application_fpga (
       .access_level_hi (irq31_eoi),
       .access_level_med(irq30_eoi),
 
+      .fw_ram_en(fw_ram_en),
+
       .cs(tk1_cs),
       .we(tk1_we),
       .address(tk1_address),

diff --git a/hw/application_fpga/tb/application_fpga_sim.v b/hw/application_fpga/tb/application_fpga_sim.v
@@ -147,6 +147,7 @@ module application_fpga_sim (
   reg  [31 : 0] fw_ram_write_data;
   wire [31 : 0] fw_ram_read_data;
   wire          fw_ram_ready;
+  wire          fw_ram_en;
 
   reg           touch_sense_cs;
   reg           touch_sense_we;
@@ -268,8 +269,7 @@ module application_fpga_sim (
       .clk(clk),
       .reset_n(reset_n),
 
-      .system_mode(system_mode),
-
+      .en(fw_ram_en),
       .cs(fw_ram_cs),
       .we(fw_ram_we),
       .address(fw_ram_address),
@@ -383,6 +383,8 @@ module application_fpga_sim (
       .access_level_hi (irq31_eoi),
       .access_level_med(irq30_eoi),
 
+      .fw_ram_en(fw_ram_en),
+
       .cs(tk1_cs),
       .we(tk1_we),
       .address(tk1_address),

diff --git a/hw/application_fpga/tb/tb_application_fpga_sim.v b/hw/application_fpga/tb/tb_application_fpga_sim.v
@@ -80,7 +80,7 @@ module tb_application_fpga_sim ();
   //----------------------------------------------------------------
   initial begin
     // End simulation after XXX time units (set by timescale)
-    #1600;
+    #3000;
     $display("TIMEOUT");
     $finish;
   end