Formal review items

.github/workflow_metadata/pr_hash

@@ -1 +1 @@
-2730a972468f44713ef2c8fcd3daafc16ddceb167141a6f0b1aa221285c4a57f4246faccf81b10dcec9061f818b5727c
+73ce940014b3267d66a729044d8baf1891c7a480835c20a37bfae6c5616d29fc656eb4941d9c293b685dfacafc6849bf

.github/workflow_metadata/pr_timestamp

@@ -1 +1 @@
-1736287539
+1736372754

src/mldsa_top/rtl/mldsa_ctrl.sv

@@ -146,6 +146,8 @@ module mldsa_ctrl
   input pcr_signing_t pcr_signing_data,
   `endif
 
+  output logic busy_o,
+
   //Interrupts
   output logic error_intr,
   output logic notif_intr
@@ -352,6 +354,9 @@ always_comb mldsa_privkey_lock = '0;
       counter_reg <= counter_reg + 1;
   end
 
+  //Set busy when engine is in progress
+  always_comb busy_o = ~mldsa_ready;
+
 //HWIF to reg block
   always_comb mldsa_reg_hwif_in.reset_b = rst_b;
   always_comb mldsa_reg_hwif_in.hard_reset_b = rst_b; //FIXME interface needs a hard reset signal?
@@ -641,18 +646,22 @@ always_comb mldsa_privkey_lock = '0;
   logic api_sig_z_re, api_sig_z_re_f, api_sig_z_we;
   logic [SIG_ADDR_W-1:0] api_sig_addr;
   mldsa_signature_z_addr_t api_sig_z_addr;
+  mldsa_signature_z_addr_t api_sig_z_addr_f;
   logic [SIG_C_REG_ADDR_W-1:0] api_sig_c_addr;
   logic [SIG_H_REG_ADDR_W-1:0] api_sig_h_addr;
   logic [DATA_WIDTH-1:0] signature_reg_rdata;
 
 
   always_ff @(posedge clk or negedge rst_b) begin
-    if (!rst_b)
+    if (!rst_b) begin
       api_sig_z_re_f <= '0;
-    else if (zeroize)
+      api_sig_z_addr_f <= '0;
+    end else if (zeroize) begin
       api_sig_z_re_f <= '0;
-    else begin
+      api_sig_z_addr_f <= '0;
+    end else begin
       api_sig_z_re_f <= api_sig_z_re;
+      api_sig_z_addr_f <= api_sig_z_addr;
     end
   end
 
@@ -691,7 +700,7 @@ always_comb mldsa_privkey_lock = '0;
                                 ({SIG_Z_MEM_ADDR_W{api_sig_z_re}} & api_sig_z_addr.addr);
 
   always_comb sigdecode_z_rd_data_o = sig_z_ram_rdata;
-  always_comb mldsa_reg_hwif_in.MLDSA_SIGNATURE.rd_data = api_sig_z_re_f ? sig_z_ram_rdata[api_sig_z_addr.offset] : signature_reg_rdata;
+  always_comb mldsa_reg_hwif_in.MLDSA_SIGNATURE.rd_data = api_sig_z_re_f ? sig_z_ram_rdata[api_sig_z_addr_f.offset] : signature_reg_rdata;
 
   //write requests
   always_comb sig_z_ram_we = (sigencode_wr_req_i.rd_wr_en == RW_WRITE) | api_sig_z_we | zeroize_mem_we;
@@ -766,6 +775,7 @@ always_comb mldsa_privkey_lock = '0;
   logic api_pubkey_re, api_pubkey_re_f, api_pubkey_we;
   logic [PK_ADDR_W-1:0] api_pubkey_addr;
   mldsa_pubkey_mem_addr_t api_pubkey_mem_addr;
+  mldsa_pubkey_mem_addr_t api_pubkey_mem_addr_f;
   mldsa_pubkey_mem_addr_t sampler_pubkey_mem_addr;
   logic [PK_RHO_REG_ADDR_W-1:0] api_pk_rho_addr;
   logic [DATA_WIDTH-1:0] pk_reg_rdata;
@@ -775,16 +785,19 @@ always_comb mldsa_privkey_lock = '0;
   always_ff @(posedge clk or negedge rst_b) begin
     if (!rst_b) begin
       api_pubkey_re_f <= '0;
+      api_pubkey_mem_addr_f <= '0;
       sampler_pk_rd_en_f <= '0;
       sampler_src_offset_f <= '0;
       pkdecode_rd_offset_f <= '0;
     end else if (zeroize) begin
       api_pubkey_re_f <= '0;
+      api_pubkey_mem_addr_f <= '0;
       sampler_pk_rd_en_f <= '0;
       sampler_src_offset_f <= '0;
       pkdecode_rd_offset_f <= '0;
     end else begin
       api_pubkey_re_f <= api_pubkey_re;
+      api_pubkey_mem_addr_f <= api_pubkey_mem_addr;
       sampler_pk_rd_en_f <= msg_hold ? sampler_pk_rd_en_f : sampler_pk_rd_en;
       sampler_src_offset_f <= msg_hold ? sampler_src_offset_f : sampler_pubkey_mem_addr.offset[2:0];
       pkdecode_rd_offset_f <= pkdecode_rd_addr_i[1:0];
@@ -842,7 +855,7 @@ always_comb mldsa_privkey_lock = '0;
     endcase
   end
 
-  always_comb mldsa_reg_hwif_in.MLDSA_PUBKEY.rd_data = api_pubkey_re_f ? pubkey_ram_rdata[api_pubkey_mem_addr.offset] : pk_reg_rdata;
+  always_comb mldsa_reg_hwif_in.MLDSA_PUBKEY.rd_data = api_pubkey_re_f ? pubkey_ram_rdata[api_pubkey_mem_addr_f.offset] : pk_reg_rdata;
 
   //write requests
   always_comb pubkey_ram_we = (pk_t1_wren_i) | api_pubkey_we | zeroize_mem_we;
@@ -899,26 +912,22 @@ always_comb mldsa_privkey_lock = '0;
     end else if (zeroize) begin
       msg_data <= '0;
     end else begin
-      if (msg_hold) begin
-        msg_data <= msg_data;
-      end else begin
-        unique case (sampler_src) inside
-          MLDSA_SEED_ID:        msg_data <= msg_done ? {48'b0,sampler_imm} : {seed_reg[{sampler_src_offset[1:0],1'b1}],seed_reg[{sampler_src_offset[1:0],1'b0}]};
-          MLDSA_RHO_ID:         msg_data <= msg_done ? {48'b0,sampler_imm} : rho_reg[sampler_src_offset[1:0]];
-          MLDSA_RHO_P_ID:       msg_data <= msg_done ? {48'b0,sampler_imm} : rho_p_reg[sampler_src_offset[2:0]];
-          MLDSA_TR_ID:          msg_data <= privatekey_reg.enc.tr[sampler_src_offset[2:0]];
-          MLDSA_MSG_ID:         msg_data <= {msg_p_reg[{sampler_src_offset[3:0],1'b1}],msg_p_reg[{sampler_src_offset[3:0],1'b0}]};
-          MLDSA_K_ID:           msg_data <= privatekey_reg.enc.K[sampler_src_offset[1:0]];
-          MLDSA_MU_ID:          msg_data <= mu_reg[sampler_src_offset[2:0]];
-          MLDSA_SIGN_RND_ID:    msg_data <= {sign_rnd_reg[{sampler_src_offset[1:0],1'b1}],sign_rnd_reg[{sampler_src_offset[1:0],1'b0}]};
-          MLDSA_RHO_P_KAPPA_ID: msg_data <= msg_done ? {48'b0,(kappa_reg + sampler_imm[2:0])} : rho_p_reg[sampler_src_offset[2:0]];
-          MLDSA_SIG_C_REG_ID:   msg_data <= {signature_reg.enc.c[{sampler_src_offset[2:0],1'b1}], signature_reg.enc.c[{sampler_src_offset[2:0],1'b0}]};
-          MLDSA_PK_REG_ID:      msg_data <= {publickey_reg.enc.rho[{sampler_src_offset[1:0],1'b1}],publickey_reg.enc.rho[{sampler_src_offset[1:0],1'b0}]};
-      	  MLDSA_ENTROPY_ID:     msg_data <= lfsr_entropy_reg[sampler_src_offset[2:0]];
-      	  MLDSA_CNT_ID:         msg_data <= counter_reg;
-          default:              msg_data <= '0;
-        endcase
-      end
+      unique case (sampler_src) inside
+        MLDSA_SEED_ID:        msg_data <= msg_done ? {48'b0,sampler_imm} : {seed_reg[{sampler_src_offset[1:0],1'b1}],seed_reg[{sampler_src_offset[1:0],1'b0}]};
+        MLDSA_RHO_ID:         msg_data <= msg_done ? {48'b0,sampler_imm} : rho_reg[sampler_src_offset[1:0]];
+        MLDSA_RHO_P_ID:       msg_data <= msg_done ? {48'b0,sampler_imm} : rho_p_reg[sampler_src_offset[2:0]];
+        MLDSA_TR_ID:          msg_data <= privatekey_reg.enc.tr[sampler_src_offset[2:0]];
+        MLDSA_MSG_ID:         msg_data <= {msg_p_reg[{sampler_src_offset[3:0],1'b1}],msg_p_reg[{sampler_src_offset[3:0],1'b0}]};
+        MLDSA_K_ID:           msg_data <= privatekey_reg.enc.K[sampler_src_offset[1:0]];
+        MLDSA_MU_ID:          msg_data <= mu_reg[sampler_src_offset[2:0]];
+        MLDSA_SIGN_RND_ID:    msg_data <= {sign_rnd_reg[{sampler_src_offset[1:0],1'b1}],sign_rnd_reg[{sampler_src_offset[1:0],1'b0}]};
+        MLDSA_RHO_P_KAPPA_ID: msg_data <= msg_done ? {48'b0,(kappa_reg + sampler_imm[2:0])} : rho_p_reg[sampler_src_offset[2:0]];
+        MLDSA_SIG_C_REG_ID:   msg_data <= {signature_reg.enc.c[{sampler_src_offset[2:0],1'b1}], signature_reg.enc.c[{sampler_src_offset[2:0],1'b0}]};
+        MLDSA_PK_REG_ID:      msg_data <= {publickey_reg.enc.rho[{sampler_src_offset[1:0],1'b1}],publickey_reg.enc.rho[{sampler_src_offset[1:0],1'b0}]};
+        MLDSA_ENTROPY_ID:     msg_data <= lfsr_entropy_reg[sampler_src_offset[2:0]];
+        MLDSA_CNT_ID:         msg_data <= counter_reg;
+        default:              msg_data <= '0;
+      endcase
     end 
   end
 
@@ -1280,7 +1289,7 @@ always_comb msg_hold = msg_valid_o & ~msg_rdy_i;
 
 //Done when msg count is equal to length
 //length is in bytes - compare against MSB from strobe width gets us the length in msg interface chunks
-always_comb msg_done = msg_cnt >= prim_instr.length[MLDSA_OPR_WIDTH-1:$clog2(MsgStrbW)];
+always_comb msg_done = (msg_cnt >= prim_instr.length[MLDSA_OPR_WIDTH-1:$clog2(MsgStrbW)]);
 
 always_ff @(posedge clk or negedge rst_b) begin
   if (!rst_b) begin
@@ -1294,8 +1303,8 @@ always_ff @(posedge clk or negedge rst_b) begin
     msg_strobe_o <= 0;
   end
   else begin
-    msg_cnt <= msg_done  ? 'd0 :
-               msg_hold  ? msg_cnt : 
+    msg_cnt <= msg_hold  ? msg_cnt : 
+               msg_done  ? 'd0 :
                msg_valid ? msg_cnt + 'd1 : msg_cnt;
     msg_valid_o <= msg_hold ? msg_valid_o : msg_valid;
     msg_strobe_o <= msg_hold ? msg_strobe_o :
@@ -1346,7 +1355,7 @@ always_comb begin : primary_ctrl_fsm_out_combo
         msg_valid = 1;
         sampler_src = prim_instr.operand1;
         sampler_imm = prim_instr.imm;
-        if (msg_done) begin
+        if (msg_done & ~msg_hold) begin
           if (prim_instr.opcode.sampler_en) ctrl_fsm_ns = MLDSA_CTRL_FUNC_START;
           else ctrl_fsm_ns = MLDSA_CTRL_MSG_WAIT;
         end

src/mldsa_top/rtl/mldsa_reg.rdl

@@ -13,8 +13,6 @@
 // limitations under the License.
 //
 
-//`include "kv_def.rdl" //fixme
-
 addrmap mldsa_reg {
     desc="address maps for adams bridge register space";
 

src/mldsa_top/rtl/mldsa_top.sv

@@ -68,6 +68,8 @@ module mldsa_top
   input pcr_signing_t pcr_signing_data,
   `endif
 
+  output logic                      busy_o,
+
   output logic                      error_intr,
   output logic                      notif_intr
 
@@ -78,7 +80,6 @@ module mldsa_top
 
 //Signal Declarations
   logic zeroize_reg;
-  logic [1:0] cmd_reg;
 
   mldsa_sampler_mode_e       sampler_mode;
   logic                      sha3_start;
@@ -400,6 +401,7 @@ mldsa_ctrl mldsa_ctrl_inst
   .lfsr_enable_o(lfsr_enable),
   .lfsr_seed_o(lfsr_seed),
 
+  .busy_o(busy_o),
   .zeroize_mem_o(zeroize_mem),
 
   .error_intr(error_intr),
@@ -636,7 +638,6 @@ skencode_inst
 
   .skencode_enable(skencode_enable),
   .skencode_done(skencode_done),
-  .skencode_error(),
 
   .keymem_a_wr_req(skencode_keymem_if),
   .keymem_a_wr_data(skencode_wr_data),
@@ -857,7 +858,7 @@ always_comb w1_mem_waddr = (w1_mem_wr_req.addr[MLDSA_MEM_W1_ADDR_WIDTH-1:0]) |
 always_comb w1_mem_wdata = w1_mem_wr_data;
 
 //w1 memory
-`ABR_MEM_TEST(512, 4)
+`ABR_MEM(512,4)
 mldsa_w1_mem_inst
 (
   .clk_i(clk),

src/mldsa_top/uvmf/uvmf_template_output/project_benches/mldsa/tb/testbench/hdl_top.sv

@@ -128,6 +128,7 @@ import kv_defines_pkg::*;
       .kv_read(),
       .kv_rd_resp(kv_rd_resp),
 `endif
+      .busy_o(),
       .error_intr(),
       .notif_intr()
     );

