amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Daniel Drake]

Hi,

We are unable to boot Acer Aspire E5-553G (AMD FX-9800P RADEON R7) nor
Acer Aspire E5-523 with standard configurations because during boot
the screen is flooded with the following error message over and over:

  AMD-Vi: Completion-Wait loop timed out

We have left the system for quite a while but the message spam does
not stop and the system doesn't complete the boot sequence.

We have reproduced on Linux 4.8 and Linux 4.10.

To avoid this, we can boot with iommu=soft or just disable the amdgpu
display driver.

Looks like this may also affect HP 15-ba012no :
https://bugzilla.redhat.com/show_bug.cgi?id=1409201

Earlier during boot the iommu is detected as:

[    1.274518] AMD-Vi: Found IOMMU at 0000:00:00.2 cap 0x40
[    1.274519] AMD-Vi: Extended features (0x37ef22294ada):
[    1.274519]  PPR NX GT IA GA PC GA_vAPIC
[    1.274523] AMD-Vi: Interrupt remapping enabled
[    1.274523] AMD-Vi: virtual APIC enabled
[    1.275144] AMD-Vi: Lazy IO/TLB flushing enabled
[    1.276498] perf: AMD NB counters detected
[    1.278096] LVT offset 0 assigned for vector 0x400
[    1.278963] perf: AMD IBS detected (0x000007ff)
[    1.278977] perf: amd_iommu: Detected. (0 banks, 0 counters/bank)

Any suggestions for how we can fix this, or get more useful debug info?

Thanks
Daniel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Drake
> Sent: Monday, March 13, 2017 3:50 PM
> To: joro@8bytes.org
> Cc: Chris Chiu; iommu@lists.linux-foundation.org; Linux Upstreaming Team;
> amd-gfx@lists.freedesktop.org
> Subject: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
> loop timed out
> 
> Hi,
> 
> We are unable to boot Acer Aspire E5-553G (AMD FX-9800P RADEON R7) nor
> Acer Aspire E5-523 with standard configurations because during boot
> the screen is flooded with the following error message over and over:
> 
>   AMD-Vi: Completion-Wait loop timed out
> 
> We have left the system for quite a while but the message spam does
> not stop and the system doesn't complete the boot sequence.
> 
> We have reproduced on Linux 4.8 and Linux 4.10.
> 
> To avoid this, we can boot with iommu=soft or just disable the amdgpu
> display driver.
> 
> Looks like this may also affect HP 15-ba012no :
> https://bugzilla.redhat.com/show_bug.cgi?id=1409201
> 
> Earlier during boot the iommu is detected as:
> 
> [    1.274518] AMD-Vi: Found IOMMU at 0000:00:00.2 cap 0x40
> [    1.274519] AMD-Vi: Extended features (0x37ef22294ada):
> [    1.274519]  PPR NX GT IA GA PC GA_vAPIC
> [    1.274523] AMD-Vi: Interrupt remapping enabled
> [    1.274523] AMD-Vi: virtual APIC enabled
> [    1.275144] AMD-Vi: Lazy IO/TLB flushing enabled
> [    1.276498] perf: AMD NB counters detected
> [    1.278096] LVT offset 0 assigned for vector 0x400
> [    1.278963] perf: AMD IBS detected (0x000007ff)
> [    1.278977] perf: amd_iommu: Detected. (0 banks, 0 counters/bank)
> 
> Any suggestions for how we can fix this, or get more useful debug info?

+Suravee and Arindam

We ran into similar issues and bisected it to commit b1516a14657acf81a587e9a6e733a881625eee53.  I'm not too familiar with the IOMMU hardware to know if this is an iommu or display driver issue yet.

Alex

> 
> Thanks
> Daniel
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Nath, Arindam]

>-----Original Message-----
>From: Deucher, Alexander
>Sent: Tuesday, March 14, 2017 1:31 AM
>To: 'Daniel Drake'; joro@8bytes.org; Suthikulpanit, Suravee; Nath, Arindam
>Cc: Chris Chiu; iommu@lists.linux-foundation.org; Linux Upstreaming Team;
>amd-gfx@lists.freedesktop.org
>Subject: RE: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
>loop timed out
>
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Daniel Drake
>> Sent: Monday, March 13, 2017 3:50 PM
>> To: joro@8bytes.org
>> Cc: Chris Chiu; iommu@lists.linux-foundation.org; Linux Upstreaming Team;
>> amd-gfx@lists.freedesktop.org
>> Subject: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
>> loop timed out
>>
>> Hi,
>>
>> We are unable to boot Acer Aspire E5-553G (AMD FX-9800P RADEON R7) nor
>> Acer Aspire E5-523 with standard configurations because during boot
>> the screen is flooded with the following error message over and over:
>>
>>   AMD-Vi: Completion-Wait loop timed out
>>
>> We have left the system for quite a while but the message spam does
>> not stop and the system doesn't complete the boot sequence.
>>
>> We have reproduced on Linux 4.8 and Linux 4.10.
>>
>> To avoid this, we can boot with iommu=soft or just disable the amdgpu
>> display driver.
>>
>> Looks like this may also affect HP 15-ba012no :
>> https://bugzilla.redhat.com/show_bug.cgi?id=1409201
>>
>> Earlier during boot the iommu is detected as:
>>
>> [    1.274518] AMD-Vi: Found IOMMU at 0000:00:00.2 cap 0x40
>> [    1.274519] AMD-Vi: Extended features (0x37ef22294ada):
>> [    1.274519]  PPR NX GT IA GA PC GA_vAPIC
>> [    1.274523] AMD-Vi: Interrupt remapping enabled
>> [    1.274523] AMD-Vi: virtual APIC enabled
>> [    1.275144] AMD-Vi: Lazy IO/TLB flushing enabled
>> [    1.276498] perf: AMD NB counters detected
>> [    1.278096] LVT offset 0 assigned for vector 0x400
>> [    1.278963] perf: AMD IBS detected (0x000007ff)
>> [    1.278977] perf: amd_iommu: Detected. (0 banks, 0 counters/bank)
>>
>> Any suggestions for how we can fix this, or get more useful debug info?
>
>+Suravee and Arindam
>
>We ran into similar issues and bisected it to commit
>b1516a14657acf81a587e9a6e733a881625eee53.  I'm not too familiar with the
>IOMMU hardware to know if this is an iommu or display driver issue yet.

Like Alex mentioned, we have not yet been able to root cause the issue. But another way to work-around the issue without disabling graphics driver or IOMMU is to use amd_iommu=fullflush kernel boot param.

Thanks,
Arindam

>
>Alex
>
>>
>> Thanks
>> Daniel
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Daniel Drake]

Hi,

On Mon, Mar 13, 2017 at 2:01 PM, Deucher, Alexander
<Alexander.Deucher-5C7GfCeVMHo@public.gmane.org> wrote:
> > We are unable to boot Acer Aspire E5-553G (AMD FX-9800P RADEON R7) nor
> > Acer Aspire E5-523 with standard configurations because during boot
> > the screen is flooded with the following error message over and over:
> >
> >   AMD-Vi: Completion-Wait loop timed out
>
> We ran into similar issues and bisected it to commit b1516a14657acf81a587e9a6e733a881625eee53.  I'm not too familiar with the IOMMU hardware to know if this is an iommu or display driver issue yet.

We can confirm that reverting this commit solves the issue.

Given that that commit is an optimization, but it has introduced a
regression on multiple platforms, and has been like this for 8 months,
it would be common practice to now revert this patch upstream until
the regression is fixed. Could you please send a new patch to do this?

Also, we would be happy to test any real solutions to this issue while
we still have the affected units in hand.

Thanks
Daniel

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Alex Deucher]

On Fri, Mar 17, 2017 at 8:15 AM, Daniel Drake <drake@endlessm.com> wrote:
> Hi,
>
> On Mon, Mar 13, 2017 at 2:01 PM, Deucher, Alexander
> <Alexander.Deucher@amd.com> wrote:
>> > We are unable to boot Acer Aspire E5-553G (AMD FX-9800P RADEON R7) nor
>> > Acer Aspire E5-523 with standard configurations because during boot
>> > the screen is flooded with the following error message over and over:
>> >
>> >   AMD-Vi: Completion-Wait loop timed out
>>
>> We ran into similar issues and bisected it to commit b1516a14657acf81a587e9a6e733a881625eee53.  I'm not too familiar with the IOMMU hardware to know if this is an iommu or display driver issue yet.
>
> We can confirm that reverting this commit solves the issue.
>
> Given that that commit is an optimization, but it has introduced a
> regression on multiple platforms, and has been like this for 8 months,
> it would be common practice to now revert this patch upstream until
> the regression is fixed. Could you please send a new patch to do this?
>
> Also, we would be happy to test any real solutions to this issue while
> we still have the affected units in hand.

No objections to a revert here.

Alex


>
> Thanks
> Daniel
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[joro-zLv9SwRftAIdnm+yROfE0A]

On Fri, Mar 17, 2017 at 11:53:09AM -0400, Alex Deucher wrote:
> On Fri, Mar 17, 2017 at 8:15 AM, Daniel Drake <drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org> wrote:
> > Hi,
> >
> > On Mon, Mar 13, 2017 at 2:01 PM, Deucher, Alexander
> > <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org> wrote:
> >> > We are unable to boot Acer Aspire E5-553G (AMD FX-9800P RADEON R7) nor
> >> > Acer Aspire E5-523 with standard configurations because during boot
> >> > the screen is flooded with the following error message over and over:
> >> >
> >> >   AMD-Vi: Completion-Wait loop timed out
> >>
> >> We ran into similar issues and bisected it to commit b1516a14657acf81a587e9a6e733a881625eee53.  I'm not too familiar with the IOMMU hardware to know if this is an iommu or display driver issue yet.
> >
> > We can confirm that reverting this commit solves the issue.
> >
> > Given that that commit is an optimization, but it has introduced a
> > regression on multiple platforms, and has been like this for 8 months,
> > it would be common practice to now revert this patch upstream until
> > the regression is fixed. Could you please send a new patch to do this?
> >
> > Also, we would be happy to test any real solutions to this issue while
> > we still have the affected units in hand.
> 
> No objections to a revert here.

Big objection here. Since this only happens with amdgpu so far we
shouldn't rule out a display-driver issue.

Reverting that patch basically destroys iommu-performance on AMD
systems. Doing this for all devices just to make amdgpu working is
overkill at this stage of the debugging.



	Joerg

