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

vhost-device-gpu: Add Initial Implementation #668

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

dorindabassey
Copy link
Contributor

@dorindabassey dorindabassey commented Jun 6, 2024

Summary of the PR

I and @mtjhrc have been working on this PR that adds a new crate - vhost-device-gpu.
This vhost-device-gpu crate provides a vhost-user backend daemon for VIRTIO GPU device emulation as specified in the
VIRTIO Spec v.1.2

This crate utilizes the rutabaga crate Imported from crosvm commit: 02110bc6fcc6af70329f410aef8859ec70dd9224
rutabaga_gfx: fixes for crrev.com/c/5279413

The rutabaga crate is vendored because it contains minor modifications to rutabaga_gfx to make it compatible with vhost-device-gpu eventually we would use the upstream version when things are more stable.

This crate utilizes the rutabaga crate v0.1.4 specifically the virglrenderer and gfxstream features.

This crate includes some modifications from crosvm and libkruns virtio-gpu device

This crate depends on this PR that implements support for QEMU's vhost-user-gpu protocol.

Supported features are:
Using gfxstream component from rutabaga.
Using virglrenderer component from rutabaga.
support the GPU device operations as specified by the virtio specification

TODO and nice to have items:
Add support for the remaining GPU device operations:

  • VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB
  • VIRTIO_GPU_CMD_SET_SCANOUT_BLOB
  • VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID

Add more tests to improve the code coverage
Add support for directly sharing display output resource using dmabuf instead of copying it by using transfer_read operation.

Fixes: #598

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@stefano-garzarella
Copy link
Member

@dorindabassey @mtjhrc great! Thanks for this PR. IIUC this is still WIP, so I marked as Draft, feel free to mark this ready when you want ;-)

@dorindabassey
Copy link
Contributor Author

@dorindabassey @mtjhrc great! Thanks for this PR. IIUC this is still WIP, so I marked as Draft, feel free to mark this ready when you want ;-)

yes, Thanks. I see that it's failing on the CI, once that's resolved we can mark it ready.

@stefano-garzarella
Copy link
Member

@dorindabassey @mtjhrc great! Thanks for this PR. IIUC this is still WIP, so I marked as Draft, feel free to mark this ready when you want ;-)

yes, Thanks. I see that it's failing on the CI, once that's resolved we can mark it ready.

Sure, but IMHO we should merge rust-vmm/vhost#239 first before marking this ready.

Copy link
Member

@epilys epilys left a comment

Choose a reason for hiding this comment

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

This is a complex device, you did such monumental effort!

Since it's large I'd have to re-read it a couple of times.

staging/vhost-device-gpu/README.md Outdated Show resolved Hide resolved
staging/vhost-device-gpu/README.md Outdated Show resolved Hide resolved
staging/vhost-device-gpu/README.md Show resolved Hide resolved
staging/vhost-device-gpu/src/device.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/device.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/virtio_gpu.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/virtio_gpu.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/virtio_gpu.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/virtio_gpu.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/rutabaga_gfx/ffi/build.rs Outdated Show resolved Hide resolved
@dorindabassey
Copy link
Contributor Author

@dorindabassey @mtjhrc great! Thanks for this PR. IIUC this is still WIP, so I marked as Draft, feel free to mark this ready when you want ;-)

yes, Thanks. I see that it's failing on the CI, once that's resolved we can mark it ready.

Sure, but IMHO we should merge rust-vmm/vhost#239 first before marking this ready.

you're right, we can wait to merge the first PR.

