Re: [PATCH v2 6/6] vfio: platform: move get/put reset at open/release

[Arnd Bergmann]

On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
> Currently reset lookup is done on probe. This introduces a
> race with new registration mechanism in the case where the
> vfio-platform driver is bound to the device before its module
> is loaded: on the load, the probe happens which triggers the
> reset module load which itself attempts to get the symbol for
> the registration function (vfio_platform_register_reset). The
> symbol is not yet available hence the lookup fails. In case we
> do the lookup in the first open we are sure the vfio-platform
> module is loaded and vfio_platform_register_reset is available.
> 
> Signed-off-by: Eric Auger 

I don't understand the explanation. I would expect the request_module()
call to block until the module is actually loaded. Is this not
what happens?

> mutex_unlock(_lock);
> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
> if (ret)
> goto err_irq;
>  
> +   vfio_platform_get_reset(vdev);
> +
> if (vdev->reset)
> vdev->reset(vdev);
> 

This needs some error handling to ensure that the open() fails
if there is no reset handler.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] vfio: platform: move get/put reset at open/release

[Eric Auger]

Hi Arnd,
On 10/22/2015 12:29 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
>> Currently reset lookup is done on probe. This introduces a
>> race with new registration mechanism in the case where the
>> vfio-platform driver is bound to the device before its module
>> is loaded: on the load, the probe happens which triggers the
>> reset module load which itself attempts to get the symbol for
>> the registration function (vfio_platform_register_reset). The
>> symbol is not yet available hence the lookup fails. In case we
>> do the lookup in the first open we are sure the vfio-platform
>> module is loaded and vfio_platform_register_reset is available.
>>
>> Signed-off-by: Eric Auger 
> 
> I don't understand the explanation. I would expect the request_module()
> call to block until the module is actually loaded. Is this not
> what happens?

Again many thanks for this new review.

My understanding is the following
1) vfio-platform is attached to the device through the override mechanism
2) vfio-platform get loaded through modprobe:
2-1) the platform driver init function eventually calls the
vfio-platform probe function.
2-2) request_module of vfio-platform-calxedaxgmac gets called.
3) The init of  vfio-platform-calxedaxgmac looks for
vfio_platform_register_reset. Unfortunately at that stage the init of
vfio-platform is not completed so the symbol is not available
3-1) in case symbol_get is used in vfio_platform_calxedaxgmac init, as
of today, this latter simply returns -EINVAL. Reset registration failed
but no stall.
3-2) in case symbol_get is *not* used, I think the module loader
attempts to load vfio-platform, which is already under load and this
causes a stall.

Let me know if you think this understanding is not correct.

Best Regards

Eric
> 
>> mutex_unlock(_lock);
>> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
>> if (ret)
>> goto err_irq;
>>  
>> +   vfio_platform_get_reset(vdev);
>> +
>> if (vdev->reset)
>> vdev->reset(vdev);
>>
> 
> This needs some error handling to ensure that the open() fails
> if there is no reset handler.
> 
>   Arnd
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] vfio: platform: move get/put reset at open/release

[Arnd Bergmann]

On Thursday 22 October 2015 13:40:16 Eric Auger wrote:
> On 10/22/2015 12:29 PM, Arnd Bergmann wrote:
> > On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
> >> Currently reset lookup is done on probe. This introduces a
> >> race with new registration mechanism in the case where the
> >> vfio-platform driver is bound to the device before its module
> >> is loaded: on the load, the probe happens which triggers the
> >> reset module load which itself attempts to get the symbol for
> >> the registration function (vfio_platform_register_reset). The
> >> symbol is not yet available hence the lookup fails. In case we
> >> do the lookup in the first open we are sure the vfio-platform
> >> module is loaded and vfio_platform_register_reset is available.
> >>
> >> Signed-off-by: Eric Auger 
> > 
> > I don't understand the explanation. I would expect the request_module()
> > call to block until the module is actually loaded. Is this not
> > what happens?
> 
> Again many thanks for this new review.
> 
> My understanding is the following
> 1) vfio-platform is attached to the device through the override mechanism
> 2) vfio-platform get loaded through modprobe:
> 2-1) the platform driver init function eventually calls the
> vfio-platform probe function.
> 2-2) request_module of vfio-platform-calxedaxgmac gets called.
> 3) The init of  vfio-platform-calxedaxgmac looks for
> vfio_platform_register_reset. Unfortunately at that stage the init of
> vfio-platform is not completed so the symbol is not available
> 3-1) in case symbol_get is used in vfio_platform_calxedaxgmac init, as
> of today, this latter simply returns -EINVAL. Reset registration failed
> but no stall.
> 3-2) in case symbol_get is *not* used, I think the module loader
> attempts to load vfio-platform, which is already under load and this
> causes a stall.
> 
> Let me know if you think this understanding is not correct.

I was expecting the vfio_platform_register_reset() function to be
available by the time of step 2-2.

