qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Kirti Wankhede
Subject: Re: [Qemu-devel] [PATCH v7 1/4] vfio: Mediated device Core driver
Date: Mon, 12 Sep 2016 13:19:11 +0530


On 9/12/2016 10:40 AM, Jike Song wrote:
> On 09/10/2016 03:55 AM, Kirti Wankhede wrote:
>> 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.
>>
> 
> It really depends on how you define 'real parent' :)
> 
> With a physical-host-mdev hierarchy, the parent of mdev devices is the host
> device, the parent of host device is the physical device. e.g.
> 
>         pdev            mdev_host       mdev_device
>         dev<------------dev<------------dev
>               parent          parent
> 
>         Figure 1: device hierarchy
> 

Right, mdev-host device doesn't represent physical device nor any mdev
device. Then what is the need of such device?

>> 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.
>>
> 
> Yes, this is the wanted directory.
> 

I don't think so.


>> 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.
>>
> 
> locks are provided to protect resources, would you elaborate more on
> what is the exact resource you want to protect by a lock in mdev_create?
> 

Simple, in your suggestion mdev-host device. Fake device will go away if
vendor driver unregisters the device from mdev module, right.

Thanks,
Kirti.

>> 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.
> 
> Please have a look at "Figure 1".
> 
> --
> Thanks,
> Jike
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]