diff mbox

VFIO: platform: AMD xgbe reset module

Message ID 1444836792-2405-1-git-send-email-eric.auger@linaro.org
State New
Headers show

Commit Message

Auger Eric Oct. 14, 2015, 3:33 p.m. UTC
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

Comments

Arnd Bergmann Oct. 14, 2015, 3:38 p.m. UTC | #1
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
Auger Eric Oct. 15, 2015, 8:08 a.m. UTC | #2
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
>
Arnd Bergmann Oct. 15, 2015, 11:21 a.m. UTC | #3
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
Christoffer Dall Oct. 15, 2015, 12:12 p.m. UTC | #4
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
Arnd Bergmann Oct. 15, 2015, 1:59 p.m. UTC | #5
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
Auger Eric Oct. 15, 2015, 2:20 p.m. UTC | #6
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
>
Arnd Bergmann Oct. 15, 2015, 2:28 p.m. UTC | #7
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
Auger Eric Oct. 15, 2015, 2:46 p.m. UTC | #8
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
>
Arnd Bergmann Oct. 15, 2015, 2:55 p.m. UTC | #9
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
Christoffer Dall Oct. 15, 2015, 3:03 p.m. UTC | #10
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
Arnd Bergmann Oct. 15, 2015, 3:49 p.m. UTC | #11
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
Christoffer Dall Oct. 15, 2015, 4:35 p.m. UTC | #12
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
Alex Williamson Oct. 15, 2015, 4:53 p.m. UTC | #13
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
Christoffer Dall Oct. 15, 2015, 7:42 p.m. UTC | #14
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
>
Alex Williamson Oct. 15, 2015, 8:26 p.m. UTC | #15
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
Auger Eric Oct. 16, 2015, 1:06 p.m. UTC | #16
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
>
Arnd Bergmann Oct. 16, 2015, 1:26 p.m. UTC | #17
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
Auger Eric Oct. 16, 2015, 1:56 p.m. UTC | #18
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
>
diff mbox

Patch

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,