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@xxxxxxxxx wrote:
> > > > From: Jérôme Glisse <jglisse@xxxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/