Re: [PATCH v2 05/17] virt: acrn: Introduce ACRN HSM basic driver

[Liu, Shuo A]

On 9/3/2020 20:53, Greg Kroah-Hartman wrote:
> On Thu, Sep 03, 2020 at 08:41:49PM +0800, shuo.a.liu@xxxxxxxxx wrote:
>> From: Shuo Liu <shuo.a.liu@xxxxxxxxx>
>>
>> ACRN Hypervisor Service Module (HSM) is a kernel module in Service VM
>> which communicates with ACRN userspace through ioctls and talks to ACRN
>> Hypervisor through hypercalls.
>>
>> Add a basic HSM driver which allows Service VM userspace to communicate
>> with ACRN. The following patches will add more ioctls, guest VM memory
>> mapping caching, I/O request processing, ioeventfd and irqfd into this
>> module. HSM exports a char device interface (/dev/acrn_hsm) to userspace.
>>
>> Signed-off-by: Shuo Liu <shuo.a.liu@xxxxxxxxx>
>> Reviewed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
>> Cc: Zhi Wang <zhi.a.wang@xxxxxxxxx>
>> Cc: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
>> Cc: Yu Wang <yu1.wang@xxxxxxxxx>
>> Cc: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> ---
>>  .../userspace-api/ioctl/ioctl-number.rst      |  1 +
>>  MAINTAINERS                                   |  2 +
>>  drivers/virt/Kconfig                          |  2 +
>>  drivers/virt/Makefile                         |  1 +
>>  drivers/virt/acrn/Kconfig                     | 14 +++
>>  drivers/virt/acrn/Makefile                    |  3 +
>>  drivers/virt/acrn/acrn_drv.h                  | 19 ++++
>>  drivers/virt/acrn/hsm.c                       | 98 +++++++++++++++++++
>>  include/uapi/linux/acrn.h                     | 17 ++++
>>  9 files changed, 157 insertions(+)
>>  create mode 100644 drivers/virt/acrn/Kconfig
>>  create mode 100644 drivers/virt/acrn/Makefile
>>  create mode 100644 drivers/virt/acrn/acrn_drv.h
>>  create mode 100644 drivers/virt/acrn/hsm.c
>>  create mode 100644 include/uapi/linux/acrn.h
>>
>> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> index 2a198838fca9..ac60efedb104 100644
>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> @@ -319,6 +319,7 @@ Code  Seq#    Include File                                           Comments
>>  0xA0  all    linux/sdp/sdp.h                                         Industrial Device Project
>>                                                                       <mailto:kenji@xxxxxxxxxxx>
>>  0xA1  0      linux/vtpm_proxy.h                                      TPM Emulator Proxy Driver
>> +0xA2  all    uapi/linux/acrn.h                                       ACRN hypervisor
> 
> You don't have any ioctls in this patch, so why add this documentation
> here? 

This was left when i removed an api version ioctl from the v1 patch set.
Let me move it to a later patch.

>>  0xA3  80-8F                                                          Port ACL  in development:
>>                                                                       <mailto:tlewis@xxxxxxxxxxxxxx>
>>  0xA3  90-9F  linux/dtlk.h
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e0fea5e464b4..d4c1ef303c2d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -442,6 +442,8 @@ L:	acrn-dev@xxxxxxxxxxxxxxxxxxxxx
>>  S:	Supported
>>  W:	https://projectacrn.org
>>  F:	Documentation/virt/acrn/
>> +F:	drivers/virt/acrn/
>> +F:	include/uapi/linux/acrn.h
> 
> This uapi file is not used in this patch, please add it in a later
> patch.

OK.

> 
>> +static long acrn_dev_ioctl(struct file *filp, unsigned int cmd,
>> +			   unsigned long ioctl_param)
>> +{
>> +	return 0;
>> +}
> 
> As your ioctl does nothing, no need to include it here, add it in a
> later patch.

OK.

> 
>> +
>> +static int acrn_dev_release(struct inode *inode, struct file *filp)
>> +{
>> +	struct acrn_vm *vm = filp->private_data;
>> +
>> +	kfree(vm);
>> +	return 0;
>> +}
>> +
>> +static const struct file_operations acrn_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.open		= acrn_dev_open,
>> +	.release	= acrn_dev_release,
>> +	.unlocked_ioctl	= acrn_dev_ioctl,
>> +};
>> +
>> +static struct miscdevice acrn_dev = {
>> +	.minor	= MISC_DYNAMIC_MINOR,
>> +	.name	= "acrn_hsm",
>> +	.fops	= &acrn_fops,
>> +};
>> +
>> +static int __init hsm_init(void)
>> +{
>> +	int ret;
>> +
>> +	if (x86_hyper_type != X86_HYPER_ACRN)
>> +		return -ENODEV;
>> +
>> +	if (!acrn_is_privileged_vm())
>> +		return -EPERM;
>> +
>> +	ret = misc_register(&acrn_dev);
>> +	if (ret) {
>> +		pr_err("Create misc dev failed!\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
> 
> Tiny tiny nit, these lines can be rewritten as:
> 	if (ret)
> 		pr_err("Create misc dev failed!\n");
> 
> 	return ret;
> 
> :)

OK. Thanks.

> 
>> +}
>> +
>> +static void __exit hsm_exit(void)
>> +{
>> +	misc_deregister(&acrn_dev);
>> +}
>> +module_init(hsm_init);
>> +module_exit(hsm_exit);
>> +
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("ACRN Hypervisor Service Module (HSM)");
>> diff --git a/include/uapi/linux/acrn.h b/include/uapi/linux/acrn.h
>> new file mode 100644
>> index 000000000000..4ae34f86e2be
>> --- /dev/null
>> +++ b/include/uapi/linux/acrn.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Userspace interface for /dev/acrn_hsm - ACRN Hypervisor Service Module
>> + *
>> + * This file can be used by applications that need to communicate with the HSM
>> + * via the ioctl interface.
>> + */
>> +
>> +#ifndef _UAPI_ACRN_H
>> +#define _UAPI_ACRN_H
>> +
>> +#include <linux/types.h>
>> +
>> +/* The ioctl type, documented in ioctl-number.rst */
>> +#define ACRN_IOCTL_TYPE			0xA2
> 
> This isn't used in this patch, so save it for a later one please.

Sure. I will move to later one.

Thanks
shuo