Add IO manager support #12

liujing2 · 2019-10-31T13:32:55Z

Let's firstly add IO manager which focuses on device Port IO and MMIO management, and static IO mapping in runtime as the first step.
@sameo @andreeaflorescu @jiangliu PTAL, thanks.

sameo · 2019-11-01T07:26:45Z

I'm marking that one as WIP for now.

src/device_manager.rs

src/lib.rs

src/device_manager.rs

src/lib.rs

liujing2 · 2019-12-05T05:20:35Z

Could @sameo @andreeaflorescu @jiangliu take a look if there are any more comments? Thanks.

sboeuf

Just a few nits, but the PR looks good to me.

src/device_manager.rs

sboeuf · 2019-12-12T12:17:25Z

@liujing2 you need to fix the CI by replacing nth_back() function with something that is not considered unstable.

liujing2 · 2019-12-13T03:15:22Z

@liujing2 you need to fix the CI by replacing nth_back() function with something that is not considered unstable.

Thanks very much for the review.
I noticed the rust issue number directed by our CI result, saying that nth_back() is stabilized in 1.37.0, see
rust-lang/rust#56995 (comment)
That's why I locally have no error. Do we need to update here? @sameo @andreeaflorescu @jiangliu
Otherwise, we need use some other complex handling here.

sboeuf · 2019-12-13T08:03:31Z

@liujing2 ah ok then I agree we should bump the CI to the latest Rust version.

sameo · 2019-12-17T08:53:24Z

@liujing2 Could you please force push to check if CI passes with the new container?

sameo

Overall, the PR looks good.
I would appreciate if we could separate the IO ops internal mutability changes into a separate commit, and also remove the vm-memory dependency (and split the addr argument into base, offset) from vm-device prior to merging that PR.

src/device_manager.rs

src/lib.rs

src/device_manager.rs

src/lib.rs

liujing2 · 2020-01-09T03:12:39Z

Overall, the PR looks good.
I would appreciate if we could separate the IO ops internal mutability changes into a separate commit, and also remove the vm-memory dependency (and split the addr argument into base, offset) from vm-device prior to merging that PR.

Sure, we can separate the PR into three.

remove vm-memory dependency; find a way for the DeviceIO read/write parameters.
Io ops internal mutability changing.
IoManager related.

For the 1st, the difference between passing base offset and passing addr will be only for non-pci device instance, I think. These device instance can use offset directly to handle r/w emulation. But for those pci device instances, especially with several windows, they still need calculate addr(base+offset) and compare with each BAR Base Address to find the right one, right?

jiangliu · 2020-01-09T04:07:32Z

Overall, the PR looks good.
I would appreciate if we could separate the IO ops internal mutability changes into a separate commit, and also remove the vm-memory dependency (and split the addr argument into base, offset) from vm-device prior to merging that PR.

Sure, we can separate the PR into three.

remove vm-memory dependency; find a way for the DeviceIO read/write parameters.

Io ops internal mutability changing.

IoManager related.

For the 1st, the difference between passing base offset and passing addr will be only for non-pci device instance, I think. These device instance can use offset directly to handle r/w emulation. But for those pci device instances, especially with several windows, they still need calculate addr(base+offset) and compare with each BAR Base Address to find the right one, right?

If 'base + offset' is used, the driver may need to lookup some internal data structures by using base as the key. And token actually caches information about the internal data structures thus avoid the looking up operations.

sameo · 2020-01-10T17:55:35Z

If 'base + offset' is used, the driver may need to lookup some internal data structures by using base as the key. And token actually caches information about the internal data structures thus avoid the looking up operations.

Yes, that's right that the device may do some lookup, but this could easily be a O(1) lookup for those devices that actually need to look it up (PCI devices, yes). This seems to me like a micro optimization at that point, and the cost for it is a bound between the IO trait and the IO manager. In other words, callers of the IO trait methods must be stateful by keeping track of a cookie/token. Using this kind of API builds an implicit dependency between the trait and a registration method. To me, that's not the cleanest design, and the benefit we'd get for it is arguable (an O(1) lookup).

jiangliu · 2020-01-13T07:21:32Z

If 'base + offset' is used, the driver may need to lookup some internal data structures by using base as the key. And token actually caches information about the internal data structures thus avoid the looking up operations.

Yes, that's right that the device may do some lookup, but this could easily be a O(1) lookup for those devices that actually need to look it up (PCI devices, yes). This seems to me like a micro optimization at that point, and the cost for it is a bound between the IO trait and the IO manager. In other words, callers of the IO trait methods must be stateful by keeping track of a cookie/token. Using this kind of API builds an implicit dependency between the trait and a registration method. To me, that's not the cleanest design, and the benefit we'd get for it is arguable (an O(1) lookup).

