[v10,10/26] gunyah: vm_mgr: Introduce basic VM Manager

  Gunyah VM manager is a kernel moduel which exposes an interface to
Gunyah userspace to load, run, and interact with other Gunyah virtual
machines. The interface is a character device at /dev/gunyah.

Add a basic VM manager driver. Upcoming patches will add more ioctls
into this driver.

Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 drivers/virt/gunyah/Makefile                  |   2 +-
 drivers/virt/gunyah/rsc_mgr.c                 |  37 +++++-
 drivers/virt/gunyah/vm_mgr.c                  | 118 ++++++++++++++++++
 drivers/virt/gunyah/vm_mgr.h                  |  22 ++++
 include/uapi/linux/gunyah.h                   |  23 ++++
 6 files changed, 201 insertions(+), 2 deletions(-)
 create mode 100644 drivers/virt/gunyah/vm_mgr.c
 create mode 100644 drivers/virt/gunyah/vm_mgr.h
 create mode 100644 include/uapi/linux/gunyah.h
  

On 14/02/2023 21:23, Elliot Berman wrote:
> 
> Gunyah VM manager is a kernel moduel which exposes an interface to
> Gunyah userspace to load, run, and interact with other Gunyah virtual
> machines. The interface is a character device at /dev/gunyah.
> 
> Add a basic VM manager driver. Upcoming patches will add more ioctls
> into this driver.
> 
> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>   .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>   drivers/virt/gunyah/Makefile                  |   2 +-
>   drivers/virt/gunyah/rsc_mgr.c                 |  37 +++++-
>   drivers/virt/gunyah/vm_mgr.c                  | 118 ++++++++++++++++++
>   drivers/virt/gunyah/vm_mgr.h                  |  22 ++++
>   include/uapi/linux/gunyah.h                   |  23 ++++
>   6 files changed, 201 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/virt/gunyah/vm_mgr.c
>   create mode 100644 drivers/virt/gunyah/vm_mgr.h
>   create mode 100644 include/uapi/linux/gunyah.h
> 
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 0a1882e296ae..2513324ae7be 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -137,6 +137,7 @@ Code  Seq#    Include File                                           Comments
>   'F'   DD     video/sstfb.h                                           conflict!
>   'G'   00-3F  drivers/misc/sgi-gru/grulib.h                           conflict!
>   'G'   00-0F  xen/gntalloc.h, xen/gntdev.h                            conflict!
> +'G'   00-0f  linux/gunyah.h                                          conflict!
>   'H'   00-7F  linux/hiddev.h                                          conflict!
>   'H'   00-0F  linux/hidraw.h                                          conflict!
>   'H'   01     linux/mei.h                                             conflict!
> diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile
> index de29769f2f3f..03951cf82023 100644
> --- a/drivers/virt/gunyah/Makefile
> +++ b/drivers/virt/gunyah/Makefile
> @@ -2,5 +2,5 @@
>   
>   obj-$(CONFIG_GUNYAH) += gunyah.o
>   
> -gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o
> +gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o
>   obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o
> diff --git a/drivers/virt/gunyah/rsc_mgr.c b/drivers/virt/gunyah/rsc_mgr.c
> index 2a47139873a8..73c5a6b7cbbc 100644
> --- a/drivers/virt/gunyah/rsc_mgr.c
> +++ b/drivers/virt/gunyah/rsc_mgr.c
> @@ -16,8 +16,10 @@
>   #include <linux/completion.h>
>   #include <linux/gunyah_rsc_mgr.h>
>   #include <linux/platform_device.h>
> +#include <linux/miscdevice.h>
>   
>   #include "rsc_mgr.h"
> +#include "vm_mgr.h"
>   
>   #define RM_RPC_API_VERSION_MASK		GENMASK(3, 0)
>   #define RM_RPC_HEADER_WORDS_MASK	GENMASK(7, 4)
> @@ -103,6 +105,8 @@ struct gh_rm {
>   	struct kmem_cache *cache;
>   	struct mutex send_lock;
>   	struct blocking_notifier_head nh;
> +
> +	struct miscdevice miscdev;
>   };
>   
>   static struct gh_rm_connection *gh_rm_alloc_connection(__le32 msg_id, u8 type)
> @@ -509,6 +513,21 @@ void put_gh_rm(struct gh_rm *rm)
>   }
>   EXPORT_SYMBOL_GPL(put_gh_rm);
>   
> +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);
> +
> +	return gh_dev_vm_mgr_ioctl(rm, cmd, arg);
> +}
> +
> +static const struct file_operations gh_dev_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= gh_dev_ioctl,
> +	.compat_ioctl	= compat_ptr_ioctl,
> +	.llseek		= noop_llseek,
> +};
> +
>   static int gh_msgq_platform_probe_direction(struct platform_device *pdev,
>   					bool tx, int idx, struct gunyah_resource *ghrsc)
>   {
> @@ -567,7 +586,22 @@ static int gh_rm_drv_probe(struct platform_device *pdev)
>   	rm->msgq_client.rx_callback = gh_rm_msgq_rx_data;
>   	rm->msgq_client.tx_done = gh_rm_msgq_tx_done;
>   
> -	return gh_msgq_init(&pdev->dev, &rm->msgq, &rm->msgq_client, &rm->tx_ghrsc, &rm->rx_ghrsc);
> +	ret = gh_msgq_init(&pdev->dev, &rm->msgq, &rm->msgq_client, &rm->tx_ghrsc, &rm->rx_ghrsc);
> +	if (ret)
> +		goto err_cache;
> +
> +	rm->miscdev.name = "gunyah";
> +	rm->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	rm->miscdev.fops = &gh_dev_fops;
> +
> +	ret = misc_register(&rm->miscdev);
> +	if (ret)
> +		goto err_msgq;
> +
> +	return 0;
> +err_msgq:
> +	mbox_free_channel(gh_msgq_chan(&rm->msgq));
> +	gh_msgq_remove(&rm->msgq);
>   err_cache:
>   	kmem_cache_destroy(rm->cache);
>   	return ret;
> @@ -577,6 +611,7 @@ static int gh_rm_drv_remove(struct platform_device *pdev)
>   {
>   	struct gh_rm *rm = platform_get_drvdata(pdev);
>   
> +	misc_deregister(&rm->miscdev);
>   	mbox_free_channel(gh_msgq_chan(&rm->msgq));
>   	gh_msgq_remove(&rm->msgq);
>   	kmem_cache_destroy(rm->cache);
> diff --git a/drivers/virt/gunyah/vm_mgr.c b/drivers/virt/gunyah/vm_mgr.c
> new file mode 100644
> index 000000000000..fd890a57172e
> --- /dev/null
> +++ b/drivers/virt/gunyah/vm_mgr.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt) "gh_vm_mgr: " fmt
> +
> +#include <linux/anon_inodes.h>
> +#include <linux/file.h>
> +#include <linux/gunyah_rsc_mgr.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +
> +#include <uapi/linux/gunyah.h>
> +
> +#include "vm_mgr.h"
> +
> +static void gh_vm_free(struct work_struct *work)
> +{
> +	struct gh_vm *ghvm = container_of(work, struct gh_vm, free_work);
> +	int ret;
> +
> +	ret = gh_rm_dealloc_vmid(ghvm->rm, ghvm->vmid);
> +	if (ret)
> +		pr_warn("Failed to deallocate vmid: %d\n", ret);
> +
> +	put_gh_rm(ghvm->rm);
> +	kfree(ghvm);
> +}
> +
> +static __must_check struct gh_vm *gh_vm_alloc(struct gh_rm *rm)
> +{
> +	struct gh_vm *ghvm;
> +	int vmid;
> +
> +	vmid = gh_rm_alloc_vmid(rm, 0);
> +	if (vmid < 0)
> +		return ERR_PTR(vmid);
> +
> +	ghvm = kzalloc(sizeof(*ghvm), GFP_KERNEL);
> +	if (!ghvm) {
> +		gh_rm_dealloc_vmid(rm, vmid);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	get_gh_rm(rm);
> +
> +	ghvm->vmid = vmid;
> +	ghvm->rm = rm;
> +
> +	INIT_WORK(&ghvm->free_work, gh_vm_free);
> +
> +	return ghvm;
> +}
> +
> +static int gh_vm_release(struct inode *inode, struct file *filp)
> +{
> +	struct gh_vm *ghvm = filp->private_data;
> +
> +	/* VM will be reset and make RM calls which can interruptible sleep.
> +	 * Defer to a work so this thread can receive signal.
> +	 */
> +	schedule_work(&ghvm->free_work);
> +	return 0;
> +}
> +
> +static const struct file_operations gh_vm_fops = {
> +	.release = gh_vm_release,

> +	.compat_ioctl	= compat_ptr_ioctl,

This line should go with the patch that adds real ioctl

> +	.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;
	}
}


> +{
> +	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.

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.

> +
> +	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?

--srini

> +
> +#endif
  

* Elliot Berman <quic_eberman@quicinc.com> [2023-02-14 13:23:54]:

> +static long gh_dev_ioctl_create_vm(struct gh_rm *rm, unsigned long arg)
> +{
> +	struct gh_vm *ghvm;
> +	struct file *file;
> +	int fd, err;
> +
> +	/* arg reserved for future use. */
> +	if (arg)
> +		return -EINVAL;
> +
> +	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);

Need a put_gh_rm() also in this case

> +	return err;
> +}
  

On 2/21/2023 2:46 AM, Srinivas Kandagatla wrote:
> 
> 
> On 14/02/2023 21:23, Elliot Berman wrote:
>>
>> Gunyah VM manager is a kernel moduel which exposes an interface to
>> Gunyah userspace to load, run, and interact with other Gunyah virtual
>> machines. The interface is a character device at /dev/gunyah.
>>
>> Add a basic VM manager driver. Upcoming patches will add more ioctls
>> into this driver.
>>
>> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>> ---
>>   .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>>   drivers/virt/gunyah/Makefile                  |   2 +-
>>   drivers/virt/gunyah/rsc_mgr.c                 |  37 +++++-
>>   drivers/virt/gunyah/vm_mgr.c                  | 118 ++++++++++++++++++
>>   drivers/virt/gunyah/vm_mgr.h                  |  22 ++++
>>   include/uapi/linux/gunyah.h                   |  23 ++++
>>   6 files changed, 201 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/virt/gunyah/vm_mgr.c
>>   create mode 100644 drivers/virt/gunyah/vm_mgr.h
>>   create mode 100644 include/uapi/linux/gunyah.h
>>
>> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
>> b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> index 0a1882e296ae..2513324ae7be 100644
>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> @@ -137,6 +137,7 @@ Code  Seq#    Include 
>> File                                           Comments
>>   'F'   DD     video/sstfb.h                                           
>> conflict!
>>   'G'   00-3F  drivers/misc/sgi-gru/grulib.h                           
>> conflict!
>>   'G'   00-0F  xen/gntalloc.h, xen/gntdev.h                            
>> conflict!
>> +'G'   00-0f  linux/gunyah.h                                          
>> conflict!
>>   'H'   00-7F  linux/hiddev.h                                          
>> conflict!
>>   'H'   00-0F  linux/hidraw.h                                          
>> conflict!
>>   'H'   01     linux/mei.h                                             
>> conflict!
>> diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile
>> index de29769f2f3f..03951cf82023 100644
>> --- a/drivers/virt/gunyah/Makefile
>> +++ b/drivers/virt/gunyah/Makefile
>> @@ -2,5 +2,5 @@
>>   obj-$(CONFIG_GUNYAH) += gunyah.o
>> -gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o
>> +gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o
>>   obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o
>> diff --git a/drivers/virt/gunyah/rsc_mgr.c 
>> b/drivers/virt/gunyah/rsc_mgr.c
>> index 2a47139873a8..73c5a6b7cbbc 100644
>> --- a/drivers/virt/gunyah/rsc_mgr.c
>> +++ b/drivers/virt/gunyah/rsc_mgr.c
>> @@ -16,8 +16,10 @@
>>   #include <linux/completion.h>
>>   #include <linux/gunyah_rsc_mgr.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/miscdevice.h>
>>   #include "rsc_mgr.h"
>> +#include "vm_mgr.h"
>>   #define RM_RPC_API_VERSION_MASK        GENMASK(3, 0)
>>   #define RM_RPC_HEADER_WORDS_MASK    GENMASK(7, 4)
>> @@ -103,6 +105,8 @@ struct gh_rm {
>>       struct kmem_cache *cache;
>>       struct mutex send_lock;
>>       struct blocking_notifier_head nh;
>> +
>> +    struct miscdevice miscdev;
>>   };
>>   static struct gh_rm_connection *gh_rm_alloc_connection(__le32 
>> msg_id, u8 type)
>> @@ -509,6 +513,21 @@ void put_gh_rm(struct gh_rm *rm)
>>   }
>>   EXPORT_SYMBOL_GPL(put_gh_rm);
>> +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);
>> +
>> +    return gh_dev_vm_mgr_ioctl(rm, cmd, arg);
>> +}
>> +
>> +static const struct file_operations gh_dev_fops = {
>> +    .owner        = THIS_MODULE,
>> +    .unlocked_ioctl    = gh_dev_ioctl,
>> +    .compat_ioctl    = compat_ptr_ioctl,
>> +    .llseek        = noop_llseek,
>> +};
>> +
>>   static int gh_msgq_platform_probe_direction(struct platform_device 
>> *pdev,
>>                       bool tx, int idx, struct gunyah_resource *ghrsc)
>>   {
>> @@ -567,7 +586,22 @@ static int gh_rm_drv_probe(struct platform_device 
>> *pdev)
>>       rm->msgq_client.rx_callback = gh_rm_msgq_rx_data;
>>       rm->msgq_client.tx_done = gh_rm_msgq_tx_done;
>> -    return gh_msgq_init(&pdev->dev, &rm->msgq, &rm->msgq_client, 
>> &rm->tx_ghrsc, &rm->rx_ghrsc);
>> +    ret = gh_msgq_init(&pdev->dev, &rm->msgq, &rm->msgq_client, 
>> &rm->tx_ghrsc, &rm->rx_ghrsc);
>> +    if (ret)
>> +        goto err_cache;
>> +
>> +    rm->miscdev.name = "gunyah";
>> +    rm->miscdev.minor = MISC_DYNAMIC_MINOR;
>> +    rm->miscdev.fops = &gh_dev_fops;
>> +
>> +    ret = misc_register(&rm->miscdev);
>> +    if (ret)
>> +        goto err_msgq;
>> +
>> +    return 0;
>> +err_msgq:
>> +    mbox_free_channel(gh_msgq_chan(&rm->msgq));
>> +    gh_msgq_remove(&rm->msgq);
>>   err_cache:
>>       kmem_cache_destroy(rm->cache);
>>       return ret;
>> @@ -577,6 +611,7 @@ static int gh_rm_drv_remove(struct platform_device 
>> *pdev)
>>   {
>>       struct gh_rm *rm = platform_get_drvdata(pdev);
>> +    misc_deregister(&rm->miscdev);
>>       mbox_free_channel(gh_msgq_chan(&rm->msgq));
>>       gh_msgq_remove(&rm->msgq);
>>       kmem_cache_destroy(rm->cache);
>> diff --git a/drivers/virt/gunyah/vm_mgr.c b/drivers/virt/gunyah/vm_mgr.c
>> new file mode 100644
>> index 000000000000..fd890a57172e
>> --- /dev/null
>> +++ b/drivers/virt/gunyah/vm_mgr.c
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 
>> rights reserved.
>> + */
>> +
>> +#define pr_fmt(fmt) "gh_vm_mgr: " fmt
>> +
>> +#include <linux/anon_inodes.h>
>> +#include <linux/file.h>
>> +#include <linux/gunyah_rsc_mgr.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +
>> +#include <uapi/linux/gunyah.h>
>> +
>> +#include "vm_mgr.h"
>> +
>> +static void gh_vm_free(struct work_struct *work)
>> +{
>> +    struct gh_vm *ghvm = container_of(work, struct gh_vm, free_work);
>> +    int ret;
>> +
>> +    ret = gh_rm_dealloc_vmid(ghvm->rm, ghvm->vmid);
>> +    if (ret)
>> +        pr_warn("Failed to deallocate vmid: %d\n", ret);
>> +
>> +    put_gh_rm(ghvm->rm);
>> +    kfree(ghvm);
>> +}
>> +
>> +static __must_check struct gh_vm *gh_vm_alloc(struct gh_rm *rm)
>> +{
>> +    struct gh_vm *ghvm;
>> +    int vmid;
>> +
>> +    vmid = gh_rm_alloc_vmid(rm, 0);
>> +    if (vmid < 0)
>> +        return ERR_PTR(vmid);
>> +
>> +    ghvm = kzalloc(sizeof(*ghvm), GFP_KERNEL);
>> +    if (!ghvm) {
>> +        gh_rm_dealloc_vmid(rm, vmid);
>> +        return ERR_PTR(-ENOMEM);
>> +    }
>> +
>> +    get_gh_rm(rm);
>> +
>> +    ghvm->vmid = vmid;
>> +    ghvm->rm = rm;
>> +
>> +    INIT_WORK(&ghvm->free_work, gh_vm_free);
>> +
>> +    return ghvm;
>> +}
>> +
>> +static int gh_vm_release(struct inode *inode, struct file *filp)
>> +{
>> +    struct gh_vm *ghvm = filp->private_data;
>> +
>> +    /* VM will be reset and make RM calls which can interruptible sleep.
>> +     * Defer to a work so this thread can receive signal.
>> +     */
>> +    schedule_work(&ghvm->free_work);
>> +    return 0;
>> +}
>> +
>> +static const struct file_operations gh_vm_fops = {
>> +    .release = gh_vm_release,
> 
>> +    .compat_ioctl    = compat_ptr_ioctl,
> 
> This line should go with the patch that adds real ioctl
> 

Done.

>> +    .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.

> 
> 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 
break compatibility with old kernels; old kernels reject it as -EINVAL.

>> +
>> +    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 
destroyed when VM refcount is dropped to 0 (close(vm_fd) and any other 
relevant file descriptors).

- Elliot
  

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.


> 
>>> +
>>> +    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
  

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
  

On 23/02/2023 22:40, Elliot Berman wrote:
> 
> 
> 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?
> 

It is recommended to only use _IO for commands without arguments, and 
use pointers for passing data. Even though _IO can indicate either 
commands with no argument or passing an integer value instead of a 
pointer. Am really not sure how this works in compat case.

Am sure there are tricks that can be done with just using _IO() macro 
(ex vfio), but this does not mean that we should not use _IOW to be more 
explicit on the type and size of argument that we are expecting.

On the other hand If its really not possible to change this IOCTL to 
_IOW and argument that you are referring would be with in integer range, 
then what you have with _IO macro should work.

--srini

>>>
>>>>> +
>>>>> +    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
  

On Fri, Feb 24, 2023, at 11:29, Srinivas Kandagatla wrote:
> On 23/02/2023 22:40, Elliot Berman wrote:

>>>> 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?
>> 
>
> It is recommended to only use _IO for commands without arguments, and 
> use pointers for passing data. Even though _IO can indicate either 
> commands with no argument or passing an integer value instead of a 
> pointer. Am really not sure how this works in compat case.
>
> Am sure there are tricks that can be done with just using _IO() macro 
> (ex vfio), but this does not mean that we should not use _IOW to be more 
> explicit on the type and size of argument that we are expecting.
>
> On the other hand If its really not possible to change this IOCTL to 
> _IOW and argument that you are referring would be with in integer range, 
> then what you have with _IO macro should work.

Passing an 'unsigned long' value instead of a pointer is fine for compat
mode, as a 32-bit compat_ulong_t always fits inside of the 64-bit
unsigned long. The downside is that portable code cannot have a
single ioctl handler function that takes both commands with pointers
and other commands with integer arguments, as some architectures
(i.e. s390, possibly arm64+morello in the future) need to mangle
pointer arguments using compat_ptr() but must not do that on integer
arguments.

     Arnd
  

On 2/24/2023 5:20 AM, Arnd Bergmann wrote:
> On Fri, Feb 24, 2023, at 11:29, Srinivas Kandagatla wrote:
>> On 23/02/2023 22:40, Elliot Berman wrote:
> 
>>>>> 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?
>>>
>>
>> It is recommended to only use _IO for commands without arguments, and
>> use pointers for passing data. Even though _IO can indicate either
>> commands with no argument or passing an integer value instead of a
>> pointer. Am really not sure how this works in compat case.
>>
>> Am sure there are tricks that can be done with just using _IO() macro
>> (ex vfio), but this does not mean that we should not use _IOW to be more
>> explicit on the type and size of argument that we are expecting.
>>
>> On the other hand If its really not possible to change this IOCTL to
>> _IOW and argument that you are referring would be with in integer range,
>> then what you have with _IO macro should work.
> 
> Passing an 'unsigned long' value instead of a pointer is fine for compat
> mode, as a 32-bit compat_ulong_t always fits inside of the 64-bit
> unsigned long. The downside is that portable code cannot have a
> single ioctl handler function that takes both commands with pointers
> and other commands with integer arguments, as some architectures
> (i.e. s390, possibly arm64+morello in the future) need to mangle
> pointer arguments using compat_ptr() but must not do that on integer
> arguments.

Thanks Arnd for helping clarify here!

I'd be open to making GH_CREATE_VM take a struct argument today, but I 
really don't know what size or what needs to be in that struct. My hope 
is that we can get away with just an integer for future needs. If 
integer doesn't suit, then new ioctl would need to be created. I think 
there's same problem if I pick some struct today (the struct may not 
suit tomorrow and we need to create new ioctl for the new struct).
  

On 2/24/23 4:48 PM, Elliot Berman wrote:
> I'd be open to making GH_CREATE_VM take a struct argument today, but I 
> really don't know what size or what needs to be in that struct. My hope 
> is that we can get away with just an integer for future needs. If 
> integer doesn't suit, then new ioctl would need to be created. I think 
> there's same problem if I pick some struct today (the struct may not 
> suit tomorrow and we need to create new ioctl for the new struct).

I'd like someone to back me up (or tell me I'm wrong), but...

