[RFC PATCH nixpkgs 5/9] crosvm: add fixes for cloud-hypervisor virtio-gpu

Alyssa Ross
2022-09-28T17:01:53Z

.../virtualization/crosvm/default.nix | 4 ++
...-consider-shm-buuffers-when-setting-.patch | 34 +++++++++
...t_user-loosen-expected-message-order.patch | 71 +++++++++++++++++++
...ces-vhost_user-remove-spurious-check.patch | 42 +++++++++++
4 files changed, 151 insertions(+)
create mode 100644 pkgs/applications/virtualization/crosvm/devices-properly-consider-shm-buuffers-when-setting-.patch
create mode 100644 pkgs/applications/virtualization/crosvm/devices-vhost_user-loosen-expected-message-order.patch
create mode 100644 pkgs/applications/virtualization/crosvm/devices-vhost_user-remove-spurious-check.patch

diff --git a/pkgs/applications/virtualization/crosvm/default.nix b/pkgs/applications/virtualization/crosvm/default.nix
index 311a2a63ae2..40e30dcd819 100644
--- a/pkgs/applications/virtualization/crosvm/default.nix
+++ b/pkgs/applications/virtualization/crosvm/default.nix
@@ -18,6 +18,10 @@ rustPlatform.buildRustPackage rec {

patches = [
./default-seccomp-policy-dir.diff
+
+ ./devices-properly-consider-shm-buuffers-when-setting-.patch
+ ./devices-vhost_user-remove-spurious-check.patch
+ ./devices-vhost_user-loosen-expected-message-order.patch
];

cargoSha256 = "18mj0zc6yfwyrw6v1vl089dhh04kv2pzb99bygnn8nymdlx4fjqa";
diff --git a/pkgs/applications/virtualization/crosvm/devices-properly-consider-shm-buuffers-when-setting-.patch b/pkgs/applications/virtualization/crosvm/devices-properly-consider-shm-buuffers-when-setting-.patch
new file mode 100644
index 00000000000..2f0f95c532a
--- /dev/null
+++ b/pkgs/applications/virtualization/crosvm/devices-properly-consider-shm-buuffers-when-setting-.patch
@@ -0,0 +1,34 @@
+From f5afb62e138017f28966d8fa20b44fe00f301d7d Mon Sep 17 00:00:00 2001
+From: Puck Meerburg <puck@puckipedia.com>
+Date: Mon, 26 Sep 2022 22:40:53 +0000
+Subject: [PATCH crosvm 1/3] devices: properly consider shm buuffers when
+ setting gpu_blob flag
+
+---
+ devices/src/virtio/gpu/virtio_gpu.rs | 3 ++-
+ 1 file changed, 2 insertions(+), 1 deletion(-)
+
+diff --git a/devices/src/virtio/gpu/virtio_gpu.rs b/devices/src/virtio/gpu/virtio_gpu.rs
+index 698215727a..ed97942f7a 100644
+--- a/devices/src/virtio/gpu/virtio_gpu.rs
++++ b/devices/src/virtio/gpu/virtio_gpu.rs
+@@ -26,6 +26,7 @@ use rutabaga_gfx::RutabagaHandle;
+ use rutabaga_gfx::RutabagaIovec;
+ use rutabaga_gfx::Transfer3D;
+ use rutabaga_gfx::RUTABAGA_MEM_HANDLE_TYPE_DMABUF;
++use rutabaga_gfx::RUTABAGA_MEM_HANDLE_TYPE_SHM;
+ use sync::Mutex;
+ use vm_control::VmMemorySource;
+ use vm_memory::udmabuf::UdmabufDriver;
+@@ -731,7 +732,7 @@ impl VirtioGpu {
+ descriptor: export.os_handle,
+ offset: 0,
+ size: resource.size,
+- gpu_blob: true,
++ gpu_blob: export.handle_type != RUTABAGA_MEM_HANDLE_TYPE_SHM,
+ },
+ }
+ } else {
+--
+2.37.1
+
diff --git a/pkgs/applications/virtualization/crosvm/devices-vhost_user-loosen-expected-message-order.patch b/pkgs/applications/virtualization/crosvm/devices-vhost_user-loosen-expected-message-order.patch
new file mode 100644
index 00000000000..bcfab6eace1
--- /dev/null
+++ b/pkgs/applications/virtualization/crosvm/devices-vhost_user-loosen-expected-message-order.patch
@@ -0,0 +1,71 @@
+From 6ac11f04ea3344ca0c3873ee9526a8863c9529ae Mon Sep 17 00:00:00 2001
+From: Alyssa Ross <alyssa.ross@unikie.com>
+Date: Wed, 28 Sep 2022 15:27:39 +0000
+Subject: [PATCH crosvm 3/3] devices: vhost_user: loosen expected message order
+
+Prior to this patch, the code assumed get_shared_memory_regions()
+would be called before set_slave_req_fd(), since
+get_shared_memory_regions() was the only place that set self.shmid,
+and set_slave_req_fd() required it to be set. There's nothing in the
+vhost-user spec that enforces this ordering though, so it could fail
+when used with a non-crosvm frontend.
+
+To fix this, just don't store the shmid here at all, and fetch it from
+the backend when it's required. set_slave_req_fd() should only be
+called once, and the two backends that implement
+get_shared_memory_region() (Wl and Gpu) both have trivial
+implementations, so there shouldn't be any performance reason to cache
+shmid here.
+
+TEST=Run cloud-hypervisor against a crosvm vhost-user backend
+
+Change-Id: Idb44aeb1dfe1aeff70081987fe6d7d215887d2e4
+---
+ devices/src/virtio/vhost/user/device/handler.rs | 10 ++++++----
+ 1 file changed, 6 insertions(+), 4 deletions(-)
+
+diff --git a/devices/src/virtio/vhost/user/device/handler.rs b/devices/src/virtio/vhost/user/device/handler.rs
+index 932d948959..27b4eb1619 100644
+--- a/devices/src/virtio/vhost/user/device/handler.rs
++++ b/devices/src/virtio/vhost/user/device/handler.rs
+@@ -408,7 +408,6 @@ where
+ mem: Option<GuestMemory>,
+ backend: Box<dyn VhostUserBackend>,
+ ops: O,
+- shmid: Option<u8>,
+ }
+
+ impl DeviceRequestHandler<VhostUserRegularOps> {
+@@ -436,7 +435,6 @@ where
+ mem: None,
+ backend,
+ ops,
+- shmid: None,
+ }
+ }
+ }
+@@ -696,7 +694,12 @@ impl<O: VhostUserPlatformOps> VhostUserSlaveReqHandlerMut for DeviceRequestHandl
+ }
+
+ fn set_slave_req_fd(&mut self, ep: Box<dyn Endpoint<SlaveReq>>) {
+- let shmid = self.shmid.expect("unexpected slave_req_fd");
++ let shmid = self
++ .backend
++ .get_shared_memory_region()
++ .expect("unexpected slave_req_fd")
++ .id;
++
+ let frontend = Slave::new(ep);
+ self.backend
+ .set_shared_memory_mapper(Box::new(VhostShmemMapper {
+@@ -738,7 +741,6 @@ impl<O: VhostUserPlatformOps> VhostUserSlaveReqHandlerMut for DeviceRequestHandl
+
+ fn get_shared_memory_regions(&mut self) -> VhostResult<Vec<VhostSharedMemoryRegion>> {
+ Ok(if let Some(r) = self.backend.get_shared_memory_region() {
+- self.shmid = Some(r.id);
+ vec![VhostSharedMemoryRegion::new(r.id, r.length)]
+ } else {
+ Vec::new()
+--
+2.37.1
+
diff --git a/pkgs/applications/virtualization/crosvm/devices-vhost_user-remove-spurious-check.patch b/pkgs/applications/virtualization/crosvm/devices-vhost_user-remove-spurious-check.patch
new file mode 100644
index 00000000000..e8ea4db2357
--- /dev/null
+++ b/pkgs/applications/virtualization/crosvm/devices-vhost_user-remove-spurious-check.patch
@@ -0,0 +1,42 @@
+From e895a064f24d0101a230790bdd6adff6cda898d5 Mon Sep 17 00:00:00 2001
+From: Alyssa Ross <alyssa.ross@unikie.com>
+Date: Wed, 28 Sep 2022 15:19:26 +0000
+Subject: [PATCH crosvm 2/3] devices: vhost_user: remove spurious check
+
+"size" is the amount of data the caller wants to read, not the size of
+the data available to read, so this check doesn't make any sense.
+It's completely valid to read 4 bytes of a 16 byte config space,
+starting at offset 8, but that would fail this check. crosvm doesn't
+seem to do this, but cloud-hypervisor does, so this caused crashes
+when running cloud-hypervisor against a crosvm vhost-user backend.
+
+I suspect what this code meant to do is check whether offset + size
+would be beyond the end of the config space, but in this part of the
+code we don't know the size of the config space, so it's not possible
+to check that here.
+
+TEST=Run cloud-hypervisor against a crosvm vhost-user backend
+
+Change-Id: I8a3d7960fb67bf8de37cb3f158081d6421859725
+---
+ devices/src/virtio/vhost/user/device/handler.rs | 4 ----
+ 1 file changed, 4 deletions(-)
+
+diff --git a/devices/src/virtio/vhost/user/device/handler.rs b/devices/src/virtio/vhost/user/device/handler.rs
+index 32e4aaf876..932d948959 100644
+--- a/devices/src/virtio/vhost/user/device/handler.rs
++++ b/devices/src/virtio/vhost/user/device/handler.rs
+@@ -680,10 +680,6 @@ impl<O: VhostUserPlatformOps> VhostUserSlaveReqHandlerMut for DeviceRequestHandl
+ size: u32,
+ _flags: VhostUserConfigFlags,
+ ) -> VhostResult<Vec<u8>> {
+- if offset >= size {
+- return Err(VhostError::InvalidParam);
+- }
+-
+ let mut data = vec![0; size as usize];
+ self.backend.read_config(u64::from(offset), &mut data);
+ Ok(data)
+--
+2.37.1
+
--
2.37.1