Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu

From: Wan Zongshun
Date: Mon Apr 18 2016 - 22:05:30 EST




-------- Original Message --------
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 <mcgrof@xxxxxxxxxx> wrote:

On Apr 18, 2016 7:48 AM, "Oded Gabbay" <oded.gabbay@xxxxxxxxx> wrote:

On Wed, Apr 13, 2016 at 1:07 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
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
<christian.koenig@xxxxxxx> wrote:
Am 09.04.2016 um 02:25 schrieb Luis R. Rodriguez:
On Tue, Mar 29, 2016 at 10:41 AM, Luis R. Rodriguez
<mcgrof@xxxxxxxxxx> 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 <oded.gabbay@xxxxxxx>
Cc: Christian KÃnig <christian.koenig@xxxxxxx>
Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>

Sorry for not responding earlier. Just coming back to all the stuff
on my TODO list.

Patch is Acked-by: Christian KÃnig <christian.koenig@xxxxxxx>

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?

We must keep AMD IOMMUv2 to get initialized after AMD IOMMUv1ï So your patch make the sequence reverse.


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().

Exactly, it is amd iommu v1 v2 call sequence.

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 ?

You have to take carefully to arrange the calling sequence for iommuv1, iommuv2, kfd module, and drm like the following sequence : v1 ->v2->kfd, drm.

iommuv1 -- rootfs_initcall(fn)
IOMMUV2 -- device_initcall(fn)
kfd module -- late_initcall(fn)
drm -- late_initcall(fn)

Thanks!
Wan Zongshun.


Luis
_______________________________________________
iommu mailing list
iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/iommu