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

Add support for VHOST_USER_GPU_SET_SOCKET #239

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

mtjhrc
Copy link

@mtjhrc mtjhrc commented Apr 18, 2024

Summary of the PR

Me and @dorindabassey have been working on this PR, which adds support for the QEMU's vhost-user-gpu protocol.
This is a protocol for sending messeges from backend to the fronted, it is needed to implement display output in QEMU to implement a vhost-user-gpu device.
See issue #236

This feature is very similar to the SET_BACKEND_REQ_FD, so it is implemented in a similar way.
Support is only added for sending these vhost messages from backend to frontend. Support for handling this protocol on frontend is not implemented.

The protocol is very similar to the normal vhost protocol, but the valid header flags of the VhostUserMsgHeader and the newly introduced VhostUserGpuMsgHeader are different. (There is no version bit, only reply bit is specified). So the Endpoint utility was made generic over the header type, not just the request. Unfortunately there is another issue, that the messages VHOST_USER_GPU_SCANOUT and VHOST_USER_GPU_CURSOR_UPDATE are bigger than the MAX_MSG_SIZE. So we added methods for sending bigger messages to Endpoint.

/// The vhost-user specification uses a field of u32 to store message length.
/// On the other hand, preallocated buffers are needed to receive messages from the Unix domain
/// socket. To preallocating a 4GB buffer for each vhost-user message is really just an overhead.
/// Among all defined vhost-user messages, only the VhostUserConfig and VhostUserMemory has variable
/// message size. For the VhostUserConfig, a maximum size of 4K is enough because the user
/// configuration space for virtio devices is (4K - 0x100) bytes at most. For the VhostUserMemory,
/// 4K should be enough too because it can support 255 memory regions at most.
pub const MAX_MSG_SIZE: usize = 0x1000;

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.

@mtjhrc mtjhrc changed the title Add support for QEMU display protocol (VHOST_USER_GPU_SET_SOCKET) Add support for VHOST_USER_GPU_SET_SOCKET Apr 18, 2024
@mtjhrc mtjhrc force-pushed the gpu-socket-final branch 3 times, most recently from 561f4c2 to 0a77153 Compare April 18, 2024 15:41
@mtjhrc mtjhrc marked this pull request as ready for review April 18, 2024 15:44
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.

I left some comments, but the code is almost fine with me.

Please fix the commit messages, some of them doesn't have any description (please also just a line describing the type of message you're adding is enough, but avoid empty), others doesn't have new lines before S-o-b.

Also check copyright and SPDX of new files.

vhost-user-backend/Cargo.toml Outdated Show resolved Hide resolved
vhost/src/vhost_user/gpu_message.rs Show resolved Hide resolved
vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/gpu_backend_req.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/backend_req_handler.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/gpu_backend_req.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/connection.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/gpu_backend_req.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/connection.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/gpu_message.rs Outdated Show resolved Hide resolved
@mtjhrc mtjhrc force-pushed the gpu-socket-final branch 4 times, most recently from 8ec2ff7 to e1b1ab4 Compare May 15, 2024 11:29
@mtjhrc
Copy link
Author

mtjhrc commented May 15, 2024

Apart from changes you requested I also changed the signature of cursor_update, to accept the array outside of the struct as I realized this is more practical to the user. This also meant I removed a test for sending such a big message body.

@mtjhrc
Copy link
Author

mtjhrc commented May 15, 2024

I am not sure about the changes in regards to sending big message. I added a constant to the MsgHeader trait and use it for checking size when sending messages. I also left a TODO comment. I think we could support u32::MAX everywhere but, this warrants a separate PR and a discussion if we want to enforce the current MAX_MSG_SIZE for vhost protocol, also there are some redundant checks for this at multiple levels and I think the checks could probably overflow when adding numbers.

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.

I did a quick review and I think we are in a good shape :-)

I just suggest that you fix the description a bit in the commits. Take a look here: https://docs.kernel.org/process/submitting-patches.html

Describe your changes in imperative mood, e.g. “make xyzzy do frotz” instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to do frotz”, as if you are giving orders to the codebase to change its behaviour.

vhost-user-backend/src/backend.rs Outdated Show resolved Hide resolved
vhost/Cargo.toml Outdated Show resolved Hide resolved
vhost/src/vhost_user/gpu_backend_req.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/gpu_backend_req.rs Show resolved Hide resolved
Copy link
Contributor