RE: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Deucher, Alexander]

> -----Original Message-----
> From: joro@8bytes.org [mailto:joro@8bytes.org]
> Sent: Tuesday, March 21, 2017 11:57 AM
> To: Alex Deucher
> Cc: Daniel Drake; Deucher, Alexander; Chris Chiu; amd-
> gfx@lists.freedesktop.org; Nath, Arindam; iommu@lists.linux-
> foundation.org; Suthikulpanit, Suravee; Linux Upstreaming Team
> Subject: Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
> loop timed out
> 
> On Fri, Mar 17, 2017 at 11:53:09AM -0400, Alex Deucher wrote:
> > On Fri, Mar 17, 2017 at 8:15 AM, Daniel Drake <drake@endlessm.com>
> wrote:
> > > Hi,
> > >
> > > On Mon, Mar 13, 2017 at 2:01 PM, Deucher, Alexander
> > > <Alexander.Deucher@amd.com> wrote:
> > >> > We are unable to boot Acer Aspire E5-553G (AMD FX-9800P RADEON
> R7) nor
> > >> > Acer Aspire E5-523 with standard configurations because during boot
> > >> > the screen is flooded with the following error message over and over:
> > >> >
> > >> >   AMD-Vi: Completion-Wait loop timed out
> > >>
> > >> We ran into similar issues and bisected it to commit
> b1516a14657acf81a587e9a6e733a881625eee53.  I'm not too familiar with the
> IOMMU hardware to know if this is an iommu or display driver issue yet.
> > >
> > > We can confirm that reverting this commit solves the issue.
> > >
> > > Given that that commit is an optimization, but it has introduced a
> > > regression on multiple platforms, and has been like this for 8 months,
> > > it would be common practice to now revert this patch upstream until
> > > the regression is fixed. Could you please send a new patch to do this?
> > >
> > > Also, we would be happy to test any real solutions to this issue while
> > > we still have the affected units in hand.
> >
> > No objections to a revert here.
> 
> Big objection here. Since this only happens with amdgpu so far we
> shouldn't rule out a display-driver issue.
> 
> Reverting that patch basically destroys iommu-performance on AMD
> systems. Doing this for all devices just to make amdgpu working is
> overkill at this stage of the debugging.

It seems to only affect Stoney systems, but not others (Carrizo, Bristol, etc.).  Maybe we could just disable it on Stoney until we root cause it.

Alex

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

['joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org']

On Tue, Mar 21, 2017 at 04:01:53PM +0000, Deucher, Alexander wrote:
> It seems to only affect Stoney systems, but not others (Carrizo,
> Bristol, etc.).  Maybe we could just disable it on Stoney until we
> root cause it.

Completion-wait loop timeouts indicate something is seriously wrong. How
can I detect whether I am running on a 'Stoney' system?

Other question, a shot into the dark, does the GPU on these systems have
ATS? Probably yes, as they are likely HSA compatible.


	Joerg

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Nath, Arindam]

>-----Original Message-----
>From: 'joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org' [mailto:joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org]
>Sent: Tuesday, March 21, 2017 9:41 PM
>To: Deucher, Alexander
>Cc: Alex Deucher; Daniel Drake; Chris Chiu; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org;
>Nath, Arindam; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Suthikulpanit, Suravee;
>Linux Upstreaming Team
>Subject: Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
>loop timed out
>
>On Tue, Mar 21, 2017 at 04:01:53PM +0000, Deucher, Alexander wrote:
>> It seems to only affect Stoney systems, but not others (Carrizo,
>> Bristol, etc.).  Maybe we could just disable it on Stoney until we
>> root cause it.
>
>Completion-wait loop timeouts indicate something is seriously wrong. How
>can I detect whether I am running on a 'Stoney' system?
>
>Other question, a shot into the dark, does the GPU on these systems have
>ATS? Probably yes, as they are likely HSA compatible.

The GPU on Stoney supports ATS.

Thanks,
Arindam

>
>
>	Joerg

RE: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Deucher, Alexander]

> -----Original Message-----
> From: 'joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org' [mailto:joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org]
> Sent: Tuesday, March 21, 2017 12:11 PM
> To: Deucher, Alexander
> Cc: Alex Deucher; Daniel Drake; Chris Chiu; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org;
> Nath, Arindam; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Suthikulpanit, Suravee;
> Linux Upstreaming Team
> Subject: Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
> loop timed out
> 
> On Tue, Mar 21, 2017 at 04:01:53PM +0000, Deucher, Alexander wrote:
> > It seems to only affect Stoney systems, but not others (Carrizo,
> > Bristol, etc.).  Maybe we could just disable it on Stoney until we
> > root cause it.
> 
> Completion-wait loop timeouts indicate something is seriously wrong. How
> can I detect whether I am running on a 'Stoney' system?

+ John

I'm not sure if the iommu ids are different on stoney systems compared to Carrizo/Bristol systems.  The pci ids of the GPUs are different.  Stoney parts have 0x98E4 as the pci id for the GPU.

> 
> Other question, a shot into the dark, does the GPU on these systems have
> ATS? Probably yes, as they are likely HSA compatible.

Stoney is a small APU.  Kind of a mini Carrizo.  While it may claim to support ATS, I don't think it was ever validated on Stoney, only Carrizo/Bristol.

Alex

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

['joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org']

On Tue, Mar 21, 2017 at 04:17:40PM +0000, Deucher, Alexander wrote:
> > -----Original Message-----
> > From: 'joro@8bytes.org' [mailto:joro@8bytes.org]
> > Sent: Tuesday, March 21, 2017 12:11 PM
> > To: Deucher, Alexander
> > Cc: Alex Deucher; Daniel Drake; Chris Chiu; amd-gfx@lists.freedesktop.org;
> > Nath, Arindam; iommu@lists.linux-foundation.org; Suthikulpanit, Suravee;
> > Linux Upstreaming Team
> > Subject: Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
> > loop timed out
> > 
> > On Tue, Mar 21, 2017 at 04:01:53PM +0000, Deucher, Alexander wrote:
> > > It seems to only affect Stoney systems, but not others (Carrizo,
> > > Bristol, etc.).  Maybe we could just disable it on Stoney until we
> > > root cause it.
> > 
> > Completion-wait loop timeouts indicate something is seriously wrong. How
> > can I detect whether I am running on a 'Stoney' system?
> 
> + John
> 
> I'm not sure if the iommu ids are different on stoney systems compared to Carrizo/Bristol systems.  The pci ids of the GPUs are different.  Stoney parts have 0x98E4 as the pci id for the GPU.
> 
> > 
> > Other question, a shot into the dark, does the GPU on these systems have
> > ATS? Probably yes, as they are likely HSA compatible.
> 
> Stoney is a small APU.  Kind of a mini Carrizo.  While it may claim to support ATS, I don't think it was ever validated on Stoney, only Carrizo/Bristol.

Okay, so maybe ATS is broken in some way on these chips. When queue
flushes happen it will also send the ATS-invalidates, and a queue flush
can cause a storm of those. This may be the issue.

I am preparing a debug-patch that disables ATS for these GPUs so someone
with such a chip can test it.


	Joerg

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Deucher, Alexander]

> -----Original Message-----
> From: 'joro@8bytes.org' [mailto:joro@8bytes.org]
> Sent: Tuesday, March 21, 2017 12:26 PM
> To: Deucher, Alexander
> Cc: Bridgman, John; Alex Deucher; Daniel Drake; Chris Chiu; amd-
> gfx@lists.freedesktop.org; Nath, Arindam; iommu@lists.linux-
> foundation.org; Suthikulpanit, Suravee; Linux Upstreaming Team
> Subject: Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
> loop timed out
> 
> On Tue, Mar 21, 2017 at 04:17:40PM +0000, Deucher, Alexander wrote:
> > > -----Original Message-----
> > > From: 'joro@8bytes.org' [mailto:joro@8bytes.org]
> > > Sent: Tuesday, March 21, 2017 12:11 PM
> > > To: Deucher, Alexander
> > > Cc: Alex Deucher; Daniel Drake; Chris Chiu; amd-
> gfx@lists.freedesktop.org;
> > > Nath, Arindam; iommu@lists.linux-foundation.org; Suthikulpanit,
> Suravee;
> > > Linux Upstreaming Team
> > > Subject: Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-
> Wait
> > > loop timed out
> > >
> > > On Tue, Mar 21, 2017 at 04:01:53PM +0000, Deucher, Alexander wrote:
> > > > It seems to only affect Stoney systems, but not others (Carrizo,
> > > > Bristol, etc.).  Maybe we could just disable it on Stoney until we
> > > > root cause it.
> > >
> > > Completion-wait loop timeouts indicate something is seriously wrong.
> How
> > > can I detect whether I am running on a 'Stoney' system?
> >
> > + John
> >
> > I'm not sure if the iommu ids are different on stoney systems compared to
> Carrizo/Bristol systems.  The pci ids of the GPUs are different.  Stoney parts
> have 0x98E4 as the pci id for the GPU.
> >
> > >
> > > Other question, a shot into the dark, does the GPU on these systems
> have
> > > ATS? Probably yes, as they are likely HSA compatible.
> >
> > Stoney is a small APU.  Kind of a mini Carrizo.  While it may claim to support
> ATS, I don't think it was ever validated on Stoney, only Carrizo/Bristol.
> 
> Okay, so maybe ATS is broken in some way on these chips. When queue
> flushes happen it will also send the ATS-invalidates, and a queue flush
> can cause a storm of those. This may be the issue.
> 
> I am preparing a debug-patch that disables ATS for these GPUs so someone
> with such a chip can test it.

Thanks Joerg.

Alex

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Tom St Denis]