I think you can still pass a void in/out pointer, which can
be interpreted in an IOCTL-specific way, as long as it can
be unambiguously processed.

So if you passed a non-null pointer, what it referred to
could contain a key that defines the way to interpret it.

You can't take away a behavior you've once supported, but I
*think* you can add a new behavior (with a new structure
that identifies itself).

So if that is correct, you can extend a single IOCTL.  But
sadly I can't tell you I'm sure this is correct.

					-Alex
  

On Tue, Feb 28, 2023, at 02:06, Alex Elder wrote:
> On 2/24/23 4:48 PM, Elliot Berman wrote:
>> I'd be open to making GH_CREATE_VM take a struct argument today, but I 
>> really don't know what size or what needs to be in that struct. My hope 
>> is that we can get away with just an integer for future needs. If 
>> integer doesn't suit, then new ioctl would need to be created. I think 
>> there's same problem if I pick some struct today (the struct may not 
>> suit tomorrow and we need to create new ioctl for the new struct).
>
> I'd like someone to back me up (or tell me I'm wrong), but...
>
> I think you can still pass a void in/out pointer, which can
> be interpreted in an IOCTL-specific way, as long as it can
> be unambiguously processed.
>
> So if you passed a non-null pointer, what it referred to
> could contain a key that defines the way to interpret it.
>
> You can't take away a behavior you've once supported, but I
> *think* you can add a new behavior (with a new structure
> that identifies itself).
>
> So if that is correct, you can extend a single IOCTL.  But
> sadly I can't tell you I'm sure this is correct.

In general you are correct that the behavior of an ioctl
command can be changed by reusing a combination of inputs that
was previously prohibited. I can't think of a case where that
would be a good idea though, as this just adds more complexity
than defining a new ioctl command code.

Interface versions and multiplexed ioctl commands are
all discouraged for the same reason.

      Arnd