Re: [Qemu-devel] [PATCH v7 1/4] vfio: Mediated device Core driver

[Kirti Wankhede]

On 9/10/2016 12:12 AM, Alex Williamson wrote:
> On Fri, 9 Sep 2016 23:18:45 +0530
> Kirti Wankhede <address@hidden> wrote:
> 
>> On 9/8/2016 1:39 PM, Jike Song wrote:
>>> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:  
>>
>>>>  +---------------+
>>>>  |               |
>>>>  | +-----------+ |  mdev_register_driver() +--------------+
>>>>  | |           | +<------------------------+ __init()     |
>>>>  | |  mdev     | |                         |              |
>>>>  | |  bus      | +------------------------>+              |<-> VFIO user
>>>>  | |  driver   | |     probe()/remove()    | vfio_mdev.ko |    APIs
>>>>  | |           | |                         |              |
>>>>  | +-----------+ |                         +--------------+
>>>>  |               |  
>>>
>>> This aimed to have only one single vfio bus driver for all mediated devices,
>>> right?
>>>  
>>
>> Yes. That's correct.
>>
>>
>>>> +
>>>> +static int mdev_add_attribute_group(struct device *dev,
>>>> +                              const struct attribute_group **groups)
>>>> +{
>>>> +  return sysfs_create_groups(&dev->kobj, groups);
>>>> +}
>>>> +
>>>> +static void mdev_remove_attribute_group(struct device *dev,
>>>> +                                  const struct attribute_group **groups)
>>>> +{
>>>> +  sysfs_remove_groups(&dev->kobj, groups);
>>>> +}  
>>>
>>> These functions are not necessary. You can always specify the attribute 
>>> groups
>>> to dev->groups before registering a new device.
>>>   
>>
>> At the time of mdev device create, I specifically didn't used
>> dev->groups because we callback in vendor driver before that, see below
>> code snippet, and those attributes should only be added if create()
>> callback returns success.
>>
>>         ret = parent->ops->create(mdev, mdev_params);
>>         if (ret)
>>                 return ret;
>>
>>         ret = mdev_add_attribute_group(&mdev->dev,
>>                                         parent->ops->mdev_attr_groups);
>>         if (ret)
>>                 parent->ops->destroy(mdev);
>>
>>
>>
>>>> +
>>>> +static struct parent_device *mdev_get_parent_from_dev(struct device *dev)
>>>> +{
>>>> +  struct parent_device *parent;
>>>> +
>>>> +  mutex_lock(&parent_list_lock);
>>>> +  parent = mdev_get_parent(__find_parent_device(dev));
>>>> +  mutex_unlock(&parent_list_lock);
>>>> +
>>>> +  return parent;
>>>> +}  
>>>
>>> As we have demonstrated, all these refs and locks and release workqueue are 
>>> not necessary,
>>> as long as you have an independent device associated with the mdev host 
>>> device
>>> ("parent" device here).
>>>  
>>
>> I don't think every lock will go away with that. This also changes how
>> mdev devices entries are created in sysfs. It adds an extra directory.
> 
> Exposing the parent-child relationship through sysfs is a desirable
> feature, so I'm not sure how this is a negative.  This part of Jike's
> conversion was a big improvement, I thought.  Thanks,
> 

Jike's suggestion is to introduced a fake device over parent device i.e.
mdev-host, and then all mdev devices are children of 'mdev-host' not
children of real parent.

For example, directory structure we have now is:
/sys/bus/pci/devices/0000\:85\:00.0/<mdev_device>

mdev devices are in real parents directory.

By introducing fake device it would be:
/sys/bus/pci/devices/0000\:85\:00.0/mdev-host/<mdev_device>

mdev devices are in fake device's directory.

Lock would be still required, to handle the race conditions like
'mdev_create' is still in process and parent device is unregistered by
vendor driver/ parent device is unbind from vendor driver.

With the new changes/discussion, we believe the locking will be
simplified without having fake parent device.

With fake device suggestion, removed pointer to parent device from
mdev_device structure. When a create(struct mdev_device *mdev) callback
comes to vendor driver, how would vendor driver know for which physical
device this mdev device create call is intended to? because then
'parent' would be newly introduced fake device, not the real parent.

Thanks,
Kirti