On 21/03/17 12:25 PM, 'joro@8bytes.org' wrote:
> On Tue, Mar 21, 2017 at 04:17:40PM +0000, Deucher, Alexander wrote:
>>> -----Original Message-----
>>> From: 'joro@8bytes.org' [mailto:joro@8bytes.org]
>>> Sent: Tuesday, March 21, 2017 12:11 PM
>>> To: Deucher, Alexander
>>> Cc: Alex Deucher; Daniel Drake; Chris Chiu; amd-gfx@lists.freedesktop.org;
>>> Nath, Arindam; iommu@lists.linux-foundation.org; Suthikulpanit, Suravee;
>>> Linux Upstreaming Team
>>> Subject: Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
>>> loop timed out
>>>
>>> On Tue, Mar 21, 2017 at 04:01:53PM +0000, Deucher, Alexander wrote:
>>>> It seems to only affect Stoney systems, but not others (Carrizo,
>>>> Bristol, etc.).  Maybe we could just disable it on Stoney until we
>>>> root cause it.
>>>
>>> Completion-wait loop timeouts indicate something is seriously wrong. How
>>> can I detect whether I am running on a 'Stoney' system?
>>
>> + John
>>
>> I'm not sure if the iommu ids are different on stoney systems compared to Carrizo/Bristol systems.  The pci ids of the GPUs are different.  Stoney parts have 0x98E4 as the pci id for the GPU.
>>
>>>
>>> Other question, a shot into the dark, does the GPU on these systems have
>>> ATS? Probably yes, as they are likely HSA compatible.
>>
>> Stoney is a small APU.  Kind of a mini Carrizo.  While it may claim to support ATS, I don't think it was ever validated on Stoney, only Carrizo/Bristol.
>
> Okay, so maybe ATS is broken in some way on these chips. When queue
> flushes happen it will also send the ATS-invalidates, and a queue flush
> can cause a storm of those. This may be the issue.
>
> I am preparing a debug-patch that disables ATS for these GPUs so someone
> with such a chip can test it.
>


I have a stoney I can test said patch on.

Thanks,
Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

['joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org']

On Tue, Mar 21, 2017 at 04:30:55PM +0000, Deucher, Alexander wrote:
> > I am preparing a debug-patch that disables ATS for these GPUs so someone
> > with such a chip can test it.
> 
> Thanks Joerg.

Here is a debug patch, using the hard hammer of disabling the use of ATS
completly in the AMD IOMMU driver. If it fixes the issue I am going to
write a more upstreamable version.

But for now, please test if this fixes the issue.

Thanks,

	Joerg

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 98940d1..f019aa6 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -467,7 +467,7 @@ static int iommu_init_device(struct device *dev)
 		struct amd_iommu *iommu;
 
 		iommu = amd_iommu_rlookup_table[dev_data->devid];
-		dev_data->iommu_v2 = iommu->is_iommu_v2;
+		dev_data->iommu_v2 = false;
 	}
 
 	dev->archdata.iommu = dev_data;
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 6130278..41d0e64 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -171,7 +171,7 @@ int amd_iommus_present;
 
 /* IOMMUs have a non-present cache? */
 bool amd_iommu_np_cache __read_mostly;
-bool amd_iommu_iotlb_sup __read_mostly = true;
+bool amd_iommu_iotlb_sup __read_mostly = false;
 
 u32 amd_iommu_max_pasid __read_mostly = ~0;
 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Tom St Denis]

On 22/03/17 07:22 AM, 'joro@8bytes.org' wrote:
> On Tue, Mar 21, 2017 at 04:30:55PM +0000, Deucher, Alexander wrote:
>>> I am preparing a debug-patch that disables ATS for these GPUs so someone
>>> with such a chip can test it.
>>
>> Thanks Joerg.
>
> Here is a debug patch, using the hard hammer of disabling the use of ATS
> completly in the AMD IOMMU driver. If it fixes the issue I am going to
> write a more upstreamable version.
>
> But for now, please test if this fixes the issue.
>
> Thanks,
>
> 	Joerg
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 98940d1..f019aa6 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -467,7 +467,7 @@ static int iommu_init_device(struct device *dev)
>  		struct amd_iommu *iommu;
>
>  		iommu = amd_iommu_rlookup_table[dev_data->devid];
> -		dev_data->iommu_v2 = iommu->is_iommu_v2;
> +		dev_data->iommu_v2 = false;
>  	}
>
>  	dev->archdata.iommu = dev_data;
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 6130278..41d0e64 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -171,7 +171,7 @@ int amd_iommus_present;
>
>  /* IOMMUs have a non-present cache? */
>  bool amd_iommu_np_cache __read_mostly;
> -bool amd_iommu_iotlb_sup __read_mostly = true;
> +bool amd_iommu_iotlb_sup __read_mostly = false;
>
>  u32 amd_iommu_max_pasid __read_mostly = ~0;


I tried this out but still got a lockup during init.  I tried getting a 
dmesg log but it locked up before it outputted anything more (after the 
init).

Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Tom St Denis]

On 22/03/17 01:18 PM, Tom St Denis wrote:

> I tried this out but still got a lockup during init.  I tried getting a
> dmesg log but it locked up before it outputted anything more (after the
> init).

nvm.  Even with iommu disabled it fails to boot.  Stoney support dies 
occasionally on staging-4.9 so I'll have to bisect this first and then 
circle back to this iommu patch.

Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] iommu/amd: flush IOTLB for specific domains only

[arindam.nath-5C7GfCeVMHo]

From: Arindam Nath <arindam.nath@amd.com>

The idea behind flush queues is to defer the IOTLB flushing
for domains for which the mappings are no longer valid. We
add such domains in queue_add(), and when the queue size
reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().

Since we have already taken lock before __queue_flush()
is called, we need to make sure the IOTLB flushing is
performed as quickly as possible.

In the current implementation, we perform IOTLB flushing
for all domains irrespective of which ones were actually
added in the flush queue initially. This can be quite
expensive especially for domains for which unmapping is
not required at this point of time.

This patch makes use of domain information in
'struct flush_queue_entry' to make sure we only flush
IOTLBs for domains who need it, skipping others.

Signed-off-by: Arindam Nath <arindam.nath@amd.com>
---
 drivers/iommu/amd_iommu.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 98940d1..6a9a048 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2227,15 +2227,16 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
 
 static void __queue_flush(struct flush_queue *queue)
 {
-	struct protection_domain *domain;
-	unsigned long flags;
 	int idx;
 
-	/* First flush TLB of all known domains */
-	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
-	list_for_each_entry(domain, &amd_iommu_pd_list, list)
-		domain_flush_tlb(domain);
-	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
+	/* First flush TLB of all domains which were added to flush queue */
+	for (idx = 0; idx < queue->next; ++idx) {
+		struct flush_queue_entry *entry;
+
+		entry = queue->entries + idx;
+
+		domain_flush_tlb(&entry->dma_dom->domain);
+	}
 
 	/* Wait until flushes have completed */
 	domain_flush_complete(NULL);
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

[Daniel Drake]

Hi Joerg,

Thanks for looking into this. We confirm that this workaround avoids
the iommu log spam and that amdgpu appears to be working fine with it.

Daniel


On Wed, Mar 22, 2017 at 5:22 AM, joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Mar 21, 2017 at 04:30:55PM +0000, Deucher, Alexander wrote:
>> > I am preparing a debug-patch that disables ATS for these GPUs so someone
>> > with such a chip can test it.
>>
>> Thanks Joerg.
>
> Here is a debug patch, using the hard hammer of disabling the use of ATS
> completly in the AMD IOMMU driver. If it fixes the issue I am going to
> write a more upstreamable version.
>
> But for now, please test if this fixes the issue.
>
> Thanks,
>
>         Joerg
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 98940d1..f019aa6 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -467,7 +467,7 @@ static int iommu_init_device(struct device *dev)
>                 struct amd_iommu *iommu;
>
>                 iommu = amd_iommu_rlookup_table[dev_data->devid];
> -               dev_data->iommu_v2 = iommu->is_iommu_v2;
> +               dev_data->iommu_v2 = false;
>         }
>
>         dev->archdata.iommu = dev_data;
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 6130278..41d0e64 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -171,7 +171,7 @@ int amd_iommus_present;
>
>  /* IOMMUs have a non-present cache? */
>  bool amd_iommu_np_cache __read_mostly;
> -bool amd_iommu_iotlb_sup __read_mostly = true;
> +bool amd_iommu_iotlb_sup __read_mostly = false;
>
>  u32 amd_iommu_max_pasid __read_mostly = ~0;
>

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only

[Daniel Drake]

Hi Arindam,

You CC'd me on this - does this mean that it is a fix for the issue
described in the thread "amd-iommu: can't boot with amdgpu, AMD-Vi:
Completion-Wait loop timed out" ?

Thanks
Daniel


