RE: [RFC PATCH v4 01/10] driver core: export driver_probe_device()

* RE: [RFC PATCH v4 01/10] driver core: export driver_probe_device()
@ 2014-03-06 22:31 Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 23+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-03-06 22:31 UTC (permalink / raw)
  To: Stuart Yoder
  Cc: kvm-u79uwXL29TY76Z2rM5mHXA, jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ,
	will.deacon-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	Michal Hocko, Bjorn Helgaas, Varun Sethi,
	kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg, Rafael J. Wysocki,
	agraf-l3A5Bk7waGM, Guenter Roeck, Dmitry Kasatkin, Tejun Heo,
	Scott Wood, Antonios Motakis,
	tech-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J, Toshi Kani, Greg KH,
	a.rigo-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J,


On Mar 6, 2014 5:25 PM, Stuart Yoder <stuart.yoder@freescale.com> wrote:
>
>
>
> > -----Original Message----- 
> > From: Greg KH [mailto:gregkh@linuxfoundation.org] 
> > Sent: Thursday, February 20, 2014 4:44 PM 
> > To: Yoder Stuart-B08248 
> > Cc: Antonios Motakis; alex.williamson@redhat.com; 
> > kvmarm@lists.cs.columbia.edu; iommu@lists.linux-foundation.org; linux- 
> > kernel@vger.kernel.org; tech@virtualopensystems.com; 
> > a.rigo@virtualopensystems.com; kim.phillips@linaro.org; 
> > jan.kiszka@siemens.com; kvm@vger.kernel.org; Bhushan Bharat-R65777; Wood 
> > Scott-B07421; christoffer.dall@linaro.org; agraf@suse.de; Sethi Varun- 
> > B16395; will.deacon@arm.com; Tejun Heo; Rafael J. Wysocki; Guenter Roeck; 
> > Toshi Kani; Joe Perches; Dmitry Kasatkin; Michal Hocko; Bjorn Helgaas 
> > Subject: Re: [RFC PATCH v4 01/10] driver core: export 
> > driver_probe_device() 
> > 
> > On Thu, Feb 20, 2014 at 10:34:35PM +0000, Stuart Yoder wrote: 
> > > 
> > > 
> > > > -----Original Message----- 
> > > > From: Yoder Stuart-B08248 
> > > > Sent: Saturday, February 15, 2014 12:19 PM 
> > > > To: 'Greg KH' 
> > > > Cc: Antonios Motakis; alex.williamson@redhat.com; 
> > > > kvmarm@lists.cs.columbia.edu; iommu@lists.linux-foundation.org; 
> > linux- 
> > > > kernel@vger.kernel.org; tech@virtualopensystems.com; 
> > > > a.rigo@virtualopensystems.com; kim.phillips@linaro.org; 
> > > > jan.kiszka@siemens.com; kvm@vger.kernel.org; Bhushan Bharat-R65777; 
> > Wood 
> > > > Scott-B07421; christoffer.dall@linaro.org; agraf@suse.de; Sethi 
> > Varun- 
> > > > B16395; will.deacon@arm.com; Tejun Heo; Rafael J. Wysocki; Guenter 
> > Roeck; 
> > > > Toshi Kani; Joe Perches; Dmitry Kasatkin; Michal Hocko; Bjorn Helgaas 
> > > > Subject: RE: [RFC PATCH v4 01/10] driver core: export 
> > > > driver_probe_device() 
> > > > 
> > > > 
> > > > 
> > > > > -----Original Message----- 
> > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] 
> > > > > Sent: Saturday, February 15, 2014 11:34 AM 
> > > > > To: Yoder Stuart-B08248 
> > > > > Cc: Antonios Motakis; alex.williamson@redhat.com; 
> > > > > kvmarm@lists.cs.columbia.edu; iommu@lists.linux-foundation.org; 
> > linux- 
> > > > > kernel@vger.kernel.org; tech@virtualopensystems.com; 
> > > > > a.rigo@virtualopensystems.com; kim.phillips@linaro.org; 
> > > > > jan.kiszka@siemens.com; kvm@vger.kernel.org; Bhushan Bharat-R65777; 
> > > > Wood 
> > > > > Scott-B07421; christoffer.dall@linaro.org; agraf@suse.de; Sethi 
> > Varun- 
> > > > > B16395; will.deacon@arm.com; Tejun Heo; Rafael J. Wysocki; Guenter 
> > > > Roeck; 
> > > > > Toshi Kani; Joe Perches; Dmitry Kasatkin; Michal Hocko; Bjorn 
> > Helgaas 
> > > > > Subject: Re: [RFC PATCH v4 01/10] driver core: export 
> > > > > driver_probe_device() 
> > > > > 
> > > > > On Sat, Feb 15, 2014 at 04:33:44PM +0000, Stuart Yoder wrote: 
> > > > > > > > Why?  driver_probe_device() allows a driver to explicitly 
> > bind 
> > > > > > > > to a specific device.   What is conceptually wrong with 
> > allowing 
> > > > > > > > that? 
> > > > > > > 
> > > > > > > Because that's not how a bus should work, and the fact that no 
> > > > other 
> > > > > > > subsystem in the kernel does that might be a hint you are 
> > trying to 
> > > > > do 
> > > > > > > something a bit "wrong" here. 
> > > > > > 
> > > > > > Let me try to succinctly as I can describe the problem we are 
> > trying 
> > > > to 
> > > > > > solve here... 
> > > > > > 
> > > > > > The vfio mechanism in the kernel (e.g. vfio-pci) allows devices 
> > to be 
> > > > > > exposed user space (via file descriptors), enabling user space 
> > > > > > drivers.  So, for example to export an e1000 card to user space, 
> > I do 
> > > > > > this: 
> > > > > > 
> > > > > >    echo 0001:03:00.0 > 
> > > > /sys/bus/pci/devices/0001:03:00.0/driver/unbind 
> > > > > >    echo 8086 10d3 > /sys/bus/pci/drivers/vfio-pci/new_id 
> > > > > 
> > > > > What's wrong with using the "bind" file instead?  That picks a 
> > specific 
> > > > > device and binds it to a specific driver.  Or have we been down 
> > this 
> > > > > path before?  :) 
> > > > 
> > > > Yes we have :)  The "bind" file does not bypass device ID checks, so 
> > > > it wouldn't work without new_id or a wildcard match of some kind. 
> > > > 
> > > > > And that is for a PCI "driver" not a totally separate bus, which it 
> > > > > looks like you are wanting to do here. 
> > > > 
> > > > vfio-pci is a PCI driver, not a bus (drivers/vfio/pci/vfio_pci.c). 
> > > > 
> > > > > > The first step unbinds the target device (0001:03:00.0) from the 
> > > > normal 
> > > > > > e1000 driver. 
> > > > > > 
> > > > > > The second step causes the vfio-pci driver to bind to device 
> > > > > 0001:03:00.0. 
> > > > > > This second step tells vfio-pci that it now handles e1000 device 
> > IDs, 
> > > > > > and the vfio-pci drivers registers with the PCI bus to handle 
> > '8086 
> > > > > 10d3'. 
> > > > > > 
> > > > > > That works, but it is ugly.  We now have 2 active drivers 
> > handling 
> > > > > > the same device type...which introduces various possible race 
> > > > > conditions. 
> > > > > > 
> > > > > > We never want vfio-pci to auto-bind to any new device that shows 
> > up 
> > > > > > on the PCI bus.  Binding a device to vfio-pci must be an explicit 
> > > > > > action by an administrator. 
> > > > > 
> > > > > Then use the "bind" file. 
> > > > 
> > > > See above. 
> > > > 
> > > > > > You mentioned previously that user space can sort out the problem 
> > > > > > of multiple drivers registered for handling the same device type. 
> > > > > > That is true, but doesn't help here.   We don't want vfio-pci 
> > > > > > to handle _all_ e1000 cards, just explicitly selected e1000 
> > cards. 
> > > > > > 
> > > > > > We want the normal e1000 driver to be loaded and to bind to new 
> > > > > > devices that may be hot-plugged. 
> > > > > 
> > > > > I want a pony too... 
> > > > 
> > > > It's not that difficult...this patch accomplishes it by 
> > > > simply allowing drivers to call driver_probe_device(). 
> > > > 
> > > > > > There are 2 proposed mechanisms that have been put forth, both of 
> > > > > > which you have now rejected: 
> > > > > > 
> > > > > >    1.  sysfs_bind_only flag was proposed which would allow a vfio 
> > > > > >        driver (like vfio-pci) to only bind by explicit request 
> > > > through 
> > > > > >        the sysfs 'bind' file. 
> > > > > 
> > > > > Why did I reject this?  What did the patch look like? 
> > > > 
> > > > https://lkml.org/lkml/2013/12/3/253 
> > > > 
> > > > 
> > > > > >    2.  Have the vfio driver call driver_probe_device() to 
> > explicitly 
> > > > > bind 
> > > > > >        a particular device instance to the driver.  Only change 
> > we 
> > > > need 
> > > > > >        here is the EXPORT_SYMBOL. 
> > > > > 
> > > > > How are you going to prevent the driver from being bound to the 
> > device 
> > > > > in the core with this change?  How are you going to call this 
> > function? 
> > > > > When?  On what action of the user? 
> > > > 
> > > > The vfio-pci driver would create a sysfs object "vfio_bind". 
> > > > 
> > > > User would do: 
> > > >    echo 0001:03:00.0 > /sys/bus/pci/drivers/vfio-pci/vfio_bind 
> > > > 
> > > > vfio-pci would call driver_probe_device() which binds 
> > > > the specific device to the vfio-pci driver...and there is 
> > > > no ambiguity. 
> > > > 
> > > > > > Are you in principle opposed to any mechanism that would allow 2 
> > > > > drivers 
> > > > > > to be resident/active and allow a sysadmin to explicitly bind a 
> > > > > > particular device instance to the driver of their choice? 
> > > > > 
> > > > > No, that works today with the bind/unbind/new_id files, it's just 
> > that 
> > > > > you don't like it :) 
> > > > 
> > > > We don't like it because of the ambiguities/race-conditions with 
> > > > the current situation. 
> > > > 
> > > > A vfio driver, like vfio-pci, certainly is a bit different than a 
> > normal 
> > > > driver, in that it really is not device ID aware.  It simply passes 
> > > > through device resources (mappable regions, IRQs) to user space 
> > without 
> > > > interpreting or understanding them.  It is kind of a "meta" driver, 
> > but 
> > > > it is not a bus.  Every bus type would need its own vfio driver to 
> > > > do this type of device pass through. 
> > > 
> > > Hi Greg, 
> > > 
> > > Any further thoughts on this? 
> > 
> > Sorry, been swamped with other patches and stable stuff and not had a 
> > time to look at it.  Give me a few days... 
>
> Hi Greg, wanted to ping you on this again... 
>
> I know some days have gone by, so let me summarize the issue-- vfio 
> drivers in the kernel (regardless of bus type) need to bind to 
> devices of any type.   There seem to be 3 approaches: 
>
>    1.  new_id -- (current approach) the user explicitly registers 
>        each new device type with the vfio driver using the new_id 
>        mechanism. 
>
>        Problem: multiple drivers will be resident that handle the 
>        same device type...and there is nothing user space hotplug 
>        infrastructure can do to help. 
>
>    2.  "any id" -- the vfio driver could specify a wildcard match of 
>        some kind so that it can bind to any possible device id.  However, 
>        we don't want vfio grabbing all devices...just the ones we 
>        explicitly want to pass to user space. 
>
>        Proposed patch to support this was to create a new flag 
>        "sysfs_bind_only" in struct device_driver.  When this flag 
>        is set, the driver can only bind to devices via the sysfs 
>        bind file.  This would allow the wildcard match to work. 
>
>        Patch is here: 
>        https://lkml.org/lkml/2013/12/3/253 
>
>    3.  Driver initiated explicit bind -- with this approach the 
>        vfio driver would create a private 'bind' sysfs object 
>        and the user would echo the requested device into it: 
>
>        echo 0001:03:00.0 > /sys/bus/pci/drivers/vfio-pci/vfio_bind 
>
>        In order to make that work, the driver would need to call 
>        driver_probe_device() and thus we need this patch: 
>        https://lkml.org/lkml/2014/2/8/175 

There is a forth way. You can to the devices using BDF. And if you want to do it at startup you can have an 'hide' parameter that will assign to itself is he devices and the be the owner of said devices.
>
>
> Thanks, 
> Stuart 
>
>
>
>
> _______________________________________________ 
> iommu mailing list 
> iommu@lists.linux-foundation.org 
> https://lists.linuxfoundation.org/mailman/listinfo/iommu 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 23+ messages in thread