Ok, let's use (base, offset) tuple, otherwise we are just blocking by small technical points:)
The vm-device is the foundation of the device model, and we have stocked here for a long time.

alexandruag · 2020-01-13T07:45:21Z

src/lib.rs

 /// Device IO trait.
 /// A device supporting memory based I/O should implement this trait, then
 /// register itself against the different IO type ranges it handles.
 /// The VMM will then dispatch IO (PIO or MMIO) VM exits by calling into the
 /// registered devices read or write method from this trait.
+/// The DeviceIo trait adopts the interior mutability pattern


Having interior mutability here is a thorny issue and needs a separate discussion :( Why do you favor this over the previous interface?

Some users of firecracker have mentioned that lacking support of multi-queue causes performance penalty, so we hope we could enable mutli-queue for virtio devices.
Moving lock into the device backend driver is the first step:)

Having interior mutability here is a thorny issue and needs a separate discussion :( Why do you favor this over the previous interface?

@alexandruag Any further comments on this? I'm ok with this PR, but I'd like to get your opinion on it and see if you want to block it until we agree on the inner mutability part.

Hi, apologies for the delay. The problem with assuming interior mutability is many devices mutate their state during normal operation, but have to go out of their way to actually implement interior mutability. I don't know what the best approach is long term (hopefully it'll become clearer as we start putting things together), but one potential solution right now is to add another trait (something like DeviceIoMut) that defines the same methods as DeviceIo but requiring &mut self instead of &self, and then also add a blanket implementation such as impl<T> DeviceIo for Mutex<T> where T: DeviceIoMut { ... }, that allows a Mutex<T> to be used as a DeviceIo object automatically as long as the inner type implements DeviceIoMut (same goes for other standard wrappers that offer interior mutability like RefCell). Does that make any sense?

Great idea, this satisfies both side with an zero overhead abstraction.
How about merging the PR first and sending another PR for the DeviceIoMut proposal?

sameo

Once rebased on top of master, this PR will look good to me

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

In order to get a real multiple threads handling to enhance performance, the DeviceIo trait need adopt interior mutability pattern. Signed-off-by: Jing Liu <[email protected]>

Based on resources definition, this adds device IO manager to manage all devices IO ranges. Signed-off-by: Jing Liu <[email protected]>

IO manager is responsible for handling IO operation when VMExit. It works out the specific device according to the address range and hand over to DeviceIo trait. Signed-off-by: Jing Liu <[email protected]>

Unit tests for IO manager. Signed-off-by: Jing Liu <[email protected]>

Append missing tests for resources and fix some typo. Signed-off-by: Jing Liu <[email protected]>

sameo changed the title ~~Add device manager support~~ [WIP] Add device manager support Nov 1, 2019

liujing2 force-pushed the devmgr-v1 branch 3 times, most recently from 8e9874b to e0424b5 Compare November 5, 2019 11:27

liujing2 mentioned this pull request Nov 8, 2019

Add interrupt traits and a KVM based implementation #11

Closed

liujing2 force-pushed the devmgr-v1 branch from 8e9874b to 985e7c3 Compare November 22, 2019 10:03

liujing2 changed the title ~~[WIP] Add device manager support~~ [WIP] Add IO manager support Nov 22, 2019

liujing2 changed the title ~~[WIP] Add IO manager support~~ Add IO manager support Nov 22, 2019

liujing2 changed the title ~~Add IO manager support~~ [WIP] Add IO manager support Nov 22, 2019

liujing2 force-pushed the devmgr-v1 branch from 985e7c3 to 7f9c715 Compare November 22, 2019 12:08

liujing2 changed the title ~~[WIP] Add IO manager support~~ Add IO manager support Nov 22, 2019

liujing2 force-pushed the devmgr-v1 branch from 7f9c715 to d00a706 Compare November 22, 2019 12:23

jiangliu reviewed Nov 22, 2019

View reviewed changes

liujing2 force-pushed the devmgr-v1 branch from d00a706 to dd18110 Compare November 25, 2019 10:44

andreeaflorescu reviewed Nov 25, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

jiangliu reviewed Nov 25, 2019

View reviewed changes

src/device_manager.rs Show resolved Hide resolved

src/device_manager.rs Outdated Show resolved Hide resolved

src/device_manager.rs Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

liujing2 force-pushed the devmgr-v1 branch from dd18110 to ac2e7af Compare November 26, 2019 05:17

liujing2 requested a review from andreeaflorescu December 5, 2019 05:18

sboeuf previously approved these changes Dec 12, 2019

View reviewed changes

sameo mentioned this pull request Dec 13, 2019

Dockerfile: Bump the rust toolchain to 1.39 rust-vmm/rust-vmm-container#21

Merged

liujing2 dismissed sboeuf’s stale review via e4ba10e December 16, 2019 08:15