On Mon, Mar 27, 2017 at 12:17 AM,  <arindam.nath@amd.com> wrote:
> From: Arindam Nath <arindam.nath@amd.com>
>
> The idea behind flush queues is to defer the IOTLB flushing
> for domains for which the mappings are no longer valid. We
> add such domains in queue_add(), and when the queue size
> reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>
> Since we have already taken lock before __queue_flush()
> is called, we need to make sure the IOTLB flushing is
> performed as quickly as possible.
>
> In the current implementation, we perform IOTLB flushing
> for all domains irrespective of which ones were actually
> added in the flush queue initially. This can be quite
> expensive especially for domains for which unmapping is
> not required at this point of time.
>
> This patch makes use of domain information in
> 'struct flush_queue_entry' to make sure we only flush
> IOTLBs for domains who need it, skipping others.
>
> Signed-off-by: Arindam Nath <arindam.nath@amd.com>
> ---
>  drivers/iommu/amd_iommu.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 98940d1..6a9a048 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2227,15 +2227,16 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
>
>  static void __queue_flush(struct flush_queue *queue)
>  {
> -       struct protection_domain *domain;
> -       unsigned long flags;
>         int idx;
>
> -       /* First flush TLB of all known domains */
> -       spin_lock_irqsave(&amd_iommu_pd_lock, flags);
> -       list_for_each_entry(domain, &amd_iommu_pd_list, list)
> -               domain_flush_tlb(domain);
> -       spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
> +       /* First flush TLB of all domains which were added to flush queue */
> +       for (idx = 0; idx < queue->next; ++idx) {
> +               struct flush_queue_entry *entry;
> +
> +               entry = queue->entries + idx;
> +
> +               domain_flush_tlb(&entry->dma_dom->domain);
> +       }
>
>         /* Wait until flushes have completed */
>         domain_flush_complete(NULL);
> --
> 1.9.1
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only

[Nath, Arindam]

>-----Original Message-----
>From: Daniel Drake [mailto:drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org]
>Sent: Monday, March 27, 2017 5:56 PM
>To: Nath, Arindam
>Cc: joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org; Deucher, Alexander; Bridgman, John; amd-
>gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Suthikulpanit,
>Suravee; Linux Upstreaming Team
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only
>
>Hi Arindam,
>
>You CC'd me on this - does this mean that it is a fix for the issue
>described in the thread "amd-iommu: can't boot with amdgpu, AMD-Vi:
>Completion-Wait loop timed out" ?

Yes Daniel, please test this patch to confirm if the issue gets resolved.

Thanks,
Arindam

>
>Thanks
>Daniel
>
>
>On Mon, Mar 27, 2017 at 12:17 AM,  <arindam.nath-5C7GfCeVMHo@public.gmane.org> wrote:
>> From: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
>>
>> The idea behind flush queues is to defer the IOTLB flushing
>> for domains for which the mappings are no longer valid. We
>> add such domains in queue_add(), and when the queue size
>> reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>>
>> Since we have already taken lock before __queue_flush()
>> is called, we need to make sure the IOTLB flushing is
>> performed as quickly as possible.
>>
>> In the current implementation, we perform IOTLB flushing
>> for all domains irrespective of which ones were actually
>> added in the flush queue initially. This can be quite
>> expensive especially for domains for which unmapping is
>> not required at this point of time.
>>
>> This patch makes use of domain information in
>> 'struct flush_queue_entry' to make sure we only flush
>> IOTLBs for domains who need it, skipping others.
>>
>> Signed-off-by: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
>> ---
>>  drivers/iommu/amd_iommu.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 98940d1..6a9a048 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -2227,15 +2227,16 @@ static struct iommu_group
>*amd_iommu_device_group(struct device *dev)
>>
>>  static void __queue_flush(struct flush_queue *queue)
>>  {
>> -       struct protection_domain *domain;
>> -       unsigned long flags;
>>         int idx;
>>
>> -       /* First flush TLB of all known domains */
>> -       spin_lock_irqsave(&amd_iommu_pd_lock, flags);
>> -       list_for_each_entry(domain, &amd_iommu_pd_list, list)
>> -               domain_flush_tlb(domain);
>> -       spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
>> +       /* First flush TLB of all domains which were added to flush queue */
>> +       for (idx = 0; idx < queue->next; ++idx) {
>> +               struct flush_queue_entry *entry;
>> +
>> +               entry = queue->entries + idx;
>> +
>> +               domain_flush_tlb(&entry->dma_dom->domain);
>> +       }
>>
>>         /* Wait until flushes have completed */
>>         domain_flush_complete(NULL);
>> --
>> 1.9.1
>>

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only

[Nath, Arindam]

>-----Original Message-----
>From: Nath, Arindam
>Sent: Monday, March 27, 2017 5:57 PM
>To: 'Daniel Drake'
>Cc: joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org; Deucher, Alexander; Bridgman, John; amd-
>gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Suthikulpanit,
>Suravee; Linux Upstreaming Team
>Subject: RE: [PATCH] iommu/amd: flush IOTLB for specific domains only
>
>>-----Original Message-----
>>From: Daniel Drake [mailto:drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org]
>>Sent: Monday, March 27, 2017 5:56 PM
>>To: Nath, Arindam
>>Cc: joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org; Deucher, Alexander; Bridgman, John; amd-
>>gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Suthikulpanit,
>>Suravee; Linux Upstreaming Team
>>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only
>>
>>Hi Arindam,
>>
>>You CC'd me on this - does this mean that it is a fix for the issue
>>described in the thread "amd-iommu: can't boot with amdgpu, AMD-Vi:
>>Completion-Wait loop timed out" ?
>
>Yes Daniel, please test this patch to confirm if the issue gets resolved.

Daniel, did you get chance to test this patch?

Thanks,
Arindam

>
>Thanks,
>Arindam
>
>>
>>Thanks
>>Daniel
>>
>>
>>On Mon, Mar 27, 2017 at 12:17 AM,  <arindam.nath-5C7GfCeVMHo@public.gmane.org> wrote:
>>> From: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
>>>
>>> The idea behind flush queues is to defer the IOTLB flushing
>>> for domains for which the mappings are no longer valid. We
>>> add such domains in queue_add(), and when the queue size
>>> reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>>>
>>> Since we have already taken lock before __queue_flush()
>>> is called, we need to make sure the IOTLB flushing is
>>> performed as quickly as possible.
>>>
>>> In the current implementation, we perform IOTLB flushing
>>> for all domains irrespective of which ones were actually
>>> added in the flush queue initially. This can be quite
>>> expensive especially for domains for which unmapping is
>>> not required at this point of time.
>>>
>>> This patch makes use of domain information in
>>> 'struct flush_queue_entry' to make sure we only flush
>>> IOTLBs for domains who need it, skipping others.
>>>
>>> Signed-off-by: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
>>> ---
>>>  drivers/iommu/amd_iommu.c | 15 ++++++++-------
>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>>> index 98940d1..6a9a048 100644
>>> --- a/drivers/iommu/amd_iommu.c
>>> +++ b/drivers/iommu/amd_iommu.c
>>> @@ -2227,15 +2227,16 @@ static struct iommu_group
>>*amd_iommu_device_group(struct device *dev)
>>>
>>>  static void __queue_flush(struct flush_queue *queue)
>>>  {
>>> -       struct protection_domain *domain;
>>> -       unsigned long flags;
>>>         int idx;
>>>
>>> -       /* First flush TLB of all known domains */
>>> -       spin_lock_irqsave(&amd_iommu_pd_lock, flags);
>>> -       list_for_each_entry(domain, &amd_iommu_pd_list, list)
>>> -               domain_flush_tlb(domain);
>>> -       spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
>>> +       /* First flush TLB of all domains which were added to flush queue */
>>> +       for (idx = 0; idx < queue->next; ++idx) {
>>> +               struct flush_queue_entry *entry;
>>> +
>>> +               entry = queue->entries + idx;
>>> +
>>> +               domain_flush_tlb(&entry->dma_dom->domain);
>>> +       }
>>>
>>>         /* Wait until flushes have completed */
>>>         domain_flush_complete(NULL);
>>> --
>>> 1.9.1
>>>

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only

[Daniel Drake]

On Thu, Mar 30, 2017 at 12:23 AM, Nath, Arindam <Arindam.Nath@amd.com> wrote:
> Daniel, did you get chance to test this patch?

Not yet. Should we test it alone or alongside "PCI: Blacklist AMD
Stoney GPU devices for ATS"?

Thanks
Daniel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only

[Nath, Arindam]

>-----Original Message-----
>From: Daniel Drake [mailto:drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org]
>Sent: Thursday, March 30, 2017 7:15 PM
>To: Nath, Arindam
>Cc: joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org; Deucher, Alexander; Bridgman, John; amd-
>gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Suthikulpanit,
>Suravee; Linux Upstreaming Team
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only
>
>On Thu, Mar 30, 2017 at 12:23 AM, Nath, Arindam <Arindam.Nath-5C7GfCeVMHo@public.gmane.org>
>wrote:
>> Daniel, did you get chance to test this patch?
>
>Not yet. Should we test it alone or alongside "PCI: Blacklist AMD
>Stoney GPU devices for ATS"?

You should test this patch alone.

Thanks,
Arindam

>
>Thanks
>Daniel

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only

[Nath, Arindam]

>-----Original Message-----
>From: Daniel Drake [mailto:drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org]
>Sent: Thursday, March 30, 2017 7:15 PM
>To: Nath, Arindam
>Cc: joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org; Deucher, Alexander; Bridgman, John; amd-
>gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Suthikulpanit,
>Suravee; Linux Upstreaming Team
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only
>
>On Thu, Mar 30, 2017 at 12:23 AM, Nath, Arindam <Arindam.Nath-5C7GfCeVMHo@public.gmane.org>
>wrote:
>> Daniel, did you get chance to test this patch?
>
>Not yet. Should we test it alone or alongside "PCI: Blacklist AMD
>Stoney GPU devices for ATS"?

Daniel, any luck with this patch?

Thanks,
Arindam

>
>Thanks
>Daniel

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only

[Joerg Roedel]

On Mon, Mar 27, 2017 at 11:47:07AM +0530, arindam.nath-5C7GfCeVMHo@public.gmane.org wrote:
> From: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
> 
> The idea behind flush queues is to defer the IOTLB flushing
> for domains for which the mappings are no longer valid. We
> add such domains in queue_add(), and when the queue size
> reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
> 
> Since we have already taken lock before __queue_flush()
> is called, we need to make sure the IOTLB flushing is
> performed as quickly as possible.
> 
> In the current implementation, we perform IOTLB flushing
> for all domains irrespective of which ones were actually
> added in the flush queue initially. This can be quite
> expensive especially for domains for which unmapping is
> not required at this point of time.
> 
> This patch makes use of domain information in
> 'struct flush_queue_entry' to make sure we only flush
> IOTLBs for domains who need it, skipping others.
> 
> Signed-off-by: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/iommu/amd_iommu.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 98940d1..6a9a048 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2227,15 +2227,16 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
>  
>  static void __queue_flush(struct flush_queue *queue)
>  {
> -	struct protection_domain *domain;
> -	unsigned long flags;
>  	int idx;
>  
> -	/* First flush TLB of all known domains */
> -	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
> -	list_for_each_entry(domain, &amd_iommu_pd_list, list)
> -		domain_flush_tlb(domain);
> -	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
> +	/* First flush TLB of all domains which were added to flush queue */
> +	for (idx = 0; idx < queue->next; ++idx) {
> +		struct flush_queue_entry *entry;
> +
> +		entry = queue->entries + idx;
> +
> +		domain_flush_tlb(&entry->dma_dom->domain);
> +	}

With this we will flush a domain every time we find one of its
iova-addresses in the flush queue, so potentially we flush a domain
multiple times per __queue_flush() call.

Its better to either add a flush-flag to the domains and evaluate that
in __queue_flush or keep a list of domains to flush to make the flushing
really more efficient.


	Joerg

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only

[Daniel Drake]

On Wed, Apr 5, 2017 at 9:01 AM, Nath, Arindam <Arindam.Nath-5C7GfCeVMHo@public.gmane.org> wrote:
>
> >-----Original Message-----
> >From: Daniel Drake [mailto:drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org]
> >Sent: Thursday, March 30, 2017 7:15 PM
> >To: Nath, Arindam
> >Cc: joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org; Deucher, Alexander; Bridgman, John; amd-
> >gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Suthikulpanit,
> >Suravee; Linux Upstreaming Team
> >Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only
> >
> >On Thu, Mar 30, 2017 at 12:23 AM, Nath, Arindam <Arindam.Nath-5C7GfCeVMHo@public.gmane.org>
> >wrote:
> >> Daniel, did you get chance to test this patch?
> >
> >Not yet. Should we test it alone or alongside "PCI: Blacklist AMD
> >Stoney GPU devices for ATS"?
>
> Daniel, any luck with this patch?

Sorry for the delay. The patch appears to be working fine.

Daniel

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only

[Michel Dänzer]

On 07/04/17 07:20 PM, Joerg Roedel wrote:
> On Mon, Mar 27, 2017 at 11:47:07AM +0530, arindam.nath@amd.com wrote:
>> From: Arindam Nath <arindam.nath@amd.com>
>>
>> The idea behind flush queues is to defer the IOTLB flushing
>> for domains for which the mappings are no longer valid. We
>> add such domains in queue_add(), and when the queue size
>> reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>>
>> Since we have already taken lock before __queue_flush()
>> is called, we need to make sure the IOTLB flushing is
>> performed as quickly as possible.
>>
>> In the current implementation, we perform IOTLB flushing
>> for all domains irrespective of which ones were actually
>> added in the flush queue initially. This can be quite
>> expensive especially for domains for which unmapping is
>> not required at this point of time.
>>
>> This patch makes use of domain information in
>> 'struct flush_queue_entry' to make sure we only flush
>> IOTLBs for domains who need it, skipping others.
>>
>> Signed-off-by: Arindam Nath <arindam.nath@amd.com>
>> ---
>>  drivers/iommu/amd_iommu.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 98940d1..6a9a048 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -2227,15 +2227,16 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
>>  
>>  static void __queue_flush(struct flush_queue *queue)
>>  {
>> -	struct protection_domain *domain;
>> -	unsigned long flags;
>>  	int idx;
>>  
>> -	/* First flush TLB of all known domains */
>> -	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
>> -	list_for_each_entry(domain, &amd_iommu_pd_list, list)
>> -		domain_flush_tlb(domain);
>> -	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
>> +	/* First flush TLB of all domains which were added to flush queue */
>> +	for (idx = 0; idx < queue->next; ++idx) {
>> +		struct flush_queue_entry *entry;
>> +
>> +		entry = queue->entries + idx;
>> +
>> +		domain_flush_tlb(&entry->dma_dom->domain);
>> +	}
> 
> With this we will flush a domain every time we find one of its
> iova-addresses in the flush queue, so potentially we flush a domain
> multiple times per __queue_flush() call.
> 
> Its better to either add a flush-flag to the domains and evaluate that
> in __queue_flush or keep a list of domains to flush to make the flushing
> really more efficient.

Arindam, can you incorporate Joerg's feedback?

FWIW, looks like Carrizo systems are affected by this as well (see e.g.
https://bugs.freedesktop.org/show_bug.cgi?id=101029#c21), so it would be
good to land this fix in some form ASAP.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

[arindam.nath-5C7GfCeVMHo]

From: Arindam Nath <arindam.nath@amd.com>

Change History
--------------

v2: changes suggested by Joerg
- add flush flag to improve efficiency of flush operation

v1:
- The idea behind flush queues is to defer the IOTLB flushing
  for domains for which the mappings are no longer valid. We
  add such domains in queue_add(), and when the queue size
  reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().

  Since we have already taken lock before __queue_flush()
  is called, we need to make sure the IOTLB flushing is
  performed as quickly as possible.

  In the current implementation, we perform IOTLB flushing
  for all domains irrespective of which ones were actually
  added in the flush queue initially. This can be quite
  expensive especially for domains for which unmapping is
  not required at this point of time.

  This patch makes use of domain information in
  'struct flush_queue_entry' to make sure we only flush
  IOTLBs for domains who need it, skipping others.

Suggested-by: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Arindam Nath <arindam.nath@amd.com>
---
 drivers/iommu/amd_iommu.c       | 27 ++++++++++++++++++++-------
 drivers/iommu/amd_iommu_types.h |  2 ++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 63cacf5..1edeebec 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2227,15 +2227,26 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
 
 static void __queue_flush(struct flush_queue *queue)
 {
-	struct protection_domain *domain;
-	unsigned long flags;
 	int idx;
 
-	/* First flush TLB of all known domains */
-	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
-	list_for_each_entry(domain, &amd_iommu_pd_list, list)
-		domain_flush_tlb(domain);
-	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
+	/* First flush TLB of all domains which were added to flush queue */
+	for (idx = 0; idx < queue->next; ++idx) {
+		struct flush_queue_entry *entry;
+
+		entry = queue->entries + idx;
+
+		/*
+		 * There might be cases where multiple IOVA entries for the
+		 * same domain are queued in the flush queue. To avoid
+		 * flushing the same domain again, we check whether the
+		 * flag is set or not. This improves the efficiency of
+		 * flush operation.
+		 */
+		if (!entry->dma_dom->domain.already_flushed) {
+			entry->dma_dom->domain.already_flushed = true;
+			domain_flush_tlb(&entry->dma_dom->domain);
+		}
+	}
 
 	/* Wait until flushes have completed */
 	domain_flush_complete(NULL);
@@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain *dma_dom,
 	pages     = __roundup_pow_of_two(pages);
 	address >>= PAGE_SHIFT;
 
+	dma_dom->domain.already_flushed = false;
+
 	queue = get_cpu_ptr(&flush_queue);
 	spin_lock_irqsave(&queue->lock, flags);
 
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 4de8f41..4f5519d 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -454,6 +454,8 @@ struct protection_domain {
 	bool updated;		/* complete domain flush required */
 	unsigned dev_cnt;	/* devices assigned to this domain */
 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
+	bool already_flushed;	/* flag to avoid flushing the same domain again
+				   in a single invocation of __queue_flush() */
 };
 
 /*
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

[Jan Vesely]

[-- Attachment #1.1: Type: text/plain, Size: 4317 bytes --]

On Fri, 2017-05-19 at 15:32 +0530, arindam.nath-5C7GfCeVMHo@public.gmane.org wrote:
> From: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
> 
> Change History
> --------------
> 
> v2: changes suggested by Joerg
> - add flush flag to improve efficiency of flush operation
> 
> v1:
> - The idea behind flush queues is to defer the IOTLB flushing
>   for domains for which the mappings are no longer valid. We
>   add such domains in queue_add(), and when the queue size
>   reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
> 
>   Since we have already taken lock before __queue_flush()
>   is called, we need to make sure the IOTLB flushing is
>   performed as quickly as possible.
> 
>   In the current implementation, we perform IOTLB flushing
>   for all domains irrespective of which ones were actually
>   added in the flush queue initially. This can be quite
>   expensive especially for domains for which unmapping is
>   not required at this point of time.
> 
>   This patch makes use of domain information in
>   'struct flush_queue_entry' to make sure we only flush
>   IOTLBs for domains who need it, skipping others.

Hi,

just a note, the patch also fixes "AMD-Vi: Completion-Wait loop timed
out" boot hang on my system (carrizo + iceland) [0,1,2]. Presumably the
old loop also tried to flush domains that included powered-off devices.

regards,
Jan

[0] https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver/issues/20
[1] https://bugs.freedesktop.org/show_bug.cgi?id=101029
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1448121

> 
> Suggested-by: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/iommu/amd_iommu.c       | 27 ++++++++++++++++++++-------
>  drivers/iommu/amd_iommu_types.h |  2 ++
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 63cacf5..1edeebec 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2227,15 +2227,26 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
>  
>  static void __queue_flush(struct flush_queue *queue)
>  {
> -	struct protection_domain *domain;
> -	unsigned long flags;
>  	int idx;
>  
> -	/* First flush TLB of all known domains */
> -	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
> -	list_for_each_entry(domain, &amd_iommu_pd_list, list)
> -		domain_flush_tlb(domain);
> -	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
> +	/* First flush TLB of all domains which were added to flush queue */
> +	for (idx = 0; idx < queue->next; ++idx) {
> +		struct flush_queue_entry *entry;
> +
> +		entry = queue->entries + idx;
> +
> +		/*
> +		 * There might be cases where multiple IOVA entries for the
> +		 * same domain are queued in the flush queue. To avoid
> +		 * flushing the same domain again, we check whether the
> +		 * flag is set or not. This improves the efficiency of
> +		 * flush operation.
> +		 */
> +		if (!entry->dma_dom->domain.already_flushed) {
> +			entry->dma_dom->domain.already_flushed = true;
> +			domain_flush_tlb(&entry->dma_dom->domain);
> +		}
> +	}
>  
>  	/* Wait until flushes have completed */
>  	domain_flush_complete(NULL);
> @@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain *dma_dom,
>  	pages     = __roundup_pow_of_two(pages);
>  	address >>= PAGE_SHIFT;
>  
> +	dma_dom->domain.already_flushed = false;
> +
>  	queue = get_cpu_ptr(&flush_queue);
>  	spin_lock_irqsave(&queue->lock, flags);
>  
> diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
> index 4de8f41..4f5519d 100644
> --- a/drivers/iommu/amd_iommu_types.h
> +++ b/drivers/iommu/amd_iommu_types.h
> @@ -454,6 +454,8 @@ struct protection_domain {
>  	bool updated;		/* complete domain flush required */
>  	unsigned dev_cnt;	/* devices assigned to this domain */
>  	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
> +	bool already_flushed;	/* flag to avoid flushing the same domain again
> +				   in a single invocation of __queue_flush() */
>  };
>  
>  /*

-- 
Jan Vesely <jan.vesely-kgbqMDwikbSVc3sceRu5cw@public.gmane.org>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

[Nath, Arindam]

>-----Original Message-----
>From: Jan Vesely [mailto:jv356-nChP9VK/LToysehGhLVACCmkPJUwDNjP@public.gmane.org] On Behalf Of Jan
>Vesely
>Sent: Friday, May 19, 2017 7:06 PM
>To: Nath, Arindam; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>Cc: Bridgman, John; Joerg Roedel; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org;
>drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org; Craig Stein; Suthikulpanit, Suravee; Deucher,
>Alexander; Kuehling, Felix; linux-6IF/jdPJHihWk0Htik3J/w@public.gmane.org; michel-otUistvHUpPR7s880joybQ@public.gmane.org
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)
>
>On Fri, 2017-05-19 at 15:32 +0530, arindam.nath-5C7GfCeVMHo@public.gmane.org wrote:
>> From: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
>>
>> Change History
>> --------------
>>
>> v2: changes suggested by Joerg
>> - add flush flag to improve efficiency of flush operation

Joerg, do you have any comments on the patch?

Thanks,
Arindam

>>
>> v1:
>> - The idea behind flush queues is to defer the IOTLB flushing
>>   for domains for which the mappings are no longer valid. We
>>   add such domains in queue_add(), and when the queue size
>>   reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>>
>>   Since we have already taken lock before __queue_flush()
>>   is called, we need to make sure the IOTLB flushing is
>>   performed as quickly as possible.
>>
>>   In the current implementation, we perform IOTLB flushing
>>   for all domains irrespective of which ones were actually
>>   added in the flush queue initially. This can be quite
>>   expensive especially for domains for which unmapping is
>>   not required at this point of time.
>>
>>   This patch makes use of domain information in
>>   'struct flush_queue_entry' to make sure we only flush
>>   IOTLBs for domains who need it, skipping others.
>
>Hi,
>
>just a note, the patch also fixes "AMD-Vi: Completion-Wait loop timed
>out" boot hang on my system (carrizo + iceland) [0,1,2]. Presumably the
>old loop also tried to flush domains that included powered-off devices.
>
>regards,
>Jan
>
>[0] https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver/issues/20
>[1] https://bugs.freedesktop.org/show_bug.cgi?id=101029
>[2] https://bugzilla.redhat.com/show_bug.cgi?id=1448121
>
>>
>> Suggested-by: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Arindam Nath <arindam.nath-5C7GfCeVMHo@public.gmane.org>
>> ---
>>  drivers/iommu/amd_iommu.c       | 27 ++++++++++++++++++++-------
>>  drivers/iommu/amd_iommu_types.h |  2 ++
>>  2 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 63cacf5..1edeebec 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -2227,15 +2227,26 @@ static struct iommu_group
>*amd_iommu_device_group(struct device *dev)
>>
>>  static void __queue_flush(struct flush_queue *queue)
>>  {
>> -	struct protection_domain *domain;
>> -	unsigned long flags;
>>  	int idx;
>>
>> -	/* First flush TLB of all known domains */
>> -	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
>> -	list_for_each_entry(domain, &amd_iommu_pd_list, list)
>> -		domain_flush_tlb(domain);
>> -	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
>> +	/* First flush TLB of all domains which were added to flush queue */
>> +	for (idx = 0; idx < queue->next; ++idx) {
>> +		struct flush_queue_entry *entry;
>> +
>> +		entry = queue->entries + idx;
>> +
>> +		/*
>> +		 * There might be cases where multiple IOVA entries for the
>> +		 * same domain are queued in the flush queue. To avoid
>> +		 * flushing the same domain again, we check whether the
>> +		 * flag is set or not. This improves the efficiency of
>> +		 * flush operation.
>> +		 */
>> +		if (!entry->dma_dom->domain.already_flushed) {
>> +			entry->dma_dom->domain.already_flushed = true;
>> +			domain_flush_tlb(&entry->dma_dom->domain);
>> +		}
>> +	}
>>
>>  	/* Wait until flushes have completed */
>>  	domain_flush_complete(NULL);
>> @@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain
>*dma_dom,
>>  	pages     = __roundup_pow_of_two(pages);
>>  	address >>= PAGE_SHIFT;
>>
>> +	dma_dom->domain.already_flushed = false;
>> +
>>  	queue = get_cpu_ptr(&flush_queue);
>>  	spin_lock_irqsave(&queue->lock, flags);
>>
>> diff --git a/drivers/iommu/amd_iommu_types.h
>b/drivers/iommu/amd_iommu_types.h
>> index 4de8f41..4f5519d 100644
>> --- a/drivers/iommu/amd_iommu_types.h
>> +++ b/drivers/iommu/amd_iommu_types.h
>> @@ -454,6 +454,8 @@ struct protection_domain {
>>  	bool updated;		/* complete domain flush required */
>>  	unsigned dev_cnt;	/* devices assigned to this domain */
>>  	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference
>count */
>> +	bool already_flushed;	/* flag to avoid flushing the same domain
>again
>> +				   in a single invocation of __queue_flush() */
>>  };
>>
>>  /*
>
>--
>Jan Vesely <jan.vesely-kgbqMDwikbSVc3sceRu5cw@public.gmane.org>

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

[Michel Dänzer]

On 19/05/17 07:02 PM, arindam.nath@amd.com wrote:
> From: Arindam Nath <arindam.nath@amd.com>
> 
> Change History
> --------------
> 
> v2: changes suggested by Joerg
> - add flush flag to improve efficiency of flush operation
> 
> v1:
> - The idea behind flush queues is to defer the IOTLB flushing
>   for domains for which the mappings are no longer valid. We
>   add such domains in queue_add(), and when the queue size
>   reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
> 
>   Since we have already taken lock before __queue_flush()
>   is called, we need to make sure the IOTLB flushing is
>   performed as quickly as possible.
> 
>   In the current implementation, we perform IOTLB flushing
>   for all domains irrespective of which ones were actually
>   added in the flush queue initially. This can be quite
>   expensive especially for domains for which unmapping is
>   not required at this point of time.
> 
>   This patch makes use of domain information in
>   'struct flush_queue_entry' to make sure we only flush
>   IOTLBs for domains who need it, skipping others.
> 
> Suggested-by: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Arindam Nath <arindam.nath@amd.com>

Please add these tags:

Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
Cc: stable@vger.kernel.org


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

[Michel Dänzer]

On 22/05/17 10:08 AM, Michel Dänzer wrote:
> On 19/05/17 07:02 PM, arindam.nath@amd.com wrote:
>> From: Arindam Nath <arindam.nath@amd.com>
>>
>> Change History
>> --------------
>>
>> v2: changes suggested by Joerg
>> - add flush flag to improve efficiency of flush operation
>>
>> v1:
>> - The idea behind flush queues is to defer the IOTLB flushing
>>   for domains for which the mappings are no longer valid. We
>>   add such domains in queue_add(), and when the queue size
>>   reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>>
>>   Since we have already taken lock before __queue_flush()
>>   is called, we need to make sure the IOTLB flushing is
>>   performed as quickly as possible.
>>
>>   In the current implementation, we perform IOTLB flushing
>>   for all domains irrespective of which ones were actually
>>   added in the flush queue initially. This can be quite
>>   expensive especially for domains for which unmapping is
>>   not required at this point of time.
>>
>>   This patch makes use of domain information in
>>   'struct flush_queue_entry' to make sure we only flush
>>   IOTLBs for domains who need it, skipping others.
>>
>> Suggested-by: Joerg Roedel <joro@8bytes.org>
>> Signed-off-by: Arindam Nath <arindam.nath@amd.com>
> 
> Please add these tags:
> 
> Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
> Cc: stable@vger.kernel.org

Also

Bugzilla: https://bugs.freedesktop.org/101029


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

[arindam.nath]

From: Arindam Nath <arindam.nath@amd.com>

Change History
--------------

v3:
- add Fixes and CC tags
- add link to Bugzilla

v2: changes suggested by Joerg
- add flush flag to improve efficiency of flush operation

v1:
- The idea behind flush queues is to defer the IOTLB flushing
  for domains for which the mappings are no longer valid. We
  add such domains in queue_add(), and when the queue size
  reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().

  Since we have already taken lock before __queue_flush()
  is called, we need to make sure the IOTLB flushing is
  performed as quickly as possible.

  In the current implementation, we perform IOTLB flushing
  for all domains irrespective of which ones were actually
  added in the flush queue initially. This can be quite
  expensive especially for domains for which unmapping is
  not required at this point of time.

  This patch makes use of domain information in
  'struct flush_queue_entry' to make sure we only flush
  IOTLBs for domains who need it, skipping others.

Bugzilla: https://bugs.freedesktop.org/101029
Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
Cc: stable@vger.kernel.org
Suggested-by: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Arindam Nath <arindam.nath@amd.com>
---
 drivers/iommu/amd_iommu.c       | 27 ++++++++++++++++++++-------
 drivers/iommu/amd_iommu_types.h |  2 ++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 63cacf5..1edeebec 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2227,15 +2227,26 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
 
 static void __queue_flush(struct flush_queue *queue)
 {
-	struct protection_domain *domain;
-	unsigned long flags;
 	int idx;
 
-	/* First flush TLB of all known domains */
-	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
-	list_for_each_entry(domain, &amd_iommu_pd_list, list)
-		domain_flush_tlb(domain);
-	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
+	/* First flush TLB of all domains which were added to flush queue */
+	for (idx = 0; idx < queue->next; ++idx) {
+		struct flush_queue_entry *entry;
+
+		entry = queue->entries + idx;
+
+		/*
+		 * There might be cases where multiple IOVA entries for the
+		 * same domain are queued in the flush queue. To avoid
+		 * flushing the same domain again, we check whether the
+		 * flag is set or not. This improves the efficiency of
+		 * flush operation.
+		 */
+		if (!entry->dma_dom->domain.already_flushed) {
+			entry->dma_dom->domain.already_flushed = true;
+			domain_flush_tlb(&entry->dma_dom->domain);
+		}
+	}
 
 	/* Wait until flushes have completed */
 	domain_flush_complete(NULL);
@@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain *dma_dom,
 	pages     = __roundup_pow_of_two(pages);
 	address >>= PAGE_SHIFT;
 
+	dma_dom->domain.already_flushed = false;
+
 	queue = get_cpu_ptr(&flush_queue);
 	spin_lock_irqsave(&queue->lock, flags);
 
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 4de8f41..4f5519d 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -454,6 +454,8 @@ struct protection_domain {
 	bool updated;		/* complete domain flush required */
 	unsigned dev_cnt;	/* devices assigned to this domain */
 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
+	bool already_flushed;	/* flag to avoid flushing the same domain again
+				   in a single invocation of __queue_flush() */
 };
 
 /*
-- 
2.7.4

[PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

[arindam.nath-5C7GfCeVMHo]

From: Arindam Nath <arindam.nath@amd.com>

Change History
--------------

v3:
- add Fixes and CC tags
- add link to Bugzilla

v2: changes suggested by Joerg
- add flush flag to improve efficiency of flush operation

v1:
- The idea behind flush queues is to defer the IOTLB flushing
  for domains for which the mappings are no longer valid. We
  add such domains in queue_add(), and when the queue size
  reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().

  Since we have already taken lock before __queue_flush()
  is called, we need to make sure the IOTLB flushing is
  performed as quickly as possible.

  In the current implementation, we perform IOTLB flushing
  for all domains irrespective of which ones were actually
  added in the flush queue initially. This can be quite
  expensive especially for domains for which unmapping is
  not required at this point of time.

  This patch makes use of domain information in
  'struct flush_queue_entry' to make sure we only flush
  IOTLBs for domains who need it, skipping others.

Bugzilla: https://bugs.freedesktop.org/101029
Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
Cc: stable@vger.kernel.org
Suggested-by: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Arindam Nath <arindam.nath@amd.com>
---
 drivers/iommu/amd_iommu.c       | 27 ++++++++++++++++++++-------
 drivers/iommu/amd_iommu_types.h |  2 ++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 63cacf5..1edeebec 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2227,15 +2227,26 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
 
 static void __queue_flush(struct flush_queue *queue)
 {
-	struct protection_domain *domain;
-	unsigned long flags;
 	int idx;
 
-	/* First flush TLB of all known domains */
-	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
-	list_for_each_entry(domain, &amd_iommu_pd_list, list)
-		domain_flush_tlb(domain);
-	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
+	/* First flush TLB of all domains which were added to flush queue */
+	for (idx = 0; idx < queue->next; ++idx) {
+		struct flush_queue_entry *entry;
+
+		entry = queue->entries + idx;
+
+		/*
+		 * There might be cases where multiple IOVA entries for the
+		 * same domain are queued in the flush queue. To avoid
+		 * flushing the same domain again, we check whether the
+		 * flag is set or not. This improves the efficiency of
+		 * flush operation.
+		 */
+		if (!entry->dma_dom->domain.already_flushed) {
+			entry->dma_dom->domain.already_flushed = true;
+			domain_flush_tlb(&entry->dma_dom->domain);
+		}
+	}
 
 	/* Wait until flushes have completed */
 	domain_flush_complete(NULL);
@@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain *dma_dom,
 	pages     = __roundup_pow_of_two(pages);
 	address >>= PAGE_SHIFT;
 
+	dma_dom->domain.already_flushed = false;
+
 	queue = get_cpu_ptr(&flush_queue);
 	spin_lock_irqsave(&queue->lock, flags);
 
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 4de8f41..4f5519d 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -454,6 +454,8 @@ struct protection_domain {
 	bool updated;		/* complete domain flush required */
 	unsigned dev_cnt;	/* devices assigned to this domain */
 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
+	bool already_flushed;	/* flag to avoid flushing the same domain again
+				   in a single invocation of __queue_flush() */
 };
 
 /*
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

[Deucher, Alexander]

> -----Original Message-----
> From: Arindam Nath [mailto:anath.amd@gmail.com] On Behalf Of
> arindam.nath@amd.com
> Sent: Monday, May 22, 2017 3:48 AM
> To: iommu@lists.linux-foundation.org
> Cc: amd-gfx@lists.freedesktop.org; Joerg Roedel; Deucher, Alexander;
> Bridgman, John; drake@endlessm.com; Suthikulpanit, Suravee;
> linux@endlessm.com; Craig Stein; michel@daenzer.net; Kuehling, Felix;
> stable@vger.kernel.org; Nath, Arindam
> Subject: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
> 
> From: Arindam Nath <arindam.nath@amd.com>
> 
> Change History
> --------------
> 
> v3:
> - add Fixes and CC tags
> - add link to Bugzilla
> 
> v2: changes suggested by Joerg
> - add flush flag to improve efficiency of flush operation
> 
> v1:
> - The idea behind flush queues is to defer the IOTLB flushing
>   for domains for which the mappings are no longer valid. We
>   add such domains in queue_add(), and when the queue size
>   reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
> 
>   Since we have already taken lock before __queue_flush()
>   is called, we need to make sure the IOTLB flushing is
>   performed as quickly as possible.
> 
>   In the current implementation, we perform IOTLB flushing
>   for all domains irrespective of which ones were actually
>   added in the flush queue initially. This can be quite
>   expensive especially for domains for which unmapping is
>   not required at this point of time.
> 
>   This patch makes use of domain information in
>   'struct flush_queue_entry' to make sure we only flush
>   IOTLBs for domains who need it, skipping others.
> 
> Bugzilla: https://bugs.freedesktop.org/101029
> Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
> Cc: stable@vger.kernel.org
> Suggested-by: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Arindam Nath <arindam.nath@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/iommu/amd_iommu.c       | 27 ++++++++++++++++++++-------
>  drivers/iommu/amd_iommu_types.h |  2 ++
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 63cacf5..1edeebec 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2227,15 +2227,26 @@ static struct iommu_group
> *amd_iommu_device_group(struct device *dev)
> 
>  static void __queue_flush(struct flush_queue *queue)
>  {
> -	struct protection_domain *domain;
> -	unsigned long flags;
>  	int idx;
> 
> -	/* First flush TLB of all known domains */
> -	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
> -	list_for_each_entry(domain, &amd_iommu_pd_list, list)
> -		domain_flush_tlb(domain);
> -	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
> +	/* First flush TLB of all domains which were added to flush queue */
> +	for (idx = 0; idx < queue->next; ++idx) {
> +		struct flush_queue_entry *entry;
> +
> +		entry = queue->entries + idx;
> +
> +		/*
> +		 * There might be cases where multiple IOVA entries for the
> +		 * same domain are queued in the flush queue. To avoid
> +		 * flushing the same domain again, we check whether the
> +		 * flag is set or not. This improves the efficiency of
> +		 * flush operation.
> +		 */
> +		if (!entry->dma_dom->domain.already_flushed) {
> +			entry->dma_dom->domain.already_flushed = true;
> +			domain_flush_tlb(&entry->dma_dom->domain);
> +		}
> +	}
> 
>  	/* Wait until flushes have completed */
>  	domain_flush_complete(NULL);
> @@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain
> *dma_dom,
>  	pages     = __roundup_pow_of_two(pages);
>  	address >>= PAGE_SHIFT;
> 
> +	dma_dom->domain.already_flushed = false;
> +
>  	queue = get_cpu_ptr(&flush_queue);
>  	spin_lock_irqsave(&queue->lock, flags);
> 
> diff --git a/drivers/iommu/amd_iommu_types.h
> b/drivers/iommu/amd_iommu_types.h
> index 4de8f41..4f5519d 100644
> --- a/drivers/iommu/amd_iommu_types.h
> +++ b/drivers/iommu/amd_iommu_types.h
> @@ -454,6 +454,8 @@ struct protection_domain {
>  	bool updated;		/* complete domain flush required */
>  	unsigned dev_cnt;	/* devices assigned to this domain */
>  	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference
> count */
> +	bool already_flushed;	/* flag to avoid flushing the same domain
> again
> +				   in a single invocation of __queue_flush() */
>  };
> 
>  /*
> --
> 2.7.4

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

[Deucher, Alexander]

> -----Original Message-----
> From: Arindam Nath [mailto:anath.amd@gmail.com] On Behalf Of
> arindam.nath@amd.com
> Sent: Monday, May 22, 2017 3:48 AM
> To: iommu@lists.linux-foundation.org
> Cc: amd-gfx@lists.freedesktop.org; Joerg Roedel; Deucher, Alexander;
> Bridgman, John; drake@endlessm.com; Suthikulpanit, Suravee;
> linux@endlessm.com; Craig Stein; michel@daenzer.net; Kuehling, Felix;
> stable@vger.kernel.org; Nath, Arindam
> Subject: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
> 
> From: Arindam Nath <arindam.nath@amd.com>
> 
> Change History
> --------------
> 
> v3:
> - add Fixes and CC tags
> - add link to Bugzilla
> 
> v2: changes suggested by Joerg
> - add flush flag to improve efficiency of flush operation
> 
> v1:
> - The idea behind flush queues is to defer the IOTLB flushing
>   for domains for which the mappings are no longer valid. We
>   add such domains in queue_add(), and when the queue size
>   reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
> 
>   Since we have already taken lock before __queue_flush()
>   is called, we need to make sure the IOTLB flushing is
>   performed as quickly as possible.
> 
>   In the current implementation, we perform IOTLB flushing
>   for all domains irrespective of which ones were actually
>   added in the flush queue initially. This can be quite
>   expensive especially for domains for which unmapping is
>   not required at this point of time.
> 
>   This patch makes use of domain information in
>   'struct flush_queue_entry' to make sure we only flush
>   IOTLBs for domains who need it, skipping others.
> 
> Bugzilla: https://bugs.freedesktop.org/101029
> Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
> Cc: stable@vger.kernel.org
> Suggested-by: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Arindam Nath <arindam.nath@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/iommu/amd_iommu.c       | 27 ++++++++++++++++++++-------
>  drivers/iommu/amd_iommu_types.h |  2 ++
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 63cacf5..1edeebec 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2227,15 +2227,26 @@ static struct iommu_group
> *amd_iommu_device_group(struct device *dev)
> 
>  static void __queue_flush(struct flush_queue *queue)
>  {
> -	struct protection_domain *domain;
> -	unsigned long flags;
>  	int idx;
> 
> -	/* First flush TLB of all known domains */
> -	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
> -	list_for_each_entry(domain, &amd_iommu_pd_list, list)
> -		domain_flush_tlb(domain);
> -	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
> +	/* First flush TLB of all domains which were added to flush queue */
> +	for (idx = 0; idx < queue->next; ++idx) {
> +		struct flush_queue_entry *entry;
> +
> +		entry = queue->entries + idx;
> +
> +		/*
> +		 * There might be cases where multiple IOVA entries for the
> +		 * same domain are queued in the flush queue. To avoid
> +		 * flushing the same domain again, we check whether the
> +		 * flag is set or not. This improves the efficiency of
> +		 * flush operation.
> +		 */
> +		if (!entry->dma_dom->domain.already_flushed) {
> +			entry->dma_dom->domain.already_flushed = true;
> +			domain_flush_tlb(&entry->dma_dom->domain);
> +		}
> +	}
> 
>  	/* Wait until flushes have completed */
>  	domain_flush_complete(NULL);
> @@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain
> *dma_dom,
>  	pages     = __roundup_pow_of_two(pages);
>  	address >>= PAGE_SHIFT;
> 
> +	dma_dom->domain.already_flushed = false;
> +
>  	queue = get_cpu_ptr(&flush_queue);
>  	spin_lock_irqsave(&queue->lock, flags);
> 
> diff --git a/drivers/iommu/amd_iommu_types.h
> b/drivers/iommu/amd_iommu_types.h
> index 4de8f41..4f5519d 100644
> --- a/drivers/iommu/amd_iommu_types.h
> +++ b/drivers/iommu/amd_iommu_types.h
> @@ -454,6 +454,8 @@ struct protection_domain {
>  	bool updated;		/* complete domain flush required */
>  	unsigned dev_cnt;	/* devices assigned to this domain */
>  	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference
> count */
> +	bool already_flushed;	/* flag to avoid flushing the same domain
> again
> +				   in a single invocation of __queue_flush() */
>  };
> 
>  /*
> --
> 2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

[Joerg Roedel]

Hi Arindam,

I met Tom Lendacky last week in Nuremberg last week and he told me he is
working on the same area of the code that this patch is for. His reason
for touching this code was to solve some locking problems. Maybe you two
can work together on a joint approach to improve this?

On Mon, May 22, 2017 at 01:18:01PM +0530, arindam.nath@amd.com wrote:
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 63cacf5..1edeebec 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2227,15 +2227,26 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
>  
>  static void __queue_flush(struct flush_queue *queue)
>  {
> -	struct protection_domain *domain;
> -	unsigned long flags;
>  	int idx;
>  
> -	/* First flush TLB of all known domains */
> -	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
> -	list_for_each_entry(domain, &amd_iommu_pd_list, list)
> -		domain_flush_tlb(domain);
> -	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
> +	/* First flush TLB of all domains which were added to flush queue */
> +	for (idx = 0; idx < queue->next; ++idx) {
> +		struct flush_queue_entry *entry;
> +
> +		entry = queue->entries + idx;
> +
> +		/*
> +		 * There might be cases where multiple IOVA entries for the
> +		 * same domain are queued in the flush queue. To avoid
> +		 * flushing the same domain again, we check whether the
> +		 * flag is set or not. This improves the efficiency of
> +		 * flush operation.
> +		 */
> +		if (!entry->dma_dom->domain.already_flushed) {
> +			entry->dma_dom->domain.already_flushed = true;
> +			domain_flush_tlb(&entry->dma_dom->domain);
> +		}
> +	}

There is also a race condition here I have to look into. It is not
introduced by your patch, but needs fixing anyway. I'll look into this
too.


Regards,

	Joerg

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

[Nath, Arindam]

>-----Original Message-----
>From: Joerg Roedel [mailto:joro@8bytes.org]
>Sent: Monday, May 29, 2017 8:09 PM
>To: Nath, Arindam <Arindam.Nath@amd.com>; Lendacky, Thomas
><Thomas.Lendacky@amd.com>
>Cc: iommu@lists.linux-foundation.org; amd-gfx@lists.freedesktop.org;
>Deucher, Alexander <Alexander.Deucher@amd.com>; Bridgman, John
><John.Bridgman@amd.com>; drake@endlessm.com; Suthikulpanit, Suravee
><Suravee.Suthikulpanit@amd.com>; linux@endlessm.com; Craig Stein
><stein12c@gmail.com>; michel@daenzer.net; Kuehling, Felix
><Felix.Kuehling@amd.com>; stable@vger.kernel.org
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
>
>Hi Arindam,
>
>I met Tom Lendacky last week in Nuremberg last week and he told me he is
>working on the same area of the code that this patch is for. His reason
>for touching this code was to solve some locking problems. Maybe you two
>can work together on a joint approach to improve this?

Sure Joerg, I will work with Tom.

Thanks.

>
>On Mon, May 22, 2017 at 01:18:01PM +0530, arindam.nath@amd.com wrote:
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 63cacf5..1edeebec 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -2227,15 +2227,26 @@ static struct iommu_group
>*amd_iommu_device_group(struct device *dev)
>>
>>  static void __queue_flush(struct flush_queue *queue)
>>  {
>> -	struct protection_domain *domain;
>> -	unsigned long flags;
>>  	int idx;
>>
>> -	/* First flush TLB of all known domains */
>> -	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
>> -	list_for_each_entry(domain, &amd_iommu_pd_list, list)
>> -		domain_flush_tlb(domain);
>> -	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
>> +	/* First flush TLB of all domains which were added to flush queue */
>> +	for (idx = 0; idx < queue->next; ++idx) {
>> +		struct flush_queue_entry *entry;
>> +
>> +		entry = queue->entries + idx;
>> +
>> +		/*
>> +		 * There might be cases where multiple IOVA entries for the
>> +		 * same domain are queued in the flush queue. To avoid
>> +		 * flushing the same domain again, we check whether the
>> +		 * flag is set or not. This improves the efficiency of
>> +		 * flush operation.
>> +		 */
>> +		if (!entry->dma_dom->domain.already_flushed) {
>> +			entry->dma_dom->domain.already_flushed = true;
>> +			domain_flush_tlb(&entry->dma_dom->domain);
>> +		}
>> +	}
>
>There is also a race condition here I have to look into. It is not
>introduced by your patch, but needs fixing anyway. I'll look into this
>too.
>
>
>Regards,
>
>	Joerg

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

[Daniel Drake]

Hi,

On Tue, May 30, 2017 at 3:38 PM, Nath, Arindam <Arindam.Nath@amd.com> wrote:
>>-----Original Message-----
>>From: Joerg Roedel [mailto:joro@8bytes.org]
>>Sent: Monday, May 29, 2017 8:09 PM
>>To: Nath, Arindam <Arindam.Nath@amd.com>; Lendacky, Thomas
>><Thomas.Lendacky@amd.com>
>>Cc: iommu@lists.linux-foundation.org; amd-gfx@lists.freedesktop.org;
>>Deucher, Alexander <Alexander.Deucher@amd.com>; Bridgman, John
>><John.Bridgman@amd.com>; drake@endlessm.com; Suthikulpanit, Suravee
>><Suravee.Suthikulpanit@amd.com>; linux@endlessm.com; Craig Stein
>><stein12c@gmail.com>; michel@daenzer.net; Kuehling, Felix
>><Felix.Kuehling@amd.com>; stable@vger.kernel.org
>>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
>>
>>Hi Arindam,
>>
>>I met Tom Lendacky last week in Nuremberg last week and he told me he is
>>working on the same area of the code that this patch is for. His reason
>>for touching this code was to solve some locking problems. Maybe you two
>>can work together on a joint approach to improve this?
>
> Sure Joerg, I will work with Tom.

What was the end result here? I see that the code has been reworked in
4.13 so your original patch no longer applies. Is the reworked version
also expected to solve the original issue?

Thanks
Daniel

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

[Daniel Drake]

Hi,

On Tue, May 30, 2017 at 3:38 PM, Nath, Arindam <Arindam.Nath@amd.com> wrote:
>>-----Original Message-----
>>From: Joerg Roedel [mailto:joro@8bytes.org]
>>Sent: Monday, May 29, 2017 8:09 PM
>>To: Nath, Arindam <Arindam.Nath@amd.com>; Lendacky, Thomas
>><Thomas.Lendacky@amd.com>
>>Cc: iommu@lists.linux-foundation.org; amd-gfx@lists.freedesktop.org;
>>Deucher, Alexander <Alexander.Deucher@amd.com>; Bridgman, John
>><John.Bridgman@amd.com>; drake@endlessm.com; Suthikulpanit, Suravee
>><Suravee.Suthikulpanit@amd.com>; linux@endlessm.com; Craig Stein
>><stein12c@gmail.com>; michel@daenzer.net; Kuehling, Felix
>><Felix.Kuehling@amd.com>; stable@vger.kernel.org
>>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
>>
>>Hi Arindam,
>>
>>I met Tom Lendacky last week in Nuremberg last week and he told me he is
>>working on the same area of the code that this patch is for. His reason
>>for touching this code was to solve some locking problems. Maybe you two
>>can work together on a joint approach to improve this?
>
> Sure Joerg, I will work with Tom.

What was the end result here? I see that the code has been reworked in
4.13 so your original patch no longer applies. Is the reworked version
also expected to solve the original issue?

Thanks
Daniel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

[Nath, Arindam]

Hi Daniel,

>-----Original Message-----
>From: Daniel Drake [mailto:drake@endlessm.com]
>Sent: Tuesday, September 12, 2017 12:20 PM
>To: Nath, Arindam <Arindam.Nath@amd.com>
>Cc: Joerg Roedel <joro@8bytes.org>; Lendacky, Thomas
><Thomas.Lendacky@amd.com>; iommu@lists.linux-foundation.org; amd-
>gfx@lists.freedesktop.org; Deucher, Alexander
><Alexander.Deucher@amd.com>; Bridgman, John
><John.Bridgman@amd.com>; Suthikulpanit, Suravee
><Suravee.Suthikulpanit@amd.com>; linux@endlessm.com; Craig Stein
><stein12c@gmail.com>; michel@daenzer.net; Kuehling, Felix
><Felix.Kuehling@amd.com>; stable@vger.kernel.org
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
>
>Hi,
>
>On Tue, May 30, 2017 at 3:38 PM, Nath, Arindam <Arindam.Nath@amd.com>
>wrote:
>>>-----Original Message-----
>>>From: Joerg Roedel [mailto:joro@8bytes.org]
>>>Sent: Monday, May 29, 2017 8:09 PM
>>>To: Nath, Arindam <Arindam.Nath@amd.com>; Lendacky, Thomas
>>><Thomas.Lendacky@amd.com>
>>>Cc: iommu@lists.linux-foundation.org; amd-gfx@lists.freedesktop.org;
>>>Deucher, Alexander <Alexander.Deucher@amd.com>; Bridgman, John
>>><John.Bridgman@amd.com>; drake@endlessm.com; Suthikulpanit,
>Suravee
>>><Suravee.Suthikulpanit@amd.com>; linux@endlessm.com; Craig Stein
>>><stein12c@gmail.com>; michel@daenzer.net; Kuehling, Felix
>>><Felix.Kuehling@amd.com>; stable@vger.kernel.org
>>>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only
>(v3)
>>>
>>>Hi Arindam,
>>>
>>>I met Tom Lendacky last week in Nuremberg last week and he told me he is
>>>working on the same area of the code that this patch is for. His reason
>>>for touching this code was to solve some locking problems. Maybe you two
>>>can work together on a joint approach to improve this?
>>
>> Sure Joerg, I will work with Tom.
>
>What was the end result here? I see that the code has been reworked in
>4.13 so your original patch no longer applies. Is the reworked version
>also expected to solve the original issue?

Yes, if the reworked patch resolves the issue, my patch won't be required anymore.

Thanks,
Arindam

>
>Thanks
>Daniel