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

Rust MMIO #1499

Merged
merged 2 commits into from
Nov 24, 2021
Merged

Rust MMIO #1499

merged 2 commits into from
Nov 24, 2021

Conversation

Kritzefitz
Copy link
Contributor

@Kritzefitz Kritzefitz commented Nov 24, 2021

I noticed that the rust bindings apparently don't have a way to map MMIO regions at the moment. This PR adds map_mmio to provide this functionality.

Some things that I'm unhappy with, but don't see a way to make them better at the moment (feedback very welcome):

  • The logic in mmio_map to take a pointer to the user data and then move the Boxes of the user data into self.inner_mut().mmio_callbacks only works, because the pointer isn't checked by regular borrowing rules. This is basically the same method as used for hook callbacks and AIUI the Box guarantees that the contents of the box will always stay in the same location, so this should be safe. But I would be slightly more satisfied with a solution that would also work under normal borrowing rules.
  • The logic in MmioCallbackScope.unmap is kinda complicated and easy to get wrong. I'm not really confident that I didn't make any off-by-one errors. Unfortunately I'm also not really sure how to properly write automated tests for this, because the test suite doesn't have access to Unicorn.inner() to check that deallocation is actually working as intended.

Unicorn<D>.mmio_unmap could probably be made more efficient by keeping the MmioCallbackScopes sorted and using some sort of binary search to find regions to unmap. However, I wanted to keep this implementation simple for the moment. The introduced logic is already complicated as-is and I would imagine that mem_unmap isn't very performance critical for most use cases. But if you think a more performant implementation is needed, I can take a look at it again.

@Kritzefitz Kritzefitz changed the title Rust mmio Rust MMIO Nov 24, 2021
Kritzefitz and others added 2 commits November 24, 2021 12:15
Previously the user data of MMIO callbacks would live until the end of
the containing Unicorn engine. Now they are deallocated once all
memory referencing those callbacks has been unmapped.
@wtdcode
Copy link
Member

wtdcode commented Nov 24, 2021

Why not take a borrow instead of the ownership of the callbacks?

Sorry I’m not a rust expert and my question may be naive.

@Kritzefitz
Copy link
Contributor Author

Mostly I tried to be consistent with the existing API for hooks where the closures are also moved instead of borrowed. I'm not very versed in rust either, so I'm not completely sure what the reason for that is. But I think it's something like this:

If we want to borrow the closures for use in the C code, we have to borrow it with a lifetime that is valid for as long as the C code might use it. The C code might use the MMIO callbacks as long as any memory is mapped that references them. I don't think this last constraint can be expressed by rust's borrowing rules. Lifetimes are always associated with scopes, but the information if some memory is still mapped does not correspond to any scope. The closest fit we could use, is to borrow the closures with the same lifetime as the Unicorn itself. But this would imply that the closures can't be freed as long as the Unicorn is still alive and would also be inconvenient to use.

By taking ownership of the closures, we can be more concise. We handle the deallocation of the closures explicitly, which is internally unsafe (because it would violate the borrowing rules if we used borrowing instead of a pointer), but we provide a safe interface over it that ensures that the closures live as long as the C code might use them, but is still able to free them as soon as they are not needed anymore.

@wtdcode wtdcode merged commit 558fb9c into unicorn-engine:dev Nov 24, 2021
@wtdcode
Copy link
Member

wtdcode commented Nov 24, 2021

Mostly I tried to be consistent with the existing API for hooks where the closures are also moved instead of borrowed. I'm not very versed in rust either, so I'm not completely sure what the reason for that is. But I think it's something like this:

If we want to borrow the closures for use in the C code, we have to borrow it with a lifetime that is valid for as long as the C code might use it. The C code might use the MMIO callbacks as long as any memory is mapped that references them. I don't think this last constraint can be expressed by rust's borrowing rules. Lifetimes are always associated with scopes, but the information if some memory is still mapped does not correspond to any scope. The closest fit we could use, is to borrow the closures with the same lifetime as the Unicorn itself. But this would imply that the closures can't be freed as long as the Unicorn is still alive and would also be inconvenient to use.

By taking ownership of the closures, we can be more concise. We handle the deallocation of the closures explicitly, which is internally unsafe (because it would violate the borrowing rules if we used borrowing instead of a pointer), but we provide a safe interface over it that ensures that the closures live as long as the C code might use them, but is still able to free them as soon as they are not needed anymore.

Nice explanation. Keeping consistent with exiting API makes sense to me. Taking ownership is also more flexible later.

@Kritzefitz Kritzefitz deleted the rust-mmio branch November 26, 2021 12:06
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