liujing2 force-pushed the devmgr-v1 branch from ac2e7af to e4ba10e Compare December 16, 2019 08:15

liujing2 mentioned this pull request Dec 16, 2019

Basic IO manager support cloud-hypervisor/vm-device#1

Closed

liujing2 force-pushed the devmgr-v1 branch from e4ba10e to 932bf55 Compare December 17, 2019 08:59

jiangliu approved these changes Dec 20, 2019

View reviewed changes

jiangliu requested review from jiangliu and sameo December 30, 2019 11:43

jiangliu previously approved these changes Dec 30, 2019

View reviewed changes

sameo reviewed Jan 8, 2020

View reviewed changes

src/device_manager.rs Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/device_manager.rs Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

sboeuf previously approved these changes Jan 13, 2020

View reviewed changes

alexandruag reviewed Jan 13, 2020

View reviewed changes

jiangliu requested review from alexandruag and sameo January 13, 2020 09:49

sameo reviewed Jan 14, 2020

View reviewed changes

liujing2 dismissed stale reviews from sboeuf and jiangliu via b9782b7 January 20, 2020 03:42

liujing2 force-pushed the devmgr-v1 branch from e637f98 to b9782b7 Compare January 20, 2020 03:42

liujing2 requested review from sameo, jiangliu and sboeuf January 20, 2020 06:41

Jing Liu added 6 commits January 20, 2020 18:13

Update rust-vmm-ci

395c26b

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

Make DeviceIo internal mutability

9eeb586

In order to get a real multiple threads handling to enhance performance, the DeviceIo trait need adopt interior mutability pattern. Signed-off-by: Jing Liu <[email protected]>

Add IO manager support

7db0747

Based on resources definition, this adds device IO manager to manage all devices IO ranges. Signed-off-by: Jing Liu <[email protected]>

Add read and write operations handling

9775e15

IO manager is responsible for handling IO operation when VMExit. It works out the specific device according to the address range and hand over to DeviceIo trait. Signed-off-by: Jing Liu <[email protected]>

Add unit tests

d9041f9

Unit tests for IO manager. Signed-off-by: Jing Liu <[email protected]>

Append missing tests for resources

b9782b7

Append missing tests for resources and fix some typo. Signed-off-by: Jing Liu <[email protected]>

jiangliu approved these changes Jan 20, 2020

View reviewed changes

sboeuf approved these changes Jan 20, 2020

View reviewed changes

jiangliu merged commit 3daea68 into rust-vmm:master Jan 22, 2020

jiangliu mentioned this pull request Jan 22, 2020

Introduce Mutex<T: DeviceIoMut> adapter for DeviceIo #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IO manager support #12

Add IO manager support #12

liujing2 commented Oct 31, 2019 •

edited

Loading

sameo commented Nov 1, 2019

liujing2 commented Dec 5, 2019

sboeuf left a comment

sboeuf commented Dec 12, 2019

liujing2 commented Dec 13, 2019 •

edited

Loading

sboeuf commented Dec 13, 2019

sameo commented Dec 17, 2019

sameo left a comment

liujing2 commented Jan 9, 2020 •

edited

Loading

jiangliu commented Jan 9, 2020

sameo commented Jan 10, 2020

jiangliu commented Jan 13, 2020 •

edited

Loading

alexandruag Jan 13, 2020

jiangliu Jan 13, 2020

sameo Jan 20, 2020

alexandruag Jan 21, 2020

jiangliu Jan 22, 2020

sameo left a comment

Add IO manager support #12

Add IO manager support #12

Conversation

liujing2 commented Oct 31, 2019 • edited Loading

sameo commented Nov 1, 2019

liujing2 commented Dec 5, 2019

sboeuf left a comment

Choose a reason for hiding this comment

sboeuf commented Dec 12, 2019

liujing2 commented Dec 13, 2019 • edited Loading

sboeuf commented Dec 13, 2019

sameo commented Dec 17, 2019

sameo left a comment

Choose a reason for hiding this comment

liujing2 commented Jan 9, 2020 • edited Loading

jiangliu commented Jan 9, 2020

sameo commented Jan 10, 2020

jiangliu commented Jan 13, 2020 • edited Loading

alexandruag Jan 13, 2020

Choose a reason for hiding this comment

jiangliu Jan 13, 2020

Choose a reason for hiding this comment

sameo Jan 20, 2020

Choose a reason for hiding this comment

alexandruag Jan 21, 2020

Choose a reason for hiding this comment

jiangliu Jan 22, 2020

Choose a reason for hiding this comment

sameo left a comment

Choose a reason for hiding this comment

liujing2 commented Oct 31, 2019 •

edited

Loading

liujing2 commented Dec 13, 2019 •

edited

Loading

liujing2 commented Jan 9, 2020 •

edited

Loading

jiangliu commented Jan 13, 2020 •

edited

Loading