What I think is going on here is that we are still inside of the
module_init() function of the vfio-platform driver at the time that
we call the request_module(), and the symbols are not made visible
to other modules until the module_init function returns with success.
This is a side-effect of the probe function for the platform driver
getting called from deep inside of the platform_driver_register()
function for all devices that are already present.

I think it already works for the AMBA case, which uses separate modules,
but we need to restructure the platform_device case slightly to do
the same:

diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 9ce8afe28450..a00a44814255 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -1,10 +1,12 @@
 
-vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o
+vfio-platform-base-y := vfio_platform_common.o vfio_platform_irq.o
 
-obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
+obj-$(CONFIG_VFIO_PLATFORM) += vfio_platform.o
+obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform-base.o
 obj-$(CONFIG_VFIO_PLATFORM) += reset/
 
 vfio-amba-y := vfio_amba.o
 
 obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
+obj-$(CONFIG_VFIO_AMBA) += vfio-platform-base.o
 obj-$(CONFIG_VFIO_AMBA) += reset/


Arnd
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] vfio: platform: move get/put reset at open/release

[Arnd Bergmann]

On Thursday 22 October 2015 15:26:55 Eric Auger wrote:
> >> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
> >> if (ret)
> >> goto err_irq;
> >>  
> >> +   vfio_platform_get_reset(vdev);
> >> +
> >> if (vdev->reset)
> >> vdev->reset(vdev);
> >>
> > 
> > This needs some error handling to ensure that the open() fails
> > if there is no reset handler.
> 
> Is that really what we want? The code was meant to allow the use case
> where the VFIO platform driver would be used without such reset module.
> 
> I think the imperious need for a reset module depends on the device and
> more importantly depends on the IOMMU mapping. With QEMU VFIO
> integration this is needed because the whole VM memory is IOMMU mapped
> but in a simpler user-space driver context, we might live without.
> 
> Any thought?

I would think we need a reset driver for any device that can start DMA,
otherwise things can go wrong as soon as you attach it to a different domain
while there is ongoing DMA.

Maybe we could just allow devices to be attached without a reset handler,
but then disallow DMA on them?

Arnd
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] vfio: platform: move get/put reset at open/release

[Eric Auger]

On 10/22/2015 04:10 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 15:26:55 Eric Auger wrote:
 @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
 if (ret)
 goto err_irq;
  
 +   vfio_platform_get_reset(vdev);
 +
 if (vdev->reset)
 vdev->reset(vdev);

>>>
>>> This needs some error handling to ensure that the open() fails
>>> if there is no reset handler.
>>
>> Is that really what we want? The code was meant to allow the use case
>> where the VFIO platform driver would be used without such reset module.
>>
>> I think the imperious need for a reset module depends on the device and
>> more importantly depends on the IOMMU mapping. With QEMU VFIO
>> integration this is needed because the whole VM memory is IOMMU mapped
>> but in a simpler user-space driver context, we might live without.
>>
>> Any thought?
> 
> I would think we need a reset driver for any device that can start DMA,
> otherwise things can go wrong as soon as you attach it to a different domain
> while there is ongoing DMA.
> 
> Maybe we could just allow devices to be attached without a reset handler,
> but then disallow DMA on them?

Well I am tempted to think that most assigned devices will perform DMA
accesses so to me this somehow comes to the same result, ie disallowing
functional passthrough for devices not properly/fully integrated.

Alex/Baptiste, any opinion on this?

Thanks

Eric


> 
>   Arnd
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] vfio: platform: move get/put reset at open/release

[Eric Auger]

On 10/22/2015 02:05 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 13:40:16 Eric Auger wrote:
>> On 10/22/2015 12:29 PM, Arnd Bergmann wrote:
>>> On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
 Currently reset lookup is done on probe. This introduces a
 race with new registration mechanism in the case where the
 vfio-platform driver is bound to the device before its module
 is loaded: on the load, the probe happens which triggers the
 reset module load which itself attempts to get the symbol for
 the registration function (vfio_platform_register_reset). The
 symbol is not yet available hence the lookup fails. In case we
 do the lookup in the first open we are sure the vfio-platform
 module is loaded and vfio_platform_register_reset is available.

 Signed-off-by: Eric Auger 
