[RFC] iommu/vt-d: Fix IOMMU field not populated on device hot re-plug
diff mbox series

Message ID 20190822142922.31526-1-janusz.krzysztofik@linux.intel.com
State New
Headers show
Series
  • [RFC] iommu/vt-d: Fix IOMMU field not populated on device hot re-plug
Related show

Commit Message

Janusz Krzysztofik Aug. 22, 2019, 2:29 p.m. UTC
When a perfectly working i915 device is hot unplugged (via sysfs) and
hot re-plugged again, its dev->archdata.iommu field is not populated
again with an IOMMU pointer.  As a result, the device probe fails on
DMA mapping error during scratch page setup.

It looks like that happens because devices are not detached from their
MMUIO bus before they are removed on device unplug.  Then, when an
already registered device/IOMMU association is identified by the
reinstantiated device's bus and function IDs on IOMMU bus re-attach
attempt, the device's archdata is not populated with IOMMU information
and the bad happens.

I'm not sure if this is a proper fix but it works for me so at least it
confirms correctness of my analysis results, I believe.  So far I
haven't been able to identify a good place where the possibly missing
IOMMU bus detach on device unplug operation could be added.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Lu Baolu Aug. 23, 2019, 1:51 a.m. UTC | #1
Hi,

On 8/22/19 10:29 PM, Janusz Krzysztofik wrote:
> When a perfectly working i915 device is hot unplugged (via sysfs) and
> hot re-plugged again, its dev->archdata.iommu field is not populated
> again with an IOMMU pointer.  As a result, the device probe fails on
> DMA mapping error during scratch page setup.
> 
> It looks like that happens because devices are not detached from their
> MMUIO bus before they are removed on device unplug.  Then, when an
> already registered device/IOMMU association is identified by the
> reinstantiated device's bus and function IDs on IOMMU bus re-attach
> attempt, the device's archdata is not populated with IOMMU information
> and the bad happens.
> 
> I'm not sure if this is a proper fix but it works for me so at least it
> confirms correctness of my analysis results, I believe.  So far I
> haven't been able to identify a good place where the possibly missing
> IOMMU bus detach on device unplug operation could be added.

Which kernel version are you testing with? Does it contain below commit?

commit 458b7c8e0dde12d140e3472b80919cbb9ae793f4
Author: Lu Baolu <baolu.lu@linux.intel.com>
Date:   Thu Aug 1 11:14:58 2019 +0800

     iommu/vt-d: Detach domain when move device out of group

     When removing a device from an iommu group, the domain should
     be detached from the device. Otherwise, the stale domain info
     will still be cached by the driver and the driver will refuse
     to attach any domain to the device again.

     Cc: Ashok Raj <ashok.raj@intel.com>
     Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
     Cc: Kevin Tian <kevin.tian@intel.com>
     Fixes: b7297783c2bb6 ("iommu/vt-d: Remove duplicated code for 
device hotplug")
     Reported-and-tested-by: Vlad Buslov <vladbu@mellanox.com>
     Suggested-by: Robin Murphy <robin.murphy@arm.com>
     Link: https://lkml.org/lkml/2019/7/26/1133
     Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
     Signed-off-by: Joerg Roedel <jroedel@suse.de>

Best regards,
Lu Baolu

> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> ---
>   drivers/iommu/intel-iommu.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 12d094d08c0a..7cdcd0595408 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2477,6 +2477,9 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
>   		if (info2) {
>   			found      = info2->domain;
>   			info2->dev = dev;
> +
> +			if (dev && !dev->archdata.iommu)
> +				dev->archdata.iommu = info2;
>   		}
>   	}
>   
>
Janusz Krzysztofik Aug. 26, 2019, 8:15 a.m. UTC | #2
Hi Lu,

