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

Crate Addition Request: vm-virtio #19

Closed
sameo opened this issue Feb 14, 2019 · 21 comments
Closed

Crate Addition Request: vm-virtio #19

sameo opened this issue Feb 14, 2019 · 21 comments

Comments

@sameo
Copy link

sameo commented Feb 14, 2019

Crate Name

The virtio name is already taken on crates.io. The crate is unmaintained and really incomplete.

Instead of virtio, we could use:

  • virtio-ng
  • virtio-vmm
  • Send your suggestion...

Short Description

This crate would provide:

  • A VirtioDevice trait
  • Implementations of VirtioDevice for the block, net, rng, ballon virtio devices
  • Implementations of VirtioDevice for the vsock, and net vhost devices
  • Virtio queues and descriptor API.
  • MMIO and PCI virtio transports

Why is this crate relevant to the rust-vmm project?

rust-vmm needs a virtio implementation.

@andreeaflorescu
Copy link
Member

virtio-devices looks free. we should indeed reserve the names.

@sameo
Copy link
Author

sameo commented Feb 14, 2019

cc @dgreid @jiangliu @dhrgit

@sameo
Copy link
Author

sameo commented Feb 14, 2019

virtio-devices looks free. we should indeed reserve the names.

I like virtio-devices.

@jiangliu
Copy link
Member

virtio-devices: +1

@dagrh
Copy link

dagrh commented Feb 14, 2019

Do we get one crate with some different sections in it or multiple crates; for example do we have one that has just all the constant/structure/interface definitions in?

@andreeaflorescu
Copy link
Member

We will probably have virtio-bindings or something of that sort for the kernel virtio headers. From my point of view the separation will come from the virtio device implementation vs the backends that you can use for those devices (the discussion about the backends is here: #20).

@jiangliu
Copy link
Member

It would be great if the virtio-binding includes binding for vhost too, so the dependency relationship among virtio-device, vhost-backend and virtio-binding will be easy to maintain.

@andreeaflorescu
Copy link
Member

@jiangliu how would it make it easier?

@jiangliu
Copy link
Member

jiangliu commented Feb 14, 2019 via email

@andreeaflorescu
Copy link
Member

I don't feel strongly for having it as a separate crate. I just thought it makes more sense to have 2 bindings for virtio-devices and vhost since we are going to have 2 crates to use those. On the other hand I wouldn't want to pollute rust-vmm with one binding crate for each feature as long as it makes sense to have them in a single crate. I really don't have a strong argument. Either way is good.

@sameo sameo changed the title Crate Addition Request: virtio Crate Addition Request: virtio-devices Feb 15, 2019
@bonzini
Copy link
Member

bonzini commented Feb 17, 2019

Having a crate for virtio devices would first require defining a trait for devices (such as devices::bus), and I think that requires a separate design discussion.

However the crate could start porting to rust_vmm the devices::virtio::queue code for virtqueue parsing and devices::virtio::vhost code which can be used standalone.

@jiangliu
Copy link
Member

Good suggestion to build a crate for device model.
The device model in crosvm and firecracker is not so strong, enhancements may be needed to support qemu and Kata container.

@liujing2
Copy link

@sameo Are we going to include all the device related into one crate? Instead of one crate for common codes like Bus, PCI and another one crate for real devices implementation.

@petreeftime
Copy link

As someone who is new to working with firecracker code but had to implement some virtio devices, I would want to see devices become quite a lot more generic than they are right now. The firecracker model does not really abstract KVM-specific design decisions all that well and this makes some things quite challenging to understand. I am not sure a single crate is a good idea, as that would lead to devices being less generic and more difficult to extend. The model behind rust-vmm in my opinion is to allow very slim and purposeful VMMs, and not a one-size-fits-all solution.

I can see a reason to separate devices into layers like: transport (PCI/MMIO), frontend (configuration of device and high-level handling of queues) and backend (handling of items in the queues for specific devices). This would allow sufficient separation to make it easy for someone to change the backend according to their needs, without having to dive too deeply into how the rest of the vmm is actually implemented.

I would see the following crates:

  • virtio-device: this crate provides the generic traits required from a device to be counted as a virtio device
  • virtio-mmio / virtio-pci: transport-specific functionality for devices.
  • virtio-device-*: this is the default frontend + backend + backend traits for a specific virtio device. Although maybe it makes more sense to separate virtio-device-vsock for only frontend and have a separate virtio-device-vsock-vhost for the backend, even if the name is a bit verbose.

@bonzini
Copy link
Member

bonzini commented Mar 26, 2019

virtio-device: this crate provides the generic traits required from a device to be counted as a virtio device

It can do much more: it can be a rust-vmm/vm-memory client that takes a dyn GuestMemory anddoes all the VirtQueue parsing. That should be the first thing to add, because it's the easiest to abstract - we already almost have vm-memory ready.

virtio-mmio / virtio-pci: transport-specific functionality for devices.

This depends on having device abstractions in rust-vmm, so it comes later.

@bonzini
Copy link
Member

bonzini commented Apr 2, 2019

Let's rename this to virtio and start with virtqueue functionality, interfacing with vm-memory. Everything else (devices, which depend on the #33 vm-device create, vhost-kernel bindings, and vhost-user) can be added later and hidden behind features to keep the dependencies as small as possible.

@sboeuf
Copy link

sboeuf commented Apr 10, 2019

@bonzini

Let's rename this to virtio

I think @sameo mentioned it's not possible because already used on crates.io.

@bonzini
Copy link
Member

bonzini commented Apr 10, 2019

Why not vm-virtio, following vm-memory, vm-devices etc.?

@sboeuf
Copy link

sboeuf commented Apr 10, 2019

Why not vm-virtio, following vm-memory, vm-devices etc.?

Fine by me, I like that we stay consistent with the naming.

@sameo
Copy link
Author

sameo commented May 1, 2019

I just created the vm-virtio repo.
I will shortly send an initial PR for it.
@bonzini iiuc you're suggesting we avoid having a virtio-bindings separate crate and use a cleaned up version of the bindgen'ed virtio code in the vm-virtio crate, right ?

@sameo sameo changed the title Crate Addition Request: virtio-devices Crate Addition Request: vm-virtio May 1, 2019
@andreeaflorescu
Copy link
Member

Repository created. Closing this issue.

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

No branches or pull requests

8 participants