From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Auger Subject: Re: [PATCH] VFIO: platform: AMD xgbe reset module Date: Fri, 16 Oct 2015 15:06:45 +0200 Message-ID: <5620F665.5010903@linaro.org> References: <1444836792-2405-1-git-send-email-eric.auger@linaro.org> <7287273.XGFpHqQ9fN@wuerfel> <20151015121228.GA965@cbox> <5270338.MbsQ1bGczu@wuerfel> <561FBC31.3020900@linaro.org> <1444927997.4059.470.camel@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 4C09F413E2 for ; Fri, 16 Oct 2015 09:04:29 -0400 (EDT) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id KOj1xSkz9TQ4 for ; Fri, 16 Oct 2015 09:04:28 -0400 (EDT) Received: from mail-wi0-f178.google.com (mail-wi0-f178.google.com [209.85.212.178]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 138D540FA6 for ; Fri, 16 Oct 2015 09:04:27 -0400 (EDT) Received: by wicll6 with SMTP id ll6so9142566wic.0 for ; Fri, 16 Oct 2015 06:06:48 -0700 (PDT) In-Reply-To: <1444927997.4059.470.camel@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Alex Williamson Cc: thomas.lendacky@amd.com, kvm@vger.kernel.org, Arnd Bergmann , patches@linaro.org, linux-kernel@vger.kernel.org, suravee.suthikulpanit@amd.com, eric.auger@st.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org List-Id: kvmarm@lists.cs.columbia.edu RGVhciBhbGwsCk9uIDEwLzE1LzIwMTUgMDY6NTMgUE0sIEFsZXggV2lsbGlhbXNvbiB3cm90ZToK PiBPbiBUaHUsIDIwMTUtMTAtMTUgYXQgMTY6NDYgKzAyMDAsIEVyaWMgQXVnZXIgd3JvdGU6Cj4+ IEhpIEFybmQsCj4+IE9uIDEwLzE1LzIwMTUgMDM6NTkgUE0sIEFybmQgQmVyZ21hbm4gd3JvdGU6 Cj4+PiBPbiBUaHVyc2RheSAxNSBPY3RvYmVyIDIwMTUgMTQ6MTI6MjggQ2hyaXN0b2ZmZXIgRGFs bCB3cm90ZToKPj4+Pj4KPj4+Pj4gZW51bSB2ZmlvX3BsYXRmb3JtX29wIHsKPj4+Pj4gICAgICAg VkZJT19QTEFURk9STV9CSU5ELAo+Pj4+PiAgICAgICBWRklPX1BMQVRGT1JNX1VOQklORCwKPj4+ Pj4gICAgICAgVkZJT19QTEFURk9STV9SRVNFVCwKPj4+Pj4gfTsKPj4+Pj4KPj4+Pj4gc3RydWN0 IHBsYXRmb3JtX2RyaXZlciB7Cj4+Pj4+ICAgICAgICAgaW50ICgqcHJvYmUpKHN0cnVjdCBwbGF0 Zm9ybV9kZXZpY2UgKik7Cj4+Pj4+ICAgICAgICAgaW50ICgqcmVtb3ZlKShzdHJ1Y3QgcGxhdGZv cm1fZGV2aWNlICopOwo+Pj4+PiAgICAgICAuLi4KPj4+Pj4gICAgICAgaW50ICgqdmZpb19tYW5h Z2UpKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKiwgZW51bSB2ZmlvX3BsYXRmb3JtX29wKTsKPj4+ Pj4gICAgICAgICBzdHJ1Y3QgZGV2aWNlX2RyaXZlciBkcml2ZXI7Cj4+Pj4+IH07Cj4+Pj4+Cj4+ Pj4+IFRoaXMgd291bGQgaW50ZWdyYXRlIG11Y2ggbW9yZSBjbG9zZWx5IGludG8gdGhlIHBsYXRm b3JtIGRyaXZlciBmcmFtZXdvcmssCj4+Pj4+IGp1c3QgbGlrZSB0aGUgcmVndWxhciB2ZmlvIGRy aXZlciBpbnRlZ3JhdGVzIGludG8gdGhlIFBDSSBmcmFtZXdvcmsuCj4+Pj4+IFVubGlrZSBQQ0kg aG93ZXZlciwgeW91IGNhbid0IGp1c3QgdXNlIHRoZSBnZW5lcmljIGRyaXZlciBmcmFtZXdvcmsg dG8KPj4+Pj4gdW5iaW5kIHRoZSBkcml2ZXIsIGJlY2F1c2UgeW91IHN0aWxsIG5lZWQgZGV2aWNl IHNwZWNpZmljIGNvZGUuCj4+Pj4+Cj4+Pj4gVGhhbmtzIGZvciB0aGVzZSBzdWdnZXN0aW9ucywg cmVhbGx5IGhlbHBmdWwuCj4+Pj4KPj4+PiBXaGF0IEkgZG9uJ3QgdW5kZXJzdGFuZCBpbiB0aGUg bGF0dGVyIGV4YW1wbGUgaXMgaG93IFZGSU8ga25vd3Mgd2hpY2gKPj4+PiBzdHJ1Y3QgcGxhdGZv cm1fZHJpdmVyIHRvIGludGVyYWN0IHdpdGg/Cj4+Pgo+Pj4gVGhpcyB3b3VsZCBhc3N1bWUgdGhh dCB0aGUgZHJpdmVyIHJlbWFpbnMgYm91bmQgdG8gdGhlIGRldmljZSwgc28gVkZJTwo+Pj4gZ2V0 cyBhIHBvaW50ZXIgdG8gdGhlIGRldmljZSBmcm9tIHNvbWV3aGVyZSAoYXMgaXQgZG9lcyB0b2Rh eSkgYW5kIHRoZW4KPj4+IGZvbGxvd3MgdGhlIGRldi0+ZHJpdmVyIHBvaW50ZXIgdG8gZ2V0IHRv IHRoZSBwbGF0Zm9ybV9kcml2ZXIuCj4gCj4gVGhlIGNvbXBsZXhpdHkgb2YgbWFuYWdpbmcgYSBi aS1tb2RhbCBkcml2ZXIgc2VlbXMgbGlrZSBmYXIgbW9yZSB0aGFuIGEKPiBsaXR0bGUgYml0IG9m IGNvZGUgZHVwbGljYXRpb24gaW4gYSBkZXZpY2Ugc3BlY2lmaWMgcmVzZXQgbW9kdWxlIGFuZAo+ IGV4dGVuZHMgaW50byBob3cgdXNlcnNwYWNlIG1ha2VzIGRldmljZXMgYXZhaWxhYmxlIHRocm91 Z2ggdmZpbywgc28gSQo+IHRoaW5rIGl0J3MgdG9vIGxhdGUgZm9yIHRoYXQgZGlzY3Vzc2lvbi4K PiAgIAo+Pj4+IEFsc28sIGp1c3Qgc28gSSdtIHN1cmUgSSB1bmRlcnN0YW5kIGNvcnJlY3RseSwg VkZJT19QTEFURk9STV9VTkJJTkQgaXMKPj4+PiB0aGVuIGNhbGxlZCBieSBWRklPIGJlZm9yZSB0 aGUgVkZJTyBkcml2ZXIgdW5iaW5kcyBmcm9tIHRoZSBkZXZpY2UKPj4+PiAodW5iaW5kaW5nIHRo ZSBwbGF0Zm9ybSBkcml2ZXIgZnJvbSB0aGUgZGV2aWNlIGJlaW5nIGEgY29tcGxldGVseQo+Pj4+ IHNlcGFyYXRlIHRoaW5nKT8KPj4+Cj4+PiBUaGlzIGlzIHdoZXJlIHdlJ2QgbmVlZCBhIGxpdHRs ZSBtb3JlIGNoYW5nZXMgZm9yIHRoaXMgYXBwcm9hY2guIEluc3RlYWQKPj4+IG9mIHVuYmluZGlu ZyB0aGUgZGV2aWNlIGZyb20gaXRzIGRyaXZlciwgdGhlIGlkZWEgd291bGQgYmUgdGhhdCB0aGUK Pj4+IGRyaXZlciByZW1haW5zIGJvdW5kIGFzIGZhciBhcyB0aGUgZHJpdmVyIG1vZGVsIGlzIGNv bmNlcm5lZCwgYnV0Cj4+PiBpdCB3b3VsZCBiZSBpbiBhIHF1aWVzY2VudCBzdGF0ZSB3aGVyZSBu byBvdGhlciBzdWJzeXN0ZW0gaW50ZXJhY3RzIHdpdGgKPj4+IGl0IChpLmUuIGl0IGdldHMgdW5y ZWdpc3RlcmVkIGZyb20gbmV0d29ya2luZyBjb3JlIG9yIHdoaWNoZXZlciBpdCB1c2VzKS4KPj4K Pj4gQ3VycmVudGx5IHdlIHVzZSB0aGUgc2FtZSBtZWNoYW5pc20gYXMgZm9yIFBDSSwgaWUuIHVu YmluZCB0aGUgbmF0aXZlCj4+IGRyaXZlciBhbmQgdGhlbiBiaW5kIFZGSU8gcGxhdGZvcm0gZHJp dmVyIGluIGl0cyBwbGFjZS4gRG9uJ3QgeW91IHRoaW5rCj4+IGNoYW5naW5nIHRoaXMgbWF5IGJl IGEgcGFpbiBmb3IgdXNlci1zcGFjZSB0b29scyB0aGF0IGFyZSBkZXNpZ25lZCB0bwo+PiB3b3Jr IHRoYXQgd2F5IGZvciBQQ0k/Cj4+Cj4+IE15IHBlcnNvbmFsIHByZWZlcmVuY2Ugd291bGQgYmUg dG8gc3RhcnQgd2l0aCB5b3VyIGZpcnN0IHByb3Bvc2FsIHNpbmNlCj4+IGl0IGxvb2tzICh0byBt ZSkgbGVzcyBjb21wbGV4IGFuZCAidW5rbm93biIgdGhhdCB0aGUgMmQgYXBwcm9hY2guCj4+Cj4+ IExldCdzIHdhaXQgZm9yIEFsZXggb3BpbmlvbiB0b28uLi4KPiAKPiBJIHRob3VnaHQgdGhlIHJl YXNvbiB3ZSB0b29rIHRoZSBhcHByb2FjaCB3ZSBoYXZlIG5vdyBpcyBzbyB0aGF0IHdlCj4gZG9u J3QgaGF2ZSByZXNldCBjb2RlIGxvYWRlZCBpbnRvIHRoZSBrZXJuZWwgdW5sZXNzIHdlIGhhdmUg YSBkZXZpY2UKPiB0aGF0IG5lZWRzIGl0LiAgVGhlcmVmb3JlIHdlIGRvbid0IHJlYWxseSB3YW50 IHRvIHByZWVtcHRpdmVseSBsb2FkIGFsbAo+IHRoZSByZXNldCBkcml2ZXJzIGFuZCBoYXZlIHRo ZW0gZG8gYSByZWdpc3RyYXRpb24uICBUaGUgdW5mb3J0dW5hdGUKPiBzaWRlLWVmZmVjdCBvZiB0 aGF0IGlzIHRoZSBwbGF0Zm9ybSBjb2RlIG5lZWRzIHRvIGdvIGxvb2tpbmcgZm9yIHRoZQo+IGRy aXZlci4gIFdlIGRvIHRoYXQgdmlhIHRoZSBfX3N5bWJvbF9nZXQoKSB0cmljaywgd2hpY2ggb25s eSBmYWlscwo+IHdpdGhvdXQgbW9kdWxlcyBiZWNhdXNlIHRoZSB1bmRlcnNjb3JlIHZhcmlhbnQg aXNuJ3QgZGVmaW5lZCBpbiB0aGF0Cj4gY2FzZS4gIEkgcmVtZW1iZXIgYXNraW5nIEVyaWMgcHJl dmlvdXNseSB3aHkgd2UncmUgdXNpbmcgdGhhdCByYXRoZXIKPiB0aGFuIHN5bWJvbF9nZXQoKSwK CnRoZSByYXRpb25hbGUgYmVoaW5kIHVzaW5nIF9fZ2V0X3N5bWJvbCBpcyB0aGF0IHRoZSBmdW5j dGlvbiB0YWtlcyBhCmNoYXIgKiBhcyBhcmd1bWVudCB3aGlsZSB0aGUgZ2V0X3N5bWJvbCBtYWNy byB0YWtlcyB0aGUgdmVyeSBzeW1ib2wuIEkKbmVlZGVkIHRvIHByb3ZpZGUgYSBzdHJpbmcgc2lu Y2UgdGhpcyBpcyB3aGF0IGlzIHN0b3JlZCBpbiB0aGUgbG9va3VwCnRhYmxlLiBVbmZvcnR1bmF0 ZWx5IEkgZGlkIG5vdCBzZWUgdGhlICNpZiBDT05GSUdfTU9EVUxFUy4gSSBTaG91bGQgaGF2ZQpi ZWVuIHdhcm5lZCBhYm91dCAiX18iIGFuZCBBbGV4IGRvdWJ0cywgc2luIG9mIG5hw692ZXR5LgoK IEkndmUgc2luY2UgZm9yZ290dGVuIGhpcyBhbnN3ZXIsIGJ1dCB0aGUgZmFjdCB0aGF0Cj4gX19z eW1ib2xfZ2V0KCkgaXMgb25seSBkZWZpbmVkIGZvciBtb2R1bGVzIG1ha2VzIGl0IG1vb3QsIHdl IGVpdGhlciBuZWVkCj4gdG8gbWFrZSBzeW1ib2xfZ2V0KCkgd29yayBvciBkZWZpbmUgX19zeW1i b2xfZ2V0KCkgZm9yIG5vbi1tb2R1bGUKPiBidWlsZHMuCkkgY3VycmVudGx5IGRvbid0IHNlZSBh bnkgc29sdXRpb24gZm9yIGFueSBvZiB0aG9zZS4gVGhlIG9ubHkgc29sdXRpb24gSQpjYW4gc2Vl IGlzIHNvbWVvbmUgcmVnaXN0ZXJzIHRoZSByZXNldCBmdW5jdGlvbiBwb2ludGVyIHRvIHZmaW8u CgpJIHRoaW5rIHdlIGNvdWxkIGtlZXAgdGhlIGV4aXN0aW5nIHJlc2V0IG1vZHVsZXMsIGRvIHRo ZSByZXF1ZXN0X21vZHVsZQpmcm9tIFZGSU8sIHVzaW5nIHRoZWlyIG1vZHVsZSBuYW1lIHJlZ2lz dGVyZWQgaW4gdGhlIGxvb2t1cCB0YWJsZS4gQnV0Cmluc3RlYWQgb2YgaGF2aW5nIHRoZSByZXNl dCBmdW5jdGlvbiBpbiB0aGUgbG9vay11cCB0YWJsZSB3ZSB3b3VsZCBoYXZlCnRoZSByZXNldCBt b2R1bGVzIHJlZ2lzdGVyIHRoZWlyIHJlc2V0IGZ1bmN0aW9uIHBvaW50ZXIgdG8gVkZJTy4gSSB0 aGluawp0aGlzIGNvdWxkIHdvcmsgYXJvdW5kIHRoZSBzeW1ib2xfZ2V0IGlzc3VlLgoKVGhpcyBz dGlsbCBsZWF2ZXMgdGhlIGxheWVyIHZpb2xhdGlvbiBhcmd1bWVudCB0aG91Z2guCgpBc3N1bWlu ZyB0aGlzIHdvcmtzLCB3b3VsZCB0aGF0IGJlIGFuIGFjY2VwdGFibGUgc29sdXRpb24sIGFsdGhv dWdoIEkKYWNrbm93bGVkZ2UgdGhpcyBkb2VzIG5vdCBwZXJmZWN0bHkgZml0IGludG8gdGhlIGRy aXZlciBtb2RlbD8KCkJlc3QgUmVnYXJkcwoKRXJpYwo+IAo+IE90aGVyd2lzZSwgd2Ugc2hvdWxk IHByb2JhYmx5IGFiYW5kb24gdGhlIGlkZWEgb2YgdGhlc2UgcmVzZXQgZnVuY3Rpb25zCj4gYmVp bmcgbW9kdWxlcyBhbmQgYnVpbGQgdGhlbSBpbnRvIHRoZSB2ZmlvIHBsYXRmb3JtIGRyaXZlciAo d2hpY2ggd291bGQKPiBzdGlsbCBiZSBsZXNzIGxvYWRlZCwgZGVhZCBjb2RlIHRoYW4gYSBiaS1t b2RhbCBob3N0IGRyaXZlcikuICBUaGFua3MsCgo+IAo+IEFsZXgKPiAKCl9fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmt2bWFybSBtYWlsaW5nIGxpc3QKa3Zt YXJtQGxpc3RzLmNzLmNvbHVtYmlhLmVkdQpodHRwczovL2xpc3RzLmNzLmNvbHVtYmlhLmVkdS9t YWlsbWFuL2xpc3RpbmZvL2t2bWFybQo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: eric.auger@linaro.org (Eric Auger) Date: Fri, 16 Oct 2015 15:06:45 +0200 Subject: [PATCH] VFIO: platform: AMD xgbe reset module In-Reply-To: <1444927997.4059.470.camel@redhat.com> References: <1444836792-2405-1-git-send-email-eric.auger@linaro.org> <7287273.XGFpHqQ9fN@wuerfel> <20151015121228.GA965@cbox> <5270338.MbsQ1bGczu@wuerfel> <561FBC31.3020900@linaro.org> <1444927997.4059.470.camel@redhat.com> Message-ID: <5620F665.5010903@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Dear all, On 10/15/2015 06:53 PM, Alex Williamson wrote: > On Thu, 2015-10-15 at 16:46 +0200, Eric Auger wrote: >> Hi Arnd, >> On 10/15/2015 03:59 PM, Arnd Bergmann wrote: >>> On Thursday 15 October 2015 14:12:28 Christoffer Dall wrote: >>>>> >>>>> enum vfio_platform_op { >>>>> VFIO_PLATFORM_BIND, >>>>> VFIO_PLATFORM_UNBIND, >>>>> VFIO_PLATFORM_RESET, >>>>> }; >>>>> >>>>> struct platform_driver { >>>>> int (*probe)(struct platform_device *); >>>>> int (*remove)(struct platform_device *); >>>>> ... >>>>> int (*vfio_manage)(struct platform_device *, enum vfio_platform_op); >>>>> struct device_driver driver; >>>>> }; >>>>> >>>>> This would integrate much more closely into the platform driver framework, >>>>> just like the regular vfio driver integrates into the PCI framework. >>>>> Unlike PCI however, you can't just use the generic driver framework to >>>>> unbind the driver, because you still need device specific code. >>>>> >>>> Thanks for these suggestions, really helpful. >>>> >>>> What I don't understand in the latter example is how VFIO knows which >>>> struct platform_driver to interact with? >>> >>> This would assume that the driver remains bound to the device, so VFIO >>> gets a pointer to the device from somewhere (as it does today) and then >>> follows the dev->driver pointer to get to the platform_driver. > > The complexity of managing a bi-modal driver seems like far more than a > little bit of code duplication in a device specific reset module and > extends into how userspace makes devices available through vfio, so I > think it's too late for that discussion. > >>>> Also, just so I'm sure I understand correctly, VFIO_PLATFORM_UNBIND is >>>> then called by VFIO before the VFIO driver unbinds from the device >>>> (unbinding the platform driver from the device being a completely >>>> separate thing)? >>> >>> This is where we'd need a little more changes for this approach. Instead >>> of unbinding the device from its driver, the idea would be that the >>> driver remains bound as far as the driver model is concerned, but >>> it would be in a quiescent state where no other subsystem interacts with >>> it (i.e. it gets unregistered from networking core or whichever it uses). >> >> Currently we use the same mechanism as for PCI, ie. unbind the native >> driver and then bind VFIO platform driver in its place. Don't you think >> changing this may be a pain for user-space tools that are designed to >> work that way for PCI? >> >> My personal preference would be to start with your first proposal since >> it looks (to me) less complex and "unknown" that the 2d approach. >> >> Let's wait for Alex opinion too... > > I thought the reason we took the approach we have now is so that we > don't have reset code loaded into the kernel unless we have a device > that needs it. Therefore we don't really want to preemptively load all > the reset drivers and have them do a registration. The unfortunate > side-effect of that is the platform code needs to go looking for the > driver. We do that via the __symbol_get() trick, which only fails > without modules because the underscore variant isn't defined in that > case. I remember asking Eric previously why we're using that rather > than symbol_get(), the rationale behind using __get_symbol is that the function takes a char * as argument while the get_symbol macro takes the very symbol. I needed to provide a string since this is what is stored in the lookup table. Unfortunately I did not see the #if CONFIG_MODULES. I Should have been warned about "__" and Alex doubts, sin of na?vety. I've since forgotten his answer, but the fact that > __symbol_get() is only defined for modules makes it moot, we either need > to make symbol_get() work or define __symbol_get() for non-module > builds. I currently don't see any solution for any of those. The only solution I can see is someone registers the reset function pointer to vfio. I think we could keep the existing reset modules, do the request_module from VFIO, using their module name registered in the lookup table. But instead of having the reset function in the look-up table we would have the reset modules register their reset function pointer to VFIO. I think this could work around the symbol_get issue. This still leaves the layer violation argument though. Assuming this works, would that be an acceptable solution, although I acknowledge this does not perfectly fit into the driver model? Best Regards Eric > > Otherwise, we should probably abandon the idea of these reset functions > being modules and build them into the vfio platform driver (which would > still be less loaded, dead code than a bi-modal host driver). Thanks, > > Alex > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754351AbbJPNGx (ORCPT ); Fri, 16 Oct 2015 09:06:53 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:37674 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932206AbbJPNGt (ORCPT ); Fri, 16 Oct 2015 09:06:49 -0400 Subject: Re: [PATCH] VFIO: platform: AMD xgbe reset module To: Alex Williamson References: <1444836792-2405-1-git-send-email-eric.auger@linaro.org> <7287273.XGFpHqQ9fN@wuerfel> <20151015121228.GA965@cbox> <5270338.MbsQ1bGczu@wuerfel> <561FBC31.3020900@linaro.org> <1444927997.4059.470.camel@redhat.com> Cc: Arnd Bergmann , Christoffer Dall , linux-arm-kernel@lists.infradead.org, eric.auger@st.com, b.reynal@virtualopensystems.com, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, thomas.lendacky@amd.com, patches@linaro.org, suravee.suthikulpanit@amd.com, linux-kernel@vger.kernel.org From: Eric Auger Message-ID: <5620F665.5010903@linaro.org> Date: Fri, 16 Oct 2015 15:06:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <1444927997.4059.470.camel@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Dear all, On 10/15/2015 06:53 PM, Alex Williamson wrote: > On Thu, 2015-10-15 at 16:46 +0200, Eric Auger wrote: >> Hi Arnd, >> On 10/15/2015 03:59 PM, Arnd Bergmann wrote: >>> On Thursday 15 October 2015 14:12:28 Christoffer Dall wrote: >>>>> >>>>> enum vfio_platform_op { >>>>> VFIO_PLATFORM_BIND, >>>>> VFIO_PLATFORM_UNBIND, >>>>> VFIO_PLATFORM_RESET, >>>>> }; >>>>> >>>>> struct platform_driver { >>>>> int (*probe)(struct platform_device *); >>>>> int (*remove)(struct platform_device *); >>>>> ... >>>>> int (*vfio_manage)(struct platform_device *, enum vfio_platform_op); >>>>> struct device_driver driver; >>>>> }; >>>>> >>>>> This would integrate much more closely into the platform driver framework, >>>>> just like the regular vfio driver integrates into the PCI framework. >>>>> Unlike PCI however, you can't just use the generic driver framework to >>>>> unbind the driver, because you still need device specific code. >>>>> >>>> Thanks for these suggestions, really helpful. >>>> >>>> What I don't understand in the latter example is how VFIO knows which >>>> struct platform_driver to interact with? >>> >>> This would assume that the driver remains bound to the device, so VFIO >>> gets a pointer to the device from somewhere (as it does today) and then >>> follows the dev->driver pointer to get to the platform_driver. > > The complexity of managing a bi-modal driver seems like far more than a > little bit of code duplication in a device specific reset module and > extends into how userspace makes devices available through vfio, so I > think it's too late for that discussion. > >>>> Also, just so I'm sure I understand correctly, VFIO_PLATFORM_UNBIND is >>>> then called by VFIO before the VFIO driver unbinds from the device >>>> (unbinding the platform driver from the device being a completely >>>> separate thing)? >>> >>> This is where we'd need a little more changes for this approach. Instead >>> of unbinding the device from its driver, the idea would be that the >>> driver remains bound as far as the driver model is concerned, but >>> it would be in a quiescent state where no other subsystem interacts with >>> it (i.e. it gets unregistered from networking core or whichever it uses). >> >> Currently we use the same mechanism as for PCI, ie. unbind the native >> driver and then bind VFIO platform driver in its place. Don't you think >> changing this may be a pain for user-space tools that are designed to >> work that way for PCI? >> >> My personal preference would be to start with your first proposal since >> it looks (to me) less complex and "unknown" that the 2d approach. >> >> Let's wait for Alex opinion too... > > I thought the reason we took the approach we have now is so that we > don't have reset code loaded into the kernel unless we have a device > that needs it. Therefore we don't really want to preemptively load all > the reset drivers and have them do a registration. The unfortunate > side-effect of that is the platform code needs to go looking for the > driver. We do that via the __symbol_get() trick, which only fails > without modules because the underscore variant isn't defined in that > case. I remember asking Eric previously why we're using that rather > than symbol_get(), the rationale behind using __get_symbol is that the function takes a char * as argument while the get_symbol macro takes the very symbol. I needed to provide a string since this is what is stored in the lookup table. Unfortunately I did not see the #if CONFIG_MODULES. I Should have been warned about "__" and Alex doubts, sin of naïvety. I've since forgotten his answer, but the fact that > __symbol_get() is only defined for modules makes it moot, we either need > to make symbol_get() work or define __symbol_get() for non-module > builds. I currently don't see any solution for any of those. The only solution I can see is someone registers the reset function pointer to vfio. I think we could keep the existing reset modules, do the request_module from VFIO, using their module name registered in the lookup table. But instead of having the reset function in the look-up table we would have the reset modules register their reset function pointer to VFIO. I think this could work around the symbol_get issue. This still leaves the layer violation argument though. Assuming this works, would that be an acceptable solution, although I acknowledge this does not perfectly fit into the driver model? Best Regards Eric > > Otherwise, we should probably abandon the idea of these reset functions > being modules and build them into the vfio platform driver (which would > still be less loaded, dead code than a bi-modal host driver). Thanks, > > Alex >