From: Alyssa Ross <alyssa.ross@unikie.com>
To: devel@spectrum-os.org
Cc: Puck Meerburg <puck@puckipedia.com>,
Ville Ilvonen <ville.ilvonen@unikie.com>
Subject: [RFC PATCH nixpkgs v2 5/9] crosvm: add fixes for cloud-hypervisor virtio-gpu
Date: Fri, 30 Sep 2022 21:09:02 +0000 [thread overview]
Message-ID: <20220930210906.1696349-6-alyssa.ross@unikie.com> (raw)
In-Reply-To: <20220930210906.1696349-1-alyssa.ross@unikie.com>
Signed-off-by: Alyssa Ross <alyssa.ross@unikie.com>
---
.../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
next prev parent reply other threads:[~2022-09-30 21:10 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-30 21:08 [RFC PATCH nixpkgs v2 0/9] virtio-gpu with crosvm and cloud-hypervisor Alyssa Ross
2022-09-30 21:08 ` [RFC PATCH nixpkgs v2 1/9] crosvm: switch back to old git repo URL Alyssa Ross
2022-09-30 21:08 ` [RFC PATCH nixpkgs v2 2/9] crosvm.updateScript: update release branch format Alyssa Ross
2022-09-30 21:09 ` [RFC PATCH nixpkgs v2 3/9] crosvm: 104.0 -> 106.2 Alyssa Ross
2022-09-30 21:09 ` [RFC PATCH nixpkgs v2 4/9] crosvm.updateScript: don't vendor Cargo.lock Alyssa Ross
2022-09-30 21:09 ` Alyssa Ross [this message]
2022-09-30 21:09 ` [RFC PATCH nixpkgs v2 6/9] rustPlatform: forward unpack hooks to cargo fetch Alyssa Ross
2022-09-30 21:09 ` [RFC PATCH nixpkgs v2 7/9] cloud-hypervisor: add virtio-gpu support Alyssa Ross
2022-09-30 21:09 ` [RFC PATCH nixpkgs v2 8/9] ocamlPackages.wayland: 1.0 -> 1.1 Alyssa Ross
2022-09-30 21:12 ` [RFC PATCH nixpkgs v2 9/9] wayland-proxy-virtwl: unstable-2021-12-05 -> unstable-2022-09-22 Alyssa Ross
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220930210906.1696349-6-alyssa.ross@unikie.com \
--to=alyssa.ross@unikie.com \
--cc=devel@spectrum-os.org \
--cc=puck@puckipedia.com \
--cc=ville.ilvonen@unikie.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://spectrum-os.org/git/crosvm
https://spectrum-os.org/git/doc
https://spectrum-os.org/git/mktuntap
https://spectrum-os.org/git/nixpkgs
https://spectrum-os.org/git/spectrum
https://spectrum-os.org/git/ucspi-vsock
https://spectrum-os.org/git/www
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).