Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1256136 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any  issues). Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> - 2015-10-26 17:10 +0100
    Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any  issues). Jerome Glisse <j.glisse@gmail.com> - 2015-10-27 01:50 +0100
      Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any  issues). Jerome Glisse <j.glisse@gmail.com> - 2015-10-27 02:00 +0100
        Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any  issues). Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> - 2015-10-27 15:20 +0100

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1256368

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256371

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256774

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web