Re: [PATCH v10 10/26] gunyah: vm_mgr: Introduce basic VM Manager
From: Elliot Berman
Date: Thu Feb 23 2023 - 17:41:32 EST
On 2/23/2023 2:08 AM, Srinivas Kandagatla wrote:
On 22/02/2023 00:27, Elliot Berman wrote:
+ .llseek = noop_llseek,
+};
+
+static long gh_dev_ioctl_create_vm(struct gh_rm *rm, unsigned long
arg)
Not sure what is the gain of this multiple levels of redirection.
How about
long gh_dev_create_vm(struct gh_rm *rm, unsigned long arg)
{
...
}
and rsc_mgr just call it as part of its ioctl call
static long gh_dev_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
{
struct miscdevice *miscdev = filp->private_data;
struct gh_rm *rm = container_of(miscdev, struct gh_rm, miscdev);
switch (cmd) {
case GH_CREATE_VM:
return gh_dev_create_vm(rm, arg);
default:
return -ENOIOCTLCMD;
}
}
I'm anticipating we will add further /dev/gunyah ioctls and I thought
it would be cleaner to have all that in vm_mgr.c itself.
+{
+ struct gh_vm *ghvm;
+ struct file *file;
+ int fd, err;
+
+ /* arg reserved for future use. */
+ if (arg)
+ return -EINVAL;
The only code path I see here is via GH_CREATE_VM ioctl which
obviously does not take any arguments, so if you are thinking of
using the argument for architecture-specific VM flags. Then this
needs to be properly done by making the ABI aware of this.
It is documented in Patch 17 (Document Gunyah VM Manager)
+GH_CREATE_VM
+~~~~~~~~~~~~
+
+Creates a Gunyah VM. The argument is reserved for future use and must
be 0.
But this conficts with the UAPIs that have been defined. GH_CREATE_VM
itself is defined to take no parameters.
#define GH_CREATE_VM _IO(GH_IOCTL_TYPE, 0x0)
so where are you expecting the argument to come from?
>>>
As you mentioned zero value arg imply an "unauthenticated VM" type,
but this was not properly encoded in the userspace ABI. Why not make
it future compatible. How about adding arguments to GH_CREATE_VM and
pass the required information correctly.
Note that once the ABI is accepted then you will not be able to
change it, other than adding a new one.
Does this means adding #define GH_VM_DEFAULT_ARG 0 ? I am not sure yet
what arguments to add here.
The ABI can add new "long" values to GH_CREATE_VM and that wouldn't
Sorry, that is exactly what we want to avoid, we can not change the UAPI
its going to break the userspace.
break compatibility with old kernels; old kernels reject it as -EINVAL.
If you have userspace built with older kernel headers then that will
break. Am not sure about old-kernels.
What exactly is the argument that you want to add to GH_CREATE_VM?
If you want to keep GH_CREATE_VM with no arguments that is fine but
remove the conflicting comments in the code and document so that its not
misleading readers/reviewers that the UAPI is going to be modified in
near future.
The convention followed here comes from KVM_CREATE_VM. Is this ioctl
considered bad example?
+
+ ghvm = gh_vm_alloc(rm);
+ if (IS_ERR(ghvm))
+ return PTR_ERR(ghvm);
+
+ fd = get_unused_fd_flags(O_CLOEXEC);
+ if (fd < 0) {
+ err = fd;
+ goto err_destroy_vm;
+ }
+
+ file = anon_inode_getfile("gunyah-vm", &gh_vm_fops, ghvm, O_RDWR);
+ if (IS_ERR(file)) {
+ err = PTR_ERR(file);
+ goto err_put_fd;
+ }
+
+ fd_install(fd, file);
+
+ return fd;
+
+err_put_fd:
+ put_unused_fd(fd);
+err_destroy_vm:
+ kfree(ghvm);
+ return err;
+}
+
+long gh_dev_vm_mgr_ioctl(struct gh_rm *rm, unsigned int cmd,
unsigned long arg)
+{
+ switch (cmd) {
+ case GH_CREATE_VM:
+ return gh_dev_ioctl_create_vm(rm, arg);
+ default:
+ return -ENOIOCTLCMD;
+ }
+}
diff --git a/drivers/virt/gunyah/vm_mgr.h
b/drivers/virt/gunyah/vm_mgr.h
new file mode 100644
index 000000000000..76954da706e9
--- /dev/null
+++ b/drivers/virt/gunyah/vm_mgr.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All
rights reserved.
+ */
+
+#ifndef _GH_PRIV_VM_MGR_H
+#define _GH_PRIV_VM_MGR_H
+
+#include <linux/gunyah_rsc_mgr.h>
+
+#include <uapi/linux/gunyah.h>
+
+long gh_dev_vm_mgr_ioctl(struct gh_rm *rm, unsigned int cmd,
unsigned long arg);
+
+struct gh_vm {
+ u16 vmid;
+ struct gh_rm *rm;
+
+ struct work_struct free_work;
+};
+
+#endif
diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h
new file mode 100644
index 000000000000..10ba32d2b0a6
--- /dev/null
+++ b/include/uapi/linux/gunyah.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
+/*
+ * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All
rights reserved.
+ */
+
+#ifndef _UAPI_LINUX_GUNYAH
+#define _UAPI_LINUX_GUNYAH
+
+/*
+ * Userspace interface for /dev/gunyah - gunyah based virtual machine
+ */
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#define GH_IOCTL_TYPE 'G'
+
+/*
+ * ioctls for /dev/gunyah fds:
+ */
+#define GH_CREATE_VM _IO(GH_IOCTL_TYPE, 0x0) /* Returns
a Gunyah VM fd */
Can HLOS forcefully destroy a VM?
If so should we have a corresponding DESTROY IOCTL?
It can forcefully destroy unauthenticated and protected virtual
machines. I don't have a userspace usecase for a DESTROY ioctl yet,
maybe this can be added later? By the way, the VM is forcefully
that should be fine, but its also nice to add it for completeness, but
not a compulsory atm
destroyed when VM refcount is dropped to 0 (close(vm_fd) and any other
relevant file descriptors).
I have noticed that path.
--srini
- Elliot