From 13205cdbdd207dfce0cba571fa19d0da7448fa0b Mon Sep 17 00:00:00 2001 From: Weicai Yang Date: Tue, 8 Nov 2022 22:06:06 -0800 Subject: [PATCH] [keymgr/dv] Update scb to check debug CSR 1. Update scb and sequence to check `debug` CSR 2. Remove regwen coverage as it's done in the base lib 3. Remove debug_csr_cg as we have alread had a fcov for that 4. small clean-up in keymgr_if Signed-off-by: Weicai Yang --- hw/ip/keymgr/data/keymgr_testplan.hjson | 13 ---- hw/ip/keymgr/dv/env/keymgr_env_cov.sv | 10 --- hw/ip/keymgr/dv/env/keymgr_if.sv | 22 ------ hw/ip/keymgr/dv/env/keymgr_scoreboard.sv | 73 +++++++++++++------ .../keymgr/dv/env/seq_lib/keymgr_base_vseq.sv | 8 +- 5 files changed, 57 insertions(+), 69 deletions(-) diff --git a/hw/ip/keymgr/data/keymgr_testplan.hjson b/hw/ip/keymgr/data/keymgr_testplan.hjson index da111d8185b90..c69735dc50f12 100644 --- a/hw/ip/keymgr/data/keymgr_testplan.hjson +++ b/hw/ip/keymgr/data/keymgr_testplan.hjson @@ -246,13 +246,6 @@ - SW input includes these CSRS: `*_sw_binding`, `salt`, `key_version`, `max_*_key_ver*`. - Cross with the corresponding regwen.''' } - { - name: control_w_regwen_cg - desc: ''' - - Similar to keymgr_sw_input_cg, create a separate cg as there are some reserved fields - scattered in the CSR. - ''' - } { name: err_code_cg desc: ''' @@ -285,12 +278,6 @@ a common direct test. - This is sampled when `fault_status` is read.''' } - { - name: csr_debug_cg - desc: ''' - - Cover all field in `debug`, which indicates different kinds of invalid inputs. - - This is sampled when `fault_status` is read.''' - } { name: sync_async_fault_cross_cg desc: ''' diff --git a/hw/ip/keymgr/dv/env/keymgr_env_cov.sv b/hw/ip/keymgr/dv/env/keymgr_env_cov.sv index 589bc7ae07426..410d7d370003b 100644 --- a/hw/ip/keymgr/dv/env/keymgr_env_cov.sv +++ b/hw/ip/keymgr/dv/env/keymgr_env_cov.sv @@ -157,15 +157,6 @@ class keymgr_env_cov extends cip_base_env_cov #(.CFG_T(keymgr_env_cfg)); key_ver_x_state_x_op_cross: cross key_version_cmp_cp, state_cp, op_cp; endgroup - covergroup control_w_regwen_cg with function sample(bit [14:0] control, bit regwen); - // There are some reserved fields, to simply it, just create 4 auto-bin - control_cp: coverpoint control { - option.auto_bin_max = 4; - } - regwen_cp: coverpoint regwen; - control_x_regwen_cr: cross control_cp, regwen_cp; - endgroup - function new(string name, uvm_component parent); super.new(name, parent); @@ -178,7 +169,6 @@ class keymgr_env_cov extends cip_base_env_cov #(.CFG_T(keymgr_env_cfg)); sync_async_fault_cross_cg = new(); reseed_interval_cg = new(); key_version_compare_cg = new(); - control_w_regwen_cg = new(); endfunction : new virtual function void build_phase(uvm_phase phase); diff --git a/hw/ip/keymgr/dv/env/keymgr_if.sv b/hw/ip/keymgr/dv/env/keymgr_if.sv index dfd94e01890cc..9b5e34eda046b 100644 --- a/hw/ip/keymgr/dv/env/keymgr_if.sv +++ b/hw/ip/keymgr/dv/env/keymgr_if.sv @@ -78,28 +78,6 @@ interface keymgr_if(input clk, input rst_n); bit edn_req_ack_sync; bit edn_req_ack_sync_done; - localparam int CtrlCntCopies = 2; - localparam int CtrlCntWitdh = 3; - localparam int KmacIfCntWitdh = 5; - localparam int StateWidth = 10; - localparam bit [StateWidth-1:0] KmacIfValidStates = {10'b1110100010, - 10'b0010011011, - 10'b0101000000, - 10'b1000101001, - 10'b1111111101, - 10'b0011101110}; - localparam bit [StateWidth-1:0] CtrlValidStates = {10'b1101100001, - 10'b1110010010, - 10'b0011110100, - 10'b0110101111, - 10'b0100000100, - 10'b1000011101, - 10'b0001001010, - 10'b1101111110, - 10'b1010101000, - 10'b0000110011, - 10'b1011000111}; - // If we need to wait for internal signal to be certain value, we may not be able to get that // when the sim is close to end. Define a cnt and MaxWaitCycle to avoid sim hang int cnt_to_wait_for_internal_value; diff --git a/hw/ip/keymgr/dv/env/keymgr_scoreboard.sv b/hw/ip/keymgr/dv/env/keymgr_scoreboard.sv index ac336defb6102..46dd48e7e92af 100644 --- a/hw/ip/keymgr/dv/env/keymgr_scoreboard.sv +++ b/hw/ip/keymgr/dv/env/keymgr_scoreboard.sv @@ -158,7 +158,8 @@ class keymgr_scoreboard extends cip_base_scoreboard #( .byte_data_q(item.byte_data_q)); end keymgr_pkg::StOwnerKey, keymgr_pkg::StDisabled, keymgr_pkg::StInvalid: begin - is_err = 0; + // set to 1 to check invalid data is used + is_err = 1; end default: `uvm_error(`gfn, $sformatf("Unexpected current_state: %0d", current_state)) endcase @@ -407,21 +408,20 @@ class keymgr_scoreboard extends cip_base_scoreboard #( // if incoming access is a write to a valid csr, then make updates right away if (addr_phase_write) begin // sample regwen and its locked CSRs - if (cfg.en_cov && ral.cfg_regwen.locks_reg_or_fld(dv_reg) && - cfg.keymgr_vif.get_keymgr_en()) begin + if (cfg.en_cov && cfg.keymgr_vif.get_keymgr_en()) begin bit cfg_regwen = (current_op_status == keymgr_pkg::OpWip); - if (csr.get_name() == "control_shadowed") begin - cov.control_w_regwen_cg.sample(item.a_data, cfg_regwen); - end else if (csr.get_name() == "sideload_clear") begin - cov.sideload_clear_cg.sample(`gmv(ral.sideload_clear.val), - current_state, - get_operation(), - cfg.keymgr_vif.aes_sideload_status == SideLoadAvail, - cfg.keymgr_vif.kmac_sideload_status == SideLoadAvail, - cfg.keymgr_vif.otbn_sideload_status == SideLoadAvail, - cfg_regwen); - end else begin - cov.sw_input_cg_wrap[csr.get_name()].sample(item.a_data, cfg_regwen); + if (ral.cfg_regwen.locks_reg_or_fld(dv_reg)) begin + if (csr.get_name() == "sideload_clear") begin + cov.sideload_clear_cg.sample(`gmv(ral.sideload_clear.val), + current_state, + get_operation(), + cfg.keymgr_vif.aes_sideload_status == SideLoadAvail, + cfg.keymgr_vif.kmac_sideload_status == SideLoadAvail, + cfg.keymgr_vif.otbn_sideload_status == SideLoadAvail, + cfg_regwen); + end else if (csr.get_name() != "control_shadowed") begin + cov.sw_input_cg_wrap[csr.get_name()].sample(item.a_data, cfg_regwen); + end end end if (cfg.en_cov && ral.sw_binding_regwen.locks_reg_or_fld(dv_reg)) begin @@ -578,8 +578,8 @@ class keymgr_scoreboard extends cip_base_scoreboard #( void'(ral.intr_state.predict(.value(1 << int'(IntrOpDone)))); end default: begin // other than StReset and StDisabled - bit good_key = get_is_kmac_key_correct(); - bit good_data = good_key && !get_sw_invalid_input() && !get_hw_invalid_input(); + bit good_key; + bit good_data; if (op == keymgr_pkg::OpAdvance) begin current_cdi = get_adv_cdi_type(); @@ -588,6 +588,20 @@ class keymgr_scoreboard extends cip_base_scoreboard #( int cdi_sel = `gmv(ral.control_shadowed.cdi_sel); `downcast(current_cdi, cdi_sel) end + // call this after latch_otp_key, as get_is_kmac_key_correct/get_hw_invalid_input + // need to know what key is used for this OP. + good_key = get_is_kmac_key_correct(); + good_data = good_key && !get_sw_invalid_input() && !get_hw_invalid_input(); + if (!good_key) begin + // if invalid key is used, design will switch to use random data for the + // operation. Set a non all 0s/1s data (use 1 here) for them in scb, so that it + // doesn't lead to an invalid_key error + current_internal_key[Sealing][0] = 1; + current_internal_key[Sealing][1] = 1; + current_internal_key[Attestation][0] = 1; + current_internal_key[Attestation][1] = 1; + end + // update kmac key for check if (current_internal_key[current_cdi] > 0) begin cfg.keymgr_vif.update_kdf_key(current_internal_key[current_cdi], current_state, @@ -701,6 +715,9 @@ class keymgr_scoreboard extends cip_base_scoreboard #( `DV_CHECK_EQ(item.d_data[keymgr_pkg::FaultKmacOut], is_kmac_invalid_data) end end + "debug": begin + // do nothing + end default: begin if (!uvm_re_match("sw_share*", csr.get_name())) begin // sw_share // if keymgr isn't On, SW output should be entropy and not match to predict value @@ -747,11 +764,12 @@ class keymgr_scoreboard extends cip_base_scoreboard #( current_cdi); endfunction - virtual function bit [TL_DW-1:0] get_current_max_version(); + virtual function bit [TL_DW-1:0] get_current_max_version( + keymgr_pkg::keymgr_working_state_e state = current_state); // design change this to 0 if LC turns off keymgr. if (!cfg.keymgr_vif.get_keymgr_en()) return 0; - case (current_state) + case (state) keymgr_pkg::StCreatorRootKey: return `gmv(ral.max_creator_key_ver_shadowed); keymgr_pkg::StOwnerIntKey: return `gmv(ral.max_owner_int_key_ver_shadowed); keymgr_pkg::StOwnerKey: return `gmv(ral.max_owner_key_ver_shadowed); @@ -835,8 +853,11 @@ class keymgr_scoreboard extends cip_base_scoreboard #( endfunction virtual function bit get_sw_invalid_input(); - if (get_operation() inside {keymgr_pkg::OpGenSwOut, keymgr_pkg::OpGenHwOut}) begin - return get_current_max_version() < `gmv(ral.key_version[0]); + if (get_operation() inside {keymgr_pkg::OpGenSwOut, keymgr_pkg::OpGenHwOut} && + current_state != keymgr_pkg::StReset && + get_current_max_version() < `gmv(ral.key_version[0])) begin + void'(ral.debug.invalid_key_version.predict(1)); + return 1; end else begin return 0; end @@ -860,9 +881,9 @@ class keymgr_scoreboard extends cip_base_scoreboard #( if ((current_internal_key[current_cdi][0] inside {0, '1} || current_internal_key[current_cdi][1] inside {0, '1}) && - current_state != keymgr_pkg::StReset) - begin + current_state != keymgr_pkg::StReset) begin invalid_hw_input_type = OtpRootKeyInvalid; + void'(ral.debug.invalid_key.predict(1)); err_cnt++; `uvm_info(`gfn, $sformatf("internal key for %s %s is invalid", current_state, current_cdi), UVM_LOW) @@ -874,30 +895,35 @@ class keymgr_scoreboard extends cip_base_scoreboard #( keymgr_pkg::StInit: begin if (cfg.keymgr_vif.keymgr_div inside {0, '1}) begin invalid_hw_input_type = LcStateInvalid; + void'(ral.debug.invalid_health_state.predict(1)); err_cnt++; `uvm_info(`gfn, "HW invalid input on keymgr_div", UVM_LOW) end if (cfg.keymgr_vif.otp_device_id inside {0, '1}) begin invalid_hw_input_type = OtpDevIdInvalid; + void'(ral.debug.invalid_dev_id.predict(1)); err_cnt++; `uvm_info(`gfn, "HW invalid input on otp_device_id", UVM_LOW) end if (cfg.keymgr_vif.rom_digest.data inside {0, '1}) begin invalid_hw_input_type = RomDigestInvalid; + void'(ral.debug.invalid_digest.predict(1)); err_cnt++; `uvm_info(`gfn, "HW invalid input on rom_digest", UVM_LOW) end if (!cfg.keymgr_vif.rom_digest.valid) begin invalid_hw_input_type = RomDigestValidLow; + void'(ral.debug.invalid_digest.predict(1)); err_cnt++; `uvm_info(`gfn, "HW invalid input, rom_digest.valid is low", UVM_LOW) end if (cfg.keymgr_vif.flash.seeds[flash_ctrl_pkg::CreatorSeedIdx] inside {0, '1}) begin invalid_hw_input_type = FlashCreatorSeedInvalid; + void'(ral.debug.invalid_creator_seed.predict(1)); err_cnt++; `uvm_info(`gfn, "HW invalid input on flash.seeds[CreatorSeedIdx]", UVM_LOW) end @@ -905,6 +931,7 @@ class keymgr_scoreboard extends cip_base_scoreboard #( keymgr_pkg::StCreatorRootKey: begin if (cfg.keymgr_vif.flash.seeds[flash_ctrl_pkg::OwnerSeedIdx] inside {0, '1}) begin invalid_hw_input_type = FlashOwnerSeedInvalid; + void'(ral.debug.invalid_owner_seed.predict(1)); err_cnt++; `uvm_info(`gfn, "HW invalid input on flash.seeds[OwnerSeedIdx]", UVM_LOW) end diff --git a/hw/ip/keymgr/dv/env/seq_lib/keymgr_base_vseq.sv b/hw/ip/keymgr/dv/env/seq_lib/keymgr_base_vseq.sv index 66b98a7baa892..c9b777634331f 100644 --- a/hw/ip/keymgr/dv/env/seq_lib/keymgr_base_vseq.sv +++ b/hw/ip/keymgr/dv/env/seq_lib/keymgr_base_vseq.sv @@ -180,7 +180,7 @@ class keymgr_base_vseq extends cip_base_vseq #( current_state.name, cast_operation.name, is_good_op), UVM_MEDIUM) // wait for status to get out of OpWip and check - csr_spinwait(.ptr(ral.op_status.status), .exp_data(keymgr_pkg::OpWip), .timeout_ns(1000_000), + csr_spinwait(.ptr(ral.op_status.status), .exp_data(keymgr_pkg::OpWip), .compare_op(CompareOpNe), .spinwait_delay_ns($urandom_range(0, 100))); exp_status = is_good_op ? keymgr_pkg::OpDoneSuccess : keymgr_pkg::OpDoneFail; @@ -211,6 +211,12 @@ class keymgr_base_vseq extends cip_base_vseq #( if (rd_val != 0) begin csr_wr(.ptr(ral.intr_state), .value(rd_val)); end + // read and clear debug CSRs, check is done in scb + csr_rd(.ptr(ral.debug), .value(rd_val)); + if (rd_val != 0) begin + // this CSR is w0c + csr_wr(.ptr(ral.debug), .value(~rd_val)); + end endtask : wait_op_done virtual task read_current_state();