Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dv,membkdr] Updating membackdoor loading utilities #20864

Open
wants to merge 1 commit into
base: integrated_dev
Choose a base branch
from
Open

[dv,membkdr] Updating membackdoor loading utilities #20864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .bazelrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ build:ubsan --linkopt -fsanitize=undefined
# Enable the rust nightly toolchain
build --@rules_rust//rust/toolchain/channel=nightly

#Add Centos bin link
build --@rules_rust//:extra_rustc_toolchain_dirs=/tools/foss/llvm/13.0.1/x86_64/centos.7/bin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be removed as it is needed in the local environment


# Configure the rust 'clippy' linter.
build --aspects=@rules_rust//rust:defs.bzl%rust_clippy_aspect
build --output_groups=+clippy_checks
Expand Down
119 changes: 109 additions & 10 deletions hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ class mem_bkdr_util extends uvm_object;
protected uint32_t addr_lsb;
protected uint32_t addr_width;
protected uint32_t byte_addr_width;
protected uint32_t non_ecc_bits_per_subword;
protected uint32_t ecc_bits_per_subword;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we actually use this variable (except where it gets its value). Maybe it could be a local there?

protected uint32_t bits_per_subword;
protected uint32_t subwords_per_word;
protected uint32_t redundant_bits_per_entry;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a doc comment somewhere (maybe above the constructor) which explains what some of these numbers mean?

Also, is there a difference in meaning between "entry" and "word" here? If not, it would probably be clearer to standardise on one of them.

protected uint32_t interleave_bits_in_entry;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a yes/no flag, right? Can't it just be a bit?

protected uint32_t scramble_before_integ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, I think this can just be a bit.


// Address range of this memory in the system address map.
protected addr_range_t addr_range;
Expand All @@ -62,6 +69,8 @@ class mem_bkdr_util extends uvm_object;
// package.
function new(string name = "", string path, int unsigned depth,
longint unsigned n_bits, err_detection_e err_detection_scheme,
int scramble_before_integ = 0,
int redundant_bits_per_entry = 0, int interleave_bits_in_entry = 0,
int extra_bits_per_subword = 0, int unsigned system_base_addr = 0);

bit res;
Expand All @@ -72,7 +81,10 @@ class mem_bkdr_util extends uvm_object;

this.path = path;
this.depth = depth;
this.width = n_bits / depth;
this.interleave_bits_in_entry = interleave_bits_in_entry;
this.scramble_before_integ = scramble_before_integ;
this.redundant_bits_per_entry = redundant_bits_per_entry;
this.width = (n_bits / depth) - redundant_bits_per_entry;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've already got a check that depth divides n_bits. But could you add one to make sure that redundant_bits_per_entry - (n_bits / depth) is positive?

this.err_detection_scheme = err_detection_scheme;