dorindabassey pushed a commit to dorindabassey/rust-vmm-container that referenced this pull request Jun 7, 2024
the vhost-device-gpu crate linked here
rust-vmm/vhost-device#668
contains rutabaga_gfx that requires epoxy library

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
dorindabassey pushed a commit to dorindabassey/rust-vmm-container that referenced this pull request Jun 7, 2024
the vhost-device-gpu crate linked here
rust-vmm/vhost-device#668
contains rutabaga_gfx that requires epoxy library

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
ShadowCurse pushed a commit to rust-vmm/rust-vmm-container that referenced this pull request Jun 10, 2024
the vhost-device-gpu crate linked here
rust-vmm/vhost-device#668
contains rutabaga_gfx that requires epoxy library

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
dorindabassey pushed a commit to dorindabassey/rust-vmm-ci that referenced this pull request Jul 24, 2024
This version adds fixes for libepoxy dependencies
which is required for vhost-device-gpu crate
more info about the crate can be found here:
rust-vmm/vhost-device#668

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
dorindabassey pushed a commit to dorindabassey/rust-vmm-ci that referenced this pull request Jul 24, 2024
This version includes libepoxy dependencies
which is required for vhost-device-gpu crate
more info about the crate can be found here:
rust-vmm/vhost-device#668
link to the container PR that adds libepoxy
where the tag is generated can be found here:
rust-vmm/rust-vmm-container#103

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
roypat pushed a commit to rust-vmm/rust-vmm-ci that referenced this pull request Jul 24, 2024
This version includes libepoxy dependencies
which is required for vhost-device-gpu crate
more info about the crate can be found here:
rust-vmm/vhost-device#668
link to the container PR that adds libepoxy
where the tag is generated can be found here:
rust-vmm/rust-vmm-container#103

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
dorindabassey pushed a commit to dorindabassey/rust-vmm-container that referenced this pull request Jul 29, 2024
the vhost-device-gpu crate linked here
rust-vmm/vhost-device#668
contains rutabaga_gfx and the system library packages for
rutabaga_gfx crate which is not available in the used ubuntu
image, so install the neccessary packages that are available in
ubuntu's repositories while for unavailable packages:
build and install it manually.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
dorindabassey pushed a commit to dorindabassey/rust-vmm-container that referenced this pull request Jul 29, 2024
The vhost-device-gpu crate linked here
rust-vmm/vhost-device#668
contains rutabaga_gfx and the system library packages for
rutabaga_gfx crate which is not available in the used ubuntu
image, so install the neccessary packages that are available in
ubuntu's repositories while for unavailable packages:
build and install it manually.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
dorindabassey pushed a commit to dorindabassey/rust-vmm-container that referenced this pull request Jul 30, 2024
The vhost-device-gpu crate linked here
rust-vmm/vhost-device#668
contains rutabaga_gfx and the system library packages for
rutabaga_gfx crate which is not available in the used ubuntu
image, so install the neccessary packages that are available in
ubuntu's repositories while for unavailable packages:
build and install it manually.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
dorindabassey pushed a commit to dorindabassey/rust-vmm-container that referenced this pull request Jul 30, 2024
The vhost-device-gpu crate linked here
rust-vmm/vhost-device#668
contains rutabaga_gfx and the system library packages for
rutabaga_gfx crate which is not available in the used ubuntu
image, so install the neccessary packages that are available in
ubuntu's repositories while for unavailable packages:
build and install it manually.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
stefano-garzarella pushed a commit to rust-vmm/rust-vmm-container that referenced this pull request Aug 1, 2024
The vhost-device-gpu crate linked here
rust-vmm/vhost-device#668
contains rutabaga_gfx and the system library packages for
rutabaga_gfx crate which is not available in the used ubuntu
image, so install the neccessary packages that are available in
ubuntu's repositories while for unavailable packages:
build and install it manually.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
@dorindabassey dorindabassey force-pushed the vhost-gpu branch 2 times, most recently from aa25731 to 11c9057 Compare October 29, 2024 15:08
Copy link
Member

@epilys epilys left a comment

Choose a reason for hiding this comment

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

Suggestion: use the rustfmt.toml file from the sound device and run cargo +nightly fmt, this will format all comments, doc comments, imports, etc as well.

staging/vhost-device-gpu/CHANGELOG.md Outdated Show resolved Hide resolved
staging/vhost-device-gpu/Cargo.toml Outdated Show resolved Hide resolved
staging/vhost-device-gpu/Cargo.toml Show resolved Hide resolved
staging/vhost-device-gpu/README.md Outdated Show resolved Hide resolved
staging/vhost-device-gpu/README.md Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/virtio_gpu.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/README.md Outdated Show resolved Hide resolved
@dorindabassey
Copy link
Contributor Author