>>>
>>> I don't understand the explanation. I would expect the request_module()
>>> call to block until the module is actually loaded. Is this not
>>> what happens?
>>
>> Again many thanks for this new review.
>>
>> My understanding is the following
>> 1) vfio-platform is attached to the device through the override mechanism
>> 2) vfio-platform get loaded through modprobe:
>> 2-1) the platform driver init function eventually calls the
>> vfio-platform probe function.
>> 2-2) request_module of vfio-platform-calxedaxgmac gets called.
>> 3) The init of  vfio-platform-calxedaxgmac looks for
>> vfio_platform_register_reset. Unfortunately at that stage the init of
>> vfio-platform is not completed so the symbol is not available
>> 3-1) in case symbol_get is used in vfio_platform_calxedaxgmac init, as
>> of today, this latter simply returns -EINVAL. Reset registration failed
>> but no stall.
>> 3-2) in case symbol_get is *not* used, I think the module loader
>> attempts to load vfio-platform, which is already under load and this
>> causes a stall.
>>
>> Let me know if you think this understanding is not correct.
> 
> I was expecting the vfio_platform_register_reset() function to be
> available by the time of step 2-2.
> 
> What I think is going on here is that we are still inside of the
> module_init() function of the vfio-platform driver at the time that
> we call the request_module(), and the symbols are not made visible
> to other modules until the module_init function returns with success.
> This is a side-effect of the probe function for the platform driver
> getting called from deep inside of the platform_driver_register()
> function for all devices that are already present.
yes we share the same understanding, this is what I meant.
> 
> I think it already works for the AMBA case, which uses separate modules,
> but we need to restructure the platform_device case slightly to do
> the same:

OK I will test the new module structure below and eventually remove the
symbol_get (I got the link issue). Thanks for the hint!

Eric
> 
> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
> index 9ce8afe28450..a00a44814255 100644
> --- a/drivers/vfio/platform/Makefile
> +++ b/drivers/vfio/platform/Makefile
> @@ -1,10 +1,12 @@
>  
> -vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o
> +vfio-platform-base-y := vfio_platform_common.o vfio_platform_irq.o
>  
> -obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
> +obj-$(CONFIG_VFIO_PLATFORM) += vfio_platform.o
> +obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform-base.o
>  obj-$(CONFIG_VFIO_PLATFORM) += reset/
>  
>  vfio-amba-y := vfio_amba.o
>  
>  obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
> +obj-$(CONFIG_VFIO_AMBA) += vfio-platform-base.o
>  obj-$(CONFIG_VFIO_AMBA) += reset/
> 
> 
>   Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] vfio: platform: move get/put reset at open/release

[Eric Auger]

On 10/22/2015 12:29 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
>> Currently reset lookup is done on probe. This introduces a
>> race with new registration mechanism in the case where the
>> vfio-platform driver is bound to the device before its module
>> is loaded: on the load, the probe happens which triggers the
>> reset module load which itself attempts to get the symbol for
>> the registration function (vfio_platform_register_reset). The
>> symbol is not yet available hence the lookup fails. In case we
>> do the lookup in the first open we are sure the vfio-platform
>> module is loaded and vfio_platform_register_reset is available.
>>
>> Signed-off-by: Eric Auger 
> 
> I don't understand the explanation. I would expect the request_module()
> call to block until the module is actually loaded. Is this not
> what happens?
> 
>> mutex_unlock(_lock);
>> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
>> if (ret)
>> goto err_irq;
>>  
>> +   vfio_platform_get_reset(vdev);
>> +
>> if (vdev->reset)
>> vdev->reset(vdev);
>>
> 
> This needs some error handling to ensure that the open() fails
> if there is no reset handler.

Is that really what we want? The code was meant to allow the use case
where the VFIO platform driver would be used without such reset module.

I think the imperious need for a reset module depends on the device and
more importantly depends on the IOMMU mapping. With QEMU VFIO
integration this is needed because the whole VM memory is IOMMU mapped
but in a simpler user-space driver context, we might live without.

Any thought?

Eric
> 
>   Arnd
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] vfio: platform: move get/put reset at open/release

[Alex Williamson]

On Thu, 2015-10-22 at 16:23 +0200, Eric Auger wrote:
> On 10/22/2015 04:10 PM, Arnd Bergmann wrote:
> > On Thursday 22 October 2015 15:26:55 Eric Auger wrote:
>  @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
>  if (ret)
>  goto err_irq;
>   
>  +   vfio_platform_get_reset(vdev);
>  +
>  if (vdev->reset)
>  vdev->reset(vdev);
> 
> >>>
> >>> This needs some error handling to ensure that the open() fails
> >>> if there is no reset handler.
> >>
> >> Is that really what we want? The code was meant to allow the use case
> >> where the VFIO platform driver would be used without such reset module.
> >>
> >> I think the imperious need for a reset module depends on the device and
> >> more importantly depends on the IOMMU mapping. With QEMU VFIO
> >> integration this is needed because the whole VM memory is IOMMU mapped
> >> but in a simpler user-space driver context, we might live without.
> >>
> >> Any thought?
> > 
> > I would think we need a reset driver for any device that can start DMA,
> > otherwise things can go wrong as soon as you attach it to a different domain
> > while there is ongoing DMA.
> > 
> > Maybe we could just allow devices to be attached without a reset handler,
> > but then disallow DMA on them?
> 
> Well I am tempted to think that most assigned devices will perform DMA
> accesses so to me this somehow comes to the same result, ie disallowing
> functional passthrough for devices not properly/fully integrated.
> 
> Alex/Baptiste, any opinion on this?

We have an IOMMU and the user doesn't get access to the device until the
IOMMU domain is established.  So, ideally yes, we should have a way to
reset the device, but I don't see it as a requirement.  Thanks,

Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html