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

Jump to bottom

[rtl] MuBi encoding of iCache memory ctrl signals #23292

Merged
merged 1 commit into from
May 26, 2024
Merged

[rtl] MuBi encoding of iCache memory ctrl signals #23292

merged 1 commit into from
May 26, 2024

Conversation

nasahlpa
Copy link
Member

This commit encodes all flopped 1-bit control signals into multi bit signals in the prim_ram_1p_scr and prim_ram_1p_adv module. When an encoding error is detected, a major alert is triggered within Ibex.

@nasahlpa nasahlpa requested a review from a team as a code owner May 23, 2024 17:44
@nasahlpa nasahlpa requested review from marnovandermaas, GregAC, moidx, vogelpi, cdgori, msfschaffner and johannheyszl and removed request for a team and marnovandermaas May 23, 2024 17:44
@nasahlpa nasahlpa force-pushed the rammubienc branch 2 times, most recently from 8f01369 to 19c9c46 Compare May 23, 2024 18:24
vogelpi
vogelpi approved these changes May 24, 2024
View reviewed changes
Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

Thanks for porting this over @nasahlpa . This looks mostly good, I've spotted one case where the MuBi protection can be very easily extended from one set of registers to the next. I would be nice to get this in, too.

hw/ip/prim/rtl/prim_ram_1p_adv.sv Outdated
prim_buf #(
.Width(MuBi4Width)
) u_req_d_buf (
.in_i ((req_d)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to remove the double parens? ((req_d)) -> (req_d)?

hw/ip/prim/rtl/prim_ram_1p_adv.sv Outdated
prim_buf #(
.Width(MuBi4Width)
) u_write_d_buf (
.in_i ((write_d)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to remove the double parens? ((write_d)) -> (write_d)?

hw/ip/prim/rtl/prim_ram_1p_scr.sv Outdated
prim_buf #(
.Width(MuBi4Width)
) u_read_en_buf (
.in_i ((read_en)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to remove the double parens? ((read_en)) -> (read_en)?

hw/ip/prim/rtl/prim_ram_1p_scr.sv Outdated
prim_buf #(
.Width(MuBi4Width)
) u_write_en_d_buf (
.in_i ((write_en_d)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to remove the double parens? ((write_en_d)) -> (write_en_d)?

hw/ip/prim/rtl/prim_ram_1p_scr.sv Outdated
Comment on lines 156 to 158
assign addr_collision_d = mubi4_bool_to_mubi(mubi4_test_true_loose(
mubi4_and_hi(mubi4_or_hi(write_en_q, write_pending_q), read_en_buf)) &
(addr_scr == waddr_scr_q));
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be converted as follows:

  1. Create a mubi signal holding addr_scr == waddr_scr_q and buffer it with a prim_buf.
  2. addr_collision_d = mubi4_and_hi(mubi4_and_hi(mubi4_or_hi(write_en_q, write_pending_q), read_en_buf), addr_match_buf);

Copy link
Contributor

Choose a reason for hiding this comment

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

Because right now, you have several mubi inputs and a mubi target register. But just before driving the register you convert to boolean and then again mubi.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I've changed the code accordingly.

@nasahlpa nasahlpa force-pushed the rammubienc branch 2 times, most recently from 4dc644d to 45e163e Compare May 25, 2024 12:54
@vogelpi
Copy link
Contributor

vogelpi commented May 25, 2024

Thanks @nasahlpa , this looks great now!

@vogelpi
Copy link
Contributor

vogelpi commented May 25, 2024

The bitstream splicing seems to not work currently in CI due to an infrastructure issue. Thus all FPGA tests fail at the moment. I think we people are working on fixing this.

@nasahlpa nasahlpa force-pushed the rammubienc branch 2 times, most recently from db3d587 to 1bbd6bc Compare May 26, 2024 15:43
@nasahlpa
[rtl] MuBi encoding of iCache memory ctrl signals
ed8b205 
This commit encodes all flopped 1-bit control signals into multi bit
signals in the `prim_ram_1p_scr` and `prim_ram_1p_adv` module. When
an encoding error is detected, a major alert is triggered within Ibex.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
@nasahlpa
Copy link
Member Author

Thanks for the review!

After a rebase CI now passed.

@andreaskurth andreaskurth merged commit 856e402 into lowRISC:master May 26, 2024
32 checks passed
@nasahlpa nasahlpa deleted the rammubienc branch May 26, 2024 20:20
nasahlpa added a commit to nasahlpa/ibex that referenced this pull request May 28, 2024
@nasahlpa
[rtl] Add error port to iCache
50322d4 
This commit adds the error port to the iCache which was introduced
with lowRISC/opentitan#23292.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
@nasahlpa nasahlpa mentioned this pull request May 28, 2024
Merged
This was referenced Jun 3, 2024
Closed
Closed
nasahlpa added a commit to nasahlpa/ibex that referenced this pull request Jun 4, 2024
@nasahlpa
[rtl] Add error port to iCache
6ad2233 
This commit adds the error port to the iCache which was introduced
with lowRISC/opentitan#23292.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
GregAC pushed a commit to lowRISC/ibex that referenced this pull request Jun 6, 2024
@nasahlpa @GregAC
[rtl] Add error port to iCache
5cea5d6 
This commit adds the error port to the iCache which was introduced
with lowRISC/opentitan#23292.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
nasahlpa added a commit to nasahlpa/opentitan that referenced this pull request Dec 6, 2024
@nasahlpa
[sram_ctrl,dv] Add mubi FI test for prim_sram
9f0ae29 
lowRISC#23292 protected security-critical signals
within the prim_ram modules against FI by multi-bit encoding.
This commit adds a FI test that checks whether injecting
bit-flips into these signals trigger the expected alert.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
@nasahlpa nasahlpa mentioned this pull request Dec 6, 2024
Merged
nasahlpa added a commit to nasahlpa/opentitan that referenced this pull request Dec 6, 2024
@nasahlpa
[sram_ctrl,dv] Add mubi FI test for prim_ram
aebcf7f 
lowRISC#23292 protected security-critical signals
within the prim_ram modules against FI by multi-bit encoding.
This commit adds a FI test that checks whether injecting
bit-flips into these signals trigger the expected alert.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
nasahlpa added a commit to nasahlpa/opentitan that referenced this pull request Dec 6, 2024
@nasahlpa
[sram_ctrl,dv] Add mubi FI test for prim_ram
293f0dc 
lowRISC#23292 protected security-critical signals
within the prim_ram modules against FI by multi-bit encoding.
This commit adds a FI test that checks whether injecting
bit-flips into these signals trigger the expected alert.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
nasahlpa added a commit to nasahlpa/opentitan that referenced this pull request Dec 6, 2024
@nasahlpa
[sram_ctrl,dv] Add mubi FI test for prim_ram
555c144 
lowRISC#23292 protected security-critical signals
within the prim_ram modules against FI by multi-bit encoding.
This commit adds a FI test that checks whether injecting
bit-flips into these signals trigger the expected alert.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
nasahlpa added a commit that referenced this pull request Dec 9, 2024
@nasahlpa
[sram_ctrl,dv] Add mubi FI test for prim_ram
59d9299 
#23292 protected security-critical signals
within the prim_ram modules against FI by multi-bit encoding.
This commit adds a FI test that checks whether injecting
bit-flips into these signals trigger the expected alert.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
Razer6 pushed a commit to Razer6/opentitan that referenced this pull request Dec 10, 2024
@nasahlpa @Razer6
[sram_ctrl,dv] Add mubi FI test for prim_ram
cbf72c2 
lowRISC#23292 protected security-critical signals
within the prim_ram modules against FI by multi-bit encoding.
This commit adds a FI test that checks whether injecting
bit-flips into these signals trigger the expected alert.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
Razer6 pushed a commit to Razer6/opentitan that referenced this pull request Dec 12, 2024
@nasahlpa @Razer6
[sram_ctrl,dv] Add mubi FI test for prim_ram
5863a44 
lowRISC#23292 protected security-critical signals
within the prim_ram modules against FI by multi-bit encoding.
This commit adds a FI test that checks whether injecting
bit-flips into these signals trigger the expected alert.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
Razer6 pushed a commit to Razer6/opentitan that referenced this pull request Jan 2, 2025
@nasahlpa @Razer6
[sram_ctrl,dv] Add mubi FI test for prim_ram
9be2130 
lowRISC#23292 protected security-critical signals
within the prim_ram modules against FI by multi-bit encoding.
This commit adds a FI test that checks whether injecting
bit-flips into these signals trigger the expected alert.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
nasahlpa added a commit to nasahlpa/opentitan that referenced this pull request Feb 19, 2025
@nasahlpa
[sram_ctrl,cm] Add CM label to prim_ram
b3e02a9 
PR lowRISC#23292 introduced MuBi encoding for the
prim_ram_1p_scr module. However, adding the corresponding
countermeasure label PRIM_RAM.CTRL.MUBI was forgotten.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
@nasahlpa nasahlpa mentioned this pull request Feb 19, 2025
Merged
vogelpi pushed a commit that referenced this pull request Feb 20, 2025
@nasahlpa @vogelpi
[sram_ctrl,cm] Add CM label to prim_ram
6e1a8b6 
PR #23292 introduced MuBi encoding for the
prim_ram_1p_scr module. However, adding the corresponding
countermeasure label PRIM_RAM.CTRL.MUBI was forgotten.

Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vogelpi vogelpi vogelpi approved these changes

@GregAC GregAC Awaiting requested review from GregAC

@moidx moidx Awaiting requested review from moidx

@cdgori cdgori Awaiting requested review from cdgori

@msfschaffner msfschaffner Awaiting requested review from msfschaffner

@johannheyszl johannheyszl Awaiting requested review from johannheyszl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nasahlpa @vogelpi @andreaskurth