vhost_user: Add support for SHMEM_MAP/UNMAP backend requests #251

mtjhrc · 2024-08-27T15:35:25Z

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.

stefano-garzarella

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

vhost/src/vhost_user/frontend_req_handler.rs

vhost/src/vhost_user/message.rs

mtjhrc · 2024-08-28T15:27:32Z

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 · 2024-08-28T15:31:45Z

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 · 2024-08-28T15:35:01Z

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 · 2024-08-29T09:14:11Z

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

vhost/src/vhost_user/message.rs

epilys

LGTM, a couple of non-code related nits

vhost/CHANGELOG.md

vhost/src/vhost_user/backend_req.rs

mtjhrc · 2025-11-27T12:23:12Z

Note that I changed the VhostUserMMapFlags:

now you should be able to use VhostUserMMapFlags::default() or VhostUserMMapFlags::empty() for just readable mapping
VhostUserMMapFlags::WRITABLE = 1 << 0 for writable.
I don't have a READABLE = 0 flag here, because that seemed weird to have a bitmask flag be zero (and also bitflags crate warns about it).

epilys · 2025-11-27T12:31:25Z

I don't have a READABLE = 0 flag here, because that seemed weird to have a bitmask flag be zero (and also bitflags crate warns about it).

Generally with bitflags crate you can do this outside of the bitflags macro call (since empty() is const)

impl VhostUserMMapFlags {
    pub const READABLE: Self = Self::empty();
}

But I don't think it's necessary here, it looks okay currently.

aesteve-rh

LGTM. Thank you, @mtjhrc !

stefano-garzarella · 2025-11-27T12:46:07Z

@aesteve-rh what is the status of the spec?
I can't see the new messages in https://qemu-project.gitlab.io/qemu/interop/vhost-user.html

epilys · 2025-11-27T12:47:52Z

It's been reviewed completely, waiting for a maintainer to pick it up.

https://lore.kernel.org/all/20251111091058.879669-1-aesteve@redhat.com/

aesteve-rh · 2025-11-27T12:56:45Z

What @epilys said, just waiting. The last PULL from mst happened during QEMU freeze, but it should get picked for the next round.

vhost/src/vhost_user/message.rs

dorindabassey · 2025-11-27T13:20:12Z

Thank you @mtjhrc for the work on this PR, just a minor comment otherwise LGTM.

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>

The last commit adds tests, that cover all of the default impls of functions, causing the coverage to increase too much - bump the value manually. Signed-off-by: Matej Hrica <mhrica@redhat.com>

stefano-garzarella · 2025-11-27T13:36:39Z

It's been reviewed completely, waiting for a maintainer to pick it up.

https://lore.kernel.org/all/20251111091058.879669-1-aesteve@redhat.com/

What @epilys said, just waiting. The last PULL from mst happened during QEMU freeze, but it should get picked for the next round.

@epilys @aesteve-rh great, thanks!
So I guess it'is very low risk to merge this before the spec.

@mtjhrc thanks for the work, I'll take a look hopefully tomorrow or on monday.

stefano-garzarella · 2025-12-02T08:41:20Z

vhost/src/vhost_user/message.rs

