rust-vmm / vhost

Apache License 2.0
126 stars 64 forks source link

Add support for VHOST_USER_GPU_SET_SOCKET #239

Open mtjhrc opened 2 months ago

mtjhrc commented 2 months ago

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. https://github.com/rust-vmm/vhost/blob/bd6bf1325b4930d4123f6154ce755bd5289c617b/vhost/src/vhost_user/message.rs#L26-L33

Requirements

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

mtjhrc commented 1 month ago

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 commented 1 month ago

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.

mtjhrc commented 4 weeks ago

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 commented 4 weeks ago

@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 commented 4 weeks ago

@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 commented 1 week ago

@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.

stefano-garzarella commented 12 hours ago

@germag @slp can you take a look?