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

2016-09-20 Thread Jike Song 
On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
/* {snip} */

To show a straight-forward way of introducing an independent
struct device for the middle layer (in Kirti's patch the parent device,
we changed it to mdev_host since 'parent' is something too
generic or misleading) between physical device and mdev devices,
and how it will make the whole thing simpler, here is the incremental patch
against Kirti's version 7, which is exactly the same as the
the standalone version sent out Sep02.

This is only for demonstration. The sysfs interfaces changes are
kept although there are lots of discussions since.

--
Thanks,
Jike




diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 4a23c13..7c70753 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -7,4 +7,4 @@ obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
 obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o
 obj-$(CONFIG_VFIO_PCI) += pci/
 obj-$(CONFIG_VFIO_PLATFORM) += platform/
-obj-$(CONFIG_VFIO_MDEV) += mdev/
+obj-$(CONFIG_MDEV) += mdev/
diff --git a/drivers/vfio/mdev/Kconfig b/drivers/vfio/mdev/Kconfig
index 703abd0..d25439f 100644
--- a/drivers/vfio/mdev/Kconfig
+++ b/drivers/vfio/mdev/Kconfig
@@ -1,5 +1,5 @@
 
-config VFIO_MDEV
+config MDEV
 tristate "Mediated device driver framework"
 depends on VFIO
 default n
@@ -8,11 +8,3 @@ config VFIO_MDEV
See Documentation/vfio-mediated-device.txt for more details.
 
 If you don't know what do here, say N.
-
-config VFIO_MDEV_DEVICE
-tristate "VFIO support for Mediated devices"
-depends on VFIO && VFIO_MDEV
-default n
-help
-VFIO based driver for mediated devices.
-
diff --git a/drivers/vfio/mdev/Makefile b/drivers/vfio/mdev/Makefile
index e5087ed..8bd78b5 100644
--- a/drivers/vfio/mdev/Makefile
+++ b/drivers/vfio/mdev/Makefile
@@ -1,6 +1,4 @@
 
 mdev-y := mdev_core.o mdev_sysfs.o mdev_driver.o
 
-obj-$(CONFIG_VFIO_MDEV) += mdev.o
-obj-$(CONFIG_VFIO_MDEV_DEVICE) += vfio_mdev.o
-
+obj-$(CONFIG_MDEV) += mdev.o
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 9f278c7..cb27ccf 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -5,6 +5,11 @@
  * Author: Neo Jia 
  *Kirti Wankhede 
  *
+ * Copyright (c) 2016 Intel Corporation.
+ * Author:
+ * Xiao Guangrong 
+ * Jike Song 
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
@@ -23,316 +28,144 @@
 
 #include "mdev_private.h"
 
-#define DRIVER_VERSION "0.1"
+#define DRIVER_VERSION "0.2"
 #define DRIVER_AUTHOR  "NVIDIA Corporation"
-#define DRIVER_DESC"Mediated device Core Driver"
-
-static LIST_HEAD(parent_list);
-static DEFINE_MUTEX(parent_list_lock);
-
-static int mdev_add_attribute_group(struct device *dev,
-   const struct attribute_group **groups)
-{
-   return sysfs_create_groups(>kobj, groups);
-}
-
-static void mdev_remove_attribute_group(struct device *dev,
-   const struct attribute_group **groups)
-{
-   sysfs_remove_groups(>kobj, groups);
-}
-
-/* Should be called holding parent->mdev_list_lock */
-static struct mdev_device *__find_mdev_device(struct parent_device *parent,
- uuid_le uuid)
-{
-   struct mdev_device *mdev;
-
-   list_for_each_entry(mdev, >mdev_list, next) {
-   if (uuid_le_cmp(mdev->uuid, uuid) == 0)
-   return mdev;
-   }
-   return NULL;
-}
-
-/* Should be called holding parent_list_lock */
-static struct parent_device *__find_parent_device(struct device *dev)
-{
-   struct parent_device *parent;
-
-   list_for_each_entry(parent, _list, next) {
-   if (parent->dev == dev)
-   return parent;
-   }
-   return NULL;
-}
+#define DRIVER_DESC"Mediated Device Core Driver"
 