src/sk_encode/rtl/skencode.sv

@@ -54,7 +54,6 @@ module skencode
         output mem_if_t mem_b_rd_req,
         output logic [AHB_DATA_WIDTH-1:0] keymem_a_wr_data,
 
-        output logic skencode_error,
         output logic skencode_done
     );
 
@@ -80,7 +79,7 @@ module skencode
 
 
     logic [2:0] main_state, next_main_state, write_state, next_write_state;
-    logic error_flag, asserted_error_flag;
+    logic error_flag;
     logic [7:0][2:0] encoded_coeffs;
     logic [7:0] encoding_error;
     logic [23:0] one_encoding_string;
@@ -92,10 +91,6 @@ module skencode
 
     assign one_encoding_string = {encoded_coeffs[7],encoded_coeffs[6],encoded_coeffs[5],encoded_coeffs[4],
                                     encoded_coeffs[3],encoded_coeffs[2],encoded_coeffs[1],encoded_coeffs[0]};
-
-    //FIXME enable this later?
-    assign error_flag = 0;//encoding_error[0] | encoding_error[1] | encoding_error[2] | encoding_error[3] |
-                        //encoding_error[4] | encoding_error[5] | encoding_error[6] | encoding_error[7];
 
 
     // State Machine: Updates main_state and write_state based on current conditions and next states.
