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

[Greg Kroah-Hartman]

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?

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

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

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

:)

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

thanks,

greg k-h