On Friday, August 23, 2019 3:51:11 AM CEST Lu Baolu wrote:
> Hi,
> 
> On 8/22/19 10:29 PM, Janusz Krzysztofik wrote:
> > When a perfectly working i915 device is hot unplugged (via sysfs) and
> > hot re-plugged again, its dev->archdata.iommu field is not populated
> > again with an IOMMU pointer.  As a result, the device probe fails on
> > DMA mapping error during scratch page setup.
> > 
> > It looks like that happens because devices are not detached from their
> > MMUIO bus before they are removed on device unplug.  Then, when an
> > already registered device/IOMMU association is identified by the
> > reinstantiated device's bus and function IDs on IOMMU bus re-attach
> > attempt, the device's archdata is not populated with IOMMU information
> > and the bad happens.
> > 
> > I'm not sure if this is a proper fix but it works for me so at least it
> > confirms correctness of my analysis results, I believe.  So far I
> > haven't been able to identify a good place where the possibly missing
> > IOMMU bus detach on device unplug operation could be added.
> 
> Which kernel version are you testing with? Does it contain below commit?
> 
> commit 458b7c8e0dde12d140e3472b80919cbb9ae793f4
> Author: Lu Baolu <baolu.lu@linux.intel.com>
> Date:   Thu Aug 1 11:14:58 2019 +0800

I was using an internal branch based on drm-tip which didn't contain this 
commit yet.  Fortunately it has been already merged into drm-tip over last 
weekend and has effectively fixed the issue.

Thanks,
Janusz

