vhost_user: Add support for SHMEM_MAP/UNMAP backend requests

[mtjhrc]

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 [email protected] 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.

[mtjhrc]

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]

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]

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]

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

[epilys]

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

https://lore.kernel.org/all/[email protected]/

[aesteve-rh]

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

[dorindabassey]

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

[stefano-garzarella]

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

https://lore.kernel.org/all/[email protected]/

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]

Is self.len == 0 valid?

[stefano-garzarella]

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 :-(

[epilys]

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?

[stefano-garzarella]

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.

[stefano-garzarella]

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 ;-)

[germag]

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

[mtjhrc]

@germag

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 don't see this being a problem here, we are not constructing uninitialized memory on the Rust side. The memory could be uninitiated on the sender side, but the sender writes the unilized bytes to a socket, and we override the whole ByteValued struct with the bytes received from the socket, hence we do initialize it (potentially to some unknown bytes from the socket though).

[mtjhrc]

@epilys

And make it NonZero<_> while we're at it...

I can't change the type to type of len field to NonZero<u64> in VhostUserMMap, because that would cause UB when other party sends us 0 in the len field.
(I could change it to Option<NonZero<u64>, which should be FFI compatible with u64, but that won't really help anything)

I mean we kind of need two msg structs for each msg, one struct for over the wire protocol, that can contain invalid msgs, and one for high level usage that cannot, doesn't have useless fields like padding, uses fancy Option, NonZero... types. (or just have one struct and make the fields private, add a safe builder + getters...). I think this is a more general problem that should probably be addressed more globally.

[epilys]

@mtjhrc good point. It's unnecessary, validation is fine also.

[mtjhrc]

I added the check that verifies that len cannot be zero. I also added a comment that we don't validate the padding because it's currently not specified in the spec.

[stefano-garzarella]

@mtjhrc LGTM, thanks for the work and the patience :-)