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

virtio core support #1

Merged
merged 9 commits into from Jun 4, 2019
Merged

virtio core support #1

merged 9 commits into from Jun 4, 2019

Conversation

sameo
Copy link

@sameo sameo commented May 14, 2019

This PR brings the initial support for virtio devices:

  • Basic virtio queue and descriptor handling, based on Firecracker v0.15
  • A virtio Device trait based on crosvm 107edb3e
  • Buildkit CI support

src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Show resolved Hide resolved
src/queue.rs Show resolved Hide resolved
src/queue.rs Show resolved Hide resolved
@sameo sameo force-pushed the topic/virtio-core branch 3 times, most recently from 8f1b5c1 to 40913d1 Compare May 20, 2019 08:35
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@andreeaflorescu andreeaflorescu left a comment

Choose a reason for hiding this comment

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

First pass.

Please also add tests to check the musl build. In Firecracker we build against musl.

Cargo.toml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
src/device.rs Outdated Show resolved Hide resolved
src/device.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@sameo sameo force-pushed the topic/virtio-core branch 3 times, most recently from 34b7af2 to ffc60dc Compare May 21, 2019 09:30
Copy link
Member

@andreeaflorescu andreeaflorescu left a comment

Choose a reason for hiding this comment

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

LGTM with a few nits and comments here and there.

Is there any way to also test the error cases? We can do that in a future PR f it is possible.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
src/queue.rs Show resolved Hide resolved
src/queue.rs Show resolved Hide resolved
@sameo sameo requested review from bonzini and jiangliu May 27, 2019 17:06
bonzini
bonzini previously approved these changes May 28, 2019
Copy link
Member

@bonzini bonzini left a comment

Choose a reason for hiding this comment

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

The remaining comments I have left only affect tests, so it's okay to do that later.

@bonzini
Copy link
Member

bonzini commented May 28, 2019

Since I don't have write access, my approval doesn't count for merging.

@andreeaflorescu
Copy link
Member

@bonzini added you as collaborator.

src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Outdated Show resolved Hide resolved
src/queue.rs Show resolved Hide resolved
src/queue.rs Show resolved Hide resolved
src/device.rs Outdated Show resolved Hide resolved
.cargo/config Show resolved Hide resolved
Samuel Ortiz added 5 commits May 29, 2019 11:13
This is based on Firecracker v0.16 and ported to the rust-vmm memory model.
It implements the virtio virtqueue specification for virtio devices.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
In order to simplify the iter and add_used methods.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
This is based on crosvm 107edb3e, with removal for all PCI specific
bits and the GuestMemory generics dependency addition.

As it depends on the EventFd API, this introduces a vmm-sys-util
dependency.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Add the Apache-2.0 and BSD-3-Clause licenses for this crate, and
dual license Apache-2.0 OR/AND BSD-3-Clause.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Provide a crate description, a brief API documentation
and some testing instructions.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
@sameo
Copy link
Author

sameo commented May 29, 2019

@aghecenco I addressed most of your comments, PTAL.

Samuel Ortiz added 4 commits May 29, 2019 11:22
And update the corresponding README sections.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
See https://buildkite.com/docs/pipelines/defining-steps

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
The linker can't find the floating point builtins.

Added target-feature=+crt-static and link-arg=-lgcc as a temporary
workaround. This seems to be the accepted fix in the Rust community:
rust-lang/compiler-builtins#201

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
@bonzini
Copy link
Member

bonzini commented Jun 4, 2019

Summary of issues to create after this is merged:

  • Use VolatileAtomicRef to access the descriptor (requires rust 1.34.0 for integer_atomics :( )

  • Use VolatileRef instead of SomePlaceInMemory and read_obj/write_obj

Copy link
Member

@bonzini bonzini left a comment

Choose a reason for hiding this comment

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

All review comments were addressed.

@bonzini bonzini merged commit cca8b81 into rust-vmm:master Jun 4, 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

5 participants