| Message ID | 1444836792-2405-1-git-send-email-eric.auger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = { > .reset_function_name = "vfio_platform_calxedaxgmac_reset", > .module_name = "vfio-platform-calxedaxgmac", > }, > + { > + .compat = "amd,xgbe-seattle-v1a", > + .reset_function_name = "vfio_platform_amdxgbe_reset", > + .module_name = "vfio-platform-amdxgbe", > + }, > }; > > static void vfio_platform_get_reset(struct vfio_platform_device *vdev, > This is causing build errors for me when CONFIG_MODULES is disabled. Could this please be restructured so vfio_platform_get_reset does not attempt to call __symbol_get() but instead has the drivers register themselves properly to a subsystem? I don't see any way this could be fixed otherwise. The problem of course showed up with calxedaxgmac already, but I'd prefer not to see anything added there until the common code has been improved. Arnd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On 10/14/2015 05:38 PM, Arnd Bergmann wrote: > On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: >> --- a/drivers/vfio/platform/vfio_platform_common.c >> +++ b/drivers/vfio/platform/vfio_platform_common.c >> @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = { >> .reset_function_name = "vfio_platform_calxedaxgmac_reset", >> .module_name = "vfio-platform-calxedaxgmac", >> }, >> + { >> + .compat = "amd,xgbe-seattle-v1a", >> + .reset_function_name = "vfio_platform_amdxgbe_reset", >> + .module_name = "vfio-platform-amdxgbe", >> + }, >> }; >> >> static void vfio_platform_get_reset(struct vfio_platform_device *vdev, >> > > This is causing build errors for me when CONFIG_MODULES is disabled. Sorry about that and thanks for reporting the issue > > Could this please be restructured so vfio_platform_get_reset does > not attempt to call __symbol_get() but instead has the drivers > register themselves properly to a subsystem? OK Could you elaborate about "has the drivers register themselves properly to a subsystem". My first proposal when coping with this problematic of being able to add reset plugins to the vfio-platform driver was to create new drivers per device requiring reset. but this was considered painful for end-users, who needed to be aware of the right driver to bind - and I think that makes sense - (https://lkml.org/lkml/2015/4/17/568) . A naive question I dare to ask, wouldn't it be acceptable to make vfio_platform depend on CONFIG_MODULES? Don't we disable modules for security purpose? In that context would we use VFIO? Best Regards Eric > > I don't see any way this could be fixed otherwise. The problem > of course showed up with calxedaxgmac already, but I'd prefer not > to see anything added there until the common code has been improved. > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 15 October 2015 10:08:02 Eric Auger wrote: > Hi Arnd, > On 10/14/2015 05:38 PM, Arnd Bergmann wrote: > > On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: > >> --- a/drivers/vfio/platform/vfio_platform_common.c > >> +++ b/drivers/vfio/platform/vfio_platform_common.c > >> @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = { > >> .reset_function_name = "vfio_platform_calxedaxgmac_reset", > >> .module_name = "vfio-platform-calxedaxgmac", > >> }, > >> + { > >> + .compat = "amd,xgbe-seattle-v1a", > >> + .reset_function_name = "vfio_platform_amdxgbe_reset", > >> + .module_name = "vfio-platform-amdxgbe", > >> + }, > >> }; > >> > >> static void vfio_platform_get_reset(struct vfio_platform_device *vdev, > >> > > > > This is causing build errors for me when CONFIG_MODULES is disabled. > Sorry about that and thanks for reporting the issue > > > > Could this please be restructured so vfio_platform_get_reset does > > not attempt to call __symbol_get() but instead has the drivers > > register themselves properly to a subsystem? > OK > > Could you elaborate about "has the drivers register themselves properly > to a subsystem". > > My first proposal when coping with this problematic of being able to add > reset plugins to the vfio-platform driver was to create new drivers per > device requiring reset. but this was considered painful for end-users, > who needed to be aware of the right driver to bind - and I think that > makes sense - (https://lkml.org/lkml/2015/4/17/568) . Having multiple drivers indeed sucks, but your current approach isn't that much better, as you still have two modules that are used to driver the same hardware. I would expect that the same driver that is used for the normal operation and that it calls a function to register itself to vfio by passing a structure with the device and reset function pointer. > A naive question I dare to ask, wouldn't it be acceptable to make > vfio_platform depend on CONFIG_MODULES? Don't we disable modules for > security purpose? In that context would we use VFIO? I think a lot of embedded systems turn off modules to save a little memory, speed up boot time and simplify their user space. Aside from that, the current method is highly unusual and looks a bit fragile to me, as you are relying on internals of the module loader code. It's also a layering violation as the generic code needs to be patched for each device specific module that is added, and we try to avoid that. A possible solution could be something inside the xgbe driver like static void xgbe_init_module(void) { int ret = 0; if (IS_ENABLED(CONFIG_AMD_XGBE_ETHERNET) ret = platform_driver_register(&xgbe_driver); if (ret) return ret; if (IS_ENABLED(CONFIG_VFIO_PLATFORM)) ret = vfio_platform_register_reset(&xgbe_of_match, xgbe_platform_reset); return ret; } This way you have exactly one driver module that gets loaded for the device and you can use it either with the platform_driver or through vfio. A nicer way that would be a little more work would be to integrate the reset infrastructure into 'struct platform_driver' framework, by adding another callback to the it for doing the interaction with vfio, something like 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. Arnd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 15, 2015 at 01:21:55PM +0200, Arnd Bergmann wrote: > On Thursday 15 October 2015 10:08:02 Eric Auger wrote: > > Hi Arnd, > > On 10/14/2015 05:38 PM, Arnd Bergmann wrote: > > > On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: > > >> --- a/drivers/vfio/platform/vfio_platform_common.c > > >> +++ b/drivers/vfio/platform/vfio_platform_common.c > > >> @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = { > > >> .reset_function_name = "vfio_platform_calxedaxgmac_reset", > > >> .module_name = "vfio-platform-calxedaxgmac", > > >> }, > > >> + { > > >> + .compat = "amd,xgbe-seattle-v1a", > > >> + .reset_function_name = "vfio_platform_amdxgbe_reset", > > >> + .module_name = "vfio-platform-amdxgbe", > > >> + }, > > >> }; > > >> > > >> static void vfio_platform_get_reset(struct vfio_platform_device *vdev, > > >> > > > > > > This is causing build errors for me when CONFIG_MODULES is disabled. > > Sorry about that and thanks for reporting the issue > > > > > > Could this please be restructured so vfio_platform_get_reset does > > > not attempt to call __symbol_get() but instead has the drivers > > > register themselves properly to a subsystem? > > OK > > > > Could you elaborate about "has the drivers register themselves properly > > to a subsystem". > > > > My first proposal when coping with this problematic of being able to add > > reset plugins to the vfio-platform driver was to create new drivers per > > device requiring reset. but this was considered painful for end-users, > > who needed to be aware of the right driver to bind - and I think that > > makes sense - (https://lkml.org/lkml/2015/4/17/568) . > > Having multiple drivers indeed sucks, but your current approach isn't > that much better, as you still have two modules that are used to driver > the same hardware. > > I would expect that the same driver that is used for the normal > operation and that it calls a function to register itself to vfio > by passing a structure with the device and reset function pointer. > > > A naive question I dare to ask, wouldn't it be acceptable to make > > vfio_platform depend on CONFIG_MODULES? Don't we disable modules for > > security purpose? In that context would we use VFIO? > > I think a lot of embedded systems turn off modules to save a little > memory, speed up boot time and simplify their user space. > > Aside from that, the current method is highly unusual and looks a bit > fragile to me, as you are relying on internals of the module loader > code. It's also a layering violation as the generic code needs to be > patched for each device specific module that is added, and we try > to avoid that. > > A possible solution could be something inside the xgbe driver like > > > static void xgbe_init_module(void) > { > int ret = 0; > > if (IS_ENABLED(CONFIG_AMD_XGBE_ETHERNET) > ret = platform_driver_register(&xgbe_driver); > if (ret) > return ret; > > if (IS_ENABLED(CONFIG_VFIO_PLATFORM)) > ret = vfio_platform_register_reset(&xgbe_of_match, xgbe_platform_reset); > > return ret; > } > > This way you have exactly one driver module that gets loaded for the > device and you can use it either with the platform_driver or through > vfio. > > A nicer way that would be a little more work would be to integrate > the reset infrastructure into 'struct platform_driver' framework, > by adding another callback to the it for doing the interaction with > vfio, something like > > 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? 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)? Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > 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). Arnd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On 10/15/2015 02:12 PM, Christoffer Dall wrote: > On Thu, Oct 15, 2015 at 01:21:55PM +0200, Arnd Bergmann wrote: >> On Thursday 15 October 2015 10:08:02 Eric Auger wrote: >>> Hi Arnd, >>> On 10/14/2015 05:38 PM, Arnd Bergmann wrote: >>>> On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: >>>>> --- a/drivers/vfio/platform/vfio_platform_common.c >>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c >>>>> @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = { >>>>> .reset_function_name = "vfio_platform_calxedaxgmac_reset", >>>>> .module_name = "vfio-platform-calxedaxgmac", >>>>> }, >>>>> + { >>>>> + .compat = "amd,xgbe-seattle-v1a", >>>>> + .reset_function_name = "vfio_platform_amdxgbe_reset", >>>>> + .module_name = "vfio-platform-amdxgbe", >>>>> + }, >>>>> }; >>>>> >>>>> static void vfio_platform_get_reset(struct vfio_platform_device *vdev, >>>>> >>>> >>>> This is causing build errors for me when CONFIG_MODULES is disabled. >>> Sorry about that and thanks for reporting the issue >>>> >>>> Could this please be restructured so vfio_platform_get_reset does >>>> not attempt to call __symbol_get() but instead has the drivers >>>> register themselves properly to a subsystem? >>> OK >>> >>> Could you elaborate about "has the drivers register themselves properly >>> to a subsystem". >>> >>> My first proposal when coping with this problematic of being able to add >>> reset plugins to the vfio-platform driver was to create new drivers per >>> device requiring reset. but this was considered painful for end-users, >>> who needed to be aware of the right driver to bind - and I think that >>> makes sense - (https://lkml.org/lkml/2015/4/17/568) . >> >> Having multiple drivers indeed sucks, but your current approach isn't >> that much better, as you still have two modules that are used to driver >> the same hardware. >> >> I would expect that the same driver that is used for the normal >> operation and that it calls a function to register itself to vfio >> by passing a structure with the device and reset function pointer. >> >>> A naive question I dare to ask, wouldn't it be acceptable to make >>> vfio_platform depend on CONFIG_MODULES? Don't we disable modules for >>> security purpose? In that context would we use VFIO? >> >> I think a lot of embedded systems turn off modules to save a little >> memory, speed up boot time and simplify their user space. >> >> Aside from that, the current method is highly unusual and looks a bit >> fragile to me, as you are relying on internals of the module loader >> code. It's also a layering violation as the generic code needs to be >> patched for each device specific module that is added, and we try >> to avoid that. Many thanks for taking the time to write this down >> >> A possible solution could be something inside the xgbe driver like >> >> >> static void xgbe_init_module(void) >> { >> int ret = 0; >> >> if (IS_ENABLED(CONFIG_AMD_XGBE_ETHERNET) >> ret = platform_driver_register(&xgbe_driver); >> if (ret) >> return ret; >> >> if (IS_ENABLED(CONFIG_VFIO_PLATFORM)) >> ret = vfio_platform_register_reset(&xgbe_of_match, xgbe_platform_reset); >> >> return ret; >> } >> >> This way you have exactly one driver module that gets loaded for the >> device and you can use it either with the platform_driver or through >> vfio. If I understand it correctly you still need 2 loaded modules (VFIO driver & XGBE driver which implements the reset function) or am I missing something? I had a similar mechanism of registration in my PATCH v1 but I did the registration from the reset module itself instead of in the native driver, as you suggest here. Best Regards Eric >> >> A nicer way that would be a little more work would be to integrate >> the reset infrastructure into 'struct platform_driver' framework, >> by adding another callback to the it for doing the interaction with >> vfio, something like >> >> 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? > > 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)? > > Thanks, > -Christoffer > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 15 October 2015 16:20:46 Eric Auger wrote: > On 10/15/2015 02:12 PM, Christoffer Dall wrote: > > On Thu, Oct 15, 2015 at 01:21:55PM +0200, Arnd Bergmann wrote: > >> On Thursday 15 October 2015 10:08:02 Eric Auger wrote: > >>> On 10/14/2015 05:38 PM, Arnd Bergmann wrote: > >>>> On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: > >> A possible solution could be something inside the xgbe driver like > >> > >> > >> static void xgbe_init_module(void) > >> { > >> int ret = 0; > >> > >> if (IS_ENABLED(CONFIG_AMD_XGBE_ETHERNET) > >> ret = platform_driver_register(&xgbe_driver); > >> if (ret) > >> return ret; > >> > >> if (IS_ENABLED(CONFIG_VFIO_PLATFORM)) > >> ret = vfio_platform_register_reset(&xgbe_of_match, xgbe_platform_reset); > >> > >> return ret; > >> } > >> > >> This way you have exactly one driver module that gets loaded for the > >> device and you can use it either with the platform_driver or through > >> vfio. > If I understand it correctly you still need 2 loaded modules (VFIO > driver & XGBE driver which implements the reset function) or am I > missing something? That is correct, yes. > I had a similar mechanism of registration in my PATCH v1 but I did the > registration from the reset module itself instead of in the native > driver, as you suggest here. Right. The main difference is that you don't have two modules fighting over the same device with the approach here. Arnd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > >> 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... Thanks again Eric > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 15 October 2015 16:46:09 Eric Auger wrote: > > > > 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. We certainly can't easily change from one approach to the other without breaking user expectations, so the decision needs to be made carefully. The main observation here is that platform devices are unlike PCI in this regard because they need extra per-device code. I have argued in the past that we should not reuse the "VFIO" name here because it's actually something else. On the other hand, there are a lot of commonalities, we just have to make sure we don't try to force the code into one model that doesn't really work just to make it look more like PCI VFIO. Arnd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 15, 2015 at 04:55:13PM +0200, Arnd Bergmann wrote: > On Thursday 15 October 2015 16:46:09 Eric Auger wrote: > > > > > > 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. > > We certainly can't easily change from one approach to the other without > breaking user expectations, so the decision needs to be made carefully. > > The main observation here is that platform devices are unlike PCI in this > regard because they need extra per-device code. I have argued in the > past that we should not reuse the "VFIO" name here because it's actually > something else. I've adjusted to consider VFIO a general purpose framework for mapping device resources into userspace/VMs, and there are certainly a lot of commonality with both PCI, platform, and potentially other devices for that to make sense. > On the other hand, there are a lot of commonalities, > we just have to make sure we don't try to force the code into one model > that doesn't really work just to make it look more like PCI VFIO. > But given that we now have code for platform device passthrough that works in both QEMU and the kernel side and is actually useful for people, is there a clear technical advantage to go back and rework thaat at this point? Don't get me wrong, I like the idea of having a single driver bound to a platform device, and then that's it, but it just feels like that discussion doesn't necessarily belong in the context of a patch that 'just' seeks to add reset functionality for a specific device for VFIO? -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 15 October 2015 17:03:21 Christoffer Dall wrote: > On Thu, Oct 15, 2015 at 04:55:13PM +0200, Arnd Bergmann wrote: > > On Thursday 15 October 2015 16:46:09 Eric Auger wrote: > > > > > > > > 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. > > > > We certainly can't easily change from one approach to the other without > > breaking user expectations, so the decision needs to be made carefully. > > > > The main observation here is that platform devices are unlike PCI in this > > regard because they need extra per-device code. I have argued in the > > past that we should not reuse the "VFIO" name here because it's actually > > something else. > > I've adjusted to consider VFIO a general purpose framework for mapping > device resources into userspace/VMs, and there are certainly a lot of > commonality with both PCI, platform, and potentially other devices for > that to make sense. > > > > On the other hand, there are a lot of commonalities, > > we just have to make sure we don't try to force the code into one model > > that doesn't really work just to make it look more like PCI VFIO. > > > > But given that we now have code for platform device passthrough that > works in both QEMU and the kernel side and is actually useful for > people, is there a clear technical advantage to go back and rework thaat > at this point? > > Don't get me wrong, I like the idea of having a single driver bound to a > platform device, and then that's it, but it just feels like that > discussion doesn't necessarily belong in the context of a patch that > 'just' seeks to add reset functionality for a specific device for VFIO? Ah, this is for qemu/kvm? If there is already upstream qemu code that finds it easier to use this approach, it's certainly more logical to deal with it in my first approach than the second. I was thinking of ODP as the primary user, and that wouldn't need the interface to be consistent with PCI as much, because the code is inherently device specific there anyway. Arnd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 15, 2015 at 5:49 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 15 October 2015 17:03:21 Christoffer Dall wrote: >> On Thu, Oct 15, 2015 at 04:55:13PM +0200, Arnd Bergmann wrote: >> > On Thursday 15 October 2015 16:46:09 Eric Auger wrote: >> > > > >> > > > 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. >> > >> > We certainly can't easily change from one approach to the other without >> > breaking user expectations, so the decision needs to be made carefully. >> > >> > The main observation here is that platform devices are unlike PCI in this >> > regard because they need extra per-device code. I have argued in the >> > past that we should not reuse the "VFIO" name here because it's actually >> > something else. >> >> I've adjusted to consider VFIO a general purpose framework for mapping >> device resources into userspace/VMs, and there are certainly a lot of >> commonality with both PCI, platform, and potentially other devices for >> that to make sense. >> >> >> > On the other hand, there are a lot of commonalities, >> > we just have to make sure we don't try to force the code into one model >> > that doesn't really work just to make it look more like PCI VFIO. >> > >> >> But given that we now have code for platform device passthrough that >> works in both QEMU and the kernel side and is actually useful for >> people, is there a clear technical advantage to go back and rework thaat >> at this point? >> >> Don't get me wrong, I like the idea of having a single driver bound to a >> platform device, and then that's it, but it just feels like that >> discussion doesn't necessarily belong in the context of a patch that >> 'just' seeks to add reset functionality for a specific device for VFIO? > > Ah, this is for qemu/kvm? If there is already upstream qemu code that > finds it easier to use this approach, it's certainly more logical to > deal with it in my first approach than the second. yes, indeed. > > I was thinking of ODP as the primary user, and that wouldn't need > the interface to be consistent with PCI as much, because the code > is inherently device specific there anyway. > I didn't think about how this applies to ODP (haven't had time to look at ODP in any technical detail TBH), but it sounds like ODP could be adapted to use either approach. In any case we can always transition towards your 2nd approach in a userspace compatible way later, if we find good reasons to. Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(), 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. 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 15, 2015 at 10:53:17AM -0600, 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. > I have had extremely limited exposure to the implementation details of the drivers for devices relevant for VFIO platform, so apologies for asking stupid questions. I'm sure that your point is valid, I just fully understand how the complexities of a bi-modal driver arise? Is it simply that the reset function in a particular device driver may not be self-contained so therefore the whole driver would need to be refactored to be able to do a reset for the purpose of VFIO? > > >> 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. Does the current approach have a separate driver for doing VFIO reset or does it reuse the existing device driver? Why does the driver registering itself instead of using symbol_get imply that we must load reset drivers that we don't need? Thanks, -Christoffer > 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(), 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. > > 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 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-10-15 at 21:42 +0200, Christoffer Dall wrote: > On Thu, Oct 15, 2015 at 10:53:17AM -0600, 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. > > > > I have had extremely limited exposure to the implementation details of > the drivers for devices relevant for VFIO platform, so apologies for > asking stupid questions. > > I'm sure that your point is valid, I just fully understand how the > complexities of a bi-modal driver arise? > > Is it simply that the reset function in a particular device driver may > not be self-contained so therefore the whole driver would need to be > refactored to be able to do a reset for the purpose of VFIO? Yes, I would expect that reset function in a driver is not typically self contained, probably referencing driver specific data structures for register offsets, relying on various mappings that are expected to be created from the driver probe() function, etc. It also creates a strange dependency on the host driver, how is the user to know they need the native host driver loaded for full functionality in a device assignment scenario? Are we going to need to do a module_request() of the host driver in vfio platform anyway? What if there are multiple devices and the host driver claims the others when loaded? In the case of PCI and SR-IOV virtual functions, I often blacklist the host VF driver because I shouldn't need it when I only intend to use the device in guests. Not to mention that we'd have to drop a little bit of vfio knowledge into each host driver that we intend to enlighten like this, and how do we resolve whether the host driver, potentially compiled from a separate source tree has this support. I really don't see the layering violation in having a set of reset functions and some lookup mechanism to pick the correct one. The vfio platform driver is a meta driver and sometimes we need to enlighten it a little bit more about the device it's operating on. For PCI we have all sorts of special functionality for reset, but we have a standard to work with there, so we may need to choose between a bus reset, PM reset, AF FLR, PCIe FLR, or device specific reset, but it's all buried in the PCI core code; where device specific resets are the exception on PCI, they are the norm on platform. > > > >> 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. > > Does the current approach have a separate driver for doing VFIO reset or > does it reuse the existing device driver? They're separate modules, not drivers. They simply export a reset function, no module init or exit function. A table lookup via the device compatibility string identifies the module to load and name of reset function. At one point I suggested that we simply use standard naming based on the compatibility string to name the module and reset function to avoid the table, but that was thought to be too hackish or perhaps dangerous to have vfio requesting modules like that. > Why does the driver registering itself instead of using symbol_get > imply that we must load reset drivers that we don't need? Who would register the reset driver if not the reset "driver" itself? Either we have a table/list of registered reset drivers and we can pick the one we want and the code is already loaded, or we have a table/list of who provides which reset driver and we load and get a pointer when we determine that we need it. Are there other options? Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 16 October 2015 15:06:45 Eric Auger wrote: > 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? I think it's possible to avoid the layering violation that way too, by loading the module based on the compatible string, with a module_alias. static void vfio_platform_get_reset(struct vfio_platform_device *vdev, struct device *dev) { const char *compat; int (*reset)(struct vfio_platform_device *); int ret, i; char modname[256]; ret = device_property_read_string(dev, "compatible", &compat); if (ret) return; reset = vfio_platform_lookup_reset(compat); if (!reset) { snprintf(modname, "vfio-reset:%s", compat); request_module(modname); reset = vfio_platform_lookup_reset(compat); } vdev->reset = reset; } --- #define module_vfio_reset_handler(compat, reset) \ MODULE_ALIAS("vfio_reset" compat); \ static int reset ## _module_init(void) \ { \ return vfio_reset_register(THIS_MODULE, compat, &reset); \ } I think that solution is good enough, as it avoids most of the problems with the current implementation but is a simple enough change. Arnd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On 10/16/2015 03:26 PM, Arnd Bergmann wrote: > On Friday 16 October 2015 15:06:45 Eric Auger wrote: > >> 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? > > I think it's possible to avoid the layering violation that way too, > by loading the module based on the compatible string, with a module_alias. > > static void vfio_platform_get_reset(struct vfio_platform_device *vdev, > struct device *dev) > { > const char *compat; > int (*reset)(struct vfio_platform_device *); > int ret, i; > char modname[256]; > > ret = device_property_read_string(dev, "compatible", &compat); > if (ret) > return; > > reset = vfio_platform_lookup_reset(compat); > if (!reset) { > snprintf(modname, "vfio-reset:%s", compat); > request_module(modname); > reset = vfio_platform_lookup_reset(compat); > } > > vdev->reset = reset; > } > > --- > > #define module_vfio_reset_handler(compat, reset) \ > MODULE_ALIAS("vfio_reset" compat); \ > static int reset ## _module_init(void) \ > { \ > return vfio_reset_register(THIS_MODULE, compat, &reset); \ > } > > I think that solution is good enough, as it avoids most of the > problems with the current implementation but is a simple enough change. That looks a good perspective. Thanks for the hint! Let's code now ;-) Best Regards Eric > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vfio/platform/reset/Kconfig b/drivers/vfio/platform/reset/Kconfig index 746b96b..ed9bb28 100644 --- a/drivers/vfio/platform/reset/Kconfig +++ b/drivers/vfio/platform/reset/Kconfig @@ -5,3 +5,10 @@ config VFIO_PLATFORM_CALXEDAXGMAC_RESET Enables the VFIO platform driver to handle reset for Calxeda xgmac If you don't know what to do here, say N. +config VFIO_PLATFORM_AMDXGBE_RESET + tristate "VFIO support for AMD XGBE reset" + depends on VFIO_PLATFORM + help + Enables the VFIO platform driver to handle reset for AMD XGBE + + If you don't know what to do here, say N. diff --git a/drivers/vfio/platform/reset/Makefile b/drivers/vfio/platform/reset/Makefile index 2a486af..93f4e23 100644 --- a/drivers/vfio/platform/reset/Makefile +++ b/drivers/vfio/platform/reset/Makefile @@ -1,5 +1,7 @@ vfio-platform-calxedaxgmac-y := vfio_platform_calxedaxgmac.o +vfio-platform-amdxgbe-y := vfio_platform_amdxgbe.o ccflags-y += -Idrivers/vfio/platform obj-$(CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET) += vfio-platform-calxedaxgmac.o +obj-$(CONFIG_VFIO_PLATFORM_AMDXGBE_RESET) += vfio-platform-amdxgbe.o diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c new file mode 100644 index 0000000..bd2189b --- /dev/null +++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c @@ -0,0 +1,130 @@ +/* + * VFIO platform driver specialized for AMD xgbe reset + * reset code is inherited from AMD xgbe native driver + * + * Copyright (c) 2015 Linaro Ltd. + * www.linaro.org + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/io.h> +#include <uapi/linux/mdio.h> +#include <linux/delay.h> + +#include "vfio_platform_private.h" + +#define DRIVER_VERSION "0.1" +#define DRIVER_AUTHOR "Eric Auger <eric.auger@linaro.org>" +#define DRIVER_DESC "Reset support for AMD xgbe vfio platform device" + +#define DMA_MR 0x3000 +#define MAC_VR 0x0110 +#define DMA_ISR 0x3008 +#define MAC_ISR 0x00b0 +#define PCS_MMD_SELECT 0xff +#define MDIO_AN_INT 0x8002 +#define MDIO_AN_INTMASK 0x8001 + +static unsigned int xmdio_read(void *ioaddr, unsigned int mmd, + unsigned int reg) +{ + unsigned int mmd_address, value; + + mmd_address = (mmd << 16) | ((reg) & 0xffff); + iowrite32(mmd_address >> 8, ioaddr + (PCS_MMD_SELECT << 2)); + value = ioread32(ioaddr + ((mmd_address & 0xff) << 2)); + return value; +} + +static void xmdio_write(void *ioaddr, unsigned int mmd, + unsigned int reg, unsigned int value) +{ + unsigned int mmd_address; + + mmd_address = (mmd << 16) | ((reg) & 0xffff); + iowrite32(mmd_address >> 8, ioaddr + (PCS_MMD_SELECT << 2)); + iowrite32(value, ioaddr + ((mmd_address & 0xff) << 2)); +} + +int vfio_platform_amdxgbe_reset(struct vfio_platform_device *vdev) +{ + struct vfio_platform_region xgmac_regs = vdev->regions[0]; + struct vfio_platform_region xpcs_regs = vdev->regions[1]; + u32 dma_mr_value, pcs_value, value; + unsigned int count; + + if (!xgmac_regs.ioaddr) { + xgmac_regs.ioaddr = + ioremap_nocache(xgmac_regs.addr, xgmac_regs.size); + if (!xgmac_regs.ioaddr) + return -ENOMEM; + } + if (!xpcs_regs.ioaddr) { + xpcs_regs.ioaddr = + ioremap_nocache(xpcs_regs.addr, xpcs_regs.size); + if (!xpcs_regs.ioaddr) + return -ENOMEM; + } + + /* reset the PHY through MDIO*/ + pcs_value = xmdio_read(xpcs_regs.ioaddr, MDIO_MMD_PCS, MDIO_CTRL1); + pcs_value |= MDIO_CTRL1_RESET; + xmdio_write(xpcs_regs.ioaddr, MDIO_MMD_PCS, MDIO_CTRL1, pcs_value); + + count = 50; + do { + msleep(20); + pcs_value = xmdio_read(xpcs_regs.ioaddr, MDIO_MMD_PCS, + MDIO_CTRL1); + } while ((pcs_value & MDIO_CTRL1_RESET) && --count); + + if (pcs_value & MDIO_CTRL1_RESET) + pr_warn("%s XGBE PHY reset timeout\n", __func__); + + /* disable auto-negotiation */ + value = xmdio_read(xpcs_regs.ioaddr, MDIO_MMD_AN, MDIO_CTRL1); + value &= ~MDIO_AN_CTRL1_ENABLE; + xmdio_write(xpcs_regs.ioaddr, MDIO_MMD_AN, MDIO_CTRL1, value); + + /* disable AN IRQ */ + xmdio_write(xpcs_regs.ioaddr, MDIO_MMD_AN, MDIO_AN_INTMASK, 0); + + /* clear AN IRQ */ + xmdio_write(xpcs_regs.ioaddr, MDIO_MMD_AN, MDIO_AN_INT, 0); + + /* MAC software reset */ + dma_mr_value = ioread32(xgmac_regs.ioaddr + DMA_MR); + dma_mr_value |= 0x1; + iowrite32(dma_mr_value, xgmac_regs.ioaddr + DMA_MR); + + usleep_range(10, 15); + + count = 2000; + while (count-- && (ioread32(xgmac_regs.ioaddr + DMA_MR) & 1)) + usleep_range(500, 600); + + if (!count) + pr_warn("%s MAC SW reset failed\n", __func__); + + return 0; +} +EXPORT_SYMBOL_GPL(vfio_platform_amdxgbe_reset); + +MODULE_VERSION(DRIVER_VERSION); +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR(DRIVER_AUTHOR); +MODULE_DESCRIPTION(DRIVER_DESC); diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index e43efb5..f1c5889 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = { .reset_function_name = "vfio_platform_calxedaxgmac_reset", .module_name = "vfio-platform-calxedaxgmac", }, + { + .compat = "amd,xgbe-seattle-v1a", + .reset_function_name = "vfio_platform_amdxgbe_reset", + .module_name = "vfio-platform-amdxgbe", + }, }; static void vfio_platform_get_reset(struct vfio_platform_device *vdev,
This patch introduces a module that registers and implements a low-level reset function for the AMD XGBE device. it performs the following actions: - reset the PHY - disable auto-negotiation - disable & clear auto-negotiation IRQ - soft-reset the MAC Those tiny pieces of code are inherited from the native xgbe driver. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- drivers/vfio/platform/reset/Kconfig | 7 ++ drivers/vfio/platform/reset/Makefile | 2 + .../vfio/platform/reset/vfio_platform_amdxgbe.c | 130 +++++++++++++++++++++ drivers/vfio/platform/vfio_platform_common.c | 5 + 4 files changed, 144 insertions(+) create mode 100644 drivers/vfio/platform/reset/vfio_platform_amdxgbe.c