if (`HAS_ECC) begin
Expand All @@ -81,11 +93,11 @@ class mem_bkdr_util extends uvm_object;
import prim_secded_pkg::get_ecc_parity_width;

prim_secded_e secded_eds = prim_secded_e'(err_detection_scheme);
int non_ecc_bits_per_subword = get_ecc_data_width(secded_eds);
int ecc_bits_per_subword = get_ecc_parity_width(secded_eds);
int bits_per_subword = non_ecc_bits_per_subword + ecc_bits_per_subword +
extra_bits_per_subword;
int subwords_per_word;
non_ecc_bits_per_subword = get_ecc_data_width(secded_eds);
ecc_bits_per_subword = get_ecc_parity_width(secded_eds);
bits_per_subword = non_ecc_bits_per_subword + ecc_bits_per_subword +
extra_bits_per_subword;
Comment on lines +98 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to read this twice to get the syntax tree! :-) Could you add some parens around the broken line?

//int subwords_per_word;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've moved this variable to a field in the class. Probably need to delete this line too.


// We shouldn't truncate the actual data word. This check ensures that err_detection_scheme
// and width are related sensibly. This only checks we've got enough space for one data word
Expand All @@ -107,7 +119,7 @@ class mem_bkdr_util extends uvm_object;

byte_width = `HAS_PARITY ? 9 : 8;
bytes_per_word = data_width / byte_width;
`DV_CHECK_LE_FATAL(bytes_per_word, 32, "data width > 32 bytes is not supported")
// `DV_CHECK_LE_FATAL(bytes_per_word, 32, "data width > 32 bytes is not supported")
Comment on lines -110 to +122
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented? (Is it no longer true? In which case, we should probably just delete it, but I'm not sure I understand the logic yet)

size_bytes = depth * bytes_per_word;
addr_lsb = $clog2(bytes_per_word);
addr_width = $clog2(depth);
Expand Down Expand Up @@ -212,10 +224,29 @@ class mem_bkdr_util extends uvm_object;
bit res;
uint32_t index;
uvm_hdl_data_t data;
uvm_hdl_data_t raw_data;

if (!check_addr_valid(addr)) return 'x;

index = addr >> addr_lsb;

res = uvm_hdl_read($sformatf("%0s[%0d]", path, index), data);
`DV_CHECK_EQ(res, 1, $sformatf("uvm_hdl_read failed at index %0d", index))


// Adjust the row data for interleaving. Was interleaved, beg back to true position
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what "beg" means here (that's probably not the verb you intend! :-D)

I'd probably also avoid the "row" name. I don't think that the code in mem_bkdr_util.sv is supposed to have opinions about that sort of geometry!

if (interleave_bits_in_entry) begin
raw_data = data;
for (int subword_idx = 0; subword_idx < subwords_per_word; subword_idx++) begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop is inverse with the one in write(), I think. (In fact, are they self-inverse so they are identical?)

I'd suggest moving the contents into a helper function. I think it would probably make things a bit easier to follow.

for (int b= 0; b < bits_per_subword; b++) begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Missing space before the '='

int true_col_pos;
int intrlv_col_pos;
intrlv_col_pos = (subword_idx + (((b * 4) + 1) * 4));
true_col_pos = (subword_idx * bits_per_subword) + b;
Comment on lines +242 to +245
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably suggest folding these lines together, so you end up with something like:

             int true_col_pos = (subword_idx + (((b * 4) + 1) * 4));
             int intrlv_col_pos = (subword_idx * bits_per_subword) + b;

data[true_col_pos] = raw_data[intrlv_col_pos];
end
end
end
return data;
endfunction

Expand Down Expand Up @@ -325,8 +356,26 @@ class mem_bkdr_util extends uvm_object;
virtual function void write(bit [bus_params_pkg::BUS_AW-1:0] addr, uvm_hdl_data_t data);
bit res;
uint32_t index;
uvm_hdl_data_t raw_data;
if (!check_addr_valid(addr)) return;
index = addr >> addr_lsb;

// Adjust the row data for interleaving
// Data passed in assumes subwords in contiguous locations
// Data to be written is converted to interleaved position
// (note the interleaving function may be different for differen memory structures
if (interleave_bits_in_entry) begin
raw_data = data;
for (int subword_idx = 0; subword_idx < subwords_per_word; subword_idx++) begin
for (int b= 0; b < bits_per_subword; b++) begin
int true_col_pos;
int intrlv_col_pos;
intrlv_col_pos = (subword_idx + (((b * 4) + 1) * 4));
true_col_pos = (subword_idx * bits_per_subword) + b;
data[intrlv_col_pos] = raw_data[true_col_pos];
end
end
end
res = uvm_hdl_deposit($sformatf("%0s[%0d]", path, index), data);
`DV_CHECK_EQ(res, 1, $sformatf("uvm_hdl_deposit failed at index %0d", index))
endfunction
Expand Down Expand Up @@ -379,9 +428,26 @@ class mem_bkdr_util extends uvm_object;

// this is used to write 32bit of data plus 7 raw integrity bits.
virtual function void write39integ(bit [bus_params_pkg::BUS_AW-1:0] addr, logic [38:0] data);
uvm_hdl_data_t row_data;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Double space.

uint32_t word_idx;
uint32_t byte_idx;
uint32_t subword_idx;

`_ACCESS_CHECKS(addr, 32) // this is essentially an aligned 32bit access.
if (!check_addr_valid(addr)) return;
write(addr, data);

// read-modify-write:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this needs a bit of thought. We're adding RMW functionality which wasn't there before. (Notice that there isn't any read() call in the old code of this function!)

Can you explain (possibly in a comment) why write39integ needs read-modify-write code and write doesn't? In the original implementation, this was essentially just a call to write.

// Each entry may have more than one subword chunk stored
// So we get current data row and insert the new subword at the right location,
// The modified data is then written back
word_idx = addr >> addr_lsb;
byte_idx = addr - (word_idx << addr_lsb);
subword_idx = byte_idx >> 2;
// Note the read function eliminates interleaving, if used, and returns data in the contiguous locations
row_data = read(addr);
row_data[subword_idx * 39 +:39]= data;
// Note the write function takes care of interleaving, if used.
write(addr, row_data);
endfunction

virtual function void write64(bit [bus_params_pkg::BUS_AW-1:0] addr, logic [63:0] data);
Expand Down Expand Up @@ -561,17 +627,50 @@ class mem_bkdr_util extends uvm_object;
`uvm_info(`gfn, "Randomizing mem contents", UVM_LOW)
for (int i = 0; i < depth; i++) begin
uvm_hdl_data_t data;
uvm_hdl_data_t raw_data;
`DV_CHECK_STD_RANDOMIZE_FATAL(data, "Randomization failed!", path)
raw_data = data;
`uvm_info(`gfn, $sformatf("Randomizing mem contents: \n Pre ECC Data is %h, \n Index: i: %d",
raw_data, i),
UVM_DEBUG)
if (`HAS_PARITY) begin
uvm_hdl_data_t raw_data = data;
for (int byte_idx = 0; byte_idx < bytes_per_word; byte_idx++) begin
bit raw_byte = raw_data[byte_idx * 8 +: 8];
bit parity = (err_detection_scheme == ParityOdd) ? ~(^raw_byte) : (^raw_byte);
data[byte_idx * 9 +: 9] = {parity, raw_byte};
end
end else begin
data = get_ecc_computed_data(data);
// There may be multiple subwords within each row/entry.
// Need to update the randomized data for each subword chunk in a Row
for (int subword_idx = 0; subword_idx < subwords_per_word; subword_idx++) begin
logic [31:0] raw_subword;
logic [38:0] raw_subword_with_ecc;

// Get the raw data subword chunk without ECC from the input data
for (int b= 0; b < non_ecc_bits_per_subword; b++)
raw_subword[b] = raw_data[b+(subword_idx * non_ecc_bits_per_subword)];

// Compute the ECC Code
raw_subword_with_ecc = get_ecc_computed_data(raw_subword);
`uvm_info(`gfn,
$sformatf("Randomizing mem contents: \n\
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [verible-verilog-lint] reported by reviewdog 🐶
659:46: The lines can't be continued with '', use concatenation operator with braces [Style: forbid-line-continuations] [forbid-line-continuations]

Pre ECC subword: %h \n\
Post ECC subword: %h \n\
Index: i: %d",
raw_subword, raw_subword_with_ecc, i),
UVM_DEBUG)
// Place the subword with ECC data at the right slot within the data to be written
for (int b= 0; b < bits_per_subword; b++) begin
int column_position;
column_position = ((subword_idx * bits_per_subword) + b);
data[column_position] = raw_subword_with_ecc[b];
end
end
end
`uvm_info(`gfn, $sformatf("Randomizing mem contents: \n\
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [verible-verilog-lint] reported by reviewdog 🐶
672:47: The lines can't be continued with '', use concatenation operator with braces [Style: forbid-line-continuations] [forbid-line-continuations]

Post ECC Data is %h,\
Index: i: %d",
data, i), UVM_DEBUG)
write(i * bytes_per_word, data);
end
endfunction
Expand Down
95 changes: 72 additions & 23 deletions hw/dv/sv/mem_bkdr_util/mem_bkdr_util__sram.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,36 @@ function logic [38:0] get_sram_encrypt32_intg_data (
addr_arr[i] = addr[addr_lsb + i];
end

// Calculate the integrity constant
integ_data = prim_secded_pkg::prim_secded_inv_39_32_enc(data);
if (scramble_before_integ) begin

// Calculate the scrambled data
wdata_arr[0:31] = {<<{data}};
wdata_arr[0:31] = sram_scrambler_pkg::encrypt_sram_data(
wdata_arr[0:31], 32, 32, addr_arr, full_addr_width, key_arr, nonce_arr
);
scrambled_data[31:0] = {<<{wdata_arr[0:31]}};

// Calculate the integrity constant
integ_data = prim_secded_pkg::prim_secded_inv_39_32_enc(scrambled_data[31:0]);

return integ_data;

end else begin

// Calculate the integrity constant
integ_data = prim_secded_pkg::prim_secded_inv_39_32_enc(data);

// Calculate the scrambled data
wdata_arr = {<<{integ_data}};
wdata_arr = sram_scrambler_pkg::encrypt_sram_data(
wdata_arr, 39, 39, addr_arr, full_addr_width, key_arr, nonce_arr
);
scrambled_data = {<<{wdata_arr}};

// Calculate the scrambled data
wdata_arr = {<<{integ_data}};
wdata_arr = sram_scrambler_pkg::encrypt_sram_data(
wdata_arr, 39, 39, addr_arr, full_addr_width, key_arr, nonce_arr
);
scrambled_data = {<<{wdata_arr}};

return scrambled_data;
return scrambled_data;

end

endfunction // get_sram_encrypt32_intg_data

Expand Down Expand Up @@ -129,22 +148,52 @@ virtual function void sram_encrypt_write32_integ(logic [bus_params_pkg::BUS_AW-1
addr_arr[i] = addr[addr_lsb + i];
end

// Calculate the scrambled address
scr_addr = get_sram_encrypt_addr(addr, nonce);

// Calculate the integrity constant
integ_data = prim_secded_pkg::prim_secded_inv_39_32_enc(data);
if (scramble_before_integ) begin
// Calculate the scrambled address
scr_addr = get_sram_encrypt_addr(addr, nonce);

// flip some bits to inject integrity fault
integ_data ^= flip_bits;
// Calculate the scrambled data
wdata_arr[0:31] = {<<{data}};
wdata_arr[0:31] = sram_scrambler_pkg::encrypt_sram_data(
wdata_arr[0:31], 32, 32, addr_arr, addr_width, key_arr, nonce_arr
);
scrambled_data[31:0] = {<<{wdata_arr[0:31]}};


// Calculate the integrity constant
integ_data = prim_secded_pkg::prim_secded_inv_39_32_enc(scrambled_data[31:0]);

// flip some bits to inject integrity fault
integ_data ^= flip_bits;


// Write the scrambled data to memory
write39integ(scr_addr, integ_data);

end else begin
// Calculate the scrambled address
scr_addr = get_sram_encrypt_addr(addr, nonce);


// Calculate the integrity constant
integ_data = prim_secded_pkg::prim_secded_inv_39_32_enc(data);

// flip some bits to inject integrity fault
integ_data ^= flip_bits;

// Calculate the scrambled data
wdata_arr = {<<{integ_data}};
wdata_arr = sram_scrambler_pkg::encrypt_sram_data(
wdata_arr, 39, 39, addr_arr, addr_width, key_arr, nonce_arr
);
scrambled_data = {<<{wdata_arr}};


// Write the scrambled data to memory
write39integ(scr_addr, scrambled_data);

end

// Calculate the scrambled data
wdata_arr = {<<{integ_data}};
wdata_arr = sram_scrambler_pkg::encrypt_sram_data(
wdata_arr, 39, 39, addr_arr, addr_width, key_arr, nonce_arr
);
scrambled_data = {<<{wdata_arr}};

// Write the scrambled data to memory
write39integ(scr_addr, scrambled_data);
endfunction