[PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).

[PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).

[j.glisse]

From: Jérôme Glisse <jglisse@redhat.com>

Fix amd_iommu_detect() to return positive value on success, like
intended, and not zero. This will not change anything in the end
as AMD IOMMU disable swiotlb and properly associate itself with
devices even if detect() doesn't return a positive value.

Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: iommu@lists.linux-foundation.org
---
 drivers/iommu/amd_iommu_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index a24495e..360a451 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2198,7 +2198,7 @@ int __init amd_iommu_detect(void)
 	iommu_detected = 1;
 	x86_init.iommu.iommu_init = amd_iommu_init;
 
-	return 0;
+	return 1;
 }
 
 /****************************************************************************
-- 
1.8.3.1

Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).

[Joerg Roedel]

On Mon, Aug 31, 2015 at 06:13:03PM -0400, j.glisse@gmail.com wrote:
> From: Jérôme Glisse <jglisse@redhat.com>
> 
> Fix amd_iommu_detect() to return positive value on success, like
> intended, and not zero. This will not change anything in the end
> as AMD IOMMU disable swiotlb and properly associate itself with
> devices even if detect() doesn't return a positive value.
> 
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: iommu@lists.linux-foundation.org

Applied, thanks.

Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).

[Konrad Rzeszutek Wilk]

On Mon, Aug 31, 2015 at 06:13:03PM -0400, j.glisse@gmail.com wrote:
> From: Jérôme Glisse <jglisse@redhat.com>
> 
> Fix amd_iommu_detect() to return positive value on success, like
> intended, and not zero. This will not change anything in the end
> as AMD IOMMU disable swiotlb and properly associate itself with

Not sure how it disables SWIOTLB? The AMD Vi does not seem to
change 'swiotlb'. While 'gart_iommu_init' does. Did you mean
the AMD GART code?

> devices even if detect() doesn't return a positive value.

Returning positive will mean that the pci_iommu_alloc will stop
processing _all_ other IOMMUs.

While returning 0 will let it detect the other IOMMUs.

Granted on an AMD machine there can be two 'IOMMU's - the GART
and the AMD Vi. The detection is always to call gart_iommu_hole_init
first, then amd_iommu_detect.

I presume if there was one more type on AMD we would run into trouble.


> 
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: iommu@lists.linux-foundation.org
> ---
>  drivers/iommu/amd_iommu_init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index a24495e..360a451 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -2198,7 +2198,7 @@ int __init amd_iommu_detect(void)
>  	iommu_detected = 1;
>  	x86_init.iommu.iommu_init = amd_iommu_init;
>  
> -	return 0;
> +	return 1;
>  }
>  
>  /****************************************************************************
> -- 
> 1.8.3.1
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).

[Jerome Glisse]

On Mon, Oct 26, 2015 at 12:07:17PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 31, 2015 at 06:13:03PM -0400, j.glisse@gmail.com wrote:
> > From: Jérôme Glisse <jglisse@redhat.com>
> > 
> > Fix amd_iommu_detect() to return positive value on success, like
> > intended, and not zero. This will not change anything in the end
> > as AMD IOMMU disable swiotlb and properly associate itself with
> 
> Not sure how it disables SWIOTLB? The AMD Vi does not seem to
> change 'swiotlb'. While 'gart_iommu_init' does. Did you mean
> the AMD GART code?

So this is convoluted and painfull, each i look back at that it takes
me time to figure out of thing happen. Basicly amd_iommu_init_dma_ops()
will replace dma_ops to no_mmu unless passthrough, and when the AMD
iommu associate itself with each device it will set the archdata.dma_ops
again this unbind the default of swiotlb that is initialize before
hw IOMMU.

> 
> > devices even if detect() doesn't return a positive value.
> 
> Returning positive will mean that the pci_iommu_alloc will stop
> processing _all_ other IOMMUs.
>
> While returning 0 will let it detect the other IOMMUs.

No see the IOMMU_FINISH_IF_DETECTED flags in pci_iommu_alloc().
Which is not set for AMD hence my patch should not change anything
it (AFAICT and from testing but i do not have all AMD hw the ever
existed).

So i am just making the detect function do what the API doc says it
should do. See line 72 to 80 of : arch/x86/include/asm/iommu_table.h

> 
> Granted on an AMD machine there can be two 'IOMMU's - the GART
> and the AMD Vi. The detection is always to call gart_iommu_hole_init
> first, then amd_iommu_detect.
> 
> I presume if there was one more type on AMD we would run into trouble.

No because of IOMMU_FINISH_IF_DETECTED flag.

Hope this clarify thing this spagethi mix :)

Cheers,
Jérôme

Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).

[Jerome Glisse]

On Tue, Oct 27, 2015 at 09:47:48AM +0900, Jerome Glisse wrote:
> On Mon, Oct 26, 2015 at 12:07:17PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Mon, Aug 31, 2015 at 06:13:03PM -0400, j.glisse@gmail.com wrote:
> > > From: Jérôme Glisse <jglisse@redhat.com>
> > > 
> > > Fix amd_iommu_detect() to return positive value on success, like
> > > intended, and not zero. This will not change anything in the end
> > > as AMD IOMMU disable swiotlb and properly associate itself with
> > 
> > Not sure how it disables SWIOTLB? The AMD Vi does not seem to
> > change 'swiotlb'. While 'gart_iommu_init' does. Did you mean
> > the AMD GART code?
> 
> So this is convoluted and painfull, each i look back at that it takes
> me time to figure out of thing happen. Basicly amd_iommu_init_dma_ops()
> will replace dma_ops to no_mmu unless passthrough, and when the AMD
> iommu associate itself with each device it will set the archdata.dma_ops
> again this unbind the default of swiotlb that is initialize before
> hw IOMMU.
> 
> > 
> > > devices even if detect() doesn't return a positive value.
> > 
> > Returning positive will mean that the pci_iommu_alloc will stop
> > processing _all_ other IOMMUs.
> >
> > While returning 0 will let it detect the other IOMMUs.
> 
> No see the IOMMU_FINISH_IF_DETECTED flags in pci_iommu_alloc().
> Which is not set for AMD hence my patch should not change anything
> it (AFAICT and from testing but i do not have all AMD hw the ever
> existed).
> 
> So i am just making the detect function do what the API doc says it
> should do. See line 72 to 80 of : arch/x86/include/asm/iommu_table.h
> 
> > 
> > Granted on an AMD machine there can be two 'IOMMU's - the GART
> > and the AMD Vi. The detection is always to call gart_iommu_hole_init
> > first, then amd_iommu_detect.
> > 
> > I presume if there was one more type on AMD we would run into trouble.
> 
> No because of IOMMU_FINISH_IF_DETECTED flag.
> 
> Hope this clarify thing this spagethi mix :)

Ok my bad amd actualy is using IOMMU_INIT_FINISH() so it finish before
trying other. Which make sense for AMD as AMD driver will call the gart
init gart_iommu_init() if it fails to initialize. If we ever end up with
a platform with multiple IOMMU beside AMD then we need to switch to the
IOMMU_INIT() instead of the finish one.

Cheers,
Jérôme

Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).

[Konrad Rzeszutek Wilk]

On Tue, Oct 27, 2015 at 09:53:30AM +0900, Jerome Glisse wrote:
> On Tue, Oct 27, 2015 at 09:47:48AM +0900, Jerome Glisse wrote:
> > On Mon, Oct 26, 2015 at 12:07:17PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Mon, Aug 31, 2015 at 06:13:03PM -0400, j.glisse@gmail.com wrote:
> > > > From: Jérôme Glisse <jglisse@redhat.com>
> > > > 
> > > > Fix amd_iommu_detect() to return positive value on success, like
> > > > intended, and not zero. This will not change anything in the end
> > > > as AMD IOMMU disable swiotlb and properly associate itself with
> > > 
> > > Not sure how it disables SWIOTLB? The AMD Vi does not seem to
> > > change 'swiotlb'. While 'gart_iommu_init' does. Did you mean
> > > the AMD GART code?
> > 
> > So this is convoluted and painfull, each i look back at that it takes
> > me time to figure out of thing happen. Basicly amd_iommu_init_dma_ops()
> > will replace dma_ops to no_mmu unless passthrough, and when the AMD
> > iommu associate itself with each device it will set the archdata.dma_ops
> > again this unbind the default of swiotlb that is initialize before
> > hw IOMMU.

Oh my. That is not exactly clear.
> > 
> > > 
> > > > devices even if detect() doesn't return a positive value.
> > > 
> > > Returning positive will mean that the pci_iommu_alloc will stop
> > > processing _all_ other IOMMUs.
> > >
> > > While returning 0 will let it detect the other IOMMUs.
> > 
> > No see the IOMMU_FINISH_IF_DETECTED flags in pci_iommu_alloc().
> > Which is not set for AMD hence my patch should not change anything
> > it (AFAICT and from testing but i do not have all AMD hw the ever
> > existed).
> > 
> > So i am just making the detect function do what the API doc says it
> > should do. See line 72 to 80 of : arch/x86/include/asm/iommu_table.h
> > 
> > > 
> > > Granted on an AMD machine there can be two 'IOMMU's - the GART
> > > and the AMD Vi. The detection is always to call gart_iommu_hole_init
> > > first, then amd_iommu_detect.
> > > 
> > > I presume if there was one more type on AMD we would run into trouble.
> > 
> > No because of IOMMU_FINISH_IF_DETECTED flag.
> > 
> > Hope this clarify thing this spagethi mix :)
> 
> Ok my bad amd actualy is using IOMMU_INIT_FINISH() so it finish before
> trying other. Which make sense for AMD as AMD driver will call the gart
> init gart_iommu_init() if it fails to initialize. If we ever end up with
> a platform with multiple IOMMU beside AMD then we need to switch to the
> IOMMU_INIT() instead of the finish one.

Right :-)

Does it make sense then to push this patch out considering that it will
make it harder in the future (aka, somebody will have to track why
their IOMMUs are not initializing)?

Or perhaps just make a patch that puts a comment around the 'return ' and
says why it has the value it has now?
> 
> Cheers,
> Jérôme