@@ -109,7 +104,7 @@ module skencode
             write_state <= IDLE;
         end
         else begin
-            if (skencode_error) begin
+            if (error_flag) begin
                 main_state     <= IDLE;
                 write_state    <= IDLE;
             end
@@ -241,7 +236,7 @@ module skencode
     // Locks destination and source addresses when skencode_enable is active.
     // Manages write_buffer and producer_selector based on the main state machine.
     // Issues read requests to memory and increments num_mem_operands during appropriate states.
-    // Signals completion (skencode_done) and checks for errors (skencode_error) based on operation progress.
+    // Signals completion (skencode_done) based on operation progress.
     always_ff @(posedge clk or negedge reset_n) begin
         if (!reset_n) begin
             write_buffer        <= 'h0;
@@ -250,7 +245,6 @@ module skencode
             locked_dest_addr    <= 'h0;
             locked_src_addr     <= 'h0;
             skencode_done       <= 'h0;
-            skencode_error      <= 'h0;
             mem_a_rd_req        <= '{rd_wr_en: RW_IDLE, addr: '0};
             mem_b_rd_req        <= '{rd_wr_en: RW_IDLE, addr: '0};
         end
@@ -261,7 +255,6 @@ module skencode
             locked_dest_addr    <= 'h0;
             locked_src_addr     <= 'h0;
             skencode_done       <= 'h0;
-            skencode_error      <= 'h0;
             mem_a_rd_req        <= '{rd_wr_en: RW_IDLE, addr: '0};
             mem_b_rd_req        <= '{rd_wr_en: RW_IDLE, addr: '0};
         end
@@ -300,9 +293,6 @@ module skencode
             else begin
                 skencode_done      <= 'h0;
             end
-
-            if (skencode_error == 1'b0 && main_state != IDLE)
-                skencode_error <= error_flag;
         end
     end
 
@@ -396,5 +386,8 @@ module skencode
         end
     end : encoding
 
+    assign error_flag = (|encoding_error) & main_state inside {READ_ENC_and_CONSUME, ENC_and_CONSUME, CONSUME, CONSUME_LAST};
+
+    `ABR_ASSERT_NEVER(SKENCODE_ERROR_FLAG, error_flag, clk, !reset_n)
 
 endmodule