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

[Reinette Chatre]

Hi Shuo and Greg,

On 9/10/2020 9:28 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 10, 2020 at 02:19:00PM +0800, Shuo A Liu wrote:
>> On Wed  9.Sep'20 at 11:45:16 +0200, Greg Kroah-Hartman wrote:
>>> On Wed, Sep 09, 2020 at 05:08:25PM +0800, shuo.a.liu@xxxxxxxxx wrote:

...

>>>>  /**
>>>>   * struct acrn_vm - Properties of ACRN User VM.
>>>> + * @dev:	The struct device this VM belongs to
>>>> + * @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 device		*dev;
>>>> +	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 28a3052ffa55..bc85a3c14f87 100644
>>>> --- a/drivers/virt/acrn/hsm.c
>>>> +++ b/drivers/virt/acrn/hsm.c
>>>> @@ -19,6 +19,8 @@
>>>>
>>>>  #include "acrn_drv.h"
>>>>
>>>> +static struct miscdevice acrn_dev;
>>>> +
>>>>  /*
>>>>   * When /dev/acrn_hsm is opened, a 'struct acrn_vm' object is created to
>>>>   * represent a VM instance and continues to be associated with the opened file
>>>> @@ -34,14 +36,77 @@ static int acrn_dev_open(struct inode *inode, struct file *filp)
>>>>  		return -ENOMEM;
>>>>
>>>>  	vm->vmid = ACRN_INVALID_VMID;
>>>> +	vm->dev = get_device(acrn_dev.this_device);
>>>
>>> You are grabbing a reference on a static structure?
>>
>> acrn_dev is static, but acrn_dev.this_device is not.
> 
> But you don't control that device, the misc device core does.
> 
>>>
>>> Ugh, who reviewed this code before it was sent out???
>>
>> This part was just newly added according to your suggestion.. Please
>> refer to
>> https://lore.kernel.org/lkml/1946bf48-fda7-20e0-246d-93414a1a67f5@xxxxxxxxx/
> 
> What you were doing there was wrong, and what you are doing here is just
> odd.
> 
> Step back please, and describe exactly what you are trying to do.  And
> then explain how grabbing a reference to the device reference count for
> the misc device is going to help solve that issue.
> 

>From what I understand this current path was entered after Greg's
original suggestion from
https://lore.kernel.org/lkml/20200828102704.GB1470435@xxxxxxxxx
to replace the pr_xxx() calls with the more informative dev_xxx() ones.

Perhaps all that is needed to follow the original suggestion is to
replace the pr_xxx() calls with dev_xxx(acrn_dev.this_device, ...) ?

(An example of this is drivers/input/serio/userio.c and some others)

The device (acrn_dev.this_device) can be trusted to exist for the
lifetime of the driver since it is removed on module unload where the
misc device is unregistered.

Reinette