Add GuestMemory method to return an Iterator #9

jiangliu · 2019-03-31T14:15:43Z

Fix #8

alexandruag

Looks good to me! I have some comments on error handling for GuestMemory in general, but I'll put those in the comment PR/RFC I'm still trying to write :(

src/guest_memory.rs

bonzini

Is this needed, or is it just a cleanup? Even if it is only used by mmap-related code, it can be useful in general.

bonzini

Can you also look into removing map_and_fold(), and replacing it with iter()?

jiangliu · 2019-04-03T01:24:23Z

Just a cleanup, so I will remove it from the patch set.

…

On Apr 3, 2019, at 1:29 AM, Paolo Bonzini ***@***.*** ***@***.***>> wrote: @bonzini commented on this pull request. Is this needed, or is it just a cleanup? Even if it is only used by mmap-related code, it can be useful in general. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB14_Jy-Vg5E8PLk5T9wDRz_6dJVRl8Wks5vc5OXgaJpZM4cUVYs>.

bonzini · 2019-04-03T16:57:55Z

Nice! Can you add some comments explaining the reason for the PhantomData?

Signed-off-by: Liu Jiang <[email protected]>

bonzini · 2019-04-04T20:27:18Z

Hey people, I placed my initial attempt at converting firecracker at https://github.com/bonzini/firecracker branch master.

A more sensible way to do it would be to split like:

Rename memory-model classes to the vm-memory equivalents
Rename memory-model methods to the vm-memory (especially Bytes) equivalents
Add "type GuestUsize = u32;" to memory-model, fix compilation
Finally switch to vm-memory and add the new traits uses.

The vm-memory is not properly vendored, it's hacked to add the mmap feature to Cargo.toml. And so on... still if it saves you a couple hours, all for the best.

andreeaflorescu · 2019-04-05T05:59:51Z

Hey @bonzini, thanks. I will open an issue in the Firecracker repository to switch to vm-memory and link the commit from your fork.

We will wait for the vm-memory to be published to crates.io before using it. Before doing the switch we want to make sure we have good code coverage and the necessary mechanisms in place to ensure that future PRs won't decrease it. I will open issues in vm-memory for the other things that I'll find.

Add an iterator interface to the GuestMemory trait, so the with_regions() and with_regions_mut() methods could be removed later. Signed-off-by: Liu Jiang <[email protected]>

Now we have iterator for the GuestMemory trait, so remove with_regions(), with_regions_mut() and map_and_fold(). Signed-off-by: Liu Jiang <[email protected]>

jiangliu · 2019-04-10T16:17:35Z

I have tried to migrate the vhost crate in firecracker to the vm-memory crate, it works with one more patch upon this PR.

andreeaflorescu · 2019-04-11T08:32:44Z

@jiangliu we are currently trying to move away from vhost. See this RFC with the design doc: firecracker-microvm/firecracker#1044

jiangliu · 2019-04-12T06:51:34Z

Thanks @andreeaflorescu , happy to see the progress on support of vsock. I will keep current firecracker's vsock implementation as an example:)

Some guest memory consumers, such as in-kernel vhost driver, boot loader etc, need to access the guest memory by pointers. So add method get_host_address() to the GuestMemory and GuestMemoryRegion traits, otherwise those guest memory consumers will explicitly depend on the GuestMemoryMmap struct. Signed-off-by: Liu Jiang <[email protected]>

Enhance the GuestMemory trait to support Clone, so guest memory consumers could hold a local guest memory object by calling guest_memory.clone(). Signed-off-by: Liu Jiang <[email protected]>

andreeaflorescu · 2019-07-26T09:55:42Z

@jiangliu what is the status here? Do we still want to merge this one?

bonzini · 2019-07-26T10:49:18Z

src/mmap.rs

 }

 impl GuestMemoryRegion for GuestRegionMmap {
+    const DIRECT_ACCESS: bool = true;


Why is DIRECT_ACCESS useful?

bonzini · 2019-07-26T10:53:46Z

src/guest_memory.rs

+    /// Convert an absolute address in an address space (GuestMemory) to a host pointer,
+    /// or return None if it is out of bounds. It always returns None when `DIRECT_ACCESS` is false.
+    /// The returned pointer may be used for ioctls etc.
+    fn get_host_address(&self, addr: MemoryRegionAddress) -> Option<*mut u8>;


This can be implemented in terms of as_mut_slice(). Also please make the result Option<NonNull<u8>>.

jiangliu · 2019-07-29T15:04:37Z

@jiangliu what is the status here? Do we still want to merge this one?

Hi Paolo,
The rust GAT(Generic Associated Type) feature is still unstable. Merging this patch will force all clients to carry on an extra lifetime parameter, which is a little heavy. So suggest to wait until that the GAT feature gets stable.

bonzini · 2019-07-29T22:23:12Z

Agreed.

jiangliu force-pushed the v4 branch from b5d3fb3 to d3cf4ab Compare March 31, 2019 14:19

jiangliu requested review from alexandruag and bonzini March 31, 2019 14:22

jiangliu force-pushed the v4 branch from d3cf4ab to 7abc455 Compare March 31, 2019 15:39

alexandruag previously approved these changes Apr 2, 2019

View reviewed changes

src/guest_memory.rs Outdated Show resolved Hide resolved

bonzini reviewed Apr 2, 2019

View reviewed changes

bonzini approved these changes Apr 2, 2019

View reviewed changes

bonzini requested changes Apr 2, 2019

View reviewed changes

Refine test cases and comments

5f54bf4

Signed-off-by: Liu Jiang <[email protected]>

jiangliu dismissed alexandruag’s stale review via e539af1 April 4, 2019 10:36

jiangliu force-pushed the v4 branch from 7abc455 to e539af1 Compare April 4, 2019 10:36

andreeaflorescu mentioned this pull request Apr 5, 2019

Replace memory-model with vm-memory from rust-vmm firecracker-microvm/firecracker#1047

Closed

jiangliu added 2 commits April 5, 2019 22:33

Add an iterator interface to the GuestMemory trait

f576ead

Add an iterator interface to the GuestMemory trait, so the with_regions() and with_regions_mut() methods could be removed later. Signed-off-by: Liu Jiang <[email protected]>

Use iterator to replace GuestMemory's redundant methods

ab37656

Now we have iterator for the GuestMemory trait, so remove with_regions(), with_regions_mut() and map_and_fold(). Signed-off-by: Liu Jiang <[email protected]>

jiangliu requested a review from bonzini April 11, 2019 08:26

jiangliu force-pushed the v4 branch from e539af1 to bb2d789 Compare April 22, 2019 16:03

jiangliu added 2 commits May 7, 2019 13:32

Enhance the GuestMemory trait to support Clone

ca719de

Enhance the GuestMemory trait to support Clone, so guest memory consumers could hold a local guest memory object by calling guest_memory.clone(). Signed-off-by: Liu Jiang <[email protected]>

jiangliu force-pushed the v4 branch from bb2d789 to ca719de Compare May 7, 2019 05:34

bonzini requested changes Jul 26, 2019

View reviewed changes

andreeaflorescu mentioned this pull request Aug 29, 2019

Adopt the vm-memory crate from rust-vmm firecracker-microvm/firecracker#1137

Closed

jiangliu closed this Oct 26, 2019

Daniel-Aaron-Bloom mentioned this pull request Dec 29, 2020

Add GuestMemory method to return an Iterator #130

Merged

Add GuestMemory method to return an Iterator #9

Add GuestMemory method to return an Iterator #9

Uh oh!

Conversation

jiangliu commented Mar 31, 2019

Uh oh!

alexandruag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bonzini left a comment

Choose a reason for hiding this comment

Uh oh!

bonzini left a comment

Choose a reason for hiding this comment

Uh oh!

jiangliu commented Apr 3, 2019 via email

Uh oh!

bonzini commented Apr 3, 2019

Uh oh!

bonzini commented Apr 4, 2019

Uh oh!

andreeaflorescu commented Apr 5, 2019

Uh oh!

jiangliu commented Apr 10, 2019

Uh oh!

andreeaflorescu commented Apr 11, 2019

Uh oh!

jiangliu commented Apr 12, 2019

Uh oh!

andreeaflorescu commented Jul 26, 2019

Uh oh!

bonzini Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

bonzini Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

jiangliu commented Jul 29, 2019

Uh oh!

bonzini commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants