From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753389AbcDRMD6 (ORCPT ); Mon, 18 Apr 2016 08:03:58 -0400 Received: from mx2.suse.de ([195.135.220.15]:54586 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751751AbcDRMD4 (ORCPT ); Mon, 18 Apr 2016 08:03:56 -0400 Date: Mon, 18 Apr 2016 14:03:50 +0200 From: "Luis R. Rodriguez" To: Oded Gabbay Cc: "Luis R. Rodriguez" , Christian =?iso-8859-1?Q?K=F6nig?= , "Linux-Kernel@Vger. Kernel. Org" , iommu@lists.linux-foundation.org, Joerg Roedel Subject: Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu Message-ID: <20160418120350.GE1990@wotan.suse.de> References: <20160316171719.GE2195@8bytes.org> <1459273313-5139-1-git-send-email-mcgrof@kernel.org> <570BA694.8040900@amd.com> <570BAC2B.4090508@amd.com> <20160412220715.GL1990@wotan.suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Apr 18, 2016 at 10:02:24AM +0300, Oded Gabbay wrote: > On Mon, Apr 18, 2016 at 9:55 AM, Luis R. Rodriguez wrote: > > > > On Apr 18, 2016 7:48 AM, "Oded Gabbay" wrote: > >> > >> On Wed, Apr 13, 2016 at 1:07 AM, Luis R. Rodriguez > >> wrote: > >> > On Mon, Apr 11, 2016 at 03:52:43PM +0200, Christian König wrote: > >> >> Am 11.04.2016 um 15:39 schrieb Oded Gabbay: > >> >> >On Mon, Apr 11, 2016 at 4:28 PM, Christian König > >> >> > wrote: > >> >> >>Am 09.04.2016 um 02:25 schrieb Luis R. Rodriguez: > >> >> >>>On Tue, Mar 29, 2016 at 10:41 AM, Luis R. Rodriguez > >> >> >>> wrote: > >> >> >>>>We need to ensure amd iommu v2 initializes before > >> >> >>>>driver uses such as drivers/gpu/drm/amd/amdkfd/kfd_module.c, > >> >> >>>>to do this make its init routine a subsys_initcall() which > >> >> >>>>ensures its load init is called first than modules when > >> >> >>>>built-in. > >> >> >>>> > >> >> >>>>This reverts the old work around implemented through commit > >> >> >>>>1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in > >> >> >>>> Makefile"), > >> >> >>>>instead of making the dependency implicit by linker order this > >> >> >>>>makes the ordering requirement explicit through proper kernel > >> >> >>>>APIs. > >> >> >>>> > >> >> >>>>Cc: Oded Gabbay > >> >> >>>>Cc: Christian König > >> >> >>>>Signed-off-by: Luis R. Rodriguez > >> >> >> > >> >> >>Sorry for not responding earlier. Just coming back to all the stuff > >> >> >> on my TODO list. > >> >> >> > >> >> >>Patch is Acked-by: Christian König > >> >> > > >> >> >Christian, > >> >> >Just wanted to be sure if you tested this patch-set or not. > >> >> > >> >> I did NOT tested it. If AMD IOMMU requires something which will now > >> >> initialize after the IOMMU module we will obviously run into trouble > >> >> again. > >> >> > >> >> I assumed that the creator of the patch did some testing. > >> > > >> > Nope, hence [RTF] Request For Testing. > >> > > >> >> >I don't think it should be merged without testing. If you already > >> >> >tested it than fine. If not, I think I can do it in the next week or > >> >> >so (just came back from PTO). > >> >> > >> >> Yeah, agree totally. > >> > > >> > Agreed, please let me know if someone is able to test and confirm > >> > this works. It should work. > >> > > >> > Luis > >> > >> Hi, > >> So I finally got to test this patch and it's not working. > >> The reason is that AMD IOMMUv2 gets initialized *before* AMD IOMMUv1 > >> driver ! > > > > Thanks can you try using late_initcall() instead then? > > > > Luis > > That will make it initialize *after* drm subsystem, which will cause > another bug. Hold up, I thought that we needed AMD IOMMUv2 to get initialized before AMD IOMMUv1 ? That's what the patch did. Can someone clarify the requirements then? I'll provide some review of the current state of affairs first, without the patch. AMD IOMMUv1 uses x86_init.iommu.iommu_init and that has its own init semantics. Specifically that gets called via pci_iommu_init() which is pegged on the init order via rootfs_initcall(pci_iommu_init); Then AMD IOMMUv2 uses module_init() and that when is built-in falls on to __initcall() which is device_initcall(). The order is: #define pure_initcall(fn) __define_initcall(fn, 0) #define core_initcall(fn) __define_initcall(fn, 1) #define core_initcall_sync(fn) __define_initcall(fn, 1s) #define postcore_initcall(fn) __define_initcall(fn, 2) #define postcore_initcall_sync(fn) __define_initcall(fn, 2s) #define arch_initcall(fn) __define_initcall(fn, 3) #define arch_initcall_sync(fn) __define_initcall(fn, 3s) #define subsys_initcall(fn) __define_initcall(fn, 4) #define subsys_initcall_sync(fn) __define_initcall(fn, 4s) #define fs_initcall(fn) __define_initcall(fn, 5) #define fs_initcall_sync(fn) __define_initcall(fn, 5s) #define rootfs_initcall(fn) __define_initcall(fn, rootfs) #define device_initcall(fn) __define_initcall(fn, 6) #define device_initcall_sync(fn) __define_initcall(fn, 6s) #define late_initcall(fn) __define_initcall(fn, 7) #define late_initcall_sync(fn) __define_initcall(fn, 7s) So technically rootfs_initcall() (v1 amd) should be being called first already, and after that AMD IOMMUv2 gets called next. You said that with my patch you saw AMD IOMMUv2 kick off first, that was intentional as I thought that's what you needed. Can someone please describe the requirements? Also what does drm use that you say has a conflict already? What drm code are we talking about exactly ? Luis From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Luis R. Rodriguez" Subject: Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu Date: Mon, 18 Apr 2016 14:03:50 +0200 Message-ID: <20160418120350.GE1990@wotan.suse.de> References: <20160316171719.GE2195@8bytes.org> <1459273313-5139-1-git-send-email-mcgrof@kernel.org> <570BA694.8040900@amd.com> <570BAC2B.4090508@amd.com> <20160412220715.GL1990@wotan.suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Oded Gabbay Cc: "Luis R. Rodriguez" , Christian =?iso-8859-1?Q?K=F6nig?= , iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, "Linux-Kernel@Vger. Kernel. Org" List-Id: iommu@lists.linux-foundation.org On Mon, Apr 18, 2016 at 10:02:24AM +0300, Oded Gabbay wrote: > On Mon, Apr 18, 2016 at 9:55 AM, Luis R. Rodriguez wr= ote: > > > > On Apr 18, 2016 7:48 AM, "Oded Gabbay" wrote: > >> > >> On Wed, Apr 13, 2016 at 1:07 AM, Luis R. Rodriguez > >> wrote: > >> > On Mon, Apr 11, 2016 at 03:52:43PM +0200, Christian K=F6nig wrote: > >> >> Am 11.04.2016 um 15:39 schrieb Oded Gabbay: > >> >> >On Mon, Apr 11, 2016 at 4:28 PM, Christian K=F6nig > >> >> > wrote: > >> >> >>Am 09.04.2016 um 02:25 schrieb Luis R. Rodriguez: > >> >> >>>On Tue, Mar 29, 2016 at 10:41 AM, Luis R. Rodriguez > >> >> >>> wrote: > >> >> >>>>We need to ensure amd iommu v2 initializes before > >> >> >>>>driver uses such as drivers/gpu/drm/amd/amdkfd/kfd_module.c, > >> >> >>>>to do this make its init routine a subsys_initcall() which > >> >> >>>>ensures its load init is called first than modules when > >> >> >>>>built-in. > >> >> >>>> > >> >> >>>>This reverts the old work around implemented through commit > >> >> >>>>1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in > >> >> >>>> Makefile"), > >> >> >>>>instead of making the dependency implicit by linker order this > >> >> >>>>makes the ordering requirement explicit through proper kernel > >> >> >>>>APIs. > >> >> >>>> > >> >> >>>>Cc: Oded Gabbay > >> >> >>>>Cc: Christian K=F6nig > >> >> >>>>Signed-off-by: Luis R. Rodriguez > >> >> >> > >> >> >>Sorry for not responding earlier. Just coming back to all the stu= ff > >> >> >> on my TODO list. > >> >> >> > >> >> >>Patch is Acked-by: Christian K=F6nig > >> >> > > >> >> >Christian, > >> >> >Just wanted to be sure if you tested this patch-set or not. > >> >> > >> >> I did NOT tested it. If AMD IOMMU requires something which will now > >> >> initialize after the IOMMU module we will obviously run into trouble > >> >> again. > >> >> > >> >> I assumed that the creator of the patch did some testing. > >> > > >> > Nope, hence [RTF] Request For Testing. > >> > > >> >> >I don't think it should be merged without testing. If you already > >> >> >tested it than fine. If not, I think I can do it in the next week = or > >> >> >so (just came back from PTO). > >> >> > >> >> Yeah, agree totally. > >> > > >> > Agreed, please let me know if someone is able to test and confirm > >> > this works. It should work. > >> > > >> > Luis > >> > >> Hi, > >> So I finally got to test this patch and it's not working. > >> The reason is that AMD IOMMUv2 gets initialized *before* AMD IOMMUv1 > >> driver ! > > > > Thanks can you try using late_initcall() instead then? > > > > Luis > = > That will make it initialize *after* drm subsystem, which will cause > another bug. Hold up, I thought that we needed AMD IOMMUv2 to get initialized before AMD IOMMUv1 ? That's what the patch did. Can someone clarify the requirements then? I'll provide some review of the current state of affairs first, without the patch. AMD IOMMUv1 uses x86_init.iommu.iommu_init and that has its own init semantics. Specifically that gets called via pci_iommu_init() which is pegg= ed on the init order via rootfs_initcall(pci_iommu_init); Then AMD IOMMUv2 uses module_init() and that when is built-in falls on to __initcall() which is device_initcall(). The order is: #define pure_initcall(fn) __define_initcall(fn, 0) = = = = #define core_initcall(fn) __define_initcall(fn, 1) = = #define core_initcall_sync(fn) __define_initcall(fn, 1s) = = #define postcore_initcall(fn) __define_initcall(fn, 2) = = #define postcore_initcall_sync(fn) __define_initcall(fn, 2s) = = #define arch_initcall(fn) __define_initcall(fn, 3) = = #define arch_initcall_sync(fn) __define_initcall(fn, 3s) = = #define subsys_initcall(fn) __define_initcall(fn, 4) = = #define subsys_initcall_sync(fn) __define_initcall(fn, 4s) = = #define fs_initcall(fn) __define_initcall(fn, 5) = = #define fs_initcall_sync(fn) __define_initcall(fn, 5s) = = #define rootfs_initcall(fn) __define_initcall(fn, rootfs) = = #define device_initcall(fn) __define_initcall(fn, 6) = = #define device_initcall_sync(fn) __define_initcall(fn, 6s) = = #define late_initcall(fn) __define_initcall(fn, 7) = = #define late_initcall_sync(fn) __define_initcall(fn, 7s) = So technically rootfs_initcall() (v1 amd) should be being called first already, and after that AMD IOMMUv2 gets called next. You said that with my patch you saw AMD IOMMUv2 kick off first, that was intentional as I thought that's what you needed. Can someone please describe the requirements? Also what does drm use that you say has a conflict already? What drm code are we talking about exactly ? Luis