| Message ID | 1445506922-6005-7-git-send-email-eric.auger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <eric.auger@linaro.org> 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(&driver_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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <eric.auger@linaro.org> > > 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(&driver_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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <eric.auger@linaro.org> > > 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(&driver_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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index 95b8294..5ebae8c 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -155,6 +155,7 @@ static void vfio_platform_release(void *device_data) vdev->reset(vdev); vfio_platform_regions_cleanup(vdev); vfio_platform_irq_cleanup(vdev); + vfio_platform_put_reset(vdev); } mutex_unlock(&driver_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); } @@ -570,8 +573,6 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, return ret; } - vfio_platform_get_reset(vdev); - mutex_init(&vdev->igate); return 0; @@ -585,7 +586,6 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev) vdev = vfio_del_group_dev(dev); if (vdev) { - vfio_platform_put_reset(vdev); iommu_group_put(dev->iommu_group); }
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 <eric.auger@linaro.org> --- drivers/vfio/platform/vfio_platform_common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)