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

volatile_memory: add VolatileArrayRef and &std::sync::atomic::Atomic* accessors #19

Merged
merged 4 commits into from
Jun 11, 2019

Conversation

bonzini
Copy link
Member

@bonzini bonzini commented May 16, 2019

Allow easily access to an array stored in a VolatileMemory object, removing
the need to do multiplications and pointer offsetting.

Copy link
Collaborator

@alexandruag alexandruag left a comment

Choose a reason for hiding this comment

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

Hi Paolo,

I was just wondering, is there any fundamental reason why we should have both VolatileSlice and VolatileArrayRef? Right now, it seems VolatileSlice basically corresponds to a byte slice, whereas VolatileArrayRef behaves like a slice of some type T. Does VolatileArrayRef still make sense if we parametrize VolatileSlice<T>? We can implement the common functionality in VolatileSlice<T>, and move all methods that only make sense for a byte slice in an impl VolatileSlice<u8> { ... } or something along those lines.

@bonzini
Copy link
Member Author

bonzini commented May 26, 2019

That's an interesting idea. I will take a look.

@bonzini bonzini force-pushed the array-ref branch 2 times, most recently from 748191a to aadfb54 Compare May 28, 2019 20:13
@bonzini
Copy link
Member Author

bonzini commented May 28, 2019

The main obstacle here is that VolatileSlice has some extra methods to copy in and out of the slice, compared to VolatileArrayRef, and I don't think they make sense for VolatileArrayRef (better: they do make sense, but with a different type bound, they can be T: ByteValued for VolatileSlice but they should not be generic for VolatileArrayRef. However, I've added a From implementation to go from VolatileArrayRef<u8> to VolatileSlice, and a bunch of other improvements.

@bonzini bonzini changed the title volatile_memory: add VolatileArrayRef volatile_memory: add VolatileArrayRef and &std::sync::atomic::Atomic* accessors May 28, 2019
@bonzini bonzini force-pushed the array-ref branch 5 times, most recently from 9dd2e06 to b1d3736 Compare May 30, 2019 19:54
src/volatile_memory.rs Show resolved Hide resolved
src/volatile_memory.rs Show resolved Hide resolved
src/volatile_memory.rs Show resolved Hide resolved
src/volatile_memory.rs Show resolved Hide resolved
src/volatile_memory.rs Show resolved Hide resolved
src/volatile_memory.rs Show resolved Hide resolved
A VolatileRef is essentially a pointer.  It can be copied or cloned as the
lifetime of the copies will be the same.

Clippy says that, now that VolatileRef is Copy, we should pass it by value
because it is small enough to be more efficient to always do so; obey.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Allow easily access to an array stored in a VolatileMemory object, removing
the need to do multiplications and pointer offsetting.  It also generalizes
the existing methods copy_to and copy_from in VolatileSlice.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This allows writing safely drivers that access memory concurrently
with the guest and that require memory barriers between DMA
accesses.

I'm adding a feature for AtomicU* types because they were only
stabilized in Rust 1.34.0.

Fixes: rust-vmm#16
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
src/volatile_memory.rs Show resolved Hide resolved
src/volatile_memory.rs Show resolved Hide resolved
src/volatile_memory.rs Show resolved Hide resolved
src/volatile_memory.rs Show resolved Hide resolved
@bonzini bonzini merged commit ffe4c9b into rust-vmm:master Jun 11, 2019
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.

None yet

3 participants