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

From: Christoffer Dall <christoffer.dall@linaro.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Eric Auger <eric.auger@linaro.org>,
	linux-arm-kernel@lists.infradead.org, eric.auger@st.com,
	alex.williamson@redhat.com, b.reynal@virtualopensystems.com,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	thomas.lendacky@amd.com, patches@linaro.org,
	suravee.suthikulpanit@amd.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] VFIO: platform: AMD xgbe reset module
Date: Thu, 15 Oct 2015 14:12:28 +0200	[thread overview]
Message-ID: <20151015121228.GA965@cbox> (raw)
In-Reply-To: <7287273.XGFpHqQ9fN@wuerfel>

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