| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1645c8cd-6c82-7a9c-803f-a92160a1f2b8@linaro.org>
Date: Mon, 5 Jun 2023 14:48:32 -0500
From: Alex Elder <elder@...aro.org>
To: Elliot Berman <quic_eberman@...cinc.com>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>
Cc: Murali Nalajala <quic_mnalajal@...cinc.com>,
Trilok Soni <quic_tsoni@...cinc.com>,
Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
Carl van Schaik <quic_cvanscha@...cinc.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Jonathan Corbet <corbet@....net>,
Bagas Sanjaya <bagasdotme@...il.com>,
Will Deacon <will@...nel.org>, Andy Gross <agross@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Jassi Brar <jassisinghbrar@...il.com>,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v13 12/24] samples: Add sample userspace Gunyah VM Manager
On 5/9/23 3:47 PM, Elliot Berman wrote:
> Add a sample Gunyah VMM capable of launching a non-proxy scheduled VM.
>
> Signed-off-by: Elliot Berman <quic_eberman@...cinc.com>
I haven't tested this, but I trust it works.
I have some trivial comments, but otherwise:
Reviewed-by: Alex Elder <elder@...aro.org>
> ---
> samples/Kconfig | 10 ++
> samples/Makefile | 1 +
> samples/gunyah/.gitignore | 2 +
> samples/gunyah/Makefile | 6 +
> samples/gunyah/gunyah_vmm.c | 270 +++++++++++++++++++++++++++++++++++
> samples/gunyah/sample_vm.dts | 68 +++++++++
> 6 files changed, 357 insertions(+)
> create mode 100644 samples/gunyah/.gitignore
> create mode 100644 samples/gunyah/Makefile
> create mode 100644 samples/gunyah/gunyah_vmm.c
> create mode 100644 samples/gunyah/sample_vm.dts
>
> diff --git a/samples/Kconfig b/samples/Kconfig
> index b2db430bd3ff..567c7a706c01 100644
> --- a/samples/Kconfig
> +++ b/samples/Kconfig
> @@ -280,6 +280,16 @@ config SAMPLE_KMEMLEAK
> Build a sample program which have explicitly leaks memory to test
> kmemleak
>
> +config SAMPLE_GUNYAH
> + bool "Build example Gunyah Virtual Machine Manager"
> + depends on CC_CAN_LINK && HEADERS_INSTALL
> + depends on GUNYAH
> + help
> + Build an example Gunyah VMM userspace program capable of launching
> + a basic virtual machine under the Gunyah hypervisor.
> + This demonstrates how to create a virtual machine under the Gunyah
> + hypervisor.
I think you can drop the second sentence above. Perhaps adjust the
first a bit if you think the second adds anything important.
> +
> source "samples/rust/Kconfig"
>
> endif # SAMPLES
> diff --git a/samples/Makefile b/samples/Makefile
> index 7727f1a0d6d1..e1b92dec169f 100644
> --- a/samples/Makefile
> +++ b/samples/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_SAMPLE_KMEMLEAK) += kmemleak/
> obj-$(CONFIG_SAMPLE_CORESIGHT_SYSCFG) += coresight/
> obj-$(CONFIG_SAMPLE_FPROBE) += fprobe/
> obj-$(CONFIG_SAMPLES_RUST) += rust/
> +obj-$(CONFIG_SAMPLE_GUNYAH) += gunyah/
> diff --git a/samples/gunyah/.gitignore b/samples/gunyah/.gitignore
> new file mode 100644
> index 000000000000..adc7d1589fde
> --- /dev/null
> +++ b/samples/gunyah/.gitignore
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +/gunyah_vmm
> diff --git a/samples/gunyah/Makefile b/samples/gunyah/Makefile
> new file mode 100644
> index 000000000000..faf14f9bb337
> --- /dev/null
> +++ b/samples/gunyah/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +userprogs-always-y += gunyah_vmm
> +dtb-y += sample_vm.dtb
> +
> +userccflags += -I usr/include
> diff --git a/samples/gunyah/gunyah_vmm.c b/samples/gunyah/gunyah_vmm.c
> new file mode 100644
> index 000000000000..d0eb49e86372
> --- /dev/null
> +++ b/samples/gunyah/gunyah_vmm.c
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
Update the copyright.
> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <sys/ioctl.h>
> +#include <getopt.h>
> +#include <limits.h>
> +#include <stdint.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/sysmacros.h>
> +#define __USE_GNU
> +#include <sys/mman.h>
> +
> +#include <linux/gunyah.h>
> +
> +struct vm_config {
> + int image_fd;
> + int dtb_fd;
> + int ramdisk_fd;
> +
> + uint64_t guest_base;
> + uint64_t guest_size;
> +
> + uint64_t image_offset;
> + off_t image_size;
> + uint64_t dtb_offset;
> + off_t dtb_size;
> + uint64_t ramdisk_offset;
> + off_t ramdisk_size;
> +};
> +
> +static struct option options[] = {
> + { "help", no_argument, NULL, 'h' },
> + { "image", required_argument, NULL, 'i' },
> + { "dtb", required_argument, NULL, 'd' },
> + { "ramdisk", optional_argument, NULL, 'r' },
> + { "base", optional_argument, NULL, 'B' },
> + { "size", optional_argument, NULL, 'S' },
> + { "image_offset", optional_argument, NULL, 'I' },
> + { "dtb_offset", optional_argument, NULL, 'D' },
> + { "ramdisk_offset", optional_argument, NULL, 'R' },
> + { }
> +};
> +
> +static void print_help(char *cmd)
> +{
> + printf("gunyah_vmm, a sample tool to launch Gunyah VMs\n"
> + "Usage: %s <options>\n"
> + " --help, -h this menu\n"
> + " --image, -i <image> VM image file to load (e.g. a kernel Image) [Required]\n"
> + " --dtb, -d <dtb> Devicetree file to load [Required]\n"
> + " --ramdisk, -r <ramdisk> Ramdisk file to load\n"
> + " --base, -B <address> Set the base address of guest's memory [Default: 0x80000000]\n"
> + " --size, -S <number> The number of bytes large to make the guest's memory [Default: 0x6400000 (100 MB)]\n"
> + " --image_offset, -I <number> Offset into guest memory to load the VM image file [Default: 0x10000]\n"
> + " --dtb_offset, -D <number> Offset into guest memory to load the DTB [Default: 0]\n"
> + " --ramdisk_offset, -R <number> Offset into guest memory to load a ramdisk [Default: 0x4600000]\n"
> + , cmd);
You could define the default values above with symbolic constants,
and print them with 0x%08x in the messages above (or something
similar).
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int gunyah_fd, vm_fd, guest_fd;
> + struct gh_userspace_memory_region guest_mem_desc = { 0 };
> + struct gh_vm_dtb_config dtb_config = { 0 };
> + char *guest_mem;
> + struct vm_config config = {
> + /* Defaults good enough to boot static kernel and a basic ramdisk */
> + .ramdisk_fd = -1,
> + .guest_base = 0x80000000,
> + .guest_size = 0x6400000, /* 100 MB */
> + .image_offset = 0,
> + .dtb_offset = 0x45f0000,
> + .ramdisk_offset = 0x4600000, /* put at +70MB (30MB for ramdisk) */
> + };
> + struct stat st;
> + int opt, optidx, ret = 0;
> + long l;
> +
> + while ((opt = getopt_long(argc, argv, "hi:d:r:B:S:I:D:R:c:", options, &optidx)) != -1) {
> + switch (opt) {
> + case 'i':
> + config.image_fd = open(optarg, O_RDONLY | O_CLOEXEC);
> + if (config.image_fd < 0) {
> + perror("Failed to open image");
> + return -1;
> + }
> + if (stat(optarg, &st) < 0) {
> + perror("Failed to stat image");
> + return -1;
> + }
> + config.image_size = st.st_size;
> + break;
> + case 'd':
> + config.dtb_fd = open(optarg, O_RDONLY | O_CLOEXEC);
> + if (config.dtb_fd < 0) {
> + perror("Failed to open dtb");
> + return -1;
> + }
> + if (stat(optarg, &st) < 0) {
> + perror("Failed to stat dtb");
> + return -1;
> + }
> + config.dtb_size = st.st_size;
> + break;
> + case 'r':
> + config.ramdisk_fd = open(optarg, O_RDONLY | O_CLOEXEC);
> + if (config.ramdisk_fd < 0) {
> + perror("Failed to open ramdisk");
> + return -1;
> + }
> + if (stat(optarg, &st) < 0) {
> + perror("Failed to stat ramdisk");
> + return -1;
> + }
> + config.ramdisk_size = st.st_size;
> + break;
> + case 'B':
> + l = strtol(optarg, NULL, 0);
> + if (l == LONG_MIN) {
> + perror("Failed to parse base address");
> + return -1;
> + }
> + config.guest_base = l;
> + break;
> + case 'S':
> + l = strtol(optarg, NULL, 0);
> + if (l == LONG_MIN) {
> + perror("Failed to parse memory size");
> + return -1;
> + }
> + config.guest_size = l;
> + break;
> + case 'I':
> + l = strtol(optarg, NULL, 0);
> + if (l == LONG_MIN) {
> + perror("Failed to parse image offset");
> + return -1;
> + }
> + config.image_offset = l;
> + break;
> + case 'D':
> + l = strtol(optarg, NULL, 0);
> + if (l == LONG_MIN) {
> + perror("Failed to parse dtb offset");
> + return -1;
> + }
> + config.dtb_offset = l;
> + break;
> + case 'R':
> + l = strtol(optarg, NULL, 0);
> + if (l == LONG_MIN) {
> + perror("Failed to parse ramdisk offset");
> + return -1;
> + }
> + config.ramdisk_offset = l;
> + break;
> + case 'h':
> + print_help(argv[0]);
> + return 0;
> + default:
> + print_help(argv[0]);
> + return -1;
> + }
> + }
> +
> + if (!config.image_fd || !config.dtb_fd) {
I *think* it's possible to have 0 be assigned as config.image_fd
if STDIN is closed when this is run. I might be wrong though, it's
been quite a while... In any case, to guarantee this works correctly
these should be set to -1 (as you do for ramdisk_fd).
> + print_help(argv[0]);
> + return -1;
> + }
> +
> + if (config.image_offset + config.image_size > config.guest_size) {
> + fprintf(stderr, "Image offset and size puts it outside guest memory. Make image smaller or increase guest memory size.\n");
> + return -1;
> + }
> +
> + if (config.dtb_offset + config.dtb_size > config.guest_size) {
> + fprintf(stderr, "DTB offset and size puts it outside guest memory. Make dtb smaller or increase guest memory size.\n");
> + return -1;
> + }
> +
> + if (config.ramdisk_fd == -1 &&
> + config.ramdisk_offset + config.ramdisk_size > config.guest_size) {
> + fprintf(stderr, "Ramdisk offset and size puts it outside guest memory. Make ramdisk smaller or increase guest memory size.\n");
> + return -1;
> + }
> +
> + gunyah_fd = open("/dev/gunyah", O_RDWR | O_CLOEXEC);
> + if (gunyah_fd < 0) {
> + perror("Failed to open /dev/gunyah");
> + return -1;
> + }
> +
> + vm_fd = ioctl(gunyah_fd, GH_CREATE_VM, 0);
> + if (vm_fd < 0) {
> + perror("Failed to create vm");
> + return -1;
> + }
> +
> + guest_fd = memfd_create("guest_memory", MFD_CLOEXEC);
> + if (guest_fd < 0) {
> + perror("Failed to create guest memfd");
> + return -1;
> + }
> +
> + if (ftruncate(guest_fd, config.guest_size) < 0) {
> + perror("Failed to grow guest memory");
> + return -1;
> + }
> +
> + guest_mem = mmap(NULL, config.guest_size, PROT_READ | PROT_WRITE, MAP_SHARED, guest_fd, 0);
> + if (guest_mem == MAP_FAILED) {
> + perror("Not enough memory");
> + return -1;
> + }
> +
> + if (read(config.image_fd, guest_mem + config.image_offset, config.image_size) < 0) {
> + perror("Failed to read image into guest memory");
> + return -1;
> + }
> +
> + if (read(config.dtb_fd, guest_mem + config.dtb_offset, config.dtb_size) < 0) {
> + perror("Failed to read dtb into guest memory");
> + return -1;
> + }
> +
> + if (config.ramdisk_fd > 0 &&
> + read(config.ramdisk_fd, guest_mem + config.ramdisk_offset,
> + config.ramdisk_size) < 0) {
> + perror("Failed to read ramdisk into guest memory");
> + return -1;
> + }
> +
> + guest_mem_desc.label = 0;
> + guest_mem_desc.flags = GH_MEM_ALLOW_READ | GH_MEM_ALLOW_WRITE | GH_MEM_ALLOW_EXEC;
> + guest_mem_desc.guest_phys_addr = config.guest_base;
> + guest_mem_desc.memory_size = config.guest_size;
> + guest_mem_desc.userspace_addr = (__u64)guest_mem;
> +
> + if (ioctl(vm_fd, GH_VM_SET_USER_MEM_REGION, &guest_mem_desc) < 0) {
> + perror("Failed to register guest memory with VM");
> + return -1;
> + }
> +
> + dtb_config.guest_phys_addr = config.guest_base + config.dtb_offset;
> + dtb_config.size = config.dtb_size;
> + if (ioctl(vm_fd, GH_VM_SET_DTB_CONFIG, &dtb_config) < 0) {
> + perror("Failed to set DTB configuration for VM");
> + return -1;
> + }
> +
> + ret = ioctl(vm_fd, GH_VM_START);
> + if (ret) {
> + perror("GH_VM_START failed");
> + return -1;
> + }
> +
> + while (1)
> + sleep(10);
Maybe call pause() instead of sleep?
> +
> + return 0;
> +}
> diff --git a/samples/gunyah/sample_vm.dts b/samples/gunyah/sample_vm.dts
> new file mode 100644
> index 000000000000..293bbc0469c8
> --- /dev/null
> +++ b/samples/gunyah/sample_vm.dts
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +/dts-v1/;
> +
> +/ {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + interrupt-parent = <&intc>;
> +
> + chosen {
> + bootargs = "nokaslr";
> + };
> +
> + cpus {
> + #address-cells = <0x2>;
> + #size-cells = <0>;
> +
> + cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,armv8";
> + reg = <0 0>;
> + };
> + };
> +
> + intc: interrupt-controller@...F0000 {
> + compatible = "arm,gic-v3";
> + #interrupt-cells = <3>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> + interrupt-controller;
> + reg = <0 0x3FFF0000 0 0x10000>,
> + <0 0x3FFD0000 0 0x20000>;
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + always-on;
> + interrupts = <1 13 0x108>,
> + <1 14 0x108>,
> + <1 11 0x108>,
> + <1 10 0x108>;
> + clock-frequency = <19200000>;
> + };
> +
> + gunyah-vm-config {
> + image-name = "linux_vm_0";
> +
> + memory {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + base-address = <0 0x80000000>;
> + };
> +
> + interrupts {
> + config = <&intc>;
> + };
> +
> + vcpus {
> + affinity-map = < 0 >;
> + sched-priority = < (-1) >;
> + sched-timeslice = < 2000 >;
> + };
> + };
> +};
Powered by blists - more mailing lists