| Message ID | 1445506922-6005-7-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| 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
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 >
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 <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. > > 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 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 >
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
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 >
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
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(-)