+    /// Flags specifying access permissions of memory mapping of a file
+    /// Use VhostUserMMapFlags::default() for a read-only mapping
+    pub struct VhostUserMMapFlags: u64 {
+        /// Read-write permission


nit: /// Read-write permission (0: read-only, 1: read-write)

The comment is for WRITABLE constant, which is both read-write. Maybe (0: read-only, 1: read-write) could go in the struct doc?

ah, I thought was a bitfields.

Or maybe /// Read-write permission (if not set, it's read-only), but I see we already have something in the struct doc, so really not a strong opinion on this, feel free to do nothing ;-)

I mean what's best here, depends how the protocol evolves. I could theoretically imagine it becoming a real bitfield in the future with more options.

stefano-garzarella · 2025-12-02T08:47:34Z

vhost/src/vhost_user/backend_req.rs

+
+    /// Forward vhost-user memory map file request to the frontend.
+    fn shmem_map(&self, req: &VhostUserMMap, fd: &dyn AsRawFd) -> HandlerResult<u64> {
+        self.send_message(BackendReq::SHMEM_MAP, req, Some(&[fd.as_raw_fd()]))


In shared_object_*() functions above we check if the related feature is negotiated, should we do something similar also here?

stefano-garzarella · 2025-12-02T08:50:03Z

vhost/src/vhost_user/message.rs

I guess we also need to add SHMEM to VhostUserProtocolFeatures as defined in https://lore.kernel.org/all/20251111091058.879669-4-aesteve@redhat.com/:

@@ -1064,6 +1086,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_XEN_MMAP 17 #define VHOST_USER_PROTOCOL_F_SHARED_OBJECT 18 #define VHOST_USER_PROTOCOL_F_DEVICE_STATE 19 + #define VHOST_USER_PROTOCOL_F_SHMEM 20 Front-end message types -----------------------

stefano-garzarella · 2025-12-02T08:52:45Z

vhost/src/vhost_user/message.rs

+// SAFETY: Safe because all fields of VhostUserMMap are POD.
+unsafe impl ByteValued for VhostUserMMap {}
+
+impl VhostUserMsgValidator for VhostUserMMap {


Should we add a test for this, as we do for other VhostUserMsgValidator implementations (e.g. check_user_memory(), check_user_memory_region(), etc.)

stefano-garzarella · 2025-12-02T08:56:19Z

vhost/src/vhost_user/message.rs

+
+impl VhostUserMsgValidator for VhostUserMMap {
+    fn is_valid(&self) -> bool {
+        VhostUserMMapFlags::from_bits(self.flags).is_some()


Is self.len == 0 valid?

Should we also check if padding is set to 0 like we do in VhostUserMemory ?
This is more for the future, where we can re-use the padding for something else. That said, in the spec I don't see any push on that :-(

Is self.len == 0 valid?

It's not mentioned in the spec text, but in the current patches in QEMU it's rejected:

+VirtioSharedMemoryMapping *virtio_shared_memory_mapping_new(uint8_t shmid, + int fd, + uint64_t fd_offset, + uint64_t shm_offset, + uint64_t len, + bool allow_write) +{ + VirtioSharedMemoryMapping *mapping; + MemoryRegion *mr; + g_autoptr(GString) mr_name = g_string_new(NULL); + uint32_t ram_flags; + Error *local_err = NULL; + + if (len == 0) { + error_report("Shared memory mapping size cannot be zero"); + return NULL; + } +

Should we also check if padding is set to 0 like we do in VhostUserMemory ?

Sounds too strict IMHO... what if padding is filled with garbage bytes but the rest of the message is valid?

IMO in the spec we should say that padding must be 0, so in the future we can potentially use it for something else. At least, this is what we usually do in Linux uAPI.

IMO in the spec we should say that padding must be 0, so in the future we can potentially use it for something else. At least, this is what we usually do in Linux uAPI.

Just to be clear, this is not a blocker for this PR ;-)

@aesteve-rh thanks for that. If it's supposed to go into the spec, IMHO we can implement that already here. In the worst case, we can relax the requirement here if the spec will not require that.

sorry if I'm misunderstanding this, I'm late for this, but uninit padding is not ok, it's a source of UB in rust, specially if the struct is ByteValued, we already had that problem (and padding is not the same that volatile memory)

I think I'm also late to ask for packed in the spec/qemu... I have in my TODO to fix a couple of struct because of that

@germag for the spec part, would that be ok if I address that in a follow up? The patch has not been merged but after how long it took to have it ready, I do not want to cause delays. I have a few other comments to address, so I will make sure to send it and put you on cc.

@germag for the spec part, would that be ok if I address that in a follow up? The patch has not been merged but after how long it took to have it ready, I do not want to cause delays. I have a few other comments to address, so I will make sure to send it and put you on cc.

Yes, I think it's fair, it was my fault not reviewing it on time, thanks!

stefano-garzarella reviewed Aug 28, 2024

View reviewed changes

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

mtjhrc force-pushed the shmem branch 13 times, most recently from 80c4bf1 to 630cd66 Compare August 28, 2024 15:06

mtjhrc marked this pull request as ready for review August 28, 2024 15:16

mtjhrc requested review from Ablu, eryugey, germag, jiangliu, sboeuf and slp as code owners August 28, 2024 15:16

alyssais reviewed Aug 29, 2024

View reviewed changes

vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved

mtjhrc force-pushed the shmem branch 3 times, most recently from 5f05fec to bf692de Compare August 29, 2024 15:24

dorindabassey reviewed Sep 3, 2024

View reviewed changes

vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved

mtjhrc force-pushed the shmem branch 3 times, most recently from abfb3a3 to 903cabf Compare November 26, 2025 10:24

mtjhrc marked this pull request as ready for review November 26, 2025 11:57

mtjhrc requested a review from epilys as a code owner November 26, 2025 11:57

epilys requested changes Nov 26, 2025

View reviewed changes

vhost/CHANGELOG.md Outdated Show resolved Hide resolved

vhost/src/vhost_user/backend_req.rs Outdated Show resolved Hide resolved

mtjhrc force-pushed the shmem branch 3 times, most recently from 6164e22 to 2f600bd Compare November 27, 2025 12:21

epilys previously approved these changes Nov 27, 2025

View reviewed changes

epilys enabled auto-merge (rebase) November 27, 2025 12:36

epilys requested a review from stefano-garzarella November 27, 2025 12:36

aesteve-rh approved these changes Nov 27, 2025

View reviewed changes

dorindabassey reviewed Nov 27, 2025

View reviewed changes

vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved

aesteve-rh and others added 3 commits November 27, 2025 14:25

Add tests for default impls of Frontend req handlers

8ef77ef

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>

Bump test coverage value for x86_64

993d885

The last commit adds tests, that cover all of the default impls of functions, causing the coverage to increase too much - bump the value manually. Signed-off-by: Matej Hrica <mhrica@redhat.com>

auto-merge was automatically disabled November 27, 2025 13:25
Head branch was pushed to by a user without write access

mtjhrc dismissed epilys’s stale review via 993d885 November 27, 2025 13:25

mtjhrc force-pushed the shmem branch from 2f600bd to 993d885 Compare November 27, 2025 13:25

epilys approved these changes Nov 27, 2025

View reviewed changes

dorindabassey approved these changes Nov 27, 2025

View reviewed changes

stefano-garzarella requested changes Dec 2, 2025

View reviewed changes

vhost_user: Add support for SHMEM_MAP/UNMAP backend requests #251

Are you sure you want to change the base?

vhost_user: Add support for SHMEM_MAP/UNMAP backend requests #251

Conversation

mtjhrc commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the PR

Requirements

Uh oh!

stefano-garzarella left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtjhrc commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtjhrc commented Aug 28, 2024

Uh oh!

stefano-garzarella commented Aug 28, 2024

Uh oh!

mtjhrc commented Aug 29, 2024

Uh oh!

Uh oh!

Uh oh!

epilys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mtjhrc commented Nov 27, 2025

Uh oh!

epilys commented Nov 27, 2025

Uh oh!

aesteve-rh left a comment

Choose a reason for hiding this comment

Uh oh!

stefano-garzarella commented Nov 27, 2025

Uh oh!

epilys commented Nov 27, 2025

Uh oh!

aesteve-rh commented Nov 27, 2025

Uh oh!

Uh oh!

dorindabassey commented Nov 27, 2025

Uh oh!

stefano-garzarella commented Nov 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

germag Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mtjhrc commented Aug 27, 2024 •

edited

Loading

mtjhrc commented Aug 28, 2024 •

edited

Loading

germag Dec 3, 2025 •

edited

Loading

aesteve-rh Dec 4, 2025 •

edited

Loading

germag Dec 4, 2025 •

edited

Loading