-static void mdev_release_parent(struct kref *kref)
-{
-   struct parent_device *parent = container_of(kref, struct parent_device,
-   ref);
-   kfree(parent);
-}
 
-static
-inline struct parent_device *mdev_get_parent(struct parent_device *parent)
+static int __find_mdev_device(struct device *dev, void *data)
 {
-   if (parent)
-   kref_get(>ref);
-
-   return parent;
-}
+   struct mdev_device *mdev = dev_to_mdev(dev);
 
-static inline void mdev_put_parent(struct parent_device *parent)
-{
-   if (parent)
-   kref_put(>ref, mdev_release_parent);
+   return (uuid_le_cmp(mdev->uuid, *(uuid_le *)data) == 0);
 }
 
-static struct parent_device *mdev_get_parent_from_dev(struct device *dev)
+static struct mdev_device

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

2016-09-19 Thread Alex Williamson 
On Tue, 20 Sep 2016 01:39:19 +0530
Kirti Wankhede  wrote:

> On 9/19/2016 11:41 PM, Alex Williamson wrote:
> > On Mon, 19 Sep 2016 22:59:34 +0530
> > Kirti Wankhede  wrote:
> >   
> >> On 9/12/2016 9:23 PM, Alex Williamson wrote:  
> >>> On Mon, 12 Sep 2016 13:19:11 +0530
> >>> Kirti Wankhede  wrote:
> >>> 
>  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  wrote:
> >>>  
> >> +
> >> +static struct parent_device *mdev_get_parent_from_dev(struct 
> >> device *dev)
> >> +{
> >> +  struct parent_device *parent;
> >> +
> >> +  mutex_lock(_list_lock);
> >> +  parent = mdev_get_parent(__find_parent_device(dev));
> >> +  mutex_unlock(_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.
> >
> > pdevmdev_host   mdev_device
> > 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?
> >>>
> >>> Is there anything implicitly wrong with using a device node to host the
> >>> mdev child devices?  Is the argument against it only that it's
> >>> unnecessary?  Can we make use of the device-core parent/child
> >>> dependencies as Jike has done w/o that extra node?
> >>>
> >>
> >> I do feel that mdev core module would get simplified with the new sysfs
> >> interface without having extra node.  
> > 
> > Can you provide an example of why that is?
> >
> 
> 'online' or earlier 'start'/'stop' interface is removed and would add
> open() and close() callbacks. ref count from struct mdev_device and
> mdev_get_device()/mdev_put_device() were added for this interface, these
> would go away.
> Using device-core parent/child dependencies between physical device and
> mdev device, other functions would get simplified.

Yes, we've refined the interface over time and I'm glad that you're
incorporating the device-core simplifications, but this doesn't really
argue for or against the extra device node.
 
> >> For example, directory structure we have now is:
> >> /sys/bus/pci/devices/\:85\:00.0/
> >>
> >> mdev devices are in real parents directory.
> >>
> >> By introducing fake device it would be:
> >> /sys/bus/pci/devices/\:85\:00.0/mdev-host/
> >>
> >> mdev devices are in fake device's directory.
> >>  
> >
> > Yes, this is the wanted directory.
> >   
> 
>  I don't think so.
> >>>
> >>> Why?
> >>> 
> >>
> >> This directory is not mandatory. right?  
> > 
> > Clearly you've done an implementation without it, so it's not
> > functionally mandatory, but Jike has made significant code reduction
> > and simplification with it.  Those are desirable things.
> >   
> >> 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

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

2016-09-19 Thread Kirti Wankhede 


On 9/19/2016 11:41 PM, Alex Williamson wrote:
> On Mon, 19 Sep 2016 22:59:34 +0530
> Kirti Wankhede  wrote:
> 
>> On 9/12/2016 9:23 PM, Alex Williamson wrote:
>>> On Mon, 12 Sep 2016 13:19:11 +0530
>>> Kirti Wankhede  wrote:
>>>   
 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  wrote:
>>>
>> +
>> +static struct parent_device *mdev_get_parent_from_dev(struct device 
>> *dev)
>> +{
>> +struct parent_device *parent;
>> +
>> +mutex_lock(_list_lock);
>> +parent = mdev_get_parent(__find_parent_device(dev));
>> +mutex_unlock(_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.
>
> pdevmdev_host   mdev_device
> 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?  
>>>
>>> Is there anything implicitly wrong with using a device node to host the
>>> mdev child devices?  Is the argument against it only that it's
>>> unnecessary?  Can we make use of the device-core parent/child
>>> dependencies as Jike has done w/o that extra node?
>>>  
>>
>> I do feel that mdev core module would get simplified with the new sysfs
>> interface without having extra node.
> 
> Can you provide an example of why that is?
>  

'online' or earlier 'start'/'stop' interface is removed and would add
open() and close() callbacks. ref count from struct mdev_device and
mdev_get_device()/mdev_put_device() were added for this interface, these
would go away.
Using device-core parent/child dependencies between physical device and
mdev device, other functions would get simplified.

>> For example, directory structure we have now is:
>> /sys/bus/pci/devices/\:85\:00.0/
>>
>> mdev devices are in real parents directory.
>>
>> By introducing fake device it would be:
>> /sys/bus/pci/devices/\:85\:00.0/mdev-host/
>>
>> mdev devices are in fake device's directory.
>>
>
> Yes, this is the wanted directory.
> 

 I don't think so.  
>>>
>>> Why?
>>>   
>>
>> This directory is not mandatory. right?
> 
> Clearly you've done an implementation without it, so it's not
> functionally mandatory, but Jike has made significant code reduction
> and simplification with it.  Those are desirable things.
> 
>> 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.  
>>>
>>> I don't follow the reply here, but aiui there's ordering implicit in
>>> the device core that Jike is trying to take advantage of that
>>> simplifies the mdev layer significantly.  In the case of an
>>> mdev_create, the device core needs to take a reference to the parent
>>> object, the mdev-host I'd guess in Jike's version, the created mdev
>>> device would also have a reference to that object, so the physical host
>>> device could not be removed so long as there are outstanding
>>> references.  It's just a matter of managing

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

2016-09-19 Thread Alex Williamson 
On Mon, 19 Sep 2016 22:59:34 +0530
Kirti Wankhede  wrote:

> On 9/12/2016 9:23 PM, Alex Williamson wrote:
> > On Mon, 12 Sep 2016 13:19:11 +0530
> > Kirti Wankhede  wrote:
> >   
> >> 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  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(>kobj, groups);
>  +}
>  +
>  +static void mdev_remove_attribute_group(struct device *dev,
>  +const struct attribute_group 
>  **groups)
>  +{
>  +sysfs_remove_groups(>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(>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(_list_lock);
>  +parent = mdev_get_parent(__find_parent_device(dev));
>  +mutex_unlock(_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.
> >>>
> >>> pdevmdev_host   mdev_device
> >>> 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?  
> > 
> > Is there anything implicitly wrong with using a device node to host the
> > mdev child devices?  Is the argument against it only that it's
> > unnecessary?  Can we make use of the device-core parent/child
> > dependencies as Jike has done w/o that extra node?
> >  
> 
> I do feel that mdev

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

2016-09-19 Thread Kirti Wankhede 


On 9/12/2016 9:23 PM, Alex Williamson wrote:
> On Mon, 12 Sep 2016 13:19:11 +0530
> Kirti Wankhede  wrote:
> 
>> 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  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(>kobj, groups);
 +}
 +
 +static void mdev_remove_attribute_group(struct device *dev,
 +  const struct attribute_group 
 **groups)
 +{
 +  sysfs_remove_groups(>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(>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(_list_lock);
 +  parent = mdev_get_parent(__find_parent_device(dev));
 +  mutex_unlock(_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.
>>>
>>> pdevmdev_host   mdev_device
>>> 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?
> 
> Is there anything implicitly wrong with using a device node to host the
> mdev child devices?  Is the argument against it only that it's
> unnecessary?  Can we make use of the device-core parent/child
> dependencies as Jike has done w/o that extra node?
>

I do feel that mdev core module would get simplified with the new sysfs
interface without having extra node.


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

 mdev devices are in real parents directory.

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

 mdev devices are

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

2016-09-19 Thread Jike Song 
On 09/12/2016 11:53 PM, Alex Williamson wrote:
> On Mon, 12 Sep 2016 13:19:11 +0530
> Kirti Wankhede  wrote:
> 
>> 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  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(>kobj, groups);
 +}
 +
 +static void mdev_remove_attribute_group(struct device *dev,
 +  const struct attribute_group 
 **groups)
 +{
 +  sysfs_remove_groups(>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(>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(_list_lock);
 +  parent = mdev_get_parent(__find_parent_device(dev));
 +  mutex_unlock(_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.
>>>
>>> pdevmdev_host   mdev_device
>>> 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?
> 
> Is there anything implicitly wrong with using a device node to host the
> mdev child devices?  Is the argument against it only that it's
> unnecessary?  Can we make use of the device-core parent/child
> dependencies as Jike has done w/o that extra node?
>  
 For example, directory structure we have now is:
 /sys/bus/pci/devices/\:85\:00.0/

 mdev devices are in real parents directory.

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

 mdev devices are in fake device's directory.
  
>>>
>>> Yes, this is the wanted directory.
>>>   
>>
>> I don't think so.
>

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

2016-09-12 Thread Alex Williamson 
On Mon, 12 Sep 2016 13:19:11 +0530
Kirti Wankhede  wrote:

> 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  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(>kobj, groups);
> >> +}
> >> +
> >> +static void mdev_remove_attribute_group(struct device *dev,
> >> +  const struct attribute_group 
> >> **groups)
> >> +{
> >> +  sysfs_remove_groups(>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(>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(_list_lock);
> >> +  parent = mdev_get_parent(__find_parent_device(dev));
> >> +  mutex_unlock(_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.
> > 
> > pdevmdev_host   mdev_device
> > 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?

Is there anything implicitly wrong with using a device node to host the
mdev child devices?  Is the argument against it only that it's
unnecessary?  Can we make use of the device-core parent/child
dependencies as Jike has done w/o that extra node?
 
> >> For example, directory structure we have now is:
> >> /sys/bus/pci/devices/\:85\:00.0/
> >>
> >> mdev devices are in real parents directory.
> >>
> >> By introducing fake device it would be:
> >> /sys/bus/pci/devices/\:85\:00.0/mdev-host/
> >>
> >> mdev devices are in fake device's directory.
> >>  
> > 
> > Yes, this is the wanted directory.
> >   
> 
> I don't think so.

Why?

> >> Lock would be still required, to handle the race

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

2016-09-12 Thread Kirti Wankhede 


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  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(>kobj, groups);
>> +}
>> +
>> +static void mdev_remove_attribute_group(struct device *dev,
>> +const struct attribute_group 
>> **groups)
>> +{
>> +sysfs_remove_groups(>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(>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(_list_lock);
>> +parent = mdev_get_parent(__find_parent_device(dev));
>> +mutex_unlock(_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.
> 
> pdevmdev_host   mdev_device
> 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/\:85\:00.0/
>>
>> mdev devices are in real parents directory.
>>
>> By introducing fake device it would be:
>> /sys/bus/pci/devices/\:85\:00.0/mdev-host/
>>
>> 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

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

2016-09-11 Thread Jike Song 
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  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(>kobj, groups);
> +}
> +
> +static void mdev_remove_attribute_group(struct device *dev,
> + const struct attribute_group **groups)
> +{
> + sysfs_remove_groups(>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(>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(_list_lock);
> + parent = mdev_get_parent(__find_parent_device(dev));
> + mutex_unlock(_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.

pdevmdev_host   mdev_device
dev For example, directory structure we have now is:
> /sys/bus/pci/devices/\:85\:00.0/
> 
> mdev devices are in real parents directory.
> 
> By introducing fake device it would be:
> /sys/bus/pci/devices/\:85\:00.0/mdev-host/
> 
> mdev devices are in fake device's directory.
>

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

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?

> 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

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

2016-09-09 Thread Kirti Wankhede 


On 9/10/2016 12:12 AM, Alex Williamson wrote:
> On Fri, 9 Sep 2016 23:18:45 +0530
> Kirti Wankhede  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(>kobj, groups);
 +}
 +
 +static void mdev_remove_attribute_group(struct device *dev,
 +  const struct attribute_group **groups)
 +{
 +  sysfs_remove_groups(>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(>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(_list_lock);
 +  parent = mdev_get_parent(__find_parent_device(dev));
 +  mutex_unlock(_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/\:85\:00.0/

mdev devices are in real parents directory.

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

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

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

2016-09-09 Thread Alex Williamson 
On Fri, 9 Sep 2016 23:18:45 +0530
Kirti Wankhede  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(>kobj, groups);
> >> +}
> >> +
> >> +static void mdev_remove_attribute_group(struct device *dev,
> >> +  const struct attribute_group **groups)
> >> +{
> >> +  sysfs_remove_groups(>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(>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(_list_lock);
> >> +  parent = mdev_get_parent(__find_parent_device(dev));
> >> +  mutex_unlock(_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,

Alex

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

2016-09-09 Thread Kirti Wankhede 


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(>kobj, groups);
>> +}
>> +
>> +static void mdev_remove_attribute_group(struct device *dev,
>> +const struct attribute_group **groups)
>> +{
>> +sysfs_remove_groups(>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(>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(_list_lock);
>> +parent = mdev_get_parent(__find_parent_device(dev));
>> +mutex_unlock(_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.


> PS, "parent" is somehow a name too generic?
>

This is the term used in Linux Kernel for such cases. See 'struct
device' in include/linux/device.h. I would prefer 'parent'.

>> +
>> +static int mdev_device_create_ops(struct mdev_device *mdev, char 
>> *mdev_params)
>> +{
>> +struct parent_device *parent = mdev->parent;
>> +int ret;
>> +
>> +ret = parent->ops->create(mdev, mdev_params);
>> +if (ret)
>> +return ret;
>> +
>> +ret = mdev_add_attribute_group(>dev,
>> +parent->ops->mdev_attr_groups);
> 
> Ditto: dev->groups.
> 

See my above response for why this is indented to be so.


>> +ret = parent_create_sysfs_files(dev);
>> +if (ret)
>> +goto add_sysfs_error;
>> +
>> +ret = mdev_add_attribute_group(dev, ops->dev_attr_groups);
> 
> parent_create_sysfs_files and mdev_add_attribute_group are kind of doing
> the same thing, do you mind to merge them into one?
> 

Ok. I'll see I can do that.


>> +int mdev_device_get_online_status(struct device *dev, bool *online)
>> +{
>> +int ret = 0;
>> +struct mdev_device *mdev;
>> +struct parent_device *parent;
>> +
>> +mdev = mdev_get_device(to_mdev_device(dev));
>> +if (!mdev)
>> +return -EINVAL;
>> +
>> +parent = mdev->parent;
>> +
>> +if (parent->ops->get_online_status)
>> +ret = parent->ops->get_online_status(mdev, online);
>> +
>> +mdev_put_device(mdev);
>> +
>> +return ret;
>> +}
> 
> The driver core has a perfect 'online' file for a device, with both
> 'show' and 'store' support, you don't need to write another one.
> 
> Please have a look at online_show and online_store in drivers/base/core.c.
> 

This is going to be removed as per the latest discussion.


> +
>> +extern struct class_attribute mdev_class_attrs[];
> 
> This is useless?
>

Oh, I missed to remove it. Thanks for pointing that out.


>> +static ssize_t mdev_create_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> +char *str, *pstr;
>> +char *uuid_str, *mdev_params = NULL, *params = NULL;
>> +uuid_le uuid;
>> +int ret;
>> +
>> +pstr = str = kstrndup(buf, count, GFP_KERNEL);
> 
> pstr is not used.
>

It is used below to free duped memory. If you see below code, 'str'
pointer gets incremented on strsep, so that can't be used to free
memory. pstr points to start of memory which we want to free while
returning from this function.

>> +

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

2016-09-09 Thread Jike Song 
On 09/08/2016 05:38 PM, Neo Jia wrote:
> On Thu, Sep 08, 2016 at 04:09:39PM +0800, Jike Song wrote:
>> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
>>> +
>>> +/**
>>> + * struct parent_ops - Structure to be registered for each parent device to
>>> + * register the device to mdev module.
>>> + *
>>> + * @owner: The module owner.
>>> + * @dev_attr_groups:   Default attributes of the parent device.
>>> + * @mdev_attr_groups:  Default attributes of the mediated device.
>>> + * @supported_config:  Called to get information about supported types.
>>> + * @dev : device structure of parent device.
>>> + * @config: should return string listing supported config
>>> + * Returns integer: success (0) or error (< 0)
>>> + * @create:Called to allocate basic resources in parent 
>>> device's
>>> + * driver for a particular mediated device. It is
>>> + * mandatory to provide create ops.
>>> + * @mdev: mdev_device structure on of mediated device
>>> + *   that is being created
>>> + * @mdev_params: extra parameters required by parent
>>> + * device's driver.
>>> + * Returns integer: success (0) or error (< 0)
>>> + * @destroy:   Called to free resources in parent device's 
>>> driver for a
>>> + * a mediated device. It is mandatory to provide destroy
>>> + * ops.
>>> + * @mdev: mdev_device device structure which is being
>>> + *destroyed
>>> + * Returns integer: success (0) or error (< 0)
>>> + * If VMM is running and destroy() is called that means the
>>> + * mdev is being hotunpluged. Return error if VMM is
>>> + * running and driver doesn't support mediated device
>>> + * hotplug.
>>> + * @reset: Called to reset mediated device.
>>> + * @mdev: mdev_device device structure.
>>> + * Returns integer: success (0) or error (< 0)
>>> + * @set_online_status: Called to change to status of mediated device.
>>> + * @mdev: mediated device.
>>> + * @online: set true or false to make mdev device online or
>>> + * offline.
>>> + * Returns integer: success (0) or error (< 0)
>>> + * @get_online_status: Called to get online/offline status of  
>>> mediated device
>>> + * @mdev: mediated device.
>>> + * @online: Returns status of mediated device.
>>> + * Returns integer: success (0) or error (< 0)
>>> + * @read:  Read emulation callback
>>> + * @mdev: mediated device structure
>>> + * @buf: read buffer
>>> + * @count: number of bytes to read
>>> + * @pos: address.
>>> + * Retuns number on bytes read on success or error.
>>> + * @write: Write emulation callback
>>> + * @mdev: mediated device structure
>>> + * @buf: write buffer
>>> + * @count: number of bytes to be written
>>> + * @pos: address.
>>> + * Retuns number on bytes written on success or error.
>>> + * @get_irq_info:  Called to retrieve information about mediated device IRQ
>>> + * @mdev: mediated device structure
>>> + * @irq_info: VFIO IRQ flags and count.
>>> + * Returns integer: success (0) or error (< 0)
>>> + * @set_irqs:  Called to send about interrupts configuration
>>> + * information that VMM sets.
>>> + * @mdev: mediated device structure
>>> + * @flags, index, start, count and *data : same as that of
>>> + * struct vfio_irq_set of VFIO_DEVICE_SET_IRQS API.
>>> + * @get_device_info:   Called to get VFIO device information for a 
>>> mediated
>>> + * device.
>>> + * @vfio_device_info: VFIO device info.
>>> + * Returns integer: success (0) or error (< 0)
>>> + * @get_region_info:   Called to get VFIO region size and flags of 
>>> mediated
>>> + * device.
>>> + * @mdev: mediated device structure
>>> + * @region_info: output, returns size and flags of
>>> + *   requested region.
>>> + * @cap_type_id: returns id of capability.
>>> + * @cap_type: returns pointer to capability structure
>>> + * corresponding to capability id.
>>> + * Returns integer: success (0) or error (< 0)
>>> + *
>>> + * Parent device that support mediated device should be registered with 
>>> mdev
>>> + * module with parent_ops structure.
>>> + */
>>> +
>>> +struct parent_ops {
>>> +   struct module

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

2016-09-08 Thread Neo Jia 
On Thu, Sep 08, 2016 at 04:09:39PM +0800, Jike Song wrote:
> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
> > +
> > +/**
> > + * struct parent_ops - Structure to be registered for each parent device to
> > + * register the device to mdev module.
> > + *
> > + * @owner: The module owner.
> > + * @dev_attr_groups:   Default attributes of the parent device.
> > + * @mdev_attr_groups:  Default attributes of the mediated device.
> > + * @supported_config:  Called to get information about supported types.
> > + * @dev : device structure of parent device.
> > + * @config: should return string listing supported config
> > + * Returns integer: success (0) or error (< 0)
> > + * @create:Called to allocate basic resources in parent 
> > device's
> > + * driver for a particular mediated device. It is
> > + * mandatory to provide create ops.
> > + * @mdev: mdev_device structure on of mediated device
> > + *   that is being created
> > + * @mdev_params: extra parameters required by parent
> > + * device's driver.
> > + * Returns integer: success (0) or error (< 0)
> > + * @destroy:   Called to free resources in parent device's 
> > driver for a
> > + * a mediated device. It is mandatory to provide destroy
> > + * ops.
> > + * @mdev: mdev_device device structure which is being
> > + *destroyed
> > + * Returns integer: success (0) or error (< 0)
> > + * If VMM is running and destroy() is called that means the
> > + * mdev is being hotunpluged. Return error if VMM is
> > + * running and driver doesn't support mediated device
> > + * hotplug.
> > + * @reset: Called to reset mediated device.
> > + * @mdev: mdev_device device structure.
> > + * Returns integer: success (0) or error (< 0)
> > + * @set_online_status: Called to change to status of mediated device.
> > + * @mdev: mediated device.
> > + * @online: set true or false to make mdev device online or
> > + * offline.
> > + * Returns integer: success (0) or error (< 0)
> > + * @get_online_status: Called to get online/offline status of  
> > mediated device
> > + * @mdev: mediated device.
> > + * @online: Returns status of mediated device.
> > + * Returns integer: success (0) or error (< 0)
> > + * @read:  Read emulation callback
> > + * @mdev: mediated device structure
> > + * @buf: read buffer
> > + * @count: number of bytes to read
> > + * @pos: address.
> > + * Retuns number on bytes read on success or error.
> > + * @write: Write emulation callback
> > + * @mdev: mediated device structure
> > + * @buf: write buffer
> > + * @count: number of bytes to be written
> > + * @pos: address.
> > + * Retuns number on bytes written on success or error.
> > + * @get_irq_info:  Called to retrieve information about mediated device IRQ
> > + * @mdev: mediated device structure
> > + * @irq_info: VFIO IRQ flags and count.
> > + * Returns integer: success (0) or error (< 0)
> > + * @set_irqs:  Called to send about interrupts configuration
> > + * information that VMM sets.
> > + * @mdev: mediated device structure
> > + * @flags, index, start, count and *data : same as that of
> > + * struct vfio_irq_set of VFIO_DEVICE_SET_IRQS API.
> > + * @get_device_info:   Called to get VFIO device information for a 
> > mediated
> > + * device.
> > + * @vfio_device_info: VFIO device info.
> > + * Returns integer: success (0) or error (< 0)
> > + * @get_region_info:   Called to get VFIO region size and flags of 
> > mediated
> > + * device.
> > + * @mdev: mediated device structure
> > + * @region_info: output, returns size and flags of
> > + *   requested region.
> > + * @cap_type_id: returns id of capability.
> > + * @cap_type: returns pointer to capability structure
> > + * corresponding to capability id.
> > + * Returns integer: success (0) or error (< 0)
> > + *
> > + * Parent device that support mediated device should be registered with 
> > mdev
> > + * module with parent_ops structure.
> > + */
> > +
> > +struct parent_ops {
> > +   struct module   *owner;
> > +   const struct

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

2016-09-08 Thread Jike Song 
On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
> Design for Mediated Device Driver:
> Main purpose of this driver is to provide a common interface for mediated
> device management that can be used by different drivers of different
> devices.
> 
> This module provides a generic interface to create the device, add it to
> mediated bus, add device to IOMMU group and then add it to vfio group.
> 
> Below is the high Level block diagram, with Nvidia, Intel and IBM devices
> as example, since these are the devices which are going to actively use
> this module as of now.
> 
>  +---+
>  |   |
>  | +---+ |  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?

>  |  MDEV CORE|
>  |   MODULE  |
>  |   mdev.ko |
>  | +---+ |  mdev_register_device() +--+
>  | |   | +<+  |
>  | |   | | |  nvidia.ko   |<-> physical
>  | |   | +>+  |device
>  | |   | |callback +--+
>  | | Physical  | |
>  | |  device   | |  mdev_register_device() +--+
>  | | interface | |<+  |
>  | |   | | |  i915.ko |<-> physical
>  | |   | +>+  |device
>  | |   | |callback +--+
>  | |   | |
>  | |   | |  mdev_register_device() +--+
>  | |   | +<+  |
>  | |   | | | ccw_device.ko|<-> physical
>  | |   | +>+  |device
>  | |   | |callback +--+
>  | +---+ |
>  +---+
> 
> Core driver provides two types of registration interfaces:
> 1. Registration interface for mediated bus driver:
> 
> /**
>   * struct mdev_driver - Mediated device's driver
>   * @name: driver name
>   * @probe: called when new device created
>   * @remove:called when device removed
>   * @driver:device driver structure
>   *
>   **/
> struct mdev_driver {
>  const char *name;
>  int  (*probe)  (struct device *dev);
>  void (*remove) (struct device *dev);
>  struct device_driverdriver;
> };
> 
> int  mdev_register_driver(struct mdev_driver *drv, struct module *owner);
> void mdev_unregister_driver(struct mdev_driver *drv);
> 
> Mediated device's driver for mdev, vfio_mdev, uses this interface to
> register with Core driver. vfio_mdev module adds mediated device to VFIO
> group.
> 
> 2. Physical device driver interface
> This interface provides vendor driver the set APIs to manage physical
> device related work in their own driver. APIs are :
> - supported_config: provide supported configuration list by the vendor
>   driver
> - create: to allocate basic resources in vendor driver for a mediated
> device.
> - destroy: to free resources in vendor driver when mediated device is
>  destroyed.
> - reset: to free and reallocate resources in vendor driver during device
>reset.
> - set_online_status: to change online status of mediated device.
> - get_online_status: to get current (online/offline) status of mediated
>device.
> - read : read emulation callback.
> - write: write emulation callback.
> - mmap: mmap emulation callback.
> - get_irq_info: to retrieve information about mediated device's IRQ.
> - set_irqs: send interrupt configuration information that VMM sets.
> - get_device_info: to retrieve VFIO device related flags, number of regions
>  and number of IRQs supported.
> - get_region_info: to provide region size and its flags for the mediated
>  device.
> 
> This registration interface should be used by vendor drivers to register
> each physical device to mdev core driver.
> Locks to serialize above callbacks are removed. If required, vendor driver
> can have locks to serialize above APIs in their driver.
> 
> Signed-off-by: Kirti Wankhede 
> Signed-off-by: Neo Jia 
> Change-Id: I73a5084574270b14541c529461ea2f03c292d510
> Reviewed-on: http://git-master/r/1175705
> Reviewed-by: Automatic_Commit_Validation_User
> ---
>  drivers/vfio/Kconfig |   1 +
>  drivers/vfio/Makefile|   1 +
>  drivers/vfio/mdev/Kconfig|  12 +
>