Re: [PATCH] VFIO: platform: AMD xgbe reset module

[Eric Auger <eric.auger@linaro.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
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm