Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
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.gli...@gmail.com wrote: > > > > From: Jérôme Glisse > > > > > > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
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.gli...@gmail.com wrote: > > > > From: Jérôme Glisse> > > > > > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
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.gli...@gmail.com wrote: > > > From: Jérôme Glisse > > > > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
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.gli...@gmail.com wrote: > > From: Jérôme Glisse > > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
On Mon, Aug 31, 2015 at 06:13:03PM -0400, j.gli...@gmail.com wrote: > From: Jérôme Glisse > > 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 > Cc: Joerg Roedel > Cc: io...@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 > io...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
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.gli...@gmail.com wrote: > > From: Jérôme Glisse> > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
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.gli...@gmail.com wrote: > > > From: Jérôme Glisse> > > > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
On Mon, Aug 31, 2015 at 06:13:03PM -0400, j.gli...@gmail.com wrote: > From: Jérôme Glisse> > 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 > Cc: Joerg Roedel > Cc: io...@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 > io...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
On Mon, Aug 31, 2015 at 06:13:03PM -0400, j.gli...@gmail.com wrote: > From: Jérôme Glisse > > 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 > Cc: Joerg Roedel > Cc: io...@lists.linux-foundation.org Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
On Mon, Aug 31, 2015 at 06:13:03PM -0400, j.gli...@gmail.com wrote: > From: Jérôme Glisse> > 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 > Cc: Joerg Roedel > Cc: io...@lists.linux-foundation.org Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
From: Jérôme Glisse 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 Cc: Joerg Roedel Cc: io...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] iommu/amd: Fix amd_iommu_detect() (does not fix any issues).
From: Jérôme GlisseFix 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 Cc: Joerg Roedel Cc: io...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/