Suggestion: use the rustfmt.toml file from the sound device and run cargo +nightly fmt, this will format all comments, doc comments, imports, etc as well.

Thank you for the review, I addressed them let me know if I missed any change.

Copy link
Member

@epilys epilys left a comment

Choose a reason for hiding this comment

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

Thanks for your patience 🫠 I recognize these are a lot of nits and addressing them takes time...

staging/vhost-device-gpu/CHANGELOG.md Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/device.rs Show resolved Hide resolved
staging/vhost-device-gpu/src/device.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/main.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/lib.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/lib.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/main.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/main.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/main.rs Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/virtio_gpu.rs Outdated Show resolved Hide resolved
@dorindabassey
Copy link
Contributor Author

Hi @epilys @stefano-garzarella @bilelmoussaoui
we've (I and @mtjhrc) adressed the comments and changes and I think this PR is in a good shape. Can you let me know if there are any more issues to address on this PR?

@stefano-garzarella
Copy link
Member

@dorindabassey can you rebase?

Copy link
Member

@epilys epilys left a comment

Choose a reason for hiding this comment

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

LGTM! Left minor comments and some questions about endianness for virtio gpu commands.

staging/vhost-device-gpu/src/device.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/device.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/device.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/device.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/device.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/lib.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/lib.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/virtio_gpu.rs Show resolved Hide resolved
Copy link
Contributor

@bilelmoussaoui bilelmoussaoui left a comment

Choose a reason for hiding this comment

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

otherwise lgtm (from rust perspective obviously)

staging/vhost-device-gpu/src/protocol.rs Outdated Show resolved Hide resolved
staging/vhost-device-gpu/src/protocol.rs Show resolved Hide resolved
@dorindabassey dorindabassey force-pushed the vhost-gpu branch 2 times, most recently from 2190df3 to 21335fe Compare November 20, 2024 10:16
@dorindabassey
Copy link
Contributor Author

Hi @stefano-garzarella @epilys, @bilelmoussaoui, I've rebased this PR and I think it's in a good shape now. Can you let me know if there are any more issues to address on this PR?

epilys
epilys previously approved these changes Nov 20, 2024
Copy link
Member

@epilys epilys left a comment

Choose a reason for hiding this comment

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

LGTM for merging, if you want to update the rust-vmm dependencies before merging this PR that also LGTM.

staging/vhost-device-gpu/Cargo.toml Outdated Show resolved Hide resolved
This program is a vhost-user backend daemon that provides
VIRTIO GPU device emulation as specified in the VIRTIO Spec v.1.2
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html
This crate utilizes the rutabaga crate from crosvm with some
minor modification to rutabaga crate to fix compilation.
This crate depends on this PR[rust-vmm/vhost#239]
that implements support for QEMU's vhost-user-gpu protocol.

This device uses the rutabaga_gfx crate to offer two rendering backends:
1. Virglrenderer:
   - Rutabaga translates OpenGL API and Vulkan calls to an intermediate
     representation and allows for OpenGL acceleration on the host.
2. Gfxstream:
   - GLES and Vulkan calls are forwarded to the host.

These backends can be used by simply changing the `--gpu-mode` command
line option.
This crate also includes some modifications from libkrun virtio-gpu device
https://github.com/containers/libkrun/tree/main/src/devices/src/virtio/gpu

Fixes: rust-vmm#598

Co-authored-by: Dorinda Bassey <dbassey@redhat.com>
Co-authored-by: Matej Hrica <mhrica@redhat.com>

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
Signed-off-by: Matej Hrica <mhrica@redhat.com>
Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@bilelmoussaoui bilelmoussaoui left a comment

Choose a reason for hiding this comment

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

LGTM from my side as well

@stefano-garzarella stefano-garzarella merged commit 391e32b into rust-vmm:main Nov 20, 2024
2 checks passed
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.

virtio-gpu device
5 participants