Messages in this thread | | | Date | Thu, 23 Feb 2023 14:40:50 -0800 | Subject | Re: [PATCH v10 10/26] gunyah: vm_mgr: Introduce basic VM Manager | From | Elliot Berman <> |
| |
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
| |