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_user: Add support for SHMEM_MAP/UNMAP backend requests #251

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mtjhrc
Copy link
Contributor

@mtjhrc mtjhrc commented Aug 27, 2024

Summary of the PR

This PR adds support for backend requests SHMEM_MAP and SHMEM_UNMAP.
This allows the device to memory map a file descriptor into a Shared Memory Region.

The SHMEM_MAP and SHMEM_UNMAP are part of this QEMU RFC:

[RFC PATCH v2 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests
https://mail.gnu.org/archive/html/qemu-devel/2024-06/msg05736.html

I don't anticipate these specific requests to change much, the discussion for this RFC is mainly about a situation where one device is accessing shared memory mapped by another device.

These requests are required for implementing blob resource support in vhost-user-gpu.
The current gpu PR, doesn't implement blob resources and as such doesn't require this, but I am working on creating another PR with blob resource support.

This is based on a commit by Albert Esteve aesteve@redhat.com in his branch/fork.

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.

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.

Author and SoB are different, can you fix it, or add @aesteve-rh SoB if he is a co-author.

Please check also the coverage and add a line in the changelog.

vhost/src/vhost_user/backend_req.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/frontend_req_handler.rs Show resolved Hide resolved
vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved
vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved
Extract creating a backend and a frontend from a socketpair to a utility
function.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
@mtjhrc mtjhrc force-pushed the shmem branch 13 times, most recently from 80c4bf1 to 630cd66 Compare August 28, 2024 15:06
@mtjhrc mtjhrc marked this pull request as ready for review August 28, 2024 15:16
@mtjhrc
Copy link
Contributor Author

mtjhrc commented Aug 28, 2024

I have addressed the feedback, you can take another look.

Also, should I add some comment that this feature is not really "stable"? Or will this PR have to wait until it gets merged in QEMU?

@mtjhrc
Copy link
Contributor Author

mtjhrc commented Aug 28, 2024

The coverage was too low, so I wanted to add a test for the default impl of the frontend request handlers, but I also added coverage for all the methods, since that seemed quite trivial. This has increased the coverage too much, so I had to increase the coverage config value.

@stefano-garzarella
Copy link
Member

The code LGTM, but before merging this PR, I'd like to wait at least the spec merged in QEMU.
We recently removed some code that was never merged in the spec, so I'd like to avoid making the same mistake.
If the QEMU RFC still requires discussion about the code, maybe we can try to merge at least the changes to the specification so we can move forward here as well.

WDYT?

@mtjhrc
Copy link
Contributor Author

mtjhrc commented Aug 29, 2024

I understand the concern. I'm talking to @aesteve-rh to see if we can figure something out

@mtjhrc mtjhrc force-pushed the shmem branch 2 times, most recently from cd80826 to 5f05fec Compare August 29, 2024 13:53
Add request defintions and methods for using the new SHMEM_MAP/UNMAP
backend requests.

Note that at the time of writing this, these requests are part of this RFC
in QEMU:
https://mail.gnu.org/archive/html/qemu-devel/2024-06/msg05736.html

Co-authored-by: Albert Esteve <aesteve@redhat.com>
Co-authored-by: Matej Hrica <mhrica@redhat.com>

Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Matej Hrica <mhrica@redhat.com>
Add tests to assert return values (and existence) of default implemenation
of VhostUserFrontendReqHandler and VhostUserFrontendReqHandlerMut trait
methods.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
Signed-off-by: Matej Hrica <mhrica@redhat.com>
Comment on lines +1034 to +1037
(self.flags & !VhostUserMMapFlags::all().bits()) == 0
&& self.fd_offset.checked_add(self.len).is_some()
&& self.shm_offset.checked_add(self.len).is_some()
}

Choose a reason for hiding this comment

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

do you want to add more checks relevant to the struct? e.g a check based on valid values for shmid?

Copy link
Contributor Author

@mtjhrc mtjhrc Sep 3, 2024

Choose a reason for hiding this comment

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

From virtio spec:

A device may have multiple shared memory regions associated with it. Each region has a shmid to identify it, the meaning of which is device-specific.

According to the virtio (not vhost) spec the valid values of shmid are dependent on the device type (And probably the advertised device features...), so I don't think you could check it here generally.

Choose a reason for hiding this comment

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

ok, fair enough for shmid, you can leave it as it is.


fn shmem_map(&mut self, req: &VhostUserMMap, _fd: &dyn AsRawFd) -> HandlerResult<u64> {
assert_eq!({ req.shmid }, 0);
Ok(!self.shmem_mappings.insert((req.shm_offset, req.len)) as u64)

Choose a reason for hiding this comment

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

i think it's better to remove the negation (!) and use a direct check to return meaningful values. e.g if self.shmem_mappings.insert(req.shm_offset, req.len) { Ok(1) } else { Ok(0) } and why the double braces? missing &?

Copy link
Contributor Author

@mtjhrc mtjhrc Sep 3, 2024

Choose a reason for hiding this comment

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

By braces do you mean ( ) or { }? The (req.shm_offset, req.len) is a tuple.

In regards to the negation with cast instead of explicit if, I considered to write it like you did, I mainly wrote it like this, because it is symmetric with the existing shared_object_add, shared_object_remove above:

fn shared_object_add(&mut self, uuid: &VhostUserSharedMsg) -> HandlerResult<u64> {
    Ok(!self.shared_objects.insert(uuid.uuid) as u64)
}
fn shared_object_remove(&mut self, uuid: &VhostUserSharedMsg) -> HandlerResult<u64> {
    Ok(!self.shared_objects.remove(&uuid.uuid) as u64)
}

I am not sure if I should change it.

Choose a reason for hiding this comment

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

The (req.shm_offset, req.len) is a tuple.

ah right!

because it is symmetric with the existing shared_object_add, shared_object_remove above:

doesn't necessarily mean it's the correct approach.


fn shmem_unmap(&mut self, req: &VhostUserMMap) -> HandlerResult<u64> {
assert_eq!({ req.shmid }, 0);
Ok(!self.shmem_mappings.remove(&(req.shm_offset, req.len)) as u64)

Choose a reason for hiding this comment

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

same here

@dorindabassey
Copy link

The code LGTM, but before merging this PR, I'd like to wait at least the spec merged in QEMU. We recently removed some code that was never merged in the spec, so I'd like to avoid making the same mistake. If the QEMU RFC still requires discussion about the code, maybe we can try to merge at least the changes to the specification so we can move forward here as well.

WDYT?

are you talking about the vhost-user specification https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#message-specification?
If so, I think it make sense to atleast have just the spec merged, since the patches are still RFC it might take a while before it's merged.

@stefano-garzarella
Copy link
Member

The code LGTM, but before merging this PR, I'd like to wait at least the spec merged in QEMU. We recently removed some code that was never merged in the spec, so I'd like to avoid making the same mistake. If the QEMU RFC still requires discussion about the code, maybe we can try to merge at least the changes to the specification so we can move forward here as well.
WDYT?

are you talking about the vhost-user specification https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#message-specification?

@dorindabassey yep

If so, I think it make sense to atleast have just the spec merged, since the patches are still RFC it might take a while before it's merged.

yeah, if it's possible, I'd go in that direction.
So, I'm marking this PR as draft to be sure we don't merge this till we have the spec changes merged in QEMU.

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.

5 participants