Re: [Qemu-devel] [PATCH 1/2] KVM: page track: add a new notifier type: track_flush_slot

[Jike Song <jike.song@intel.com>]

On 10/26/2016 10:45 PM, Paolo Bonzini wrote:
> On 26/10/2016 15:44, Jike Song wrote:
>> On 10/21/2016 01:06 AM, Paolo Bonzini wrote:
>>> On 20/10/2016 03:48, Xiao Guangrong wrote:
>>>> I understood that KVM side is safe, however, vfio side is independent with
>>>> kvm and the user of usrdata can fetch kvm struct at any time, consider
>>>> this scenario:
>>>>
>>>> CPU 0                         CPU 1
>>>> KVM:                         VFIO/userdata user
>>>>   kvm_ioctl_create_device
>>>>      get_kvm()
>>>>                             vfio_group_get_usrdata(vfio_group)
>>>>   kvm_device_release
>>>>     put_kvm()
>>>>                             !!! kvm refcount has gone
>>>>                             use KVM struct
>>>>
>>>> Then, the user of userdata have fetched kvm struct but the refcount has
>>>> already gone.
>>>
>>> vfio_group_set_usrdata (actually) kvm_vfio_group_set_kvm has called
>>> kvm_get_kvm too, however.  What you need is a mutex that is taken by
>>> vfio_group_set_usrdata and by the callers of vfio_group_get_usrdata.
>>
>> Hi Paolo & Guangrong,
>>
>> I walked the whole thread and became a little nervous: I don't want
>> to introduce a global mutex.
>>
>> The problem is, as I understand, vfio_group_get_usrdata() returns a
>> KVM pointer but it may be stale. To make the pointer always valid,
>> it can call kvm_get_kvm() *before* return the pointer.
> 
> That doesn't work, you still have to protect get against concurrent set.
>  But the mutex need not be global, it is specific to the vfio device.
> You probably have such a mutex anyway...

Thanks Paolo, I agree whatsoever a mutex is necessary. I cooked a patch
sent to you and Alex, please kindly have a look :-)

--
Thanks,
Jike

>> I would apologize in advance if this idea turns out totally
>> nonsense, but hey, please kindly help fix my whim :-)
>>
>>
>> [vfio.h]
>>
>> 	struct vfio_usrdata {
>> 		void *data;
>> 		void (*get)(void *data);
>> 		void (*put)(void *data)
>> 	};
>>
>> 	vfio_group {
>> 		...
>> 		vfio_usrdata *usrdata;
>>
>> [kvm.ko]
>>
>> 	struvt vfio_usrdata kvmdata = {
>> 		.data = kvm,
>> 		.get = kvm_get_kvm,
>> 		.put = kvm_put_kvm,
>> 	};
>>
>> 	fn = symbol_get(vfio_group_set_usrdata)
>> 	fn(vfio_group, &kvmdata)
>>
>>
>> [vfio.ko]
>>
>> 	vfio_group_set_usrdata
>> 		lock
>> 		vfio_group->d = kvmdata
>> 		unlock
>>
>> 	void *vfio_group_get_usrdata
>> 		lock
>> 		struct vfio_usrdata *d = vfio_group->usrdata;
>> 		d->get(d->data);
>> 		unlock
>> 		return d->data;
>>
>> 	void vfio_group_put_usrdata
>> 		lock
>> 		struct vfio_usrdata *d = vfio_group->usrdata;
>> 		d->put(d->data)
>> 		unlock
>>
>> [kvmgt.ko]
>>
>> 	call vfio_group_get_usrdata to get kvm,
>> 	call vfio_group_put_usrdata to release it
>> 	*never* call kvm_get_kvm/kvm_put_kvm