@aesteve-rh aesteve-rh left a comment

Choose a reason for hiding this comment

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

In general LGTM, but the PR is really long, not sure if it would be better to split some of the initial commits that have fewer dependencies.

I left a few comments, and I will give it another pass for the next round.

vhost/src/vhost_user/message.rs Show resolved Hide resolved
vhost/src/vhost_user/gpu_message.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/gpu_message.rs Outdated Show resolved Hide resolved
vhost-user-backend/src/backend.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/backend_req_handler.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/backend_req_handler.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/gpu_message.rs Outdated Show resolved Hide resolved
dorindabassey pushed a commit to dorindabassey/vhost-device that referenced this pull request Jun 6, 2024
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 Imported from crosvm
This crate depends on this PR[rust-vmm/vhost#239]
that implements support for QEMU's vhost-user-gpu protocol.

This device can be tested following the instructions explained in the
README.md file under staging/vhost-device-gpu/.

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>
dorindabassey pushed a commit to dorindabassey/vhost-device that referenced this pull request Jun 6, 2024
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 Imported from crosvm
This crate depends on this PR[rust-vmm/vhost#239]
that implements support for QEMU's vhost-user-gpu protocol.

This device can be tested following the instructions explained in the
README.md file under staging/vhost-device-gpu/.

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>
dorindabassey pushed a commit to dorindabassey/vhost-device that referenced this pull request Jun 10, 2024
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 Imported from crosvm
This crate depends on this PR[rust-vmm/vhost#239]
that implements support for QEMU's vhost-user-gpu protocol.

This crate also includes some modifications from libkrun virtio-gpu device
https://github.com/containers/libkrun/tree/main/src/devices/src/virtio/gpu

This device can be tested following the instructions explained in the
README.md file under staging/vhost-device-gpu/.

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>
@mtjhrc mtjhrc force-pushed the gpu-socket-final branch 2 times, most recently from 0d5f48b to f05e322 Compare June 11, 2024 11:15
@mtjhrc
Copy link
Author

mtjhrc commented Jun 11, 2024

I changed the internal API used here like discussed here with @stefano-garzarella, this makes each message method explicitly call recv_reply (instead of wait_for_ack) if it is needed.
So instead of

pub fn get_display_info(&self) -> io::Result<VirtioGpuRespDisplayInfo> {
    self.send_header(GpuBackendReq::GET_DISPLAY_INFO, None)
}

the public methods now do:

pub fn get_display_info(&self) -> io::Result<VirtioGpuRespDisplayInfo> {
    let node = self.node(); // locks the mutex and returns a reference 
   
    let hdr = node.send_header(GpuBackendReq::GET_DISPLAY_INFO, None)?;
    node.recv_reply(&hdr)
}

Note that some messeges aren't supposed to wait for a reply so this removes the need for overly specific utility methods like send_message_with_payload_no_reply.
I also added a comment in the code explaining that there is no VHOST_USER_PROTOCOL_F_REPLY_ACK flag for this protocol. Because of this I now changed the tests to call the public methods, since more of the code moved there.

@stefano-garzarella
Copy link
Member

@mtjhrc great! Thanks, I'll review in the next days (I'm going to DevConf at Brno, so maybe it will take a while, sorry in advance).

Since you are here, can you also rebase on the latest main?

@mtjhrc
Copy link
Author

mtjhrc commented Jun 11, 2024

@mtjhrc great! Thanks, I'll review in the next days (I'm going to DevConf at Brno, so maybe it will take a while, sorry in advance).

Since you are here, can you also rebase on the latest main?

Rebased. No problem, also sorry for the delay fixing this on my side too.

@mtjhrc mtjhrc force-pushed the gpu-socket-final branch 2 times, most recently from 6a8c374 to 9c50abb Compare June 11, 2024 12:20
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.

@mtjhrc great work about send_*() API!

The PR LGTM, but maybe during the rebase the change to enable gpu-socket in .buildkite/rust-vmm-ci-tests.json are lost. Can you check it?

dorindabassey pushed a commit to dorindabassey/vhost-device that referenced this pull request Jun 18, 2024
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 Imported from crosvm
This crate depends on this PR[rust-vmm/vhost#239]
that implements support for QEMU's vhost-user-gpu protocol.

This crate also includes some modifications from libkrun virtio-gpu device
https://github.com/containers/libkrun/tree/main/src/devices/src/virtio/gpu

This device can be tested following the instructions explained in the
README.md file under staging/vhost-device-gpu/.

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
Contributor

@aesteve-rh aesteve-rh left a comment

Choose a reason for hiding this comment

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

LGTM!

@mtjhrc
Copy link
Author

mtjhrc commented Jul 1, 2024

@mtjhrc great work about send_*() API!

The PR LGTM, but maybe during the rebase the change to enable gpu-socket in .buildkite/rust-vmm-ci-tests.json are lost. Can you check it?

I have fixed this and rebased the PR.

mtjhrc and others added 15 commits July 3, 2024 10:48
The Endpoint was previusly generic only over the Request.
This commit allows Endpoint to be used with a protocol with a slightly
different header such as the QEMU GPU protocol on the socket from
VHOST_USER_GPU_SET_SOCKET.
https://www.qemu.org/docs/master/interop/vhost-user-gpu.html

Signed-off-by: Matej Hrica <mhrica@redhat.com>
Move the enum_value macro into vhost_user module and make it public within
this module. This will be used in the next commit for defining gpu message
enum.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
This commit adds the basic definitions of GPU commands on the socket
obtained from VHOST_USER_GPU_SET_SOCKET. This also introduces a new
feature flag `gpu-socket` in the vhost crate.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
The VHOST_USER_GPU_SET_SOCKET is only handled when the feature gpu-socket
is enabled. This also introduces a GpuBackend for handling comunication
over the socket.
Signed-off-by: Matej Hrica <mhrica@redhat.com>
Introduce another constant MAX_MSG_SIZE that is part of the MsgHeader
trait. For now this is only used for sending messages.
Consider using the more specific trait constant everywhere.

The VHOST_USER_GPU_UPDATE and VHOST_USER_GPU_CURSOR_UPDATE contain image
data and are larger than the existing MAX_MSG_SIZE.

The existing MAX_MSG_SIZE wasn't really a limitation of the protocol,
just an implementation detail limitation in this crate.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
Add a get_display_info() method and the related reply structs to send
VHOST_USER_GPU_GET_DISPLAY_INFO and receive the reply.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
Signed-off-by: Matej Hrica <mhrica@redhat.com>
Add a method and related structs to send VHOST_USER_GPU_GET_EDID and
receive the reply.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
Add a method set_scanout() that sends VHOST_USER_GPU_SCANOUT and doesn't
wait for a reply.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
Add a method and related struct to send VHOST_USER_GPU_UPDATE.

The data part of the message is not part of the struct like sugested by
the spec but a separate argument to update_scanout. This is necessary
because of limitations of having an unsized array inside of struct in Rust.
But this aproach seems preferable anyway, because it allows the consumer of
the crate to store the array in a diferent location than the struct.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
Signed-off-by: Matej Hrica <mhrica@redhat.com>
Add methods and related structs to send the VHOST_USER_GPU_DMABUF_SCANOUT
and VHOST_USER_GPU_DMABUF_UPDATE update messages.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
Add get_protocol_features method to send
VHOST_USER_GPU_GET_PROTOCOL_FEATURES and receive the reply. Also add
a method to send SET_PROTOCOL_FEATURES.
Introduce VhostUserGpuProtocolFeatures bitmap that defines possible feature
flags.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
Add methods to send cursor related messages: VHOST_USER_GPU_CURSOR_POS,
VHOST_USER_GPU_CURSOR_POS_HIDE and VHOST_USER_GPU_CURSOR_UPDATE.

VhostUserGpuCursorUpdate's `data` field is passed as a separate argument
into cursor_update method. The type is also an u8 array instead of u32
array like in the spec. Having the type be u8 array makes it easier to work
with the data without unsafe code for the consumer of this crate. Having it
be a separate argument is useful, because the user of the method doesn't
have to copy the array into a struct.

Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
Add a method and related struct to send the VHOST_USER_GPU_DMABUF_SCANOUT2
message.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
Adds links to PR in CHANGELOG.md for both vhost and vhost-user-backend.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
Compile and test with `gpu-socket` feature enabled.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
@stefano-garzarella
Copy link
Member

@germag @slp can you take a look?

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

4 participants