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.

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

[rom_ctrl,util] Rework memory collision check #24422

Conversation

rivos-eblot
Copy link
Contributor

As ROMs get bigger, scrambled memory collision checker in mem.py becomes slower.

On my local machine using this revised implementation, scrambling the second rom only takes 1.75 seconds vs. 7.68 seconds with the previous implementation (> 4x speed up).

I've tested collisions detections forcing several of them and checking the collision report remains the same:

diff --git a/hw/ip/rom_ctrl/util/scramble_image.py b/hw/ip/rom_ctrl/util/scramble_image.py
index c3e1cd0d63..6d70c2144e 100755
--- a/hw/ip/rom_ctrl/util/scramble_image.py
+++ b/hw/ip/rom_ctrl/util/scramble_image.py
@@ -493,6 +493,8 @@ def main() -> int:
         scr_mem.add_ecc32()
         assert scr_mem.width == 39

+    scr_mem.chunks[0].words[4] = scr_mem.chunks[0].words[78] = scr_mem.chunks[0].words[453]
+    scr_mem.chunks[0].words[12] = scr_mem.chunks[0].words[1000]
     # Check for collisions
     collisions = scr_mem.collisions()
     if collisions:

@rivos-eblot rivos-eblot force-pushed the dev/ebl/optim_mem_collision_check branch from 0499ba9 to a68eac4 Compare August 28, 2024 10:30
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

This looks sensible to me (and I clearly did something a little silly with the original implementation). But I think we can be lazier: we don't actually care all that much about the collisions, only about whether they exist. So a better approach would probably be something like this: (completely untested!)

    def first_collision(self) -> Optional[Tuple[int, int]]:
        '''Return the address of the first pair of colliding addresses

        If there is no such pair (which is hopefully the case), return None.
        '''
        known = {}
        for idx, chunk in enumerate(self.chunks):
            for word_idx, word in enumerate(chunk.words):
                addr = chunk.base_addr + word_idx
                if word in known:
                    return (known[word], addr)
                else:
                    known[word] = addr
        return None

Then the call site would look something like this:

    collision = scr_mem.first_collision()
    if collision is not None:
        print(
            'ERROR: This combination of ROM contents and scrambling\n'
            '       key results in one or more collisions where\n'
            '       different addresses have the same data.\n'
            '\n'
            '       Looks like we\'ve been (very) unlucky with the\n'
            '       birthday problem. As a work-around, try again after\n'
            '       generating some different RndCnst* parameters.\n',
            file=sys.stderr)
        addr0, addr1 = collision
        print(f'First colliding addresses: {addr0:#010x}, {addr1:#010x}')
        return 1

@rivos-eblot
Copy link
Contributor Author

Sure. I wrote the patch to get the exact same output as the original implementation. If only the first collision is of some interest, I can simplify this quite a bit. Pls let me know.

@rivos-eblot rivos-eblot force-pushed the dev/ebl/optim_mem_collision_check branch from a68eac4 to 5edaff0 Compare September 2, 2024 14:45
Update the collision check implementation using a dict to traverse the
chunks only once.

Signed-off-by: Emmanuel Blot <[email protected]>
@rivos-eblot rivos-eblot force-pushed the dev/ebl/optim_mem_collision_check branch from 5edaff0 to 703b425 Compare September 9, 2024 15:31
@rivos-eblot
Copy link
Contributor Author

Hi @rswarbrick. Is this new version ok for you? It seems the CI fails with some unrelated issues to this change though.

Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

I'm really sorry for not replying before. Yes, this looks really good to me!

@rivos-eblot
Copy link
Contributor Author

cc @sameo

@rswarbrick rswarbrick merged commit 2585f4a into lowRISC:integrated_dev Sep 16, 2024
19 of 22 checks passed
@rivos-eblot
Copy link
Contributor Author

Thanks

@rivos-eblot rivos-eblot deleted the dev/ebl/optim_mem_collision_check branch September 16, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants