Skip to content

Use of syx_snapshot_dirty_list_add_hostaddr in cputlb.c #105

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

Open
enisrat opened this issue Mar 7, 2025 · 5 comments
Open

Use of syx_snapshot_dirty_list_add_hostaddr in cputlb.c #105

enisrat opened this issue Mar 7, 2025 · 5 comments

Comments

@enisrat
Copy link

enisrat commented Mar 7, 2025

I noticed that this access type check in cputlb.c has been commented out:

// TODO: Does not work?
// if (type == MMU_DATA_STORE) {
syx_snapshot_dirty_list_add_hostaddr(l->page[0].haddr);
// }

I don't see why this check would not work?

Also, pages with flags like TLB_MMIO and TLB_DISCARD_WRITE would not need to be considered for snapshot. Would it not make sense to check those as well?

@rmalmain
Copy link
Member

it's true. i remember doing tests with complex targets and getting snapshotting errors if not commenting this out.
ideally, we should check for those flags. i can try to re-run a linux target and see if i still get those issues.

@enisrat
Copy link
Author

enisrat commented Mar 19, 2025

Ok, would be great to be able to reproduce this with your command line.
I am on a debug branch with some additional HMP commands in monitor, trying to verify everything for my custom board.

@enisrat
Copy link
Author

enisrat commented Apr 5, 2025

@rmalmain : Did you get some errors I could reproduce?

@enisrat
Copy link
Author

enisrat commented Apr 5, 2025

Ok, found a fix (I think):

  • Use memory_region_reset_dirty on each RAM region in syx_snapshot_new and syx_snapshot_root_restore
  • This marks all RAM as notdirty, so a WRITE will not be missed
  • Additionally CPUTLB should be flushed completely in syx_snapshot_new (it is implicitely flushed in syx_snapshot_root_restore)

@enisrat
Copy link
Author

enisrat commented Apr 8, 2025

I created a PR for this (and other features):
#112

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

No branches or pull requests

2 participants