>      iommu/vt-d: Detach domain when move device out of group
> 
>      When removing a device from an iommu group, the domain should
>      be detached from the device. Otherwise, the stale domain info
>      will still be cached by the driver and the driver will refuse
>      to attach any domain to the device again.
> 
>      Cc: Ashok Raj <ashok.raj@intel.com>
>      Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>      Cc: Kevin Tian <kevin.tian@intel.com>
>      Fixes: b7297783c2bb6 ("iommu/vt-d: Remove duplicated code for 
> device hotplug")
>      Reported-and-tested-by: Vlad Buslov <vladbu@mellanox.com>
>      Suggested-by: Robin Murphy <robin.murphy@arm.com>
>      Link: https://lkml.org/lkml/2019/7/26/1133
>      Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>      Signed-off-by: Joerg Roedel <jroedel@suse.de>
> 
> Best regards,
> Lu Baolu
> 
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > ---
> >   drivers/iommu/intel-iommu.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 12d094d08c0a..7cdcd0595408 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -2477,6 +2477,9 @@ static struct dmar_domain 
*dmar_insert_one_dev_info(struct intel_iommu *iommu,
> >   		if (info2) {
> >   			found      = info2->domain;
> >   			info2->dev = dev;
> > +
> > +			if (dev && !dev->archdata.iommu)
> > +				dev->archdata.iommu = info2;
> >   		}
> >   	}
> >   
> > 
>
Lu Baolu Aug. 26, 2019, 8:29 a.m. UTC | #3
Hi Janusz,

On 8/26/19 4:15 PM, Janusz Krzysztofik wrote:
> Hi Lu,
> 
> On Friday, August 23, 2019 3:51:11 AM CEST Lu Baolu wrote:
>> Hi,
>>
>> On 8/22/19 10:29 PM, Janusz Krzysztofik wrote:
>>> When a perfectly working i915 device is hot unplugged (via sysfs) and
>>> hot re-plugged again, its dev->archdata.iommu field is not populated
>>> again with an IOMMU pointer.  As a result, the device probe fails on
>>> DMA mapping error during scratch page setup.
>>>
>>> It looks like that happens because devices are not detached from their
>>> MMUIO bus before they are removed on device unplug.  Then, when an
>>> already registered device/IOMMU association is identified by the
>>> reinstantiated device's bus and function IDs on IOMMU bus re-attach
>>> attempt, the device's archdata is not populated with IOMMU information
>>> and the bad happens.
>>>
>>> I'm not sure if this is a proper fix but it works for me so at least it
>>> confirms correctness of my analysis results, I believe.  So far I
>>> haven't been able to identify a good place where the possibly missing
>>> IOMMU bus detach on device unplug operation could be added.
>>
>> Which kernel version are you testing with? Does it contain below commit?
>>
>> commit 458b7c8e0dde12d140e3472b80919cbb9ae793f4
>> Author: Lu Baolu <baolu.lu@linux.intel.com>
>> Date:   Thu Aug 1 11:14:58 2019 +0800
> 
> I was using an internal branch based on drm-tip which didn't contain this
> commit yet.  Fortunately it has been already merged into drm-tip over last
> weekend and has effectively fixed the issue.

Thanks for testing this.

Best regards,
Lu Baolu
Janusz Krzysztofik Aug. 27, 2019, 9:35 a.m. UTC | #4
Hi Lu,

On Monday, August 26, 2019 10:29:12 AM CEST Lu Baolu wrote:
> Hi Janusz,
> 
> On 8/26/19 4:15 PM, Janusz Krzysztofik wrote:
> > Hi Lu,
> > 
> > On Friday, August 23, 2019 3:51:11 AM CEST Lu Baolu wrote:
> >> Hi,
> >>
> >> On 8/22/19 10:29 PM, Janusz Krzysztofik wrote:
> >>> When a perfectly working i915 device is hot unplugged (via sysfs) and
> >>> hot re-plugged again, its dev->archdata.iommu field is not populated
> >>> again with an IOMMU pointer.  As a result, the device probe fails on
> >>> DMA mapping error during scratch page setup.
> >>>
> >>> It looks like that happens because devices are not detached from their
> >>> MMUIO bus before they are removed on device unplug.  Then, when an
> >>> already registered device/IOMMU association is identified by the
> >>> reinstantiated device's bus and function IDs on IOMMU bus re-attach
> >>> attempt, the device's archdata is not populated with IOMMU information
> >>> and the bad happens.
> >>>
> >>> I'm not sure if this is a proper fix but it works for me so at least it
> >>> confirms correctness of my analysis results, I believe.  So far I
> >>> haven't been able to identify a good place where the possibly missing
> >>> IOMMU bus detach on device unplug operation could be added.
> >>
> >> Which kernel version are you testing with? Does it contain below commit?
> >>
> >> commit 458b7c8e0dde12d140e3472b80919cbb9ae793f4
> >> Author: Lu Baolu <baolu.lu@linux.intel.com>
> >> Date:   Thu Aug 1 11:14:58 2019 +0800
> > 
> > I was using an internal branch based on drm-tip which didn't contain this
> > commit yet.  Fortunately it has been already merged into drm-tip over last
> > weekend and has effectively fixed the issue.
> 
> Thanks for testing this.

My testing appeared not sufficiently exhaustive. The fix indeed resolved my 
initially discovered issue of not being able to rebind the i915 driver to a 
re-plugged device, however it brought another, probably more serious problem 
to light.

When an open i915 device is hot unplugged, IOMMU bus notifier now cleans up 
IOMMU info for the device on PCI device remove while the i915 driver is still 
not released, kept by open file descriptors.  Then, on last device close, 
cleanup attempts lead to kernel panic raised from intel_unmap() on unresolved 
IOMMU domain.

With commit 458b7c8e0dde reverted and my fix applied, both late device close 
and device re-plug work for me.  However, I can realize that's probably still 
not a complete solution, possibly missing some protection against reuse of a 
removed device other than for cleanup.  If you think that's the right way to 
go, I can work more on that.

I've had a look at other drivers and found AMD is using somehow similar 
approach.  On the other hand, looking at the IOMMU common code I couldn't 
identify any arrangement that would support deferred device cleanup.

If that approach is not acceptable for Intel IOMMU, please suggest a way you'd 
like to have it resolved and I can try to implement it.

Thanks,
Janusz

> Best regards,
> Lu Baolu
>
Lu Baolu Aug. 28, 2019, 12:56 a.m. UTC | #5
Hi Janusz,

On 8/27/19 5:35 PM, Janusz Krzysztofik wrote:
> Hi Lu,
> 
> On Monday, August 26, 2019 10:29:12 AM CEST Lu Baolu wrote:
>> Hi Janusz,
>>
>> On 8/26/19 4:15 PM, Janusz Krzysztofik wrote:
>>> Hi Lu,
>>>
>>> On Friday, August 23, 2019 3:51:11 AM CEST Lu Baolu wrote:
>>>> Hi,
>>>>
>>>> On 8/22/19 10:29 PM, Janusz Krzysztofik wrote:
>>>>> When a perfectly working i915 device is hot unplugged (via sysfs) and
>>>>> hot re-plugged again, its dev->archdata.iommu field is not populated
>>>>> again with an IOMMU pointer.  As a result, the device probe fails on
>>>>> DMA mapping error during scratch page setup.
>>>>>
>>>>> It looks like that happens because devices are not detached from their
>>>>> MMUIO bus before they are removed on device unplug.  Then, when an
>>>>> already registered device/IOMMU association is identified by the
>>>>> reinstantiated device's bus and function IDs on IOMMU bus re-attach
>>>>> attempt, the device's archdata is not populated with IOMMU information
>>>>> and the bad happens.
>>>>>
>>>>> I'm not sure if this is a proper fix but it works for me so at least it
>>>>> confirms correctness of my analysis results, I believe.  So far I
>>>>> haven't been able to identify a good place where the possibly missing
>>>>> IOMMU bus detach on device unplug operation could be added.
>>>>
>>>> Which kernel version are you testing with? Does it contain below commit?
>>>>
>>>> commit 458b7c8e0dde12d140e3472b80919cbb9ae793f4
>>>> Author: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Date:   Thu Aug 1 11:14:58 2019 +0800
>>>
>>> I was using an internal branch based on drm-tip which didn't contain this
>>> commit yet.  Fortunately it has been already merged into drm-tip over last
>>> weekend and has effectively fixed the issue.
>>
>> Thanks for testing this.
> 
> My testing appeared not sufficiently exhaustive. The fix indeed resolved my
> initially discovered issue of not being able to rebind the i915 driver to a
> re-plugged device, however it brought another, probably more serious problem
> to light.
> 
> When an open i915 device is hot unplugged, IOMMU bus notifier now cleans up
> IOMMU info for the device on PCI device remove while the i915 driver is still
> not released, kept by open file descriptors.  Then, on last device close,
> cleanup attempts lead to kernel panic raised from intel_unmap() on unresolved
> IOMMU domain.

We should avoid kernel panic when a intel_unmap() is called against
a non-existent domain. But we shouldn't expect the IOMMU driver not
cleaning up the domain info when a device remove notification comes and 
wait until all file descriptors being closed, right?

Best regards,
Baolu

> 
> With commit 458b7c8e0dde reverted and my fix applied, both late device close
> and device re-plug work for me.  However, I can realize that's probably still
> not a complete solution, possibly missing some protection against reuse of a
> removed device other than for cleanup.  If you think that's the right way to
> go, I can work more on that.
> 
> I've had a look at other drivers and found AMD is using somehow similar
> approach.  On the other hand, looking at the IOMMU common code I couldn't
> identify any arrangement that would support deferred device cleanup.
> 
> If that approach is not acceptable for Intel IOMMU, please suggest a way you'd
> like to have it resolved and I can try to implement it.
> 
> Thanks,
> Janusz
> 
>> Best regards,
>> Lu Baolu
>>
Janusz Krzysztofik Aug. 28, 2019, 2:17 p.m. UTC | #6
On Wednesday, August 28, 2019 2:56:18 AM CEST Lu Baolu wrote:
> Hi Janusz,
> 
> On 8/27/19 5:35 PM, Janusz Krzysztofik wrote:
> > Hi Lu,
> > 
> > On Monday, August 26, 2019 10:29:12 AM CEST Lu Baolu wrote:
> >> Hi Janusz,
> >>
> >> On 8/26/19 4:15 PM, Janusz Krzysztofik wrote:
> >>> Hi Lu,
> >>>
> >>> On Friday, August 23, 2019 3:51:11 AM CEST Lu Baolu wrote:
> >>>> Hi,
> >>>>
> >>>> On 8/22/19 10:29 PM, Janusz Krzysztofik wrote:
> >>>>> When a perfectly working i915 device is hot unplugged (via sysfs) and
> >>>>> hot re-plugged again, its dev->archdata.iommu field is not populated
> >>>>> again with an IOMMU pointer.  As a result, the device probe fails on
> >>>>> DMA mapping error during scratch page setup.
> >>>>>
> >>>>> It looks like that happens because devices are not detached from their
> >>>>> MMUIO bus before they are removed on device unplug.  Then, when an
> >>>>> already registered device/IOMMU association is identified by the
> >>>>> reinstantiated device's bus and function IDs on IOMMU bus re-attach
> >>>>> attempt, the device's archdata is not populated with IOMMU information
> >>>>> and the bad happens.
> >>>>>
> >>>>> I'm not sure if this is a proper fix but it works for me so at least 
it
> >>>>> confirms correctness of my analysis results, I believe.  So far I
> >>>>> haven't been able to identify a good place where the possibly missing
> >>>>> IOMMU bus detach on device unplug operation could be added.
> >>>>
> >>>> Which kernel version are you testing with? Does it contain below 
commit?
> >>>>
> >>>> commit 458b7c8e0dde12d140e3472b80919cbb9ae793f4
> >>>> Author: Lu Baolu <baolu.lu@linux.intel.com>
> >>>> Date:   Thu Aug 1 11:14:58 2019 +0800
> >>>
> >>> I was using an internal branch based on drm-tip which didn't contain 
this
> >>> commit yet.  Fortunately it has been already merged into drm-tip over 
last
> >>> weekend and has effectively fixed the issue.
> >>
> >> Thanks for testing this.
> > 
> > My testing appeared not sufficiently exhaustive. The fix indeed resolved 
my
> > initially discovered issue of not being able to rebind the i915 driver to 
a
> > re-plugged device, however it brought another, probably more serious 
problem
> > to light.
> > 
> > When an open i915 device is hot unplugged, IOMMU bus notifier now cleans 
up
> > IOMMU info for the device on PCI device remove while the i915 driver is 
still
> > not released, kept by open file descriptors.  Then, on last device close,
> > cleanup attempts lead to kernel panic raised from intel_unmap() on 
unresolved
> > IOMMU domain.
> 
> We should avoid kernel panic when a intel_unmap() is called against
> a non-existent domain.

Does that mean you suggest to replace
	BUG_ON(!domain);
with something like
	if (WARN_ON(!domain))
		return;
and to not care of orphaned mappings left allocated?  Is there a way to inform 
users that their active DMA mappings are no longer valid and they shouldn't 
call dma_unmap_*()?

> But we shouldn't expect the IOMMU driver not
> cleaning up the domain info when a device remove notification comes and 
> wait until all file descriptors being closed, right?

Shouldn't then the IOMMU driver take care of cleaning up resources still 
allocated on device remove before it invalidates and forgets their pointers?

Thanks,
Janusz


> Best regards,
> Baolu
> 
> > 
> > With commit 458b7c8e0dde reverted and my fix applied, both late device 
close
> > and device re-plug work for me.  However, I can realize that's probably 
still
> > not a complete solution, possibly missing some protection against reuse of 
a
> > removed device other than for cleanup.  If you think that's the right way 
to
> > go, I can work more on that.
> > 
> > I've had a look at other drivers and found AMD is using somehow similar
> > approach.  On the other hand, looking at the IOMMU common code I couldn't
> > identify any arrangement that would support deferred device cleanup.
> > 
> > If that approach is not acceptable for Intel IOMMU, please suggest a way 
you'd
> > like to have it resolved and I can try to implement it.
> > 
> > Thanks,
> > Janusz
> > 
> >> Best regards,
> >> Lu Baolu
> >>
>
Lu Baolu Aug. 29, 2019, 1:43 a.m. UTC | #7
Hi Janusz,

On 8/28/19 10:17 PM, Janusz Krzysztofik wrote:
>> We should avoid kernel panic when a intel_unmap() is called against
>> a non-existent domain.
> Does that mean you suggest to replace
> 	BUG_ON(!domain);
> with something like
> 	if (WARN_ON(!domain))
> 		return;
> and to not care of orphaned mappings left allocated?  Is there a way to inform
> users that their active DMA mappings are no longer valid and they shouldn't
> call dma_unmap_*()?
> 
>> But we shouldn't expect the IOMMU driver not
>> cleaning up the domain info when a device remove notification comes and
>> wait until all file descriptors being closed, right?
> Shouldn't then the IOMMU driver take care of cleaning up resources still
> allocated on device remove before it invalidates and forgets their pointers?
> 

You are right. We need to wait until all allocated resources (iova and
mappings) to be released.

How about registering a callback for BUS_NOTIFY_UNBOUND_DRIVER, and
removing the domain info when the driver detachment completes?

> Thanks,
> Janusz

Best regards,
Baolu
Janusz Krzysztofik Aug. 29, 2019, 7:58 a.m. UTC | #8
Hi Baolu,

On Thursday, August 29, 2019 3:43:31 AM CEST Lu Baolu wrote:
> Hi Janusz,
> 
> On 8/28/19 10:17 PM, Janusz Krzysztofik wrote:
> >> We should avoid kernel panic when a intel_unmap() is called against
> >> a non-existent domain.
> > Does that mean you suggest to replace
> > 	BUG_ON(!domain);
> > with something like
> > 	if (WARN_ON(!domain))
> > 		return;
> > and to not care of orphaned mappings left allocated?  Is there a way to 
inform
> > users that their active DMA mappings are no longer valid and they 
shouldn't
> > call dma_unmap_*()?
> > 
> >> But we shouldn't expect the IOMMU driver not
> >> cleaning up the domain info when a device remove notification comes and
> >> wait until all file descriptors being closed, right?
> > Shouldn't then the IOMMU driver take care of cleaning up resources still
> > allocated on device remove before it invalidates and forgets their 
pointers?
> > 
> 
> You are right. We need to wait until all allocated resources (iova and
> mappings) to be released.
> 
> How about registering a callback for BUS_NOTIFY_UNBOUND_DRIVER, and
> removing the domain info when the driver detachment completes?

Device core calls BUS_NOTIFY_UNBOUND_DRIVER on each driver unbind, regardless 
of a device being removed or not.  As long as the device is not unplugged and 
the BUS_NOTIFY_REMOVED_DEVICE notification not generated, an unbound driver is 
not a problem here.
Morever, BUS_NOTIFY_UNBOUND_DRIVER  is called even before 
BUS_NOTIFY_REMOVED_DEVICE so that wouldn't help anyway.
Last but not least, bus events are independent of the IOMMU driver use via 
DMA-API it exposes.

If keeping data for unplugged devices and reusing it on device re-plug is not 
acceptable then maybe the IOMMU driver should perform reference counting of 
its internal resources occupied by DMA-API users and perform cleanups on last 
release?

Thanks,
Janusz


> > Thanks,
> > Janusz
> 
> Best regards,
> Baolu
>
Lu Baolu Aug. 29, 2019, 9:08 a.m. UTC | #9
Hi,

On 8/29/19 3:58 PM, Janusz Krzysztofik wrote:
> Hi Baolu,
> 
> On Thursday, August 29, 2019 3:43:31 AM CEST Lu Baolu wrote:
>> Hi Janusz,
>>
>> On 8/28/19 10:17 PM, Janusz Krzysztofik wrote:
>>>> We should avoid kernel panic when a intel_unmap() is called against
>>>> a non-existent domain.
>>> Does that mean you suggest to replace
>>> 	BUG_ON(!domain);
>>> with something like
>>> 	if (WARN_ON(!domain))
>>> 		return;
>>> and to not care of orphaned mappings left allocated?  Is there a way to
> inform
>>> users that their active DMA mappings are no longer valid and they
> shouldn't
>>> call dma_unmap_*()?
>>>
>>>> But we shouldn't expect the IOMMU driver not
>>>> cleaning up the domain info when a device remove notification comes and
>>>> wait until all file descriptors being closed, right?
>>> Shouldn't then the IOMMU driver take care of cleaning up resources still
>>> allocated on device remove before it invalidates and forgets their
> pointers?
>>>
>>
>> You are right. We need to wait until all allocated resources (iova and
>> mappings) to be released.
>>
>> How about registering a callback for BUS_NOTIFY_UNBOUND_DRIVER, and
>> removing the domain info when the driver detachment completes?
> 
> Device core calls BUS_NOTIFY_UNBOUND_DRIVER on each driver unbind, regardless
> of a device being removed or not.  As long as the device is not unplugged and
> the BUS_NOTIFY_REMOVED_DEVICE notification not generated, an unbound driver is
> not a problem here.
> Morever, BUS_NOTIFY_UNBOUND_DRIVER  is called even before
> BUS_NOTIFY_REMOVED_DEVICE so that wouldn't help anyway.
> Last but not least, bus events are independent of the IOMMU driver use via
> DMA-API it exposes.

Fair enough.

> 
> If keeping data for unplugged devices and reusing it on device re-plug is not
> acceptable then maybe the IOMMU driver should perform reference counting of
> its internal resources occupied by DMA-API users and perform cleanups on last
> release?

I am not saying that keeping data is not acceptable. I just want to
check whether there are any other solutions.

Best regards,
Baolu
Janusz Krzysztofik Sept. 2, 2019, 8:37 a.m. UTC | #10
Hi Baolu,

On Thursday, August 29, 2019 11:08:18 AM CEST Lu Baolu wrote:
> Hi,
> 
> On 8/29/19 3:58 PM, Janusz Krzysztofik wrote:
> > Hi Baolu,
> > 
> > On Thursday, August 29, 2019 3:43:31 AM CEST Lu Baolu wrote:
> >> Hi Janusz,
> >>
> >> On 8/28/19 10:17 PM, Janusz Krzysztofik wrote:
> >>>> We should avoid kernel panic when a intel_unmap() is called against
> >>>> a non-existent domain.
> >>> Does that mean you suggest to replace
> >>> 	BUG_ON(!domain);
> >>> with something like
> >>> 	if (WARN_ON(!domain))
> >>> 		return;
> >>> and to not care of orphaned mappings left allocated?  Is there a way to
> > inform
> >>> users that their active DMA mappings are no longer valid and they
> > shouldn't
> >>> call dma_unmap_*()?
> >>>
> >>>> But we shouldn't expect the IOMMU driver not
> >>>> cleaning up the domain info when a device remove notification comes and
> >>>> wait until all file descriptors being closed, right?
> >>> Shouldn't then the IOMMU driver take care of cleaning up resources still
> >>> allocated on device remove before it invalidates and forgets their
> > pointers?
> >>>
> >>
> >> You are right. We need to wait until all allocated resources (iova and
> >> mappings) to be released.
> >>
> >> How about registering a callback for BUS_NOTIFY_UNBOUND_DRIVER, and
> >> removing the domain info when the driver detachment completes?
> > 
> > Device core calls BUS_NOTIFY_UNBOUND_DRIVER on each driver unbind, 
regardless
> > of a device being removed or not.  As long as the device is not unplugged 
and
> > the BUS_NOTIFY_REMOVED_DEVICE notification not generated, an unbound 
driver is
> > not a problem here.
> > Morever, BUS_NOTIFY_UNBOUND_DRIVER  is called even before
> > BUS_NOTIFY_REMOVED_DEVICE so that wouldn't help anyway.
> > Last but not least, bus events are independent of the IOMMU driver use via
> > DMA-API it exposes.
> 
> Fair enough.
> 
> > 
> > If keeping data for unplugged devices and reusing it on device re-plug is 
not
> > acceptable then maybe the IOMMU driver should perform reference counting 
of
> > its internal resources occupied by DMA-API users and perform cleanups on 
last
> > release?
> 
> I am not saying that keeping data is not acceptable. I just want to
> check whether there are any other solutions.

Then reverting 458b7c8e0dde and applying this patch still resolves the issue 
for me.  No errors appear when mappings are unmapped on device close after the 
device has been removed, and domain info preserved on device removal is 
successfully reused on device re-plug.

Is there anything else I can do to help?

Thanks,
Janusz

> 
> Best regards,
> Baolu
>
Lu Baolu Sept. 3, 2019, 1:29 a.m. UTC | #11
Hi Janusz,

On 9/2/19 4:37 PM, Janusz Krzysztofik wrote:
>> I am not saying that keeping data is not acceptable. I just want to
>> check whether there are any other solutions.
> Then reverting 458b7c8e0dde and applying this patch still resolves the issue
> for me.  No errors appear when mappings are unmapped on device close after the
> device has been removed, and domain info preserved on device removal is
> successfully reused on device re-plug.

This patch doesn't look good to me although I agree that keeping data is
acceptable. It updates dev->archdata.iommu, but leaves the hardware
context/pasid table unchanged. This might cause problems somewhere.

> 
> Is there anything else I can do to help?

Can you please tell me how to reproduce the problem? Keeping the per
device domain info while device is unplugged is a bit dangerous because
info->dev might be a wild pointer. We need to work out a clean fix.

> 
> Thanks,
> Janusz
> 

Best regards,
Baolu
Janusz Krzysztofik Sept. 3, 2019, 7:41 a.m. UTC | #12
Hi Baolu,

On Tuesday, September 3, 2019 3:29:40 AM CEST Lu Baolu wrote:
> Hi Janusz,
> 
> On 9/2/19 4:37 PM, Janusz Krzysztofik wrote:
> >> I am not saying that keeping data is not acceptable. I just want to
> >> check whether there are any other solutions.
> > Then reverting 458b7c8e0dde and applying this patch still resolves the 
issue
> > for me.  No errors appear when mappings are unmapped on device close after 
the
> > device has been removed, and domain info preserved on device removal is
> > successfully reused on device re-plug.
> 
> This patch doesn't look good to me although I agree that keeping data is
> acceptable. It updates dev->archdata.iommu, but leaves the hardware
> context/pasid table unchanged. This might cause problems somewhere.
> 
> > 
> > Is there anything else I can do to help?
> 
> Can you please tell me how to reproduce the problem? 

The most simple way to reproduce the issue, assuming there are no non-Intel 
graphics adapters installed, is to run the following shell commands:

#!/bin/sh
# load i915 module
modprobe i915
# open an i915 device and keep it open in background
cat /dev/dri/card0 >/dev/null &
sleep 2
# simulate device unplug
echo 1 >/sys/class/drm/card0/device/remove
# make the background process close the device on exit
kill $!

Thanks,
Janusz


> Keeping the per
> device domain info while device is unplugged is a bit dangerous because
> info->dev might be a wild pointer. We need to work out a clean fix.
> 
> > 
> > Thanks,
> > Janusz
> > 
> 
> Best regards,
> Baolu
>

Patch
diff mbox series

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 12d094d08c0a..7cdcd0595408 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2477,6 +2477,9 @@  static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
 		if (info2) {
 			found      = info2->domain;
 			info2->dev = dev;
+
+			if (dev && !dev->archdata.iommu)
+				dev->archdata.iommu = info2;
 		}
 	}