From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.6 Received: from atuin.qyliss.net (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id 7EFFD7CCFD; Wed, 28 Sep 2022 17:01:53 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 496) id 246657CC45; Wed, 28 Sep 2022 17:01:43 +0000 (UTC) Received: from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com [IPv6:2a00:1450:4864:20::62b]) by atuin.qyliss.net (Postfix) with ESMTPS id 3FCF67CC27 for ; Wed, 28 Sep 2022 17:01:40 +0000 (UTC) Received: by mail-ej1-x62b.google.com with SMTP id lh5so28460631ejb.10 for ; Wed, 28 Sep 2022 10:01:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=unikie.com; s=google; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date; bh=N5MSHkKadxTYvpd8ycn7M1gDt3Hd8HBKZswk7D+JB6M=; b=dgUHmSYzHpH38aNgKLXLPanu0FBxRf15v7JXmysKlpdjWGCJ5MzmkwJ6WnkPAta1KT iOD9D1scGOpLMD3HQKy2bn0LVPUOaEUHsSNTKOtXJ6IIDaVb5KKAhyFibJOq4N60F4HU GMt0rG0FfJ7BFpwVSuw0VaHdwcPiEClv7OBmTUivsTlceWFUwxZGvpOu3WPsEcopiqYm 2vPbRqBLYYFOtEeyx94npG6T0k8VdUMAXaREBedVnnQBzSHx1E0YIVgzT2m8H+s82kEC /Lg8a7ZAfigCkWoAjKcSr3ynJtlgpKWaS4ZFTK8XN/OmlgEmMh3XBlduJGIwilwkpblq WnJw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date; bh=N5MSHkKadxTYvpd8ycn7M1gDt3Hd8HBKZswk7D+JB6M=; b=XAb79FN5cLT3xT35HLRMVqdMfJFsPJ8wu0t5mSpDeXl4bZeIxG98QABPsVEyRRW8oN pTJMBOW11jtCbo1bm54jmyh1vtXRmkD+ewruXhujUgNsO2lGz8tBP9M3hUdvCMBbp1jt xop5o0KokHJ/8gK5CBsavyyMZyQLdqS61mNIneJCRINelXTEuxL7jx+iLYjsR5wWpkCL A9Bsox4TNUSOLTTnYB+zHlXe9v3TigC1BF2/iY5PErVlrM2Rk21rY5iA0wHshuUjMfEd 2WUFeFOsYNRsFzZm0iSnEIK+JGlkJjxWM6lRgdYdmGBj+i+eXmozS6R73hRNgyPGR79A ZYOg== X-Gm-Message-State: ACrzQf0Y4fu/QGoSMx/gPer2UnoDYbFnawGVFwESDmOdKmwzBM5AJ96E 1d1U2WB/mixkWvd7iix09zdvmcTHmkiS0svI X-Google-Smtp-Source: AMsMyM4EPapYBPisxt6g39bHXDlj7Kd89wDkiX9BmQK9EslXQrH8kdLaEEdsGYnMezQ9e6sbQmMd1A== X-Received: by 2002:a17:907:a05:b0:77b:b538:6476 with SMTP id bb5-20020a1709070a0500b0077bb5386476mr29073331ejc.324.1664384499901; Wed, 28 Sep 2022 10:01:39 -0700 (PDT) Received: from x220.qyliss.net (p4feb786f.dip0.t-ipconnect.de. [79.235.120.111]) by smtp.gmail.com with ESMTPSA id kx6-20020a170907774600b007708635be05sm2711539ejc.4.2022.09.28.10.01.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 28 Sep 2022 10:01:38 -0700 (PDT) Received: by x220.qyliss.net (Postfix, from userid 1000) id C913F2E7; Wed, 28 Sep 2022 17:01:37 +0000 (UTC) From: Alyssa Ross To: devel@spectrum-os.org Subject: [RFC PATCH nixpkgs 5/9] crosvm: add fixes for cloud-hypervisor virtio-gpu Date: Wed, 28 Sep 2022 17:01:24 +0000 Message-Id: <20220928170128.1583791-6-alyssa.ross@unikie.com> X-Mailer: git-send-email 2.37.1 In-Reply-To: <20220928170128.1583791-1-alyssa.ross@unikie.com> References: <20220928170128.1583791-1-alyssa.ross@unikie.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: G6KPQSXV35ZTX5CSUAOU6FI5W3LWYI74 X-Message-ID-Hash: G6KPQSXV35ZTX5CSUAOU6FI5W3LWYI74 X-MailFrom: alyssa.ross@unikie.com X-Mailman-Rule-Hits: header-match-devel.spectrum-os.org-0 X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1 CC: Ville Ilvonen X-Mailman-Version: 3.3.5 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Signed-off-by: Alyssa Ross --- .../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 +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 +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, + backend: Box, + ops: O, +- shmid: Option, + } + + impl DeviceRequestHandler { +@@ -436,7 +435,6 @@ where + mem: None, + backend, + ops, +- shmid: None, + } + } + } +@@ -696,7 +694,12 @@ impl VhostUserSlaveReqHandlerMut for DeviceRequestHandl + } + + fn set_slave_req_fd(&mut self, ep: Box>) { +- 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 VhostUserSlaveReqHandlerMut for DeviceRequestHandl + + fn get_shared_memory_regions(&mut self) -> VhostResult> { + 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 +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 VhostUserSlaveReqHandlerMut for DeviceRequestHandl + size: u32, + _flags: VhostUserConfigFlags, + ) -> VhostResult> { +- 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