From: Arnd Bergmann <[email protected]>
Using 'imply AMD_IOMMU_V2' does not guarantee that the driver can link
against the exported functions. If the GPU driver is built-in but the
IOMMU driver is a loadable module, the kfd_iommu.c file is indeed
built but does not work:
x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_bind_process_to_device':
kfd_iommu.c:(.text+0x516): undefined reference to `amd_iommu_bind_pasid'
x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_unbind_process':
kfd_iommu.c:(.text+0x691): undefined reference to `amd_iommu_unbind_pasid'
x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_suspend':
kfd_iommu.c:(.text+0x966): undefined reference to `amd_iommu_set_invalidate_ctx_cb'
x86_64-linux-ld: kfd_iommu.c:(.text+0x97f): undefined reference to `amd_iommu_set_invalid_ppr_cb'
x86_64-linux-ld: kfd_iommu.c:(.text+0x9a4): undefined reference to `amd_iommu_free_device'
x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_resume':
kfd_iommu.c:(.text+0xa9a): undefined reference to `amd_iommu_init_device'
x86_64-linux-ld: kfd_iommu.c:(.text+0xadc): undefined reference to `amd_iommu_set_invalidate_ctx_cb'
x86_64-linux-ld: kfd_iommu.c:(.text+0xaff): undefined reference to `amd_iommu_set_invalid_ppr_cb'
x86_64-linux-ld: kfd_iommu.c:(.text+0xc72): undefined reference to `amd_iommu_bind_pasid'
x86_64-linux-ld: kfd_iommu.c:(.text+0xe08): undefined reference to `amd_iommu_set_invalidate_ctx_cb'
x86_64-linux-ld: kfd_iommu.c:(.text+0xe26): undefined reference to `amd_iommu_set_invalid_ppr_cb'
x86_64-linux-ld: kfd_iommu.c:(.text+0xe42): undefined reference to `amd_iommu_free_device'
Use a stronger 'select' instead.
Fixes: 64d1c3a43a6f ("drm/amdkfd: Centralize IOMMUv2 code and make it conditional")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/gpu/drm/amd/amdkfd/Kconfig | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/Kconfig b/drivers/gpu/drm/amd/amdkfd/Kconfig
index f02c938f75da..91f85dfb7ba6 100644
--- a/drivers/gpu/drm/amd/amdkfd/Kconfig
+++ b/drivers/gpu/drm/amd/amdkfd/Kconfig
@@ -5,8 +5,9 @@
config HSA_AMD
bool "HSA kernel driver for AMD GPU devices"
- depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64)
- imply AMD_IOMMU_V2 if X86_64
+ depends on DRM_AMDGPU && ((X86_64 && IOMMU_SUPPORT && ACPI) || ARM64 || PPC64)
+ select AMD_IOMMU if X86_64
+ select AMD_IOMMU_V2 if X86_64
select HMM_MIRROR
select MMU_NOTIFIER
select DRM_AMDGPU_USERPTR
--
2.29.2
The driver build should work without IOMMUv2. In amdkfd/Makefile, we
have this condition:
ifneq ($(CONFIG_AMD_IOMMU_V2),)
AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o
endif
In amdkfd/kfd_iommu.h we define inline stubs of the functions that are
causing your link-failures if IOMMU_V2 is not enabled:
#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
... function declarations ...
#else
... stubs ...
#endif
Regards,
Felix
Am 2021-03-08 um 10:33 a.m. schrieb Arnd Bergmann:
> From: Arnd Bergmann <[email protected]>
>
> Using 'imply AMD_IOMMU_V2' does not guarantee that the driver can link
> against the exported functions. If the GPU driver is built-in but the
> IOMMU driver is a loadable module, the kfd_iommu.c file is indeed
> built but does not work:
>
> x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_bind_process_to_device':
> kfd_iommu.c:(.text+0x516): undefined reference to `amd_iommu_bind_pasid'
> x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_unbind_process':
> kfd_iommu.c:(.text+0x691): undefined reference to `amd_iommu_unbind_pasid'
> x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_suspend':
> kfd_iommu.c:(.text+0x966): undefined reference to `amd_iommu_set_invalidate_ctx_cb'
> x86_64-linux-ld: kfd_iommu.c:(.text+0x97f): undefined reference to `amd_iommu_set_invalid_ppr_cb'
> x86_64-linux-ld: kfd_iommu.c:(.text+0x9a4): undefined reference to `amd_iommu_free_device'
> x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_resume':
> kfd_iommu.c:(.text+0xa9a): undefined reference to `amd_iommu_init_device'
> x86_64-linux-ld: kfd_iommu.c:(.text+0xadc): undefined reference to `amd_iommu_set_invalidate_ctx_cb'
> x86_64-linux-ld: kfd_iommu.c:(.text+0xaff): undefined reference to `amd_iommu_set_invalid_ppr_cb'
> x86_64-linux-ld: kfd_iommu.c:(.text+0xc72): undefined reference to `amd_iommu_bind_pasid'
> x86_64-linux-ld: kfd_iommu.c:(.text+0xe08): undefined reference to `amd_iommu_set_invalidate_ctx_cb'
> x86_64-linux-ld: kfd_iommu.c:(.text+0xe26): undefined reference to `amd_iommu_set_invalid_ppr_cb'
> x86_64-linux-ld: kfd_iommu.c:(.text+0xe42): undefined reference to `amd_iommu_free_device'
>
> Use a stronger 'select' instead.
>
> Fixes: 64d1c3a43a6f ("drm/amdkfd: Centralize IOMMUv2 code and make it conditional")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/gpu/drm/amd/amdkfd/Kconfig | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/Kconfig b/drivers/gpu/drm/amd/amdkfd/Kconfig
> index f02c938f75da..91f85dfb7ba6 100644
> --- a/drivers/gpu/drm/amd/amdkfd/Kconfig
> +++ b/drivers/gpu/drm/amd/amdkfd/Kconfig
> @@ -5,8 +5,9 @@
>
> config HSA_AMD
> bool "HSA kernel driver for AMD GPU devices"
> - depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64)
> - imply AMD_IOMMU_V2 if X86_64
> + depends on DRM_AMDGPU && ((X86_64 && IOMMU_SUPPORT && ACPI) || ARM64 || PPC64)
> + select AMD_IOMMU if X86_64
> + select AMD_IOMMU_V2 if X86_64
> select HMM_MIRROR
> select MMU_NOTIFIER
> select DRM_AMDGPU_USERPTR
On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <[email protected]> wrote:
>
> The driver build should work without IOMMUv2. In amdkfd/Makefile, we
> have this condition:
>
> ifneq ($(CONFIG_AMD_IOMMU_V2),)
> AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o
> endif
>
> In amdkfd/kfd_iommu.h we define inline stubs of the functions that are
> causing your link-failures if IOMMU_V2 is not enabled:
>
> #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
> ... function declarations ...
> #else
> ... stubs ...
> #endif
Right, that is the problem I tried to explain in my patch description.
Should we just drop the 'imply' then and add a proper dependency like this?
depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64)
depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m
I can send a v2 after some testing if you prefer this version.
Arnd
Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann:
> On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <[email protected]> wrote:
>> The driver build should work without IOMMUv2. In amdkfd/Makefile, we
>> have this condition:
>>
>> ifneq ($(CONFIG_AMD_IOMMU_V2),)
>> AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o
>> endif
>>
>> In amdkfd/kfd_iommu.h we define inline stubs of the functions that are
>> causing your link-failures if IOMMU_V2 is not enabled:
>>
>> #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>> ... function declarations ...
>> #else
>> ... stubs ...
>> #endif
> Right, that is the problem I tried to explain in my patch description.
>
> Should we just drop the 'imply' then and add a proper dependency like this?
>
> depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64)
> depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m
>
> I can send a v2 after some testing if you prefer this version.
No. My point is, there should not be a hard dependency. The build should
work without CONFIG_AMD_IOMMU_V2. I don't understand why it's not
working for you. It looks like you're building kfd_iommu.o, which should
not be happening when AMD_IOMMU_V2 is not enabled. The condition in
amdkfd/Makefile should make sure that kfd_iommu.o doesn't get built with
your kernel config.
Regards,
Felix
>
> Arnd
On Mon, Mar 8, 2021 at 8:11 PM Felix Kuehling <[email protected]> wrote:
>
> Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann:
> > On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <[email protected]> wrote:
> >> The driver build should work without IOMMUv2. In amdkfd/Makefile, we
> >> have this condition:
> >>
> >> ifneq ($(CONFIG_AMD_IOMMU_V2),)
> >> AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o
> >> endif
> >>
> >> In amdkfd/kfd_iommu.h we define inline stubs of the functions that are
> >> causing your link-failures if IOMMU_V2 is not enabled:
> >>
> >> #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
> >> ... function declarations ...
> >> #else
> >> ... stubs ...
> >> #endif
> > Right, that is the problem I tried to explain in my patch description.
> >
> > Should we just drop the 'imply' then and add a proper dependency like this?
> >
> > depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64)
> > depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m
> >
> > I can send a v2 after some testing if you prefer this version.
>
> No. My point is, there should not be a hard dependency. The build should
> work without CONFIG_AMD_IOMMU_V2. I don't understand why it's not
> working for you. It looks like you're building kfd_iommu.o, which should
> not be happening when AMD_IOMMU_V2 is not enabled. The condition in
> amdkfd/Makefile should make sure that kfd_iommu.o doesn't get built with
> your kernel config.
Again, as I explained in the changelog text, AMD_IOMMU_V2 configured as
a loadable module, while AMDGPU is configured as built-in.
The causes a link failure for the vmlinux file, because the linker cannot
resolve addresses of loadable modules at compile time -- they have
not been loaded yet.
Arnd
Am 2021-03-08 um 2:33 p.m. schrieb Arnd Bergmann:
> On Mon, Mar 8, 2021 at 8:11 PM Felix Kuehling <[email protected]> wrote:
>> Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann:
>>> On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <[email protected]> wrote:
>>>> The driver build should work without IOMMUv2. In amdkfd/Makefile, we
>>>> have this condition:
>>>>
>>>> ifneq ($(CONFIG_AMD_IOMMU_V2),)
>>>> AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o
>>>> endif
>>>>
>>>> In amdkfd/kfd_iommu.h we define inline stubs of the functions that are
>>>> causing your link-failures if IOMMU_V2 is not enabled:
>>>>
>>>> #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>>> ... function declarations ...
>>>> #else
>>>> ... stubs ...
>>>> #endif
>>> Right, that is the problem I tried to explain in my patch description.
>>>
>>> Should we just drop the 'imply' then and add a proper dependency like this?
>>>
>>> depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64)
>>> depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m
>>>
>>> I can send a v2 after some testing if you prefer this version.
>> No. My point is, there should not be a hard dependency. The build should
>> work without CONFIG_AMD_IOMMU_V2. I don't understand why it's not
>> working for you. It looks like you're building kfd_iommu.o, which should
>> not be happening when AMD_IOMMU_V2 is not enabled. The condition in
>> amdkfd/Makefile should make sure that kfd_iommu.o doesn't get built with
>> your kernel config.
> Again, as I explained in the changelog text, AMD_IOMMU_V2 configured as
> a loadable module, while AMDGPU is configured as built-in.
I'm sorry, I didn't read it carefully. And I thought "imply" was meant
to fix exactly this kind of issue.
I don't want to create a hard dependency on AMD_IOMMU_V2 if I can avoid
it, because it is only really needed for a small number of AMD APUs, and
even there it is now optional for more recent ones.
Is there a better way to avoid build failures without creating a hard
dependency? The documentation in
Documentation/kbuild/kconfig-language.rst suggests using if
(IS_REACHABLE(CONFIG_AMD_IOMMU_V2)) to guard those problematic function
calls. I think more generally, we could guard all of kfd_iommu.c with
#if IS_REACHABLE(CONFIG_AMD_IOMMU_V2)
And use the same condition to define the stubs in kfd_iommu.h.
Regards,
Felix
>
> The causes a link failure for the vmlinux file, because the linker cannot
> resolve addresses of loadable modules at compile time -- they have
> not been loaded yet.
>
> Arnd
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 08.03.21 um 21:02 schrieb Felix Kuehling:
> Am 2021-03-08 um 2:33 p.m. schrieb Arnd Bergmann:
>> On Mon, Mar 8, 2021 at 8:11 PM Felix Kuehling <[email protected]> wrote:
>>> Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann:
>>>> On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <[email protected]> wrote:
>>>>> The driver build should work without IOMMUv2. In amdkfd/Makefile, we
>>>>> have this condition:
>>>>>
>>>>> ifneq ($(CONFIG_AMD_IOMMU_V2),)
>>>>> AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o
>>>>> endif
>>>>>
>>>>> In amdkfd/kfd_iommu.h we define inline stubs of the functions that are
>>>>> causing your link-failures if IOMMU_V2 is not enabled:
>>>>>
>>>>> #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
>>>>> ... function declarations ...
>>>>> #else
>>>>> ... stubs ...
>>>>> #endif
>>>> Right, that is the problem I tried to explain in my patch description.
>>>>
>>>> Should we just drop the 'imply' then and add a proper dependency like this?
>>>>
>>>> depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64)
>>>> depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m
>>>>
>>>> I can send a v2 after some testing if you prefer this version.
>>> No. My point is, there should not be a hard dependency. The build should
>>> work without CONFIG_AMD_IOMMU_V2. I don't understand why it's not
>>> working for you. It looks like you're building kfd_iommu.o, which should
>>> not be happening when AMD_IOMMU_V2 is not enabled. The condition in
>>> amdkfd/Makefile should make sure that kfd_iommu.o doesn't get built with
>>> your kernel config.
>> Again, as I explained in the changelog text, AMD_IOMMU_V2 configured as
>> a loadable module, while AMDGPU is configured as built-in.
> I'm sorry, I didn't read it carefully. And I thought "imply" was meant
> to fix exactly this kind of issue.
>
> I don't want to create a hard dependency on AMD_IOMMU_V2 if I can avoid
> it, because it is only really needed for a small number of AMD APUs, and
> even there it is now optional for more recent ones.
>
> Is there a better way to avoid build failures without creating a hard
> dependency?
What you need is the same trick we used for AGP on radeon/nouveau:
depends on AMD_IOMMU_V2 || !AMD_IOMMU_V2
This way when AMD_IOMMU_V2 is build as a module DRM_AMDGPU will be build
as a module as well. When it is disabled completely we don't care.
Regards,
Christian.
> The documentation in
> Documentation/kbuild/kconfig-language.rst suggests using if
> (IS_REACHABLE(CONFIG_AMD_IOMMU_V2)) to guard those problematic function
> calls. I think more generally, we could guard all of kfd_iommu.c with
>
> #if IS_REACHABLE(CONFIG_AMD_IOMMU_V2)
>
> And use the same condition to define the stubs in kfd_iommu.h.
>
> Regards,
> Felix
>
>
>> The causes a link failure for the vmlinux file, because the linker cannot
>> resolve addresses of loadable modules at compile time -- they have
>> not been loaded yet.
>>
>> Arnd
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> amd-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Mon, Mar 8, 2021 at 9:12 PM Christian König
<[email protected]> wrote:
> Am 08.03.21 um 21:02 schrieb Felix Kuehling:
> > Am 2021-03-08 um 2:33 p.m. schrieb Arnd Bergmann:
> > I don't want to create a hard dependency on AMD_IOMMU_V2 if I can avoid
> > it, because it is only really needed for a small number of AMD APUs, and
> > even there it is now optional for more recent ones.
> >
> > Is there a better way to avoid build failures without creating a hard
> > dependency?
>
> What you need is the same trick we used for AGP on radeon/nouveau:
>
> depends on AMD_IOMMU_V2 || !AMD_IOMMU_V2
>
> This way when AMD_IOMMU_V2 is build as a module DRM_AMDGPU will be build
> as a module as well. When it is disabled completely we don't care.
Note that this trick only works if you put it into the DRM_AMDGPU Kconfig option
itself, since that decides if the driver is built-in or a loadable module. If
HSA_AMD is disabled, that dependency is not really necessary.
The version I suggested -- adding "depends on AMD_IOMMU_V2=y ||
DRM_AMDGPU=m" to the HSA_AMD option -- might be slightly nicer
since it lets you still build a kernel with DRM_AMDGPU=y and
AMD_IOMMU_V2=m, but without the HSA_AMD.
I can send a patch with either of those two options to replace my
original patch.
> > The documentation in
> > Documentation/kbuild/kconfig-language.rst suggests using if
> > (IS_REACHABLE(CONFIG_AMD_IOMMU_V2)) to guard those problematic function
> > calls. I think more generally, we could guard all of kfd_iommu.c with
> >
> > #if IS_REACHABLE(CONFIG_AMD_IOMMU_V2)
> >
> > And use the same condition to define the stubs in kfd_iommu.h.
This would fix the compile-time error, but it's also the one I'd least
recommend out of all the options, because that causes the driver to
silently not work as expected. This seems even worse than failing
the build.
Arnd