Re: [PATCH 06/17] virt: acrn: Introduce VM management interfaces

[Shuo A Liu]

Hi Greg,

On Fri 28.Aug'20 at 12:27:04 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 25, 2020 at 10:45:06AM +0800, shuo.a.liu@xxxxxxxxx wrote:
> > From: Shuo Liu <shuo.a.liu@xxxxxxxxx>
> >
> > The VM management interfaces expose several VM operations to ACRN
> > userspace via ioctls. For example, creating VM, starting VM, destroying
> > VM and so on.
> >
> > The ACRN Hypervisor needs to exchange data with the ACRN userspace
> > during the VM operations. HSM provides VM operation ioctls to the ACRN
> > userspace and communicates with the ACRN Hypervisor for VM operations
> > via hypercalls.
> >
> > HSM maintains a list of User VM. Each User VM will be bound to an
> > existing file descriptor of /dev/acrn_hsm. The User VM will be
> > destroyed when the file descriptor is closed.
> >
> > Signed-off-by: Shuo Liu <shuo.a.liu@xxxxxxxxx>
> > Reviewed-by: Zhi Wang <zhi.a.wang@xxxxxxxxx>
> > Reviewed-by: Reinette Chatre <reinette.chatre@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>
> > ---
> >  drivers/virt/acrn/Makefile    |  2 +-
> >  drivers/virt/acrn/acrn_drv.h  | 16 ++++++++-
> >  drivers/virt/acrn/hsm.c       | 58 +++++++++++++++++++++++++++++-
> >  drivers/virt/acrn/hypercall.h | 62 ++++++++++++++++++++++++++++++++
> >  drivers/virt/acrn/vm.c        | 66 +++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/acrn.h     | 36 +++++++++++++++++++
> >  6 files changed, 237 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/virt/acrn/vm.c
> >
> > diff --git a/drivers/virt/acrn/Makefile b/drivers/virt/acrn/Makefile
> > index 6920ed798aaf..cf8b4ed5e74e 100644
> > --- a/drivers/virt/acrn/Makefile
> > +++ b/drivers/virt/acrn/Makefile
> > @@ -1,3 +1,3 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_ACRN_HSM)	:= acrn.o
> > -acrn-y := hsm.o
> > +acrn-y := hsm.o vm.o
> > diff --git a/drivers/virt/acrn/acrn_drv.h b/drivers/virt/acrn/acrn_drv.h
> > index 36f43d8d43d0..35fcb5cbbff3 100644
> > --- a/drivers/virt/acrn/acrn_drv.h
> > +++ b/drivers/virt/acrn/acrn_drv.h
> > @@ -10,12 +10,26 @@
> >
> >  #define ACRN_INVALID_VMID (0xffffU)
> >
> > +#define ACRN_VM_FLAG_DESTROYED		0U
> > +extern struct list_head acrn_vm_list;
> > +extern rwlock_t acrn_vm_list_lock;
> >  /**
> >   * struct acrn_vm - Properties of ACRN User VM.
> > + * @list:	Entry within global list of all VMs
> >   * @vmid:	User VM ID
> > + * @vcpu_num:	Number of virtual CPUs in the VM
> > + * @flags:	Flags (ACRN_VM_FLAG_*) of the VM. This is VM flag management
> > + *		in HSM which is different from the &acrn_vm_creation.vm_flag.
> >   */
> >  struct acrn_vm {
> > -	u16	vmid;
> > +	struct list_head	list;
> > +	u16			vmid;
> > +	int			vcpu_num;
> > +	unsigned long		flags;
> >  };
> >
> > +struct acrn_vm *acrn_vm_create(struct acrn_vm *vm,
> > +			       struct acrn_vm_creation *vm_param);
> > +int acrn_vm_destroy(struct acrn_vm *vm);
> > +
> >  #endif /* __ACRN_HSM_DRV_H */
> > diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
> > index a08169f35c96..ed8921a6c68b 100644
> > --- a/drivers/virt/acrn/hsm.c
> > +++ b/drivers/virt/acrn/hsm.c
> > @@ -45,19 +45,75 @@ static int acrn_dev_open(struct inode *inode, struct file *filp)
> >  static long acrn_dev_ioctl(struct file *filp, unsigned int cmd,
> >  			   unsigned long ioctl_param)
> >  {
> > +	struct acrn_vm *vm = filp->private_data;
> > +	struct acrn_vm_creation *vm_param;
> > +	int ret = 0;
> > +
> >  	if (cmd == ACRN_IOCTL_GET_API_VERSION) {
> >  		if (copy_to_user((void __user *)ioctl_param,
> >  				 &api_version, sizeof(api_version)))
> >  			return -EFAULT;
> > +		return 0;
> >  	}
> >
> > -	return 0;
> > +	if (vm->vmid == ACRN_INVALID_VMID && cmd != ACRN_IOCTL_CREATE_VM) {
> > +		pr_err("ioctl 0x%x: Invalid VM state!\n", cmd);
>
> For this whole driver, you have a real 'struct device' to use, please
> use it for all of these error messages everywhere.  dev_err() gives you
> much more information than pr_err() does.

OK. I will use dev_err() instead.

> Same everywhere in this patch series.

Sure.

Thanks
shuo