Re: Linux Kernel build bug with AMD_IOMMU_V2=M and HSA_AMD=Y

[Felix Kuehling]

This should have been fixed by this commit in amd-staging-drm-next: 
https://lore.kernel.org/patchwork/patch/1392368/


commit b8aff1f3a0b3d8434f8ccf5d3017137c29aca77b
Author: Felix Kuehling 
Date:   Mon Mar 8 22:15:42 2021 -0500

drm/amdkfd: fix build error with AMD_IOMMU_V2=m

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 IS_REACHABLE to only build IOMMU-V2 support if the amd_iommu symbols

are reachable by the amdkfd driver. Output a warning if they are not,
because that may not be what the user was expecting.

Fixes: 64d1c3a43a6f ("drm/amdkfd: Centralize IOMMUv2 code and make it conditional")

Reported-by: Arnd Bergmann 
Signed-off-by: Felix Kuehling 
Reviewed-by: Christian König 

Regards,
  Felix

On 2021-04-08 7:04 p.m., David Niklas wrote:

Hello,
(There are so many maintainers for kfd_iommu.c I feel like I'm spamming.)

When compiling for Linux version 5.11.12 using the AMDGPU GPU driver
with HSA_AMD enabled, I get the below set of errors. To work around this,
I need to change AMD_IOMMU_V2 from M to Y. This bug doesn't affect linux
kernel version 5.6 as it requires AMD_IOMMU_V2 to by Y when HSA_AMD is
enabled.
I'd bisect and request the removal of the relevant patch, but it's
possible that building the linux kernel should work this way and so a fix,
not a patch removal, is what should be issued.
I'm attaching my kernel config for 5.11.

Thanks,
David

PS: I made an official bug report in case you'd prefer that:
https://bugzilla.kernel.org/show_bug.cgi?id=212619

drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: In function
`kfd_iommu_bind_process_to_device': 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:120:
undefined reference to `amd_iommu_bind_pasid'
drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: In function
`kfd_iommu_unbind_process': 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:138:
undefined reference to `amd_iommu_unbind_pasid'
drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: In function
`kfd_iommu_suspend': 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:292:
undefined reference to
`amd_iommu_set_invalidate_ctx_cb' 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:293:
undefined reference to `amd_iommu_set_invalid_ppr_cb'
drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: In function
`kfd_iommu_resume': 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:312:
undefined reference to
`amd_iommu_init_device' 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:316:
undefined reference to
`amd_iommu_set_invalidate_ctx_cb' 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:318:
undefined reference to
`amd_iommu_set_invalid_ppr_cb' 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:323:
undefined reference to
`amd_iommu_set_invalidate_ctx_cb' 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:324:
undefined reference to
`amd_iommu_set_invalid_ppr_cb' 
/root/working/linux-5.11.12/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_iommu.c:325:
undefined reference to
`amd_iommu_free_device' 
/root/working/linux-5.11.12/drivers/g

Re: [PATCH] drm/amdkfd: dqm fence memory corruption

[Felix Kuehling]

Am 2021-03-26 um 5:38 a.m. schrieb Qu Huang:
> On 2021/1/28 5:50, Felix Kuehling wrote:
>> Am 2021-01-27 um 7:33 a.m. schrieb Qu Huang:
>>> Amdgpu driver uses 4-byte data type as DQM fence memory,
>>> and transmits GPU address of fence memory to microcode
>>> through query status PM4 message. However, query status
>>> PM4 message definition and microcode processing are all
>>> processed according to 8 bytes. Fence memory only allocates
>>> 4 bytes of memory, but microcode does write 8 bytes of memory,
>>> so there is a memory corruption.
>>
>> Thank you for pointing out that discrepancy. That's a good catch!
>>
>> I'd prefer to fix this properly by making dqm->fence_addr a u64 pointer.
>> We should probably also fix up the query_status and
>> amdkfd_fence_wait_timeout function interfaces to use a 64 bit fence
>> values everywhere to be consistent.
>>
>> Regards,
>>    Felix
> Hi Felix, Thanks for your advice, please check v2 at
> https://lore.kernel.org/patchwork/patch/1372584/

Thank you for the reminder. I somehow missed your v2 patch on the
mailing list. I have reviewed and applied it to amd-staging-drm-next now.

Regards,
  Felix


> Thanks,
> Qu.
>>
>>
>>>
>>> Signed-off-by: Qu Huang 
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> index e686ce2..8b38d0c 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> @@ -1161,7 +1161,7 @@ static int start_cpsch(struct
>>> device_queue_manager *dqm)
>>>   pr_debug("Allocating fence memory\n");
>>>     /* allocate fence memory on the gart */
>>> -    retval = kfd_gtt_sa_allocate(dqm->dev, sizeof(*dqm->fence_addr),
>>> +    retval = kfd_gtt_sa_allocate(dqm->dev, sizeof(uint64_t),
>>>   >fence_mem);
>>>     if (retval)
>

Re: [PATCH] drm/amdkfd: Fix cat debugfs hang_hws file causes system crash bug

[Felix Kuehling]

Am 2021-03-23 um 10:56 a.m. schrieb Alex Deucher:
> Applied.  Thanks!

Thanks. I thought we fixed this before by making the file write-only.
But I guess that's not sufficient to stop root from reading it:

commit 2bdac179e217a0c0b548a8c60524977586621b19
Author: Felix Kuehling 
Date:   Thu Dec 19 22:36:55 2019 -0500

drm/amdkfd: Fix permissions of hang_hws

Reading from /sys/kernel/debug/kfd/hang_hws would cause a kernel
oops because we didn't implement a read callback. Set the permission
to write-only to prevent that.

Signed-off-by: Felix Kuehling 
Reviewed-by: shaoyunl  
Signed-off-by: Alex Deucher 


Now that we have a sensible message in the file, I guess we should
officially make it readable again.

Regards,
  Felix

>
> Alex
>
> On Sun, Mar 21, 2021 at 5:33 AM Qu Huang  wrote:
>> Here is the system crash log:
>> [ 1272.884438] BUG: unable to handle kernel NULL pointer dereference at
>> (null)
>> [ 1272.88] IP: [<  (null)>]   (null)
>> [ 1272.884447] PGD 825b09067 PUD 8267c8067 PMD 0
>> [ 1272.884452] Oops: 0010 [#1] SMP
>> [ 1272.884509] CPU: 13 PID: 3485 Comm: cat Kdump: loaded Tainted: G
>> [ 1272.884515] task: 9a38dbd4d140 ti: 9a37cd3b8000 task.ti:
>> 9a37cd3b8000
>> [ 1272.884517] RIP: 0010:[<>]  [<  (null)>]
>> (null)
>> [ 1272.884520] RSP: 0018:9a37cd3bbe68  EFLAGS: 00010203
>> [ 1272.884522] RAX:  RBX:  RCX:
>> 00014d5f
>> [ 1272.884524] RDX: fff4 RSI: 0001 RDI:
>> 9a38aca4d200
>> [ 1272.884526] RBP: 9a37cd3bbed0 R08: 9a38dcd5f1a0 R09:
>> 9a31ffc07300
>> [ 1272.884527] R10: 9a31ffc07300 R11: addd5e9d R12:
>> 9a38b4e0fb00
>> [ 1272.884529] R13: 0001 R14: 9a37cd3bbf18 R15:
>> 9a38aca4d200
>> [ 1272.884532] FS:  7feccaa67740() GS:9a38dcd4()
>> knlGS:
>> [ 1272.884534] CS:  0010 DS:  ES:  CR0: 80050033
>> [ 1272.884536] CR2:  CR3: 0008267c CR4:
>> 003407e0
>> [ 1272.884537] Call Trace:
>> [ 1272.884544]  [] ? seq_read+0x130/0x440
>> [ 1272.884548]  [] vfs_read+0x9f/0x170
>> [ 1272.884552]  [] SyS_read+0x7f/0xf0
>> [ 1272.884557]  [] system_call_fastpath+0x22/0x27
>> [ 1272.884558] Code:  Bad RIP value.
>> [ 1272.884562] RIP  [<  (null)>]   (null)
>> [ 1272.884564]  RSP 
>> [ 1272.884566] CR2: 
>>
>> Signed-off-by: Qu Huang 
>> ---
>>  drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c
>> index 511712c..673d5e3 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debugfs.c
>> @@ -33,6 +33,11 @@ static int kfd_debugfs_open(struct inode *inode, struct 
>> file *file)
>>
>> return single_open(file, show, NULL);
>>  }
>> +static int kfd_debugfs_hang_hws_read(struct seq_file *m, void *data)
>> +{
>> +   seq_printf(m, "echo gpu_id > hang_hws\n");
>> +   return 0;
>> +}
>>
>>  static ssize_t kfd_debugfs_hang_hws_write(struct file *file,
>> const char __user *user_buf, size_t size, loff_t *ppos)
>> @@ -94,7 +99,7 @@ void kfd_debugfs_init(void)
>> debugfs_create_file("rls", S_IFREG | 0444, debugfs_root,
>> kfd_debugfs_rls_by_device, _debugfs_fops);
>> debugfs_create_file("hang_hws", S_IFREG | 0200, debugfs_root,
>> -   NULL, _debugfs_hang_hws_fops);
>> +   kfd_debugfs_hang_hws_read, 
>> _debugfs_hang_hws_fops);
>>  }
>>
>>  void kfd_debugfs_fini(void)
>> --
>> 1.8.3.1
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/ttm: Fix memory leak userptr pages

[Felix Kuehling]

This caused a regression in kfdtest in a large-buffer stress test after 
memory allocation for user pages fails:


[17359.536303] amdgpu: init_user_pages: Failed to get user pages: -16
[17359.543746] BUG: kernel NULL pointer dereference, address: 
[17359.551494] #PF: supervisor read access in kernel mode
[17359.557375] #PF: error_code(0x) - not-present page
[17359.563247] PGD 0 P4D 0
[17359.566514] Oops:  [#1] SMP PTI
[17359.570728] CPU: 8 PID: 5944 Comm: kfdtest Not tainted 5.11.0-kfd-fkuehlin 
#193
[17359.578760] Hardware name: ASUS All Series/X99-E WS/USB 3.1, BIOS 3201 
06/17/2016
[17359.586971] RIP: 0010:amdgpu_ttm_backend_unbind+0x52/0x110 [amdgpu]
[17359.594075] Code: 48 39 c6 74 1b 8b 53 0c 48 8d bd 80 a1 ff ff e8 24 62 00 00 85 
c0 0f 85 ab 00 00 00 c6 43 54 00 5b 5d c3 48 8b 46 10 8b 4e 50 <48> 8b 30 48 85 
f6 74 ba 8b 50 0c 48 8b bf 80 a1 ff ff 83 e1 01 45
[17359.614340] RSP: 0018:a4764971fc98 EFLAGS: 00010206
[17359.620315] RAX:  RBX: 950e8d4edf00 RCX: 
[17359.628204] RDX:  RSI: 950e8d4edf00 RDI: 950eadec5e80
[17359.636084] RBP: 950eadec5e80 R08:  R09: 
[17359.643958] R10: 0246 R11: 0001 R12: 950c03377800
[17359.651833] R13: 950eadec5e80 R14: 950c03377858 R15: 
[17359.659701] FS:  7febb20cb740() GS:950ebfc0() 
knlGS:
[17359.668528] CS:  0010 DS:  ES:  CR0: 80050033
[17359.675012] CR2:  CR3: 0006d700e005 CR4: 001706e0
[17359.682883] Call Trace:
[17359.686063]  amdgpu_ttm_backend_destroy+0x12/0x70 [amdgpu]
[17359.692349]  ttm_bo_cleanup_memtype_use+0x37/0x60 [ttm]
[17359.698307]  ttm_bo_release+0x278/0x5e0 [ttm]
[17359.703385]  amdgpu_bo_unref+0x1a/0x30 [amdgpu]
[17359.708701]  amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu+0x7e5/0x910 [amdgpu]
[17359.716307]  kfd_ioctl_alloc_memory_of_gpu+0x11a/0x220 [amdgpu]
[17359.723036]  kfd_ioctl+0x223/0x400 [amdgpu]
[17359.728017]  ? kfd_dev_is_large_bar+0x90/0x90 [amdgpu]
[17359.734152]  __x64_sys_ioctl+0x8b/0xd0
[17359.738796]  do_syscall_64+0x2d/0x40
[17359.743259]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[17359.749205] RIP: 0033:0x7febb083b6d7
[17359.753681] Code: b3 66 90 48 8b 05 b1 47 2d 00 64 c7 00 26 00 00 00 48 c7 c0 ff 
ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff 
ff 73 01 c3 48 8b 0d 81 47 2d 00 f7 d8 64 89 01 48
[17359.774340] RSP: 002b:7ffdb5522cd8 EFLAGS: 0202 ORIG_RAX: 
0010
[17359.782668] RAX: ffda RBX: 0001 RCX: 7febb083b6d7
[17359.790566] RDX: 7ffdb5522d60 RSI: c0284b16 RDI: 0003
[17359.798459] RBP: 7ffdb5522d10 R08: 7ffdb5522dd0 R09: c404
[17359.806352] R10:  R11: 0202 R12: 559416e4e2aa
[17359.814251] R13:  R14: 0021 R15: 
[17359.822140] Modules linked in: ip6table_filter ip6_tables iptable_filter 
amdgpu x86_pkg_temp_thermal drm_ttm_helper ttm iommu_v2 gpu_sched ip_tables 
x_tables
[17359.837776] CR2: 
[17359.841888] ---[ end trace a6f27d64475b28c8 ]---
[17359.847318] RIP: 0010:amdgpu_ttm_backend_unbind+0x52/0x110 [amdgpu]
[17359.854479] Code: 48 39 c6 74 1b 8b 53 0c 48 8d bd 80 a1 ff ff e8 24 62 00 00 85 
c0 0f 85 ab 00 00 00 c6 43 54 00 5b 5d c3 48 8b 46 10 8b 4e 50 <48> 8b 30 48 85 
f6 74 ba 8b 50 0c 48 8b bf 80 a1 ff ff 83 e1 01 45
[17359.874929] RSP: 0018:a4764971fc98 EFLAGS: 00010206
[17359.881014] RAX:  RBX: 950e8d4edf00 RCX: 
[17359.889007] RDX:  RSI: 950e8d4edf00 RDI: 950eadec5e80
[17359.897008] RBP: 950eadec5e80 R08:  R09: 
[17359.905020] R10: 0246 R11: 0001 R12: 950c03377800
[17359.913034] R13: 950eadec5e80 R14: 950c03377858 R15: 
[17359.921050] FS:  7febb20cb740() GS:950ebfc0() 
knlGS:
[17359.930047] CS:  0010 DS:  ES:  CR0: 80050033
[17359.936674] CR2:  CR3: 0006d700e005 CR4: 001706e0

Reverting this patch fixes the problem for me.

Regards,
  Felix

On 2021-03-18 10:57 p.m., Alex Deucher wrote:

Applied.  Thanks!

Alex

On Thu, Mar 18, 2021 at 5:00 AM Koenig, Christian
 wrote:

Reviewed-by: Christian König 

Von: Daniel Gomez 
Gesendet: Donnerstag, 18. März 2021 09:32
Cc: dag...@gmail.com ; Daniel Gomez ; Deucher, Alexander 
; Koenig, Christian ; David Airlie ; Daniel 
Vetter ; amd-...@lists.freedesktop.org ; dri-de...@lists.freedesktop.org 
; linux-kernel@vger.kernel.org 
Betreff: [PATCH] drm/radeon/ttm: Fix memory leak userptr pages

If userptr pages have been pinned but not bounded,
they remain uncleared.

Signed-off-by: Daniel Gomez 
---
  drivers/gpu/drm/radeon/radeon_ttm.c | 5 +++--
  1 file 

Re: [PATCH v2 1/1] drm/amdkfd: fix build error with AMD_IOMMU_V2=m

[Felix Kuehling]

On 2021-03-09 11:50 a.m., Felix Kuehling wrote:

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 IS_REACHABLE to only build IOMMU-V2 support if the amd_iommu symbols
are reachable by the amdkfd driver. Output a warning if they are not,
because that may not be what the user was expecting.

Fixes: 64d1c3a43a6f ("drm/amdkfd: Centralize IOMMUv2 code and make it 
conditional")
Reported-by: Arnd Bergmann 
Signed-off-by: Felix Kuehling 

Ping. Can I get an R-b for this patch.

Thanks,
  Felix



---
  drivers/gpu/drm/amd/amdkfd/kfd_iommu.c | 6 ++
  drivers/gpu/drm/amd/amdkfd/kfd_iommu.h | 9 +++--
  2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
index 66bbca61e3ef..9318936aa805 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
@@ -20,6 +20,10 @@
   * OTHER DEALINGS IN THE SOFTWARE.
   */
  
+#include 

+
+#if IS_REACHABLE(CONFIG_AMD_IOMMU_V2)
+
  #include 
  #include 
  #include 
@@ -355,3 +359,5 @@ int kfd_iommu_add_perf_counters(struct kfd_topology_device 
*kdev)
  
  	return 0;

  }
+
+#endif
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h 
b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h
index dd23d9fdf6a8..afd420b01a0c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h
@@ -23,7 +23,9 @@
  #ifndef __KFD_IOMMU_H__
  #define __KFD_IOMMU_H__
  
-#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)

+#include 
+
+#if IS_REACHABLE(CONFIG_AMD_IOMMU_V2)
  
  #define KFD_SUPPORT_IOMMU_V2
  
@@ -46,6 +48,9 @@ static inline int kfd_iommu_check_device(struct kfd_dev *kfd)

  }
  static inline int kfd_iommu_device_init(struct kfd_dev *kfd)
  {
+#if IS_MODULE(CONFIG_AMD_IOMMU_V2)
+   WARN_ONCE(1, "iommu_v2 module is not usable by built-in KFD");
+#endif
return 0;
  }
  
@@ -73,6 +78,6 @@ static inline int kfd_iommu_add_perf_counters(struct kfd_topology_device *kdev)

return 0;
  }
  
-#endif /* defined(CONFIG_AMD_IOMMU_V2) */

+#endif /* IS_REACHABLE(CONFIG_AMD_IOMMU_V2) */
  
  #endif /* __KFD_IOMMU_H__ */

[PATCH v2 1/1] drm/amdkfd: fix build error with AMD_IOMMU_V2=m

[Felix Kuehling]

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 IS_REACHABLE to only build IOMMU-V2 support if the amd_iommu symbols
are reachable by the amdkfd driver. Output a warning if they are not,
because that may not be what the user was expecting.

Fixes: 64d1c3a43a6f ("drm/amdkfd: Centralize IOMMUv2 code and make it 
conditional")
Reported-by: Arnd Bergmann 
Signed-off-by: Felix Kuehling 
---
 drivers/gpu/drm/amd/amdkfd/kfd_iommu.c | 6 ++
 drivers/gpu/drm/amd/amdkfd/kfd_iommu.h | 9 +++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
index 66bbca61e3ef..9318936aa805 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
@@ -20,6 +20,10 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include 
+
+#if IS_REACHABLE(CONFIG_AMD_IOMMU_V2)
+
 #include 
 #include 
 #include 
@@ -355,3 +359,5 @@ int kfd_iommu_add_perf_counters(struct kfd_topology_device 
*kdev)
 
return 0;
 }
+
+#endif
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h 
b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h
index dd23d9fdf6a8..afd420b01a0c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h
@@ -23,7 +23,9 @@
 #ifndef __KFD_IOMMU_H__
 #define __KFD_IOMMU_H__
 
-#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
+#include 
+
+#if IS_REACHABLE(CONFIG_AMD_IOMMU_V2)
 
 #define KFD_SUPPORT_IOMMU_V2
 
@@ -46,6 +48,9 @@ static inline int kfd_iommu_check_device(struct kfd_dev *kfd)
 }
 static inline int kfd_iommu_device_init(struct kfd_dev *kfd)
 {
+#if IS_MODULE(CONFIG_AMD_IOMMU_V2)
+   WARN_ONCE(1, "iommu_v2 module is not usable by built-in KFD");
+#endif
return 0;
 }
 
@@ -73,6 +78,6 @@ static inline int kfd_iommu_add_perf_counters(struct 
kfd_topology_device *kdev)
return 0;
 }
 
-#endif /* defined(CONFIG_AMD_IOMMU_V2) */
+#endif /* IS_REACHABLE(CONFIG_AMD_IOMMU_V2) */
 
 #endif /* __KFD_IOMMU_H__ */
-- 
2.30.0

Re: [PATCH 1/1] drm/amdkfd: fix build error with AMD_IOMMU_V2=m

[Felix Kuehling]

Am 2021-03-09 um 3:58 a.m. schrieb Arnd Bergmann:
> On Tue, Mar 9, 2021 at 4:23 AM Felix Kuehling  wrote:
>> 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 IS_REACHABLE to only build IOMMU-V2 support if the amd_iommu symbols
>> are reachable by the amdkfd driver. Output a warning if they are not,
>> because that may not be what the user was expecting.
> This would fix the compile-time failure, but it still fails my CI
> because you introduce
> a compile-time warning. Can you change it into a runtime warning instead?

Sure.


>
> Neither type of warning is likely to actually reach the person trying
> to debug the
> problem, so you still end up with machines that don't do what they should,
> but I can live with the runtime warning as long as the build doesn't break.

OK.


>
> I think the proper fix would be to not rely on custom hooks into a particular
> IOMMU driver, but to instead ensure that the amdgpu driver can do everything
> it needs through the regular linux/iommu.h interfaces. I realize this
> is more work,
> but I wonder if you've tried that, and why it didn't work out.

As far as I know this hasn't been tried. I see that intel-iommu has its
own SVM thing, which seems to be similar to what our IOMMUv2 does. I
guess we'd have to abstract that into a common API.

Regards,
  Felix


>
>Arnd

[PATCH 1/1] drm/amdkfd: fix build error with AMD_IOMMU_V2=m

[Felix Kuehling]

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 IS_REACHABLE to only build IOMMU-V2 support if the amd_iommu symbols
are reachable by the amdkfd driver. Output a warning if they are not,
because that may not be what the user was expecting.

Fixes: 64d1c3a43a6f ("drm/amdkfd: Centralize IOMMUv2 code and make it 
conditional")
Reported-by: Arnd Bergmann 
Signed-off-by: Felix Kuehling 
---
 drivers/gpu/drm/amd/amdkfd/kfd_iommu.c | 10 ++
 drivers/gpu/drm/amd/amdkfd/kfd_iommu.h |  6 --
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
index 66bbca61e3ef..7199eb833f66 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
@@ -20,6 +20,10 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include 
+
+#if IS_REACHABLE(CONFIG_AMD_IOMMU_V2)
+
 #include 
 #include 
 #include 
@@ -355,3 +359,9 @@ int kfd_iommu_add_perf_counters(struct kfd_topology_device 
*kdev)
 
return 0;
 }
+
+#else
+
+#warning "Moldular IOMMU-V2 is not usable by built-in KFD"
+
+#endif
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h 
b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h
index dd23d9fdf6a8..b25365fc2c4e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.h
@@ -23,7 +23,9 @@
 #ifndef __KFD_IOMMU_H__
 #define __KFD_IOMMU_H__
 
-#if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2)
+#include 
+
+#if IS_REACHABLE(CONFIG_AMD_IOMMU_V2)
 
 #define KFD_SUPPORT_IOMMU_V2
 
@@ -73,6 +75,6 @@ static inline int kfd_iommu_add_perf_counters(struct 
kfd_topology_device *kdev)
return 0;
 }
 
-#endif /* defined(CONFIG_AMD_IOMMU_V2) */
+#endif /* IS_REACHABLE(CONFIG_AMD_IOMMU_V2) */
 
 #endif /* __KFD_IOMMU_H__ */
-- 
2.30.0

Re: [PATCH] [variant b] drm/amdkfd: fix build error with missing AMD_IOMMU_V2

[Felix Kuehling]

Am 2021-03-08 um 3:45 p.m. schrieb Arnd Bergmann:
> From: Arnd Bergmann 
>
> 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'
>
> Change the 'imply' to a weak dependency that still allows compiling
> in all other configurations but disallows the configuration that
> causes a link failure.

I don't like this solution. It hides the HSA_AMD option in menuconfig
and it's not intuitively obvious to someone configuring a kernel why
it's not available. They may not even notice that it's missing and then
wonder later, why KFD functionality is missing in their kernel.

What I'm trying to achieve is, that KFD can be compiled without
AMD-IOMMU-V2 support in this case. I just tested my version using
IS_REACHABLE. I'll reply with that patch in a moment.

Regards,
  Felix


>
> Fixes: 64d1c3a43a6f ("drm/amdkfd: Centralize IOMMUv2 code and make it 
> conditional")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/gpu/drm/amd/amdkfd/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/Kconfig 
> b/drivers/gpu/drm/amd/amdkfd/Kconfig
> index f02c938f75da..d01dba2af3bb 100644
> --- a/drivers/gpu/drm/amd/amdkfd/Kconfig
> +++ b/drivers/gpu/drm/amd/amdkfd/Kconfig
> @@ -6,7 +6,7 @@
>  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 AMD_IOMMU_V2=y || DRM_AMDGPU=m
>   select HMM_MIRROR
>   select MMU_NOTIFIER
>   select DRM_AMDGPU_USERPTR

Re: [PATCH] drm/amdkfd: fix build error with missing AMD_IOMMU_V2

[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  wrote:
>> Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann:
>>> On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling  
>>> 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
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amdkfd: fix build error with missing AMD_IOMMU_V2

[Felix Kuehling]

Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann:
> On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling  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

Re: [PATCH] drm/amdkfd: fix build error with missing AMD_IOMMU_V2

[Felix Kuehling]

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 
>
> 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 
> ---
>  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

Re: [PATCH] drm/amdkfd: dqm fence memory corruption

[Felix Kuehling]

Am 2021-01-27 um 7:33 a.m. schrieb Qu Huang:
> Amdgpu driver uses 4-byte data type as DQM fence memory,
> and transmits GPU address of fence memory to microcode
> through query status PM4 message. However, query status
> PM4 message definition and microcode processing are all
> processed according to 8 bytes. Fence memory only allocates
> 4 bytes of memory, but microcode does write 8 bytes of memory,
> so there is a memory corruption.

Thank you for pointing out that discrepancy. That's a good catch!

I'd prefer to fix this properly by making dqm->fence_addr a u64 pointer.
We should probably also fix up the query_status and
amdkfd_fence_wait_timeout function interfaces to use a 64 bit fence
values everywhere to be consistent.

Regards,
  Felix


>
> Signed-off-by: Qu Huang 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index e686ce2..8b38d0c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -1161,7 +1161,7 @@ static int start_cpsch(struct device_queue_manager *dqm)
>   pr_debug("Allocating fence memory\n");
>  
>   /* allocate fence memory on the gart */
> - retval = kfd_gtt_sa_allocate(dqm->dev, sizeof(*dqm->fence_addr),
> + retval = kfd_gtt_sa_allocate(dqm->dev, sizeof(uint64_t),
>   >fence_mem);
>  
>   if (retval)

Re: [PATCH v2] drm/amdkfd: Fix out-of-bounds read in kdf_create_vcrat_image_cpu()

[Felix Kuehling]

Am 2021-01-11 um 4:05 p.m. schrieb Jeremy Cline:
> KASAN reported a slab-out-of-bounds read of size 1 in
> kdf_create_vcrat_image_cpu().
>
> This occurs when, for example, when on an x86_64 with a single NUMA node
> because kfd_fill_iolink_info_for_cpu() is a no-op, but afterwards the
> sub_type_hdr->length, which is out-of-bounds, is read and multiplied by
> entries. Fortunately, entries is 0 in this case so the overall
> crat_table->length is still correct.
>
> Check if there were any entries before de-referencing sub_type_hdr which
> may be pointing to out-of-bounds memory.
>
> Fixes: b7b6c38529c9 ("drm/amdkfd: Calculate CPU VCRAT size dynamically (v2)")
> Suggested-by: Felix Kuehling 
> Signed-off-by: Jeremy Cline 

Thanks. I'll apply this patch.

Reviewed-by: Felix Kuehling 


> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> index 8cac497c2c45..a5640a6138cf 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> @@ -1040,11 +1040,14 @@ static int kfd_create_vcrat_image_cpu(void 
> *pcrat_image, size_t *size)
>   (struct crat_subtype_iolink *)sub_type_hdr);
>   if (ret < 0)
>   return ret;
> - crat_table->length += (sub_type_hdr->length * entries);
> - crat_table->total_entries += entries;
>  
> - sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
> - sub_type_hdr->length * entries);
> + if (entries) {
> + crat_table->length += (sub_type_hdr->length * entries);
> + crat_table->total_entries += entries;
> +
> + sub_type_hdr = (typeof(sub_type_hdr))((char 
> *)sub_type_hdr +
> + sub_type_hdr->length * entries);
> + }
>  #else
>   pr_info("IO link not available for non x86 platforms\n");
>  #endif

Re: [PATCH] drm/amdkfd: Fix out-of-bounds read in kdf_create_vcrat_image_cpu()

[Felix Kuehling]

Am 2021-01-08 um 11:31 a.m. schrieb Jeremy Cline:
> KASAN reported a slab-out-of-bounds read of size 1 in
> kdf_create_vcrat_image_cpu().
>
> This occurs when, for example, when on an x86_64 with a single NUMA node
> because kfd_fill_iolink_info_for_cpu() is a no-op, but afterwards the
> sub_type_hdr->length, which is out-of-bounds, is read and multiplied by
> entries. Fortunately, entries is 0 in this case so the overall
> crat_table->length is still correct.

That's a pretty big change to fix that. Wouldn't it be enough to add a
simple check after calling kfd_fill_iolink_info_for_cpu:

if (entries) {
crat_table->length += (sub_type_hdr->length * entries);
crat_table->total_entries += entries;
}

Or change the output parameters of the kfd_fill_..._for_cpu functions
from num_entries to size_filled, so the caller doesn't need to read
sub_type_hdr->length any more.

Regards,
  Felix


>
> This refactors the helper functions to accept the crat_table directly
> and calculate the table entry pointer based on the current table length.
> This allows us to avoid an out-of-bounds read and hopefully makes the
> pointer arithmetic clearer. It should have no functional change beyond
> removing the out-of-bounds read.
>
> Fixes: b7b6c38529c9 ("drm/amdkfd: Calculate CPU VCRAT size dynamically (v2)")
> Signed-off-by: Jeremy Cline 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 86 +--
>  1 file changed, 40 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> index 8cac497c2c45..e50db2c0f4ee 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> @@ -829,21 +829,24 @@ int kfd_create_crat_image_acpi(void **crat_image, 
> size_t *size)
>  /* kfd_fill_cu_for_cpu - Fill in Compute info for the given CPU NUMA node
>   *
>   *   @numa_node_id: CPU NUMA node id
> - *   @avail_size: Available size in the memory
> - *   @sub_type_hdr: Memory into which compute info will be filled in
> + *   @avail_size: Available space in bytes at the end of the @crat_table.
> + *   @crat_table: The CRAT table to append the Compute info to;
> + *   on success the table length and total_entries count is updated.
>   *
>   *   Return 0 if successful else return -ve value
>   */
>  static int kfd_fill_cu_for_cpu(int numa_node_id, int *avail_size,
> - int proximity_domain,
> - struct crat_subtype_computeunit *sub_type_hdr)
> + struct crat_header *crat_table)
>  {
>   const struct cpumask *cpumask;
> + struct crat_subtype_computeunit *sub_type_hdr;
>  
>   *avail_size -= sizeof(struct crat_subtype_computeunit);
>   if (*avail_size < 0)
>   return -ENOMEM;
>  
> + sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table +
> + crat_table->length);
>   memset(sub_type_hdr, 0, sizeof(struct crat_subtype_computeunit));
>  
>   /* Fill in subtype header data */
> @@ -855,36 +858,42 @@ static int kfd_fill_cu_for_cpu(int numa_node_id, int 
> *avail_size,
>  
>   /* Fill in CU data */
>   sub_type_hdr->flags |= CRAT_CU_FLAGS_CPU_PRESENT;
> - sub_type_hdr->proximity_domain = proximity_domain;
> + sub_type_hdr->proximity_domain = crat_table->num_domains;
>   sub_type_hdr->processor_id_low = kfd_numa_node_to_apic_id(numa_node_id);
>   if (sub_type_hdr->processor_id_low == -1)
>   return -EINVAL;
>  
>   sub_type_hdr->num_cpu_cores = cpumask_weight(cpumask);
>  
> + crat_table->length += sub_type_hdr->length;
> + crat_table->total_entries++;
> +
>   return 0;
>  }
>  
>  /* kfd_fill_mem_info_for_cpu - Fill in Memory info for the given CPU NUMA 
> node
>   *
>   *   @numa_node_id: CPU NUMA node id
> - *   @avail_size: Available size in the memory
> - *   @sub_type_hdr: Memory into which compute info will be filled in
> + *   @avail_size: Available space in bytes at the end of the @crat_table.
> + *   @crat_table: The CRAT table to append the Memory info to;
> + *   on success the table length and total_entries count is updated.
>   *
>   *   Return 0 if successful else return -ve value
>   */
>  static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size,
> - int proximity_domain,
> - struct crat_subtype_memory *sub_type_hdr)
> + struct crat_header *crat_table)
>  {
>   uint64_t mem_in_bytes = 0;
>   pg_data_t *pgdat;
>   int zone_type;
> + struct crat_subtype_memory *sub_type_hdr;
>  
>   *avail_size -= sizeof(struct crat_subtype_memory);
>   if (*avail_size < 0)
>   return -ENOMEM;
>  
> + sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table +
> + crat_table->length);
>   memset(sub_type_hdr, 0, sizeof(struct crat_subtype_memory));
>  
>   /* Fill in subtype 

Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as broken

[Felix Kuehling]

On 2020-11-23 5:33 p.m., Will Deacon wrote:

On Mon, Nov 23, 2020 at 09:04:14PM +, Deucher, Alexander wrote:

[AMD Public Use]


-Original Message-
From: Will Deacon 
Sent: Monday, November 23, 2020 8:44 AM
To: linux-kernel@vger.kernel.org
Cc: linux-...@vger.kernel.org; io...@lists.linux-foundation.org; Will
Deacon ; Bjorn Helgaas ;
Deucher, Alexander ; Edgar Merger
; Joerg Roedel 
Subject: [PATCH] PCI: Mark AMD Raven iGPU ATS as broken

Edgar Merger reports that the AMD Raven GPU does not work reliably on his
system when the IOMMU is enabled:

   | [drm:amdgpu_job_timedout [amdgpu]] *ERROR* ring gfx timeout,
signaled seq=1, emitted seq=3
   | [...]
   | amdgpu :0b:00.0: GPU reset begin!
   | AMD-Vi: Completion-Wait loop timed out
   | iommu ivhd0: AMD-Vi: Event logged [IOTLB_INV_TIMEOUT
device=0b:00.0 address=0x38edc0970]

This is indicative of a hardware/platform configuration issue so, since
disabling ATS has been shown to resolve the problem, add a quirk to match
this particular device while Edgar follows-up with AMD for more information.

Cc: Bjorn Helgaas 
Cc: Alex Deucher 
Reported-by: Edgar Merger 
Suggested-by: Joerg Roedel 
Link:
https://lore.
kernel.org/linux-
iommu/MWHPR10MB1310F042A30661D4158520B589FC0@MWHPR10M
B1310.namprd10.prod.outlook.com
her%40amd.com%7C1a883fe14d0c408e7d9508d88fb5df4e%7C3dd8961fe488
4e608e11a82d994e183d%7C0%7C0%7C637417358593629699%7CUnknown%7
CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
LCJXVCI6Mn0%3D%7C1000sdata=TMgKldWzsX8XZ0l7q3%2BszDWXQJJ
LOUfX5oGaoLN8n%2B8%3Dreserved=0
Signed-off-by: Will Deacon 
---

Hi all,

Since Joerg is away at the moment, I'm posting this to try to make some
progress with the thread in the Link: tag.

+ Felix

What system is this?  Can you provide more details?  Does a sbios update
fix this?  Disabling ATS for all Ravens will break GPU compute for a lot
of people.  I'd prefer to just black list this particular system (e.g.,
just SSIDs or revision) if possible.


+Ray

There are already many systems where the IOMMU is disabled in the BIOS, 
or the CRAT table reporting the APU compute capabilities is broken. Ray 
has been working on a fallback to make APUs behave like dGPUs on such 
systems. That should also cover this case where ATS is blacklisted. That 
said, it affects the programming model, because we don't support the 
unified and coherent memory model on dGPUs like we do on APUs with 
IOMMUv2. So it would be good to make the conditions for this workaround 
as narrow as possible.


These are the relevant changes in KFD and Thunk for reference:

### KFD ###

commit 914913ab04dfbcd0226ecb6bc99d276832ea2908
Author: Huang Rui 
Date:   Tue Aug 18 14:54:23 2020 +0800

    drm/amdkfd: implement the dGPU fallback path for apu (v6)

    We still have a few iommu issues which need to address, so force raven
    as "dgpu" path for the moment.

    This is to add the fallback path to bypass IOMMU if IOMMU v2 is 
disabled

    or ACPI CRAT table not correct.

    v2: Use ignore_crat parameter to decide whether it will go with 
IOMMUv2.
    v3: Align with existed thunk, don't change the way of raven, only 
renoir

    will use "dgpu" path by default.
    v4: don't update global ignore_crat in the driver, and revise fallback
    function if CRAT is broken.
    v5: refine acpi crat good but no iommu support case, and rename the
    title.
    v6: fix the issue of dGPU initialized firstly, just modify the report
    value in the node_show().

    Signed-off-by: Huang Rui 
    Reviewed-by: Felix Kuehling 
    Signed-off-by: Alex Deucher 

### Thunk ###

commit e32482fa4b9ca398c8bdc303920abfd672592764
Author: Huang Rui 
Date:   Tue Aug 18 18:54:05 2020 +0800

    libhsakmt: remove is_dgpu flag in the hsa_gfxip_table

    Whether use dgpu path will check the props which exposed from kernel.
    We won't need hard code in the ASIC table.

    Signed-off-by: Huang Rui 
    Change-Id: I0c018a26b219914a41197ff36dbec7a75945d452

commit 7c60f6d912034aa67ed27b47a29221422423f5cc
Author: Huang Rui 
Date:   Thu Jul 30 10:22:23 2020 +0800

    libhsakmt: implement the method that using flag which exposed by 
kfd to configure is_dgpu


    KFD already implemented the fallback path for APU. Thunk will use flag
    which exposed by kfd to configure is_dgpu instead of hardcode before.

    Signed-off-by: Huang Rui 
    Change-Id: I445f6cf668f9484dd06cd9ae1bb3cfe7428ec7eb

Regards,
  Felix



Cheers, Alex. I'll have to defer to Edgar for the details, as my
understanding from the original thread over at:

https://lore.kernel.org/linux-iommu/mwhpr10mb1310cdb6829ddcf5ea84a14689...@mwhpr10mb1310.namprd10.prod.outlook.com/

is that this is a board developed by his company.

Edgar -- please can you answer Alex's questions?

Will

Re: [PATCH v3 1/2] drm/amdkfd: Move the ignore_crat check before the CRAT table get

[Felix Kuehling]

Am 2020-11-12 um 10:11 p.m. schrieb Hanjun Guo:
> If the ignore_crat is set to non-zero value, it's no point getting
> the CRAT table, so just move the ignore_crat check before we get the
> CRAT table.
>
> Signed-off-by: Hanjun Guo 

Thank you! I applied the patches.

Regards,
  Felix


> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> index 3de5e14..c23e571 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> @@ -780,6 +780,11 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t 
> *size)
>  
>   *crat_image = NULL;
>  
> + if (kfd_ignore_crat()) {
> + pr_info("CRAT table disabled by module option\n");
> + return -ENODATA;
> + }
> +
>   /* Fetch the CRAT table from ACPI */
>   status = acpi_get_table(CRAT_SIGNATURE, 0, _table);
>   if (status == AE_NOT_FOUND) {
> @@ -792,11 +797,6 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t 
> *size)
>   return -EINVAL;
>   }
>  
> - if (kfd_ignore_crat()) {
> - pr_info("CRAT table disabled by module option\n");
> - return -ENODATA;
> - }
> -
>   pcrat_image = kvmalloc(crat_table->length, GFP_KERNEL);
>   if (!pcrat_image)
>   return -ENOMEM;

Re: [PATCH] drm/amdkfd: replace idr_init() by idr_init_base()

[Felix Kuehling]

On 2020-11-04 10:13 a.m., Deepak R Varma wrote:

idr_init() uses base 0 which is an invalid identifier. The new function
idr_init_base allows IDR to set the ID lookup from base 1. This avoids
all lookups that otherwise starts from 0 since 0 is always unused.


I disagree. We call idr_alloc with start=0 for both these IDRs. That 
means 0 seems to be a valid handle.


Regards,
  Felix




References: commit 6ce711f27500 ("idr: Make 1-based IDRs more efficient")

Signed-off-by: Deepak R Varma 
---
  drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 2 +-
  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index ba2c2ce0c55a..b3339b53c8ad 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -230,7 +230,7 @@ static int create_other_event(struct kfd_process *p, struct 
kfd_event *ev)
  void kfd_event_init_process(struct kfd_process *p)
  {
mutex_init(>event_mutex);
-   idr_init(>event_idr);
+   idr_init_base(>event_idr, 1);
p->signal_page = NULL;
p->signal_event_count = 0;
  }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 65803e153a22..022e61babe30 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -1289,7 +1289,7 @@ struct kfd_process_device 
*kfd_create_process_device_data(struct kfd_dev *dev,
list_add(>per_device_list, >per_device_data);
  
  	/* Init idr used for memory handle translation */

-   idr_init(>alloc_idr);
+   idr_init_base(>alloc_idr, 1);
  
  	return pdd;
  

Re: [PATCH v2 -next] drm/amdkfd: Fix -Wunused-const-variable warning

[Felix Kuehling]

Am 2020-09-10 um 3:50 a.m. schrieb YueHaibing:
> If KFD_SUPPORT_IOMMU_V2 is not set, gcc warns:
>
> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device.c:121:37: warning: 
> ‘raven_device_info’ defined but not used [-Wunused-const-variable=]
>  static const struct kfd_device_info raven_device_info = {
>  ^
>
> As Huang Rui suggested, Raven already has the fallback path,
> so it should be out of IOMMU v2 flag.
>
> Suggested-by: Huang Rui 
> Signed-off-by: YueHaibing 

Reviewed-by: Felix Kuehling 

I applied your patch to the amd-staging-drm-next branch.

Thank you,
  Felix

> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 0e71a0543f98..e3fc6ed7b79c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -503,8 +503,8 @@ static const struct kfd_device_info 
> *kfd_supported_devices[][2] = {
>  #ifdef KFD_SUPPORT_IOMMU_V2
>   [CHIP_KAVERI] = {_device_info, NULL},
>   [CHIP_CARRIZO] = {_device_info, NULL},
> - [CHIP_RAVEN] = {_device_info, NULL},
>  #endif
> + [CHIP_RAVEN] = {_device_info, NULL},
>   [CHIP_HAWAII] = {_device_info, NULL},
>   [CHIP_TONGA] = {_device_info, NULL},
>   [CHIP_FIJI] = {_device_info, _vf_device_info},

Re: [PATCH 0/2] iommu/amd: Fix IOMMUv2 devices when SME is active

[Felix Kuehling]

Am 2020-09-06 um 12:08 p.m. schrieb Deucher, Alexander:
> [AMD Official Use Only - Internal Distribution Only]
>
>> -Original Message-
>> From: Joerg Roedel 
>> Sent: Friday, September 4, 2020 6:06 AM
>> To: Deucher, Alexander 
>> Cc: jroe...@suse.de; Kuehling, Felix ;
>> io...@lists.linux-foundation.org; Huang, Ray ;
>> Koenig, Christian ; Lendacky, Thomas
>> ; Suthikulpanit, Suravee
>> ; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 0/2] iommu/amd: Fix IOMMUv2 devices when SME is
>> active
>>
>> On Fri, Aug 28, 2020 at 03:47:07PM +, Deucher, Alexander wrote:
>>> Ah, right,  So CZ and ST are not an issue.  Raven is paired with Zen based
>> CPUs.
>>
>> Okay, so for the Raven case, can you add code to the amdgpu driver which
>> makes it fail to initialize on Raven when SME is active? There is a global
>> checking function for that, so that shouldn't be hard to do.
>>
> Sure.  How about the attached patch?

The patch is

Acked-by: Felix Kuehling 

Thanks,
  Felix


>
> Alex
>

Re: [PATCH 0/2] iommu/amd: Fix IOMMUv2 devices when SME is active

[Felix Kuehling]

Am 2020-08-28 um 9:46 a.m. schrieb jroe...@suse.de:
> On Wed, Aug 26, 2020 at 03:25:58PM +, Deucher, Alexander wrote:
>>> Alex, do you know if anyone has tested amdgpu on an APU with SME
>>> enabled? Is this considered something we support?
>> It's not something we've tested.  I'm not even sure the GPU portion of
>> APUs will work properly without an identity mapping.  SME should work
>> properly with dGPUs however, so this is a proper fix for them.  We
>> don't use the IOMMUv2 path on dGPUs at all.
> Is it possible to make the IOMMUv2 paths optional on iGPUs as well when
> SME is active (or better, when the GPU is not identity mapped)?

Yes, we're working on this. IOMMUv2 is only needed for KFD. It's not
needed for graphics. And we're making it optional for KFD as well.

The question Alex and I raised here is more general. We may have some
assumptions in the amdgpu driver that are broken when the framebuffer is
not identity mapped. This would break the iGPU in a more general sense,
regardless of KFD and IOMMUv2. In that case, we don't really need to
worry about breaking KFD because we have a much bigger problem.

Regards,
  Felix


>
> Regards,
>
>   Joerg

Re: [PATCH v2 1/2] drm/amdkfd: Move the ignore_crat check before the CRAT table get

[Felix Kuehling]

Am 2020-08-26 um 4:29 a.m. schrieb Hanjun Guo:
> If the ignore_crat is set to non-zero value, it's no point getting
> the CRAT table, so just move the ignore_crat check before we get the
> CRAT table.
>
> Signed-off-by: Hanjun Guo 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> index 6a250f8..ed77b109 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> @@ -764,6 +764,11 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t 
> *size)
>  
>   *crat_image = NULL;
>  
> + if (ignore_crat) {

A conflicting change in this area was just submitted on Monday to
amd-staging-drm-next. You'll need to rebase your patch. It should be
straight-forward. ignore_crat was replaced with a function call
kfd_ignore_crat().

Other than that, your patch series looks good to me. If I don't see an
update from you in a day or two, I'll fix it up myself and add my R-b.

Thanks,
  Felix


> + pr_info("CRAT table disabled by module option\n");
> + return -ENODATA;
> + }
> +
>   /* Fetch the CRAT table from ACPI */
>   status = acpi_get_table(CRAT_SIGNATURE, 0, _table);
>   if (status == AE_NOT_FOUND) {
> @@ -776,11 +781,6 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t 
> *size)
>   return -EINVAL;
>   }
>  
> - if (ignore_crat) {
> - pr_info("CRAT table disabled by module option\n");
> - return -ENODATA;
> - }
> -
>   pcrat_image = kmemdup(crat_table, crat_table->length, GFP_KERNEL);
>   if (!pcrat_image)
>   return -ENOMEM;

Re: [PATCH 0/2] iommu/amd: Fix IOMMUv2 devices when SME is active

[Felix Kuehling]

[+Ray]


Thanks for the heads up. Currently KFD won't work on APUs when IOMMUv2
is disabled. But Ray is working on fallbacks that will allow KFD to work
on APUs even without IOMMUv2, similar to our dGPUs. Along with changes
in ROCm user mode, those fallbacks are necessary for making ROCm on APUs
generally useful.


How common is SME on typical PCs or laptops that would use AMD APUs?


Alex, do you know if anyone has tested amdgpu on an APU with SME
enabled? Is this considered something we support?


Thanks,
  Felix


Am 2020-08-26 um 10:14 a.m. schrieb Deucher, Alexander:
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
> + Felix
> 
> *From:* Joerg Roedel 
> *Sent:* Monday, August 24, 2020 6:54 AM
> *To:* io...@lists.linux-foundation.org 
> *Cc:* Joerg Roedel ; jroe...@suse.de
> ; Lendacky, Thomas ;
> Suthikulpanit, Suravee ; Deucher,
> Alexander ; linux-kernel@vger.kernel.org
> 
> *Subject:* [PATCH 0/2] iommu/amd: Fix IOMMUv2 devices when SME is active
>  
> From: Joerg Roedel 
>
> Hi,
>
> Some IOMMUv2 capable devices do not work correctly when SME is
> active, because their DMA mask does not include the encryption bit, so
> that they can not DMA to encrypted memory directly.
>
> The IOMMU can jump in here, but the AMD IOMMU driver puts IOMMUv2
> capable devices into an identity mapped domain. Fix that by not
> forcing an identity mapped domain on devices when SME is active and
> forbid using their IOMMUv2 functionality.
>
> Please review.
>
> Thanks,
>
>     Joerg
>
> Joerg Roedel (2):
>   iommu/amd: Do not force direct mapping when SME is active
>   iommu/amd: Do not use IOMMUv2 functionality when SME is active
>
>  drivers/iommu/amd/iommu.c    | 7 ++-
>  drivers/iommu/amd/iommu_v2.c | 7 +++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> -- 
> 2.28.0
>

Re: [PATCH] drm/amdkfd: Put ACPI table after using it

[Felix Kuehling]

Hi Hanjun,

Sorry for the late reply.

Thank you for the patch and the explanation. This seems to have been
broken since the first version of KFD in 2014. See one suggestion inline.

Am 2020-07-22 um 5:48 a.m. schrieb Hanjun Guo:
> The acpi_get_table() should be coupled with acpi_put_table() if
> the mapped table is not used at runtime to release the table
> mapping.
>
> In kfd_create_crat_image_acpi(), crat_table is copied to pcrat_image,
> and in kfd_create_vcrat_image_cpu(), the acpi_table is only used to
> get the OEM info, so those table mappings need to be release after
> using it.
>
> Signed-off-by: Hanjun Guo 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 16 +++-
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> index 1009a3b..d378e61 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> @@ -756,6 +756,7 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t 
> *size)
>   struct acpi_table_header *crat_table;
>   acpi_status status;
>   void *pcrat_image;
> + int rc = 0;
>  
>   if (!crat_image)
>   return -EINVAL;
> @@ -776,17 +777,21 @@ int kfd_create_crat_image_acpi(void **crat_image, 
> size_t *size)
>  
>   if (ignore_crat) {
>   pr_info("CRAT table disabled by module option\n");

We should probably move this check to before we get the CRAT table.
There is not point getting and putting it if we're going to ignore it
anyway.

Do you want to send an updated patch with that change? Or maybe do it as
a 2-patch series?

Regards,
  Felix


> - return -ENODATA;
> + rc = -ENODATA;
> + goto out;
>   }
>  
>   pcrat_image = kmemdup(crat_table, crat_table->length, GFP_KERNEL);
> - if (!pcrat_image)
> - return -ENOMEM;
> + if (!pcrat_image) {
> + rc = -ENOMEM;
> + goto out;
> + }
>  
>   *crat_image = pcrat_image;
>   *size = crat_table->length;
> -
> - return 0;
> +out:
> + acpi_put_table(crat_table);
> + return rc;
>  }
>  
>  /* Memory required to create Virtual CRAT.
> @@ -970,6 +975,7 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, 
> size_t *size)
>   CRAT_OEMID_LENGTH);
>   memcpy(crat_table->oem_table_id, acpi_table->oem_table_id,
>   CRAT_OEMTABLEID_LENGTH);
> + acpi_put_table(acpi_table);
>   }
>   crat_table->total_entries = 0;
>   crat_table->num_domains = 0;

Re: [PATCH v5 01/12] iommu: Change type of pasid to u32

[Felix Kuehling]

Am 2020-07-02 um 3:10 p.m. schrieb Fenghua Yu:
> Hi, Felix, Thomas, Joerg and maintainers,
>
> On Tue, Jun 30, 2020 at 10:12:38PM -0400, Felix Kuehling wrote:
>> Am 2020-06-30 um 7:44 p.m. schrieb Fenghua Yu:
>> You didn't change the return types of amdgpu_pasid_alloc and
>> kfd_pasid_alloc. amdgpu_pasid_alloc returns int, because it can return
>> negative error codes. But kfd_pasid_alloc could be updated, because it
>> returns 0 for errors.
> I fixed return type as "u32" for kfd_pasid_alloc().

Thank you. The patch is

Acked-by: Felix Kuehling 



>
> The fix is minor and only limited in patch 1. So instead of sending the
> whole series, I only send the updated patch 1 here. If you want me to
> send the whole series with the fix, I can do that too.
>
> Thanks.
>
> -Fenghua
>
> From 4ff6c14bb0761dd97d803350d31f87edc4336345 Mon Sep 17 00:00:00 2001
> From: Fenghua Yu 
> Date: Mon, 4 May 2020 18:00:55 +
> Subject: [PATCH v5.1 01/12] iommu: Change type of pasid to u32
>
> PASID is defined as a few different types in iommu including "int",
> "u32", and "unsigned int". To be consistent and to match with uapi
> definitions, define PASID and its variations (e.g. max PASID) as "u32".
> "u32" is also shorter and a little more explicit than "unsigned int".
>
> No PASID type change in uapi although it defines PASID as __u64 in
> some places.
>
> Suggested-by: Thomas Gleixner 
> Signed-off-by: Fenghua Yu 
> Reviewed-by: Tony Luck 
> Reviewed-by: Lu Baolu 
> ---
> v5.1:
> - Change return type to u32 for kfd_pasid_alloc() (Felix)
>
> v5:
> - Reviewed by Lu Baolu
>
> v4:
> - Change PASID type from "unsigned int" to "u32" (Christoph)
>
> v2:
> - Create this new patch to define PASID as "unsigned int" consistently in
>   iommu (Thomas)
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h|  4 +--
>  .../drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c|  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h |  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  4 +--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c   |  6 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h   |  4 +--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c|  8 ++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h|  8 ++---
>  .../gpu/drm/amd/amdkfd/cik_event_interrupt.c  |  2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c   |  2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_dbgmgr.h   |  2 +-
>  .../drm/amd/amdkfd/kfd_device_queue_manager.c |  7 ++---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c   |  8 ++---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.h   |  4 +--
>  drivers/gpu/drm/amd/amdkfd/kfd_iommu.c|  6 ++--
>  drivers/gpu/drm/amd/amdkfd/kfd_pasid.c|  4 +--
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 20 ++--
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c  |  2 +-
>  .../gpu/drm/amd/include/kgd_kfd_interface.h   |  2 +-
>  drivers/iommu/amd/amd_iommu.h | 10 +++---
>  drivers/iommu/amd/iommu.c | 31 ++-
>  drivers/iommu/amd/iommu_v2.c  | 20 ++--
>  drivers/iommu/intel/dmar.c|  7 +++--
>  drivers/iommu/intel/intel-pasid.h | 24 +++---
>  drivers/iommu/intel/iommu.c   |  4 +--
>  drivers/iommu/intel/pasid.c   | 31 +--
>  drivers/iommu/intel/svm.c | 12 +++
>  drivers/iommu/iommu.c |  2 +-
>  drivers/misc/uacce/uacce.c|  2 +-
>  include/linux/amd-iommu.h |  8 ++---
>  include/linux/intel-iommu.h   | 12 +++
>  include/linux/intel-svm.h |  2 +-
>  include/linux/iommu.h | 10 +++---
>  include/linux/uacce.h |  2 +-
>  38 files changed, 141 insertions(+), 141 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index ffe149aafc39..dfef5a7e0f5a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -207,11 +207,11 @@ uint8_t amdgpu_amdkfd_get_xgmi_hops_count(struct 
> kgd_dev *dst, struct kgd_dev *s
>   })
>  
>  /* GPUVM API */
> -int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, unsigned int 
> pasid,
>

Re: [PATCH v5 01/12] iommu: Change type of pasid to u32

[Felix Kuehling]

Am 2020-06-30 um 7:44 p.m. schrieb Fenghua Yu:
> PASID is defined as a few different types in iommu including "int",
> "u32", and "unsigned int". To be consistent and to match with uapi
> definitions, define PASID and its variations (e.g. max PASID) as "u32".
> "u32" is also shorter and a little more explicit than "unsigned int".

You didn't change the return types of amdgpu_pasid_alloc and
kfd_pasid_alloc. amdgpu_pasid_alloc returns int, because it can return
negative error codes. But kfd_pasid_alloc could be updated, because it
returns 0 for errors.

Regards,
  Felix

>
> No PASID type change in uapi although it defines PASID as __u64 in
> some places.
>
> Suggested-by: Thomas Gleixner 
> Signed-off-by: Fenghua Yu 
> Reviewed-by: Tony Luck 
> Reviewed-by: Lu Baolu 
> ---
> v5:
> - Reviewed by Lu Baolu
>
> v4:
> - Change PASID type from "unsigned int" to "u32" (Christoph)
>
> v2:
> - Create this new patch to define PASID as "unsigned int" consistently in
>   iommu (Thomas)
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h|  4 +--
>  .../drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c|  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h |  2 +-
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  4 +--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c   |  6 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h   |  4 +--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c|  8 ++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h|  8 ++---
>  .../gpu/drm/amd/amdkfd/cik_event_interrupt.c  |  2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c   |  2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_dbgmgr.h   |  2 +-
>  .../drm/amd/amdkfd/kfd_device_queue_manager.c |  7 ++---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c   |  8 ++---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.h   |  4 +--
>  drivers/gpu/drm/amd/amdkfd/kfd_iommu.c|  6 ++--
>  drivers/gpu/drm/amd/amdkfd/kfd_pasid.c|  2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 18 +--
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c  |  2 +-
>  .../gpu/drm/amd/include/kgd_kfd_interface.h   |  2 +-
>  drivers/iommu/amd/amd_iommu.h | 10 +++---
>  drivers/iommu/amd/iommu.c | 31 ++-
>  drivers/iommu/amd/iommu_v2.c  | 20 ++--
>  drivers/iommu/intel/dmar.c|  7 +++--
>  drivers/iommu/intel/intel-pasid.h | 24 +++---
>  drivers/iommu/intel/iommu.c   |  4 +--
>  drivers/iommu/intel/pasid.c   | 31 +--
>  drivers/iommu/intel/svm.c | 12 +++
>  drivers/iommu/iommu.c |  2 +-
>  drivers/misc/uacce/uacce.c|  2 +-
>  include/linux/amd-iommu.h |  8 ++---
>  include/linux/intel-iommu.h   | 12 +++
>  include/linux/intel-svm.h |  2 +-
>  include/linux/iommu.h | 10 +++---
>  include/linux/uacce.h |  2 +-
>  38 files changed, 139 insertions(+), 139 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index ffe149aafc39..dfef5a7e0f5a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -207,11 +207,11 @@ uint8_t amdgpu_amdkfd_get_xgmi_hops_count(struct 
> kgd_dev *dst, struct kgd_dev *s
>   })
>  
>  /* GPUVM API */
> -int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, unsigned int 
> pasid,
> +int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, u32 pasid,
>   void **vm, void **process_info,
>   struct dma_fence **ef);
>  int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
> - struct file *filp, unsigned int pasid,
> + struct file *filp, u32 pasid,
>   void **vm, void **process_info,
>   struct dma_fence **ef);
>  void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> index bf927f432506..ee531c3988d1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> @@ -105,7 +105,7 @@ static void kgd_program_sh_mem_settings(struct kgd_dev 
> *kgd, uint32_t vmid,
>   unlock_srbm(kgd);
>  }
>  
> -static int kgd_set_pasid_vmid_mapping(struct kgd_dev *kgd, unsigned int 
> pasid,
> +static int kgd_set_pasid_vmid_mapping(struct kgd_dev 

Re: [PATCH v2] drm/amd: fix potential memleak in err branch

[Felix Kuehling]

Am 2020-06-20 um 4:54 a.m. schrieb Bernard Zhao:
> The function kobject_init_and_add alloc memory like:
> kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs
> ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller
> ->kmalloc_slab, in err branch this memory not free. If use
> kmemleak, this path maybe catched.
> These changes are to add kobject_put in kobject_init_and_add
> failed branch, fix potential memleak.
>
> Signed-off-by: Bernard Zhao 

The patch is

Reviewed-by: Felix Kuehling 

I'll apply it to amd-staging-drm-next.

Thanks,
  Felix

> ---
> Changes since V1:
> *Remove duplicate changed file kfd_topology.c, this file`s fix
> already applied to the main line.
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index d27221ddcdeb..5ee4d6cfb16d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -124,6 +124,7 @@ void kfd_procfs_init(void)
>   if (ret) {
>   pr_warn("Could not create procfs proc folder");
>   /* If we fail to create the procfs, clean up */
> + kobject_put(procfs.kobj);
>   kfd_procfs_shutdown();
>   }
>  }
> @@ -428,6 +429,7 @@ struct kfd_process *kfd_create_process(struct file *filep)
>  (int)process->lead_thread->pid);
>   if (ret) {
>   pr_warn("Creating procfs pid directory failed");
> + kobject_put(process->kobj);
>   goto out;
>   }
>  

Re: [Linaro-mm-sig] [PATCH 04/18] dma-fence: prime lockdep annotations

[Felix Kuehling]

Am 2020-06-23 um 3:39 a.m. schrieb Daniel Vetter:
> On Fri, Jun 12, 2020 at 1:35 AM Felix Kuehling  wrote:
>> Am 2020-06-11 um 10:15 a.m. schrieb Jason Gunthorpe:
>>> On Thu, Jun 11, 2020 at 10:34:30AM +0200, Daniel Vetter wrote:
>>>>> I still have my doubts about allowing fence waiting from within shrinkers.
>>>>> IMO ideally they should use a trywait approach, in order to allow memory
>>>>> allocation during command submission for drivers that
>>>>> publish fences before command submission. (Since early reservation object
>>>>> release requires that).
>>>> Yeah it is a bit annoying, e.g. for drm/scheduler I think we'll end up
>>>> with a mempool to make sure it can handle it's allocations.
>>>>
>>>>> But since drivers are already waiting from within shrinkers and I take 
>>>>> your
>>>>> word for HMM requiring this,
>>>> Yeah the big trouble is HMM and mmu notifiers. That's the really awkward
>>>> one, the shrinker one is a lot less established.
>>> I really question if HW that needs something like DMA fence should
>>> even be using mmu notifiers - the best use is HW that can fence the
>>> DMA directly without having to get involved with some command stream
>>> processing.
>>>
>>> Or at the very least it should not be a generic DMA fence but a
>>> narrowed completion tied only into the same GPU driver's command
>>> completion processing which should be able to progress without
>>> blocking.
>>>
>>> The intent of notifiers was never to endlessly block while vast
>>> amounts of SW does work.
>>>
>>> Going around and switching everything in a GPU to GFP_ATOMIC seems
>>> like bad idea.
>>>
>>>> I've pinged a bunch of armsoc gpu driver people and ask them how much this
>>>> hurts, so that we have a clear answer. On x86 I don't think we have much
>>>> of a choice on this, with userptr in amd and i915 and hmm work in nouveau
>>>> (but nouveau I think doesn't use dma_fence in there).
>> Soon nouveau will get company. We're working on a recoverable page fault
>> implementation for HMM in amdgpu where we'll need to update page tables
>> using the GPUs SDMA engine and wait for corresponding fences in MMU
>> notifiers.
> Can you pls cc these patches to dri-devel when they show up? Depending
> upon how your hw works there's and endless amount of bad things that
> can happen.

Yes, I'll do that.


>
> Also I think (again depending upon how the hw exactly works) this
> stuff would be a perfect example for the dma_fence annotations.

We have already applied your patch series to our development branch. I
haven't looked into what annotations we'd have to add to our new code yet.


>
> The worst case is if your hw cannot preempt while a hw page fault is
> pending. That means none of the dma_fence will ever signal (the amdkfd
> preempt ctx fences wont, and the classic fences from amdgpu might be
> also stall). At least when you're unlucky and the fence you're waiting
> on somehow (anywhere in its dependency chain really) need the engine
> that's currently blocked waiting for the hw page fault.

Our HW can preempt while handling a page fault, at least on the GPU
generation we're working on now. On other GPUs we haven't included in
our initial effort, we will not be able to preempt while a page fault is
in progress. This is problematic, but that's for reasons related to our
GPU hardware scheduler and unrelated to fences.


>
> That in turn means anything you do in your hw page fault handler is in
> the critical section for dma fence signalling, which has far reaching
> implications.

I'm not sure I agree, at least for KFD. The only place where KFD uses
fences that depend on preemptions is eviction fences. And we can get rid
of those if we can preempt GPU access to specific BOs by invalidating
GPU PTEs. That way we don't need to preempt the GPU queues while a page
fault is in progress. Instead we would create more page faults.

That assumes that we can invalidate GPU PTEs without depending on
fences. We've discussed possible deadlocks due to memory allocations
needed on that code paths for IBs or page tables. We've already
eliminated page table allocations and reservation locks on the PTE
invalidation code path. And we're using a separate scheduler entity so
we can't get stuck behind other IBs that depend on fences. IIRC,
Christian also implemented a separate memory pool for IBs for this code
path.

Regards,
  Felix


> -Daniel
>
>> Regards,
>>   Felix
>>
>>
>>> Right, nor will RDMA ODP.
>>>
>>> Jason
>>> ___
>>> amd-gfx mailing list
>>> amd-...@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH] drm/amd: fix potential memleak in err branch

[Felix Kuehling]

Hi Bernard,

I just applied a patch from another contributor for the kfd_topology
part of this. See
https://cgit.freedesktop.org/~agd5f/linux/commit/?h=amd-staging-drm-next=fc0fe8138309fee303bd12991f12b23f01bbf58c

Please rebase your patch on that. I believe that should only leave the
fixes in kfd_process.c.

Thanks,
  Felix

Am 2020-06-19 um 7:45 a.m. schrieb Bernard Zhao:
> The function kobject_init_and_add alloc memory like:
> kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs
> ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller
> ->kmalloc_slab, in err branch this memory not free. If use
> kmemleak, this path maybe catched.
> These changes are to add kobject_put in kobject_init_and_add
> failed branch, fix potential memleak.
>
> Signed-off-by: Bernard Zhao 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c  |  2 ++
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 20 +++-
>  2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index d27221ddcdeb..5ee4d6cfb16d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -124,6 +124,7 @@ void kfd_procfs_init(void)
>   if (ret) {
>   pr_warn("Could not create procfs proc folder");
>   /* If we fail to create the procfs, clean up */
> + kobject_put(procfs.kobj);
>   kfd_procfs_shutdown();
>   }
>  }
> @@ -428,6 +429,7 @@ struct kfd_process *kfd_create_process(struct file *filep)
>  (int)process->lead_thread->pid);
>   if (ret) {
>   pr_warn("Creating procfs pid directory failed");
> + kobject_put(process->kobj);
>   goto out;
>   }
>  
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index bb77f7af2b6d..dc3c4149f860 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -632,8 +632,10 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>  
>   ret = kobject_init_and_add(dev->kobj_node, _type,
>   sys_props.kobj_nodes, "%d", id);
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(dev->kobj_node);
>   return ret;
> + }
>  
>   dev->kobj_mem = kobject_create_and_add("mem_banks", dev->kobj_node);
>   if (!dev->kobj_mem)
> @@ -680,8 +682,10 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>   return -ENOMEM;
>   ret = kobject_init_and_add(mem->kobj, _type,
>   dev->kobj_mem, "%d", i);
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(mem->kobj);
>   return ret;
> + }
>  
>   mem->attr.name = "properties";
>   mem->attr.mode = KFD_SYSFS_FILE_MODE;
> @@ -699,8 +703,10 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>   return -ENOMEM;
>   ret = kobject_init_and_add(cache->kobj, _type,
>   dev->kobj_cache, "%d", i);
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(cache->kobj);
>   return ret;
> + }
>  
>   cache->attr.name = "properties";
>   cache->attr.mode = KFD_SYSFS_FILE_MODE;
> @@ -718,8 +724,10 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>   return -ENOMEM;
>   ret = kobject_init_and_add(iolink->kobj, _type,
>   dev->kobj_iolink, "%d", i);
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(iolink->kobj);
>   return ret;
> + }
>  
>   iolink->attr.name = "properties";
>   iolink->attr.mode = KFD_SYSFS_FILE_MODE;
> @@ -798,8 +806,10 @@ static int kfd_topology_update_sysfs(void)
>   ret = kobject_init_and_add(sys_props.kobj_topology,
>   _type,  _device->kobj,
>   "topology");
> - if (ret < 0)
> + if (ret < 0) {
> + kobject_put(sys_props.kobj_topology);
>   return ret;
> + }
>  
>   sys_props.kobj_nodes = kobject_create_and_add("nodes",
>   sys_props.kobj_topology);

Re: [Linaro-mm-sig] [PATCH 04/18] dma-fence: prime lockdep annotations

[Felix Kuehling]

Am 2020-06-19 um 3:55 p.m. schrieb Jason Gunthorpe:
> On Fri, Jun 19, 2020 at 03:48:49PM -0400, Felix Kuehling wrote:
>> Am 2020-06-19 um 2:18 p.m. schrieb Jason Gunthorpe:
>>> On Fri, Jun 19, 2020 at 02:09:35PM -0400, Jerome Glisse wrote:
>>>> On Fri, Jun 19, 2020 at 02:23:08PM -0300, Jason Gunthorpe wrote:
>>>>> On Fri, Jun 19, 2020 at 06:19:41PM +0200, Daniel Vetter wrote:
>>>>>
>>>>>> The madness is only that device B's mmu notifier might need to wait
>>>>>> for fence_B so that the dma operation finishes. Which in turn has to
>>>>>> wait for device A to finish first.
>>>>> So, it sound, fundamentally you've got this graph of operations across
>>>>> an unknown set of drivers and the kernel cannot insert itself in
>>>>> dma_fence hand offs to re-validate any of the buffers involved?
>>>>> Buffers which by definition cannot be touched by the hardware yet.
>>>>>
>>>>> That really is a pretty horrible place to end up..
>>>>>
>>>>> Pinning really is right answer for this kind of work flow. I think
>>>>> converting pinning to notifers should not be done unless notifier
>>>>> invalidation is relatively bounded. 
>>>>>
>>>>> I know people like notifiers because they give a bit nicer performance
>>>>> in some happy cases, but this cripples all the bad cases..
>>>>>
>>>>> If pinning doesn't work for some reason maybe we should address that?
>>>> Note that the dma fence is only true for user ptr buffer which predate
>>>> any HMM work and thus were using mmu notifier already. You need the
>>>> mmu notifier there because of fork and other corner cases.
>>> I wonder if we should try to fix the fork case more directly - RDMA
>>> has this same problem and added MADV_DONTFORK a long time ago as a
>>> hacky way to deal with it.
>>>
>>> Some crazy page pin that resolved COW in a way that always kept the
>>> physical memory with the mm that initiated the pin?
>>>
>>> (isn't this broken for O_DIRECT as well anyhow?)
>>>
>>> How does mmu_notifiers help the fork case anyhow? Block fork from
>>> progressing?
>> How much the mmu_notifier blocks fork progress depends, on quickly we
>> can preempt GPU jobs accessing affected memory. If we don't have
>> fine-grained preemption capability (graphics), the best we can do is
>> wait for the GPU jobs to complete. We can also delay submission of new
>> GPU jobs to the same memory until the MMU notifier is done. Future jobs
>> would use the new page addresses.
>>
>> With fine-grained preemption (ROCm compute), we can preempt GPU work on
>> the affected adders space to minimize the delay seen by fork.
>>
>> With recoverable device page faults, we can invalidate GPU page table
>> entries, so device access to the affected pages stops immediately.
>>
>> In all cases, the end result is, that the device page table gets updated
>> with the address of the copied pages before the GPU accesses the COW
>> memory again.Without the MMU notifier, we'd end up with the GPU
>> corrupting memory of the other process.
> The model here in fork has been wrong for a long time, and I do wonder
> how O_DIRECT manages to not be broken too.. I guess the time windows
> there are too small to get unlucky.
>
> If you have a write pin on a page then it should not be COW'd into the
> fork'd process but copied with the originating page remaining with the
> original mm. 
>
> I wonder if there is some easy way to achive that - if that is the
> main reason to use notifiers then it would be a better solution.

Other than the application changing its own virtual address mappings
(mprotect, munmap, etc.), triggering MMU notifiers, we also get MMU
notifiers from THP worker threads, and NUMA balancing.

When we start doing migration to DEVICE_PRIVATE memory with HMM, we also
get MMU notifiers during those driver-initiated migrations.

Regards,
  Felix


>
> Jason

Re: [Linaro-mm-sig] [PATCH 04/18] dma-fence: prime lockdep annotations

[Felix Kuehling]

Am 2020-06-19 um 2:18 p.m. schrieb Jason Gunthorpe:
> On Fri, Jun 19, 2020 at 02:09:35PM -0400, Jerome Glisse wrote:
>> On Fri, Jun 19, 2020 at 02:23:08PM -0300, Jason Gunthorpe wrote:
>>> On Fri, Jun 19, 2020 at 06:19:41PM +0200, Daniel Vetter wrote:
>>>
 The madness is only that device B's mmu notifier might need to wait
 for fence_B so that the dma operation finishes. Which in turn has to
 wait for device A to finish first.
>>> So, it sound, fundamentally you've got this graph of operations across
>>> an unknown set of drivers and the kernel cannot insert itself in
>>> dma_fence hand offs to re-validate any of the buffers involved?
>>> Buffers which by definition cannot be touched by the hardware yet.
>>>
>>> That really is a pretty horrible place to end up..
>>>
>>> Pinning really is right answer for this kind of work flow. I think
>>> converting pinning to notifers should not be done unless notifier
>>> invalidation is relatively bounded. 
>>>
>>> I know people like notifiers because they give a bit nicer performance
>>> in some happy cases, but this cripples all the bad cases..
>>>
>>> If pinning doesn't work for some reason maybe we should address that?
>> Note that the dma fence is only true for user ptr buffer which predate
>> any HMM work and thus were using mmu notifier already. You need the
>> mmu notifier there because of fork and other corner cases.
> I wonder if we should try to fix the fork case more directly - RDMA
> has this same problem and added MADV_DONTFORK a long time ago as a
> hacky way to deal with it.
>
> Some crazy page pin that resolved COW in a way that always kept the
> physical memory with the mm that initiated the pin?
>
> (isn't this broken for O_DIRECT as well anyhow?)
>
> How does mmu_notifiers help the fork case anyhow? Block fork from
> progressing?

How much the mmu_notifier blocks fork progress depends, on quickly we
can preempt GPU jobs accessing affected memory. If we don't have
fine-grained preemption capability (graphics), the best we can do is
wait for the GPU jobs to complete. We can also delay submission of new
GPU jobs to the same memory until the MMU notifier is done. Future jobs
would use the new page addresses.

With fine-grained preemption (ROCm compute), we can preempt GPU work on
the affected adders space to minimize the delay seen by fork.

With recoverable device page faults, we can invalidate GPU page table
entries, so device access to the affected pages stops immediately.

In all cases, the end result is, that the device page table gets updated
with the address of the copied pages before the GPU accesses the COW
memory again.Without the MMU notifier, we'd end up with the GPU
corrupting memory of the other process.

Regards,
  Felix


>
>> I probably need to warn AMD folks again that using HMM means that you
>> must be able to update the GPU page table asynchronously without
>> fence wait.
> It is kind of unrelated to HMM, it just shouldn't be using mmu
> notifiers to replace page pinning..
>
>> The issue for AMD is that they already update their GPU page table
>> using DMA engine. I believe this is still doable if they use a
>> kernel only DMA engine context, where only kernel can queue up jobs
>> so that you do not need to wait for unrelated things and you can
>> prioritize GPU page table update which should translate in fast GPU
>> page table update without DMA fence.
> Make sense
>
> I'm not sure I saw this in the AMD hmm stuff - it would be good if
> someone would look at that. Every time I do it looks like the locking
> is wrong.
>
> Jason
> ___
> amd-gfx mailing list
> amd-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Linaro-mm-sig] [PATCH 04/18] dma-fence: prime lockdep annotations

[Felix Kuehling]

Am 2020-06-19 um 3:11 p.m. schrieb Alex Deucher:
> On Fri, Jun 19, 2020 at 2:09 PM Jerome Glisse  wrote:
>> On Fri, Jun 19, 2020 at 02:23:08PM -0300, Jason Gunthorpe wrote:
>>> On Fri, Jun 19, 2020 at 06:19:41PM +0200, Daniel Vetter wrote:
>>>
 The madness is only that device B's mmu notifier might need to wait
 for fence_B so that the dma operation finishes. Which in turn has to
 wait for device A to finish first.
>>> So, it sound, fundamentally you've got this graph of operations across
>>> an unknown set of drivers and the kernel cannot insert itself in
>>> dma_fence hand offs to re-validate any of the buffers involved?
>>> Buffers which by definition cannot be touched by the hardware yet.
>>>
>>> That really is a pretty horrible place to end up..
>>>
>>> Pinning really is right answer for this kind of work flow. I think
>>> converting pinning to notifers should not be done unless notifier
>>> invalidation is relatively bounded.
>>>
>>> I know people like notifiers because they give a bit nicer performance
>>> in some happy cases, but this cripples all the bad cases..
>>>
>>> If pinning doesn't work for some reason maybe we should address that?
>> Note that the dma fence is only true for user ptr buffer which predate
>> any HMM work and thus were using mmu notifier already. You need the
>> mmu notifier there because of fork and other corner cases.
>>
>> For nouveau the notifier do not need to wait for anything it can update
>> the GPU page table right away. Modulo needing to write to GPU memory
>> using dma engine if the GPU page table is in GPU memory that is not
>> accessible from the CPU but that's never the case for nouveau so far
>> (but i expect it will be at one point).
>>
>>
>> So i see this as 2 different cases, the user ptr case, which does pin
>> pages by the way, where things are synchronous. Versus the HMM cases
>> where everything is asynchronous.
>>
>>
>> I probably need to warn AMD folks again that using HMM means that you
>> must be able to update the GPU page table asynchronously without
>> fence wait. The issue for AMD is that they already update their GPU
>> page table using DMA engine. I believe this is still doable if they
>> use a kernel only DMA engine context, where only kernel can queue up
>> jobs so that you do not need to wait for unrelated things and you can
>> prioritize GPU page table update which should translate in fast GPU
>> page table update without DMA fence.
> All devices which support recoverable page faults also have a
> dedicated paging engine for the kernel driver which the driver already
> makes use of.  We can also update the GPU page tables with the CPU.

We have a potential problem with CPU updating page tables while the GPU
is retrying on page table entries because 64 bit CPU transactions don't
arrive in device memory atomically.

We are using SDMA for page table updates. This currently goes through a
the DRM GPU scheduler to a special SDMA queue that's used by kernel-mode
only. But since it's based on the DRM GPU scheduler, we do use dma-fence
to wait for completion.

Regards,
  Felix


>
> Alex
>
>>
 Full disclosure: We are aware that we've designed ourselves into an
 impressive corner here, and there's lots of talks going on about
 untangling the dma synchronization from the memory management
 completely. But
>>> I think the documenting is really important: only GPU should be using
>>> this stuff and driving notifiers this way. Complete NO for any
>>> totally-not-a-GPU things in drivers/accel for sure.
>> Yes for user that expect HMM they need to be asynchronous. But it is
>> hard to revert user ptr has it was done a long time ago.
>>
>> Cheers,
>> Jérôme
>>
>> ___
>> amd-gfx mailing list
>> amd-...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> ___
> amd-gfx mailing list
> amd-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdkfd: Fix reference count leaks.

[Felix Kuehling]

On 2020-06-13 15:32, wu000...@umn.edu wrote:

From: Qiushi Wu 

kobject_init_and_add() takes reference even when it fails.
If this function returns an error, kobject_put() must be called to
properly clean up the memory associated with the object.

Signed-off-by: Qiushi Wu 


Thank you. The patch is

Reviewed-by: Felix Kuehling 

I'm applying the patch to our amd-staging-drm-next branch.

Regards,
  Felix



---
  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 20 +++-
  1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index bb77f7af2b6d..dc3c4149f860 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -632,8 +632,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
  
  	ret = kobject_init_and_add(dev->kobj_node, _type,

sys_props.kobj_nodes, "%d", id);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(dev->kobj_node);
return ret;
+   }
  
  	dev->kobj_mem = kobject_create_and_add("mem_banks", dev->kobj_node);

if (!dev->kobj_mem)
@@ -680,8 +682,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(mem->kobj, _type,
dev->kobj_mem, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(mem->kobj);
return ret;
+   }
  
  		mem->attr.name = "properties";

mem->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -699,8 +703,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(cache->kobj, _type,
dev->kobj_cache, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(cache->kobj);
return ret;
+   }
  
  		cache->attr.name = "properties";

cache->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -718,8 +724,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(iolink->kobj, _type,
dev->kobj_iolink, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(iolink->kobj);
return ret;
+   }
  
  		iolink->attr.name = "properties";

iolink->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -798,8 +806,10 @@ static int kfd_topology_update_sysfs(void)
ret = kobject_init_and_add(sys_props.kobj_topology,
_type,  _device->kobj,
"topology");
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(sys_props.kobj_topology);
return ret;
+   }
  
  		sys_props.kobj_nodes = kobject_create_and_add("nodes",

sys_props.kobj_topology);

Re: [Linaro-mm-sig] [PATCH 04/18] dma-fence: prime lockdep annotations

[Felix Kuehling]

Am 2020-06-11 um 10:15 a.m. schrieb Jason Gunthorpe:
> On Thu, Jun 11, 2020 at 10:34:30AM +0200, Daniel Vetter wrote:
>>> I still have my doubts about allowing fence waiting from within shrinkers.
>>> IMO ideally they should use a trywait approach, in order to allow memory
>>> allocation during command submission for drivers that
>>> publish fences before command submission. (Since early reservation object
>>> release requires that).
>> Yeah it is a bit annoying, e.g. for drm/scheduler I think we'll end up
>> with a mempool to make sure it can handle it's allocations.
>>
>>> But since drivers are already waiting from within shrinkers and I take your
>>> word for HMM requiring this,
>> Yeah the big trouble is HMM and mmu notifiers. That's the really awkward
>> one, the shrinker one is a lot less established.
> I really question if HW that needs something like DMA fence should
> even be using mmu notifiers - the best use is HW that can fence the
> DMA directly without having to get involved with some command stream
> processing.
>
> Or at the very least it should not be a generic DMA fence but a
> narrowed completion tied only into the same GPU driver's command
> completion processing which should be able to progress without
> blocking.
>
> The intent of notifiers was never to endlessly block while vast
> amounts of SW does work.
>
> Going around and switching everything in a GPU to GFP_ATOMIC seems
> like bad idea.
>
>> I've pinged a bunch of armsoc gpu driver people and ask them how much this
>> hurts, so that we have a clear answer. On x86 I don't think we have much
>> of a choice on this, with userptr in amd and i915 and hmm work in nouveau
>> (but nouveau I think doesn't use dma_fence in there). 

Soon nouveau will get company. We're working on a recoverable page fault
implementation for HMM in amdgpu where we'll need to update page tables
using the GPUs SDMA engine and wait for corresponding fences in MMU
notifiers.

Regards,
  Felix


> Right, nor will RDMA ODP. 
>
> Jason
> ___
> amd-gfx mailing list
> amd-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH][next] drm/amdkfd: fix a dereference of pdd before it is null checked

[Felix Kuehling]

On 2020-05-28 18:24, Colin King wrote:

From: Colin Ian King 

Currently pointer pdd is being dereferenced when assigning pointer
dpm and then pdd is being null checked.  Fix this by checking if
pdd is null before the dereference of pdd occurs.

Addresses-Coverity: ("Dereference before null check")
Fixes: 522b89c63370 ("drm/amdkfd: Track SDMA utilization per process")
Signed-off-by: Colin Ian King 


Reviewed-by: Felix Kuehling 

I applied the patch to our internal amd-staging-drm-next.

Regards,
  Felix



---
  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 25636789f3d3..bdc58741b32e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -103,10 +103,11 @@ static void kfd_sdma_activity_worker(struct work_struct 
*work)
return;
  
  	pdd = workarea->pdd;

+   if (!pdd)
+   return;
dqm = pdd->dev->dqm;
qpd = >qpd;
-
-   if (!pdd || !dqm || !qpd)
+   if (!dqm || !qpd)
return;
  
  	mm = get_task_mm(pdd->process->lead_thread);

Re: [PATCH] drm/amd/amdkfd: Fix large framesize for kfd_smi_ev_read()

[Felix Kuehling]

Am 2020-05-20 um 9:53 a.m. schrieb Aurabindo Pillai:
> The buffer allocated is of 1024 bytes. Allocate this from
> heap instead of stack.
>
> Also remove check for stack size since we're allocating from heap
>
> Signed-off-by: Aurabindo Pillai 
> Tested-by: Amber Lin 

See one comment inline. With that fixed, the patch is

Reviewed-by: Felix Kuehling 


> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c | 26 +++--
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> index f5fd18eacf0d..5aebe169f8c6 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> @@ -77,9 +77,11 @@ static ssize_t kfd_smi_ev_read(struct file *filep, char 
> __user *user,
>   int ret;
>   size_t to_copy;
>   struct kfd_smi_client *client = filep->private_data;
> - unsigned char buf[MAX_KFIFO_SIZE];
> + unsigned char *buf;
>  
> - BUILD_BUG_ON(MAX_KFIFO_SIZE > 1024);
> + buf = kzalloc(MAX_KFIFO_SIZE * sizeof(*buf), GFP_KERNEL);

kzalloc is not necessary here, you could use kmalloc. The part of that
allocation that matters will be overwritten by kfifo_out.

Regards,
  Felix


> + if (!buf)
> + return -ENOMEM;
>  
>   /* kfifo_to_user can sleep so we can't use spinlock protection around
>* it. Instead, we kfifo out as spinlocked then copy them to the user.
> @@ -88,19 +90,29 @@ static ssize_t kfd_smi_ev_read(struct file *filep, char 
> __user *user,
>   to_copy = kfifo_len(>fifo);
>   if (!to_copy) {
>   spin_unlock(>lock);
> - return -EAGAIN;
> + ret = -EAGAIN;
> + goto ret_err;
>   }
>   to_copy = min3(size, sizeof(buf), to_copy);
>   ret = kfifo_out(>fifo, buf, to_copy);
>   spin_unlock(>lock);
> - if (ret <= 0)
> - return -EAGAIN;
> + if (ret <= 0) {
> + ret = -EAGAIN;
> + goto ret_err;
> + }
>  
>   ret = copy_to_user(user, buf, to_copy);
> - if (ret)
> - return -EFAULT;
> + if (ret) {
> + ret = -EFAULT;
> + goto ret_err;
> + }
>  
> + kfree(buf);
>   return to_copy;
> +
> +ret_err:
> + kfree(buf);
> + return ret;
>  }
>  
>  static ssize_t kfd_smi_ev_write(struct file *filep, const char __user *user,

Re: [PATCH] drm/amdkfd: fix typing in (*hqd_destroy)

[Felix Kuehling]

On 2018-04-24 09:14 AM, Luc Van Oostenryck wrote:
> The method (*hqd_destroy) is defined as using an 'uint32_t'
> as 3rd argument but the the actual implementation of this
> method and all its calls actually uses an 'enum kfd_preempt_type'
> for this argument.
>
> Fix this by using 'enum kfd_preempt_type' for (*hqd_destroy) too.
>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenr...@gmail.com>

Interesting. I'm surprised I never saw a compiler warning about a
function pointer type mismatch. I guess C isn't picky about integer
types as long as the size is the same.

Anyway, this patch is Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com>

> ---
>  drivers/gpu/drm/amd/include/kgd_kfd_interface.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h 
> b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index 237289a72..2890e8581 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -298,7 +298,7 @@ struct kfd2kgd_calls {
>   bool (*hqd_is_occupied)(struct kgd_dev *kgd, uint64_t queue_address,
>   uint32_t pipe_id, uint32_t queue_id);
>  
> - int (*hqd_destroy)(struct kgd_dev *kgd, void *mqd, uint32_t reset_type,
> + int (*hqd_destroy)(struct kgd_dev *kgd, void *mqd, enum 
> kfd_preempt_type reset_type,
>   unsigned int timeout, uint32_t pipe_id,
>   uint32_t queue_id);
>  

Re: [PATCH] drm/amdkfd: fix typing in (*hqd_destroy)

[Felix Kuehling]

On 2018-04-24 09:14 AM, Luc Van Oostenryck wrote:
> The method (*hqd_destroy) is defined as using an 'uint32_t'
> as 3rd argument but the the actual implementation of this
> method and all its calls actually uses an 'enum kfd_preempt_type'
> for this argument.
>
> Fix this by using 'enum kfd_preempt_type' for (*hqd_destroy) too.
>
> Signed-off-by: Luc Van Oostenryck 

Interesting. I'm surprised I never saw a compiler warning about a
function pointer type mismatch. I guess C isn't picky about integer
types as long as the size is the same.

Anyway, this patch is Reviewed-by: Felix Kuehling 

> ---
>  drivers/gpu/drm/amd/include/kgd_kfd_interface.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h 
> b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index 237289a72..2890e8581 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -298,7 +298,7 @@ struct kfd2kgd_calls {
>   bool (*hqd_is_occupied)(struct kgd_dev *kgd, uint64_t queue_address,
>   uint32_t pipe_id, uint32_t queue_id);
>  
> - int (*hqd_destroy)(struct kgd_dev *kgd, void *mqd, uint32_t reset_type,
> + int (*hqd_destroy)(struct kgd_dev *kgd, void *mqd, enum 
> kfd_preempt_type reset_type,
>   unsigned int timeout, uint32_t pipe_id,
>   uint32_t queue_id);
>  

Re: AMD graphics performance regression in 4.15 and later

[Felix Kuehling]

[+Philip]

On 2018-04-20 10:47 AM, Michel Dänzer wrote:
> On 2018-04-11 11:37 AM, Christian König wrote:
>> Am 11.04.2018 um 06:00 schrieb Gabriel C:
>>> 2018-04-09 11:42 GMT+02:00 Christian König
>>> :
 Am 07.04.2018 um 00:00 schrieb Jean-Marc Valin:
> Hi Christian,
>
> Thanks for the info. FYI, I've also opened a Firefox bug for that at:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1448778
> Feel free to comment since you have a better understanding of what's
> going on.
>
> One last question: right now I'm running 4.15.0 with the "offending"
> patch reverted. Is that safe to run or are there possible bad
> interactions with other changes.
 That should work without problems.

 But I just had another idea as well, if you want you could still test
 the
 new code path which will be using in 4.17.

>>> While Firefox may do some strange things is not about only Firefox.
>>>
>>> With your patches my EPYC box is unusable with  4.15++ kernels.
>>> The whole Desktop is acting weird.  This one is using
>>> an Cape Verde PRO [Radeon HD 7750/8740 / R7 250E] GPU.
>>>
>>> Box is  2 * EPYC 7281 with 128 GB ECC RAM
>>>
>>> Also a 14C Xeon box with a HD7700 is broken same way.
>> The hardware is irrelevant for this. We need to know what software stack
>> you use on top of it.
>>
>> E.g. desktop environment/Mesa and DDX version etc...
>>
>>> Everything breaks in X .. scrolling , moving windows , flickering etc.
>>>
>>>
>>> reverting f4c809914a7c3e4a59cf543da6c2a15d0f75ee38 and
>>> 648bc3574716400acc06f99915815f80d9563783
>>> from an 4.15 kernel makes things work again.
>>>
>>>
 Backporting all the detection logic is to invasive, but you could
 just go
 into drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c and forcefull use the other
 code path.

 Just look out for "#ifdef CONFIG_SWIOTLB" checks and disable those.

>>> Well you really can't be serious about these suggestions ? Are you ?
>>>
>>> Telling peoples to #if 0 random code is not a solution.
>> That is for testing and not a permanent solution.
>>
>>> You broke existsing working userland with your patches and at least
>>> please fix that for 4.16.
>>>
>>> I can help testing code for 4.17/++ if you wish but that is
>>> *different* storry.
>> Please test Alex's amd-staging-drm-next branch from
>> git://people.freedesktop.org/~agd5f/linux.
> I think we're still missing something here.
>
> I'm currently running 4.16.2 + the DRM subsystem changes which are going
> into 4.17 (so I have the changes Christian is referring to) with a
> Kaveri APU, and I'm seeing similar symptoms as described by Jean-Marc.
> Some observations:
>
> Firefox, Thunderbird, or worst, gnome-shell, can freeze for up to on the
> order of a minute, during which the kernel is spending most of one
> core's cycles inside alloc_pages (__alloc_pages_nodemask to be more
> precise), called from ttm_alloc_new_pages.
Philip debugged a similar problem with a KFD memory stress test about
two weeks ago, where the kernel was seemingly stuck in an infinite loop
trying to allocate huge pages. I'm pasting his analysis for the record:

> [...] it uses huge_flags GFP_TRANSHUGE to call alloc_pages(), this
> seems a corner case inside __alloc_pages_slowpath(), it never exits
> but goes to retry path every time. It can reclaim pages and
> did_some_progress (as a result, no_progress_loops is reset to 0 every
> loop, never reach MAX_RECLAIM_RETRIES) but cannot finish huge page
> allocations under this specific memory pressure.  
As a workaround to unblock our release branch testing we removed
transparent huge page allocation from  ttm_get_pages. We're seeing this
as far back as 4.13 on our release branch.

If we're really talking about the same problem, I don't think it's
caused by recent page allocator changes, but rather exposed by recent
TTM changes.

Regards,
  Felix

>
> At least in the case of Firefox, this happens due to Mesa internal BO
> allocations for glTex(Sub)Image, so it's not obvious that Firefox is
> doing something wrong.
>
> I never noticed this before this week. Before, I was running 4.15.y +
> DRM subsystem changes from 4.16. Maybe something has changed in core
> code, trying harder to allocate huge pages.
>
>
> Maybe TTM should only try to use any huge pages that happen to be
> available, not spend any (/ "too much"?) additional effort trying to
> free up huge pages?
>
>

Re: AMD graphics performance regression in 4.15 and later

[Felix Kuehling]

[+Philip]

On 2018-04-20 10:47 AM, Michel Dänzer wrote:
> On 2018-04-11 11:37 AM, Christian König wrote:
>> Am 11.04.2018 um 06:00 schrieb Gabriel C:
>>> 2018-04-09 11:42 GMT+02:00 Christian König
>>> :
 Am 07.04.2018 um 00:00 schrieb Jean-Marc Valin:
> Hi Christian,
>
> Thanks for the info. FYI, I've also opened a Firefox bug for that at:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1448778
> Feel free to comment since you have a better understanding of what's
> going on.
>
> One last question: right now I'm running 4.15.0 with the "offending"
> patch reverted. Is that safe to run or are there possible bad
> interactions with other changes.
 That should work without problems.

 But I just had another idea as well, if you want you could still test
 the
 new code path which will be using in 4.17.

>>> While Firefox may do some strange things is not about only Firefox.
>>>
>>> With your patches my EPYC box is unusable with  4.15++ kernels.
>>> The whole Desktop is acting weird.  This one is using
>>> an Cape Verde PRO [Radeon HD 7750/8740 / R7 250E] GPU.
>>>
>>> Box is  2 * EPYC 7281 with 128 GB ECC RAM
>>>
>>> Also a 14C Xeon box with a HD7700 is broken same way.
>> The hardware is irrelevant for this. We need to know what software stack
>> you use on top of it.
>>
>> E.g. desktop environment/Mesa and DDX version etc...
>>
>>> Everything breaks in X .. scrolling , moving windows , flickering etc.
>>>
>>>
>>> reverting f4c809914a7c3e4a59cf543da6c2a15d0f75ee38 and
>>> 648bc3574716400acc06f99915815f80d9563783
>>> from an 4.15 kernel makes things work again.
>>>
>>>
 Backporting all the detection logic is to invasive, but you could
 just go
 into drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c and forcefull use the other
 code path.

 Just look out for "#ifdef CONFIG_SWIOTLB" checks and disable those.

>>> Well you really can't be serious about these suggestions ? Are you ?
>>>
>>> Telling peoples to #if 0 random code is not a solution.
>> That is for testing and not a permanent solution.
>>
>>> You broke existsing working userland with your patches and at least
>>> please fix that for 4.16.
>>>
>>> I can help testing code for 4.17/++ if you wish but that is
>>> *different* storry.
>> Please test Alex's amd-staging-drm-next branch from
>> git://people.freedesktop.org/~agd5f/linux.
> I think we're still missing something here.
>
> I'm currently running 4.16.2 + the DRM subsystem changes which are going
> into 4.17 (so I have the changes Christian is referring to) with a
> Kaveri APU, and I'm seeing similar symptoms as described by Jean-Marc.
> Some observations:
>
> Firefox, Thunderbird, or worst, gnome-shell, can freeze for up to on the
> order of a minute, during which the kernel is spending most of one
> core's cycles inside alloc_pages (__alloc_pages_nodemask to be more
> precise), called from ttm_alloc_new_pages.
Philip debugged a similar problem with a KFD memory stress test about
two weeks ago, where the kernel was seemingly stuck in an infinite loop
trying to allocate huge pages. I'm pasting his analysis for the record:

> [...] it uses huge_flags GFP_TRANSHUGE to call alloc_pages(), this
> seems a corner case inside __alloc_pages_slowpath(), it never exits
> but goes to retry path every time. It can reclaim pages and
> did_some_progress (as a result, no_progress_loops is reset to 0 every
> loop, never reach MAX_RECLAIM_RETRIES) but cannot finish huge page
> allocations under this specific memory pressure.  
As a workaround to unblock our release branch testing we removed
transparent huge page allocation from  ttm_get_pages. We're seeing this
as far back as 4.13 on our release branch.

If we're really talking about the same problem, I don't think it's
caused by recent page allocator changes, but rather exposed by recent
TTM changes.

Regards,
  Felix

>
> At least in the case of Firefox, this happens due to Mesa internal BO
> allocations for glTex(Sub)Image, so it's not obvious that Firefox is
> doing something wrong.
>
> I never noticed this before this week. Before, I was running 4.15.y +
> DRM subsystem changes from 4.16. Maybe something has changed in core
> code, trying harder to allocate huge pages.
>
>
> Maybe TTM should only try to use any huge pages that happen to be
> available, not spend any (/ "too much"?) additional effort trying to
> free up huge pages?
>
>

Re: [PATCH] amdkfd: always select MMU_NOTIFIER

[Felix Kuehling]

On 2018-04-19 06:56 AM, Anders Roxell wrote:
> On 28 March 2018 at 18:04, Christian König <christian.koe...@amd.com> wrote:
>> Am 28.03.2018 um 17:53 schrieb Arnd Bergmann:
>>> Building amdkfd without MMU notifiers is broken:
>>>
>>> In file included from drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c:28:
>>> drivers/gpu/drm/amd/amdkfd/kfd_priv.h:584:22: error: field 'mmu_notifier'
>>> has incomplete type
>>>
>>> This adds the missing 'select MMU_NOTIFIER' line to make it build
>>> cleanly all the time.
>>>
>>> Signed-off-by: Arnd Bergmann <a...@arndb.de>
>>
>> Acked-by: Christian König <christian.koe...@amd.com>, but I would wait on
>> what Felix says to that.
> Tested-by: Anders Roxell <anders.rox...@linaro.org>
>
> Randy sent the same patch [1] and its still required.
>
> Cheers,
> Anders
> [1] https://patchwork.kernel.org/patch/10340885/
Yes, looks good. I think this probably broke when we relaxed the
dependency on iommuv2.

Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com>

Regards,
  Felix


>
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/Kconfig | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> b/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> index ed2f06c9f346..5a26acb90e19 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> +++ b/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> @@ -4,6 +4,7 @@
>>> config HSA_AMD
>>> tristate "HSA kernel driver for AMD GPU devices"
>>> +   select MMU_NOTIFIER
>>> depends on DRM_AMDGPU && X86_64
>>> imply AMD_IOMMU_V2
>>> help
>>

Re: [PATCH] amdkfd: always select MMU_NOTIFIER

[Felix Kuehling]

On 2018-04-19 06:56 AM, Anders Roxell wrote:
> On 28 March 2018 at 18:04, Christian König  wrote:
>> Am 28.03.2018 um 17:53 schrieb Arnd Bergmann:
>>> Building amdkfd without MMU notifiers is broken:
>>>
>>> In file included from drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c:28:
>>> drivers/gpu/drm/amd/amdkfd/kfd_priv.h:584:22: error: field 'mmu_notifier'
>>> has incomplete type
>>>
>>> This adds the missing 'select MMU_NOTIFIER' line to make it build
>>> cleanly all the time.
>>>
>>> Signed-off-by: Arnd Bergmann 
>>
>> Acked-by: Christian König , but I would wait on
>> what Felix says to that.
> Tested-by: Anders Roxell 
>
> Randy sent the same patch [1] and its still required.
>
> Cheers,
> Anders
> [1] https://patchwork.kernel.org/patch/10340885/
Yes, looks good. I think this probably broke when we relaxed the
dependency on iommuv2.

Reviewed-by: Felix Kuehling 

Regards,
  Felix


>
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/Kconfig | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> b/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> index ed2f06c9f346..5a26acb90e19 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> +++ b/drivers/gpu/drm/amd/amdkfd/Kconfig
>>> @@ -4,6 +4,7 @@
>>> config HSA_AMD
>>> tristate "HSA kernel driver for AMD GPU devices"
>>> +   select MMU_NOTIFIER
>>> depends on DRM_AMDGPU && X86_64
>>> imply AMD_IOMMU_V2
>>> help
>>

Re: [PATCHv2] drm/amdkfd: Remove vla

[Felix Kuehling]

On 2018-04-10 09:02 PM, Laura Abbott wrote:
> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> turn on -Wvla. Switch to a constant value that covers all hardware.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Laura Abbott <labb...@redhat.com>
> ---
> v2: Switch to a larger size to account for other hardware
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> index 035c351f47c5..c3a5a80e31ae 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> @@ -139,10 +139,12 @@ static void interrupt_wq(struct work_struct *work)
>  {
>   struct kfd_dev *dev = container_of(work, struct kfd_dev,
>   interrupt_work);
> + uint32_t ih_ring_entry[8];
>  
> - uint32_t ih_ring_entry[DIV_ROUND_UP(
> - dev->device_info->ih_ring_entry_size,
> - sizeof(uint32_t))];
> + if (dev->device_info->ih_ring_entry_size > (8 * sizeof(uint32_t))) {
> + dev_err(kfd_chardev(), "Ring entry too small\n");

When the ring entry size really is too small, this will potentially
flood the kernel log. Maybe a WARN_ONCE would be more helpful. With that
fixed, this patch is Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com>

Regards,
  Felix

> + return;
> + }
>  
>   while (dequeue_ih_ring_entry(dev, ih_ring_entry))
>   dev->device_info->event_interrupt_class->interrupt_wq(dev,

Re: [PATCHv2] drm/amdkfd: Remove vla

[Felix Kuehling]

On 2018-04-10 09:02 PM, Laura Abbott wrote:
> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> turn on -Wvla. Switch to a constant value that covers all hardware.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Laura Abbott 
> ---
> v2: Switch to a larger size to account for other hardware
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> index 035c351f47c5..c3a5a80e31ae 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> @@ -139,10 +139,12 @@ static void interrupt_wq(struct work_struct *work)
>  {
>   struct kfd_dev *dev = container_of(work, struct kfd_dev,
>   interrupt_work);
> + uint32_t ih_ring_entry[8];
>  
> - uint32_t ih_ring_entry[DIV_ROUND_UP(
> - dev->device_info->ih_ring_entry_size,
> - sizeof(uint32_t))];
> + if (dev->device_info->ih_ring_entry_size > (8 * sizeof(uint32_t))) {
> + dev_err(kfd_chardev(), "Ring entry too small\n");

When the ring entry size really is too small, this will potentially
flood the kernel log. Maybe a WARN_ONCE would be more helpful. With that
fixed, this patch is Reviewed-by: Felix Kuehling 

Regards,
  Felix

> + return;
> + }
>  
>   while (dequeue_ih_ring_entry(dev, ih_ring_entry))
>   dev->device_info->event_interrupt_class->interrupt_wq(dev,

Re: [PATCH] drm/amdkfd: Remove vla

[Felix Kuehling]

Thanks Christian for catching that. I'm working on a patch series to
upstream Vega10 support, about 95% done. It will add this ASIC info for
Vega10:

static const struct kfd_device_info vega10_device_info = {
.asic_family = CHIP_VEGA10,
.max_pasid_bits = 16,
.max_no_of_hqd  = 24,
.doorbell_size  = 8,
.ih_ring_entry_size = 8 * sizeof(uint32_t), /* !!! IH ring entry size 
is bigger on Vega10 !!! */
.event_interrupt_class = _interrupt_class_v9,
.num_of_watch_points = 4,
.mqd_size_aligned = MQD_SIZE_ALIGNED,
.supports_cwsr = true,
.needs_iommu_device = false,
.needs_pci_atomics = false,
};

If you change it to uint32_t ih_ring_entry[8] and update the check, it
should be reasonably future proof.

Regards,
  Felix


On 2018-04-10 02:38 AM, Christian König wrote:
> Am 09.04.2018 um 23:06 schrieb Laura Abbott:
>> There's an ongoing effort to remove VLAs[1] from the kernel to
>> eventually
>> turn on -Wvla. The single VLA usage in the amdkfd driver is actually
>> constant across all current platforms.
>
> Actually that isn't correct.
>
> Could be that we haven't upstreamed KFD support for them, but Vega10
> have a different interrupt ring entry size and so would cause the
> error message here.
>
>> Switch to a constant size array
>> instead.
>
> I would say to just make make the array bigger.
>
> Regards,
> Christian.
>
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Laura Abbott 
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 8 +---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
>> index 035c351f47c5..c9863858f343 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
>> @@ -139,10 +139,12 @@ static void interrupt_wq(struct work_struct *work)
>>   {
>>   struct kfd_dev *dev = container_of(work, struct kfd_dev,
>>   interrupt_work);
>> +    uint32_t ih_ring_entry[4];
>>   -    uint32_t ih_ring_entry[DIV_ROUND_UP(
>> -    dev->device_info->ih_ring_entry_size,
>> -    sizeof(uint32_t))];
>> +    if (dev->device_info->ih_ring_entry_size > (4 *
>> sizeof(uint32_t))) {
>> +    dev_err(kfd_chardev(), "Ring entry too small\n");
>> +    return;
>> +    }
>>     while (dequeue_ih_ring_entry(dev, ih_ring_entry))
>>   dev->device_info->event_interrupt_class->interrupt_wq(dev,
>
> ___
> amd-gfx mailing list
> amd-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdkfd: Remove vla

[Felix Kuehling]

Thanks Christian for catching that. I'm working on a patch series to
upstream Vega10 support, about 95% done. It will add this ASIC info for
Vega10:

static const struct kfd_device_info vega10_device_info = {
.asic_family = CHIP_VEGA10,
.max_pasid_bits = 16,
.max_no_of_hqd  = 24,
.doorbell_size  = 8,
.ih_ring_entry_size = 8 * sizeof(uint32_t), /* !!! IH ring entry size 
is bigger on Vega10 !!! */
.event_interrupt_class = _interrupt_class_v9,
.num_of_watch_points = 4,
.mqd_size_aligned = MQD_SIZE_ALIGNED,
.supports_cwsr = true,
.needs_iommu_device = false,
.needs_pci_atomics = false,
};

If you change it to uint32_t ih_ring_entry[8] and update the check, it
should be reasonably future proof.

Regards,
  Felix


On 2018-04-10 02:38 AM, Christian König wrote:
> Am 09.04.2018 um 23:06 schrieb Laura Abbott:
>> There's an ongoing effort to remove VLAs[1] from the kernel to
>> eventually
>> turn on -Wvla. The single VLA usage in the amdkfd driver is actually
>> constant across all current platforms.
>
> Actually that isn't correct.
>
> Could be that we haven't upstreamed KFD support for them, but Vega10
> have a different interrupt ring entry size and so would cause the
> error message here.
>
>> Switch to a constant size array
>> instead.
>
> I would say to just make make the array bigger.
>
> Regards,
> Christian.
>
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Laura Abbott 
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 8 +---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
>> index 035c351f47c5..c9863858f343 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
>> @@ -139,10 +139,12 @@ static void interrupt_wq(struct work_struct *work)
>>   {
>>   struct kfd_dev *dev = container_of(work, struct kfd_dev,
>>   interrupt_work);
>> +    uint32_t ih_ring_entry[4];
>>   -    uint32_t ih_ring_entry[DIV_ROUND_UP(
>> -    dev->device_info->ih_ring_entry_size,
>> -    sizeof(uint32_t))];
>> +    if (dev->device_info->ih_ring_entry_size > (4 *
>> sizeof(uint32_t))) {
>> +    dev_err(kfd_chardev(), "Ring entry too small\n");
>> +    return;
>> +    }
>>     while (dequeue_ih_ring_entry(dev, ih_ring_entry))
>>   dev->device_info->event_interrupt_class->interrupt_wq(dev,
>
> ___
> amd-gfx mailing list
> amd-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH -next] drm/amdkfd: Make function kfd_dev_is_large_bar() static

[Felix Kuehling]

This patch is Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com>


On 2018-03-29 10:25 PM, Wei Yongjun wrote:
> Fixes the following sparse warning:
>
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:1150:6: warning:
>  symbol 'kfd_dev_is_large_bar' was not declared. Should it be static?
>
> Signed-off-by: Wei Yongjun <weiyongj...@huawei.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index cd679cf..fb5d997 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1147,7 +1147,7 @@ static int kfd_ioctl_acquire_vm(struct file *filep, 
> struct kfd_process *p,
>   return ret;
>  }
>  
> -bool kfd_dev_is_large_bar(struct kfd_dev *dev)
> +static bool kfd_dev_is_large_bar(struct kfd_dev *dev)
>  {
>   struct kfd_local_mem_info mem_info;
>

Re: [PATCH -next] drm/amdkfd: Make function kfd_dev_is_large_bar() static

[Felix Kuehling]

This patch is Reviewed-by: Felix Kuehling 


On 2018-03-29 10:25 PM, Wei Yongjun wrote:
> Fixes the following sparse warning:
>
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:1150:6: warning:
>  symbol 'kfd_dev_is_large_bar' was not declared. Should it be static?
>
> Signed-off-by: Wei Yongjun 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index cd679cf..fb5d997 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1147,7 +1147,7 @@ static int kfd_ioctl_acquire_vm(struct file *filep, 
> struct kfd_process *p,
>   return ret;
>  }
>  
> -bool kfd_dev_is_large_bar(struct kfd_dev *dev)
> +static bool kfd_dev_is_large_bar(struct kfd_dev *dev)
>  {
>   struct kfd_local_mem_info mem_info;
>

Re: [PATCH -next] drm/amdkfd: Fix the error return code in kfd_ioctl_unmap_memory_from_gpu()

[Felix Kuehling]

Thanks for catching that. I'd use -ENODEV to match what is done a few
lines below for peer_pdd. With that fixed, this patch is Reviewed-by:
Felix Kuehling <felix.kuehl...@amd.com>

Regards,
  Felix


On 2018-03-29 10:25 PM, Wei Yongjun wrote:
> Passing NULL pointer to PTR_ERR will result in return value of 0
> indicating success which is clearly not what it is intended here.
> This patch returns -EINVAL instead.
>
> Fixes: 5ec7e02854b3 ("drm/amdkfd: Add ioctls for GPUVM memory management")
> Signed-off-by: Wei Yongjun <weiyongj...@huawei.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index cd679cf..c32a341 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1421,7 +1421,7 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file 
> *filep,
>  
>   pdd = kfd_get_process_device_data(dev, p);
>   if (!pdd) {
> - err = PTR_ERR(pdd);
> + err = -EINVAL;
>   goto bind_process_to_device_failed;
>   }
>

Re: [PATCH -next] drm/amdkfd: Fix the error return code in kfd_ioctl_unmap_memory_from_gpu()

[Felix Kuehling]

Thanks for catching that. I'd use -ENODEV to match what is done a few
lines below for peer_pdd. With that fixed, this patch is Reviewed-by:
Felix Kuehling 

Regards,
  Felix


On 2018-03-29 10:25 PM, Wei Yongjun wrote:
> Passing NULL pointer to PTR_ERR will result in return value of 0
> indicating success which is clearly not what it is intended here.
> This patch returns -EINVAL instead.
>
> Fixes: 5ec7e02854b3 ("drm/amdkfd: Add ioctls for GPUVM memory management")
> Signed-off-by: Wei Yongjun 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index cd679cf..c32a341 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1421,7 +1421,7 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file 
> *filep,
>  
>   pdd = kfd_get_process_device_data(dev, p);
>   if (!pdd) {
> - err = PTR_ERR(pdd);
> + err = -EINVAL;
>   goto bind_process_to_device_failed;
>   }
>

Re: [PATCH] drm/amdkfd: Use ARRAY_SIZE macro in kfd_build_sysfs_node_entry

[Felix Kuehling]

Looks good. This change is Reviewed-by: Felix Kuehling
<felix.kuehl...@amd.com>

Regards,
  Felix


On 2018-01-18 07:39 PM, Gustavo A. R. Silva wrote:
> Use ARRAY_SIZE instead of dividing sizeof array with sizeof an element.
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index c6a7609..7783250 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -677,7 +677,7 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>   }
>  
>   /* All hardware blocks have the same number of attributes. */
> - num_attrs = sizeof(perf_attr_iommu)/sizeof(struct kfd_perf_attr);
> + num_attrs = ARRAY_SIZE(perf_attr_iommu);
>   list_for_each_entry(perf, >perf_props, list) {
>   perf->attr_group = kzalloc(sizeof(struct kfd_perf_attr)
>   * num_attrs + sizeof(struct attribute_group),

Re: [PATCH] drm/amdkfd: Use ARRAY_SIZE macro in kfd_build_sysfs_node_entry

[Felix Kuehling]

Looks good. This change is Reviewed-by: Felix Kuehling


Regards,
  Felix


On 2018-01-18 07:39 PM, Gustavo A. R. Silva wrote:
> Use ARRAY_SIZE instead of dividing sizeof array with sizeof an element.
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index c6a7609..7783250 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -677,7 +677,7 @@ static int kfd_build_sysfs_node_entry(struct 
> kfd_topology_device *dev,
>   }
>  
>   /* All hardware blocks have the same number of attributes. */
> - num_attrs = sizeof(perf_attr_iommu)/sizeof(struct kfd_perf_attr);
> + num_attrs = ARRAY_SIZE(perf_attr_iommu);
>   list_for_each_entry(perf, >perf_props, list) {
>   perf->attr_group = kzalloc(sizeof(struct kfd_perf_attr)
>   * num_attrs + sizeof(struct attribute_group),

Re: [PATCH] drm/amdkfd: Fix potential NULL pointer dereferences

[Felix Kuehling]

Yeah, this looks good to me.

Regards,
  Felix


On 2018-01-10 04:58 PM, Gustavo A. R. Silva wrote:
> Hi Felix,
>
> Quoting Felix Kuehling <felix.kuehl...@amd.com>:
>
>> Hi Gustavo,
>>
>> Thanks for catching that. When returning a fault, I think you also need
>> to srcu_read_unlock(_processes_srcu, idx).
>>
>> However, instead of returning an error, I think I'd prefer to skip PDDs
>> that can't be found with continue statements. That way others would
>> still suspend and resume successfully. Maybe just print a WARN_ON for
>> PDDs that aren't found, because that's an unexpected situation,
>> currently. Maybe in the future it could be normal thing if we ever
>> support GPU hotplug.
>>
>
> I got it. In that case, what do you think about the following patch
> instead?
>
> index a22fb071..4ff5f0f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -461,7 +461,8 @@ int kfd_bind_processes_to_device(struct kfd_dev *dev)
>     hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>     mutex_lock(>mutex);
>     pdd = kfd_get_process_device_data(dev, p);
> -   if (pdd->bound != PDD_BOUND_SUSPENDED) {
> +
> +   if (WARN_ON(!pdd) || pdd->bound != PDD_BOUND_SUSPENDED) {
>     mutex_unlock(>mutex);
>     continue;
>     }
> @@ -501,6 +502,11 @@ void kfd_unbind_processes_from_device(struct
> kfd_dev *dev)
>     mutex_lock(>mutex);
>     pdd = kfd_get_process_device_data(dev, p);
>
> +   if (WARN_ON(!pdd)) {
> +   mutex_unlock(>mutex);
> +   continue;
> +   }
> +
>     if (pdd->bound == PDD_BOUND)
>     pdd->bound = PDD_BOUND_SUSPENDED;
>     mutex_unlock(>mutex);
>
>
> Thank you for the feedback.
> -- 
> Gustavo
>
>> Regards,
>>   Felix
>>
>>
>> On 2018-01-10 11:50 AM, Gustavo A. R. Silva wrote:
>>> In case kfd_get_process_device_data returns null, there are some
>>> null pointer dereferences in functions kfd_bind_processes_to_device
>>> and kfd_unbind_processes_from_device.
>>>
>>> Fix this by null checking pdd before dereferencing it.
>>>
>>> Addresses-Coverity-ID: 1463794 ("Dereference null return value")
>>> Addresses-Coverity-ID: 1463772 ("Dereference null return value")
>>> Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 12 
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> index a22fb071..29d51d5 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> @@ -461,6 +461,13 @@ int kfd_bind_processes_to_device(struct kfd_dev
>>> *dev)
>>>  hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>>>  mutex_lock(>mutex);
>>>  pdd = kfd_get_process_device_data(dev, p);
>>> +
>>> +    if (!pdd) {
>>> +    pr_err("Process device data doesn't exist\n");
>>> +    mutex_unlock(>mutex);
>>> +    return -EFAULT;
>>> +    }
>>> +
>>>  if (pdd->bound != PDD_BOUND_SUSPENDED) {
>>>  mutex_unlock(>mutex);
>>>  continue;
>>> @@ -501,6 +508,11 @@ void kfd_unbind_processes_from_device(struct
>>> kfd_dev *dev)
>>>  mutex_lock(>mutex);
>>>  pdd = kfd_get_process_device_data(dev, p);
>>>
>>> +    if (!pdd) {
>>> +    mutex_unlock(>mutex);
>>> +    return;
>>> +    }
>>> +
>>>  if (pdd->bound == PDD_BOUND)
>>>  pdd->bound = PDD_BOUND_SUSPENDED;
>>>  mutex_unlock(>mutex);
>
>
>
>
>
>

Re: [PATCH] drm/amdkfd: Fix potential NULL pointer dereferences

[Felix Kuehling]

Yeah, this looks good to me.

Regards,
  Felix


On 2018-01-10 04:58 PM, Gustavo A. R. Silva wrote:
> Hi Felix,
>
> Quoting Felix Kuehling :
>
>> Hi Gustavo,
>>
>> Thanks for catching that. When returning a fault, I think you also need
>> to srcu_read_unlock(_processes_srcu, idx).
>>
>> However, instead of returning an error, I think I'd prefer to skip PDDs
>> that can't be found with continue statements. That way others would
>> still suspend and resume successfully. Maybe just print a WARN_ON for
>> PDDs that aren't found, because that's an unexpected situation,
>> currently. Maybe in the future it could be normal thing if we ever
>> support GPU hotplug.
>>
>
> I got it. In that case, what do you think about the following patch
> instead?
>
> index a22fb071..4ff5f0f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -461,7 +461,8 @@ int kfd_bind_processes_to_device(struct kfd_dev *dev)
>     hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>     mutex_lock(>mutex);
>     pdd = kfd_get_process_device_data(dev, p);
> -   if (pdd->bound != PDD_BOUND_SUSPENDED) {
> +
> +   if (WARN_ON(!pdd) || pdd->bound != PDD_BOUND_SUSPENDED) {
>     mutex_unlock(>mutex);
>     continue;
>     }
> @@ -501,6 +502,11 @@ void kfd_unbind_processes_from_device(struct
> kfd_dev *dev)
>     mutex_lock(>mutex);
>     pdd = kfd_get_process_device_data(dev, p);
>
> +   if (WARN_ON(!pdd)) {
> +   mutex_unlock(>mutex);
> +   continue;
> +   }
> +
>     if (pdd->bound == PDD_BOUND)
>     pdd->bound = PDD_BOUND_SUSPENDED;
>     mutex_unlock(>mutex);
>
>
> Thank you for the feedback.
> -- 
> Gustavo
>
>> Regards,
>>   Felix
>>
>>
>> On 2018-01-10 11:50 AM, Gustavo A. R. Silva wrote:
>>> In case kfd_get_process_device_data returns null, there are some
>>> null pointer dereferences in functions kfd_bind_processes_to_device
>>> and kfd_unbind_processes_from_device.
>>>
>>> Fix this by null checking pdd before dereferencing it.
>>>
>>> Addresses-Coverity-ID: 1463794 ("Dereference null return value")
>>> Addresses-Coverity-ID: 1463772 ("Dereference null return value")
>>> Signed-off-by: Gustavo A. R. Silva 
>>> ---
>>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 12 
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> index a22fb071..29d51d5 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> @@ -461,6 +461,13 @@ int kfd_bind_processes_to_device(struct kfd_dev
>>> *dev)
>>>  hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>>>  mutex_lock(>mutex);
>>>  pdd = kfd_get_process_device_data(dev, p);
>>> +
>>> +    if (!pdd) {
>>> +    pr_err("Process device data doesn't exist\n");
>>> +    mutex_unlock(>mutex);
>>> +    return -EFAULT;
>>> +    }
>>> +
>>>  if (pdd->bound != PDD_BOUND_SUSPENDED) {
>>>  mutex_unlock(>mutex);
>>>  continue;
>>> @@ -501,6 +508,11 @@ void kfd_unbind_processes_from_device(struct
>>> kfd_dev *dev)
>>>  mutex_lock(>mutex);
>>>  pdd = kfd_get_process_device_data(dev, p);
>>>
>>> +    if (!pdd) {
>>> +    mutex_unlock(>mutex);
>>> +    return;
>>> +    }
>>> +
>>>  if (pdd->bound == PDD_BOUND)
>>>  pdd->bound = PDD_BOUND_SUSPENDED;
>>>  mutex_unlock(>mutex);
>
>
>
>
>
>

Re: [PATCH] drm/amdkfd: Fix potential NULL pointer dereferences

[Felix Kuehling]

Hi Gustavo,

Thanks for catching that. When returning a fault, I think you also need
to srcu_read_unlock(_processes_srcu, idx).

However, instead of returning an error, I think I'd prefer to skip PDDs
that can't be found with continue statements. That way others would
still suspend and resume successfully. Maybe just print a WARN_ON for
PDDs that aren't found, because that's an unexpected situation,
currently. Maybe in the future it could be normal thing if we ever
support GPU hotplug.

Regards,
  Felix


On 2018-01-10 11:50 AM, Gustavo A. R. Silva wrote:
> In case kfd_get_process_device_data returns null, there are some
> null pointer dereferences in functions kfd_bind_processes_to_device
> and kfd_unbind_processes_from_device.
>
> Fix this by null checking pdd before dereferencing it.
>
> Addresses-Coverity-ID: 1463794 ("Dereference null return value")
> Addresses-Coverity-ID: 1463772 ("Dereference null return value")
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index a22fb071..29d51d5 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -461,6 +461,13 @@ int kfd_bind_processes_to_device(struct kfd_dev *dev)
>   hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>   mutex_lock(>mutex);
>   pdd = kfd_get_process_device_data(dev, p);
> +
> + if (!pdd) {
> + pr_err("Process device data doesn't exist\n");
> + mutex_unlock(>mutex);
> + return -EFAULT;
> + }
> +
>   if (pdd->bound != PDD_BOUND_SUSPENDED) {
>   mutex_unlock(>mutex);
>   continue;
> @@ -501,6 +508,11 @@ void kfd_unbind_processes_from_device(struct kfd_dev 
> *dev)
>   mutex_lock(>mutex);
>   pdd = kfd_get_process_device_data(dev, p);
>  
> + if (!pdd) {
> + mutex_unlock(>mutex);
> + return;
> + }
> +
>   if (pdd->bound == PDD_BOUND)
>   pdd->bound = PDD_BOUND_SUSPENDED;
>   mutex_unlock(>mutex);

Re: [PATCH] drm/amdkfd: Fix potential NULL pointer dereferences

[Felix Kuehling]

Hi Gustavo,

Thanks for catching that. When returning a fault, I think you also need
to srcu_read_unlock(_processes_srcu, idx).

However, instead of returning an error, I think I'd prefer to skip PDDs
that can't be found with continue statements. That way others would
still suspend and resume successfully. Maybe just print a WARN_ON for
PDDs that aren't found, because that's an unexpected situation,
currently. Maybe in the future it could be normal thing if we ever
support GPU hotplug.

Regards,
  Felix


On 2018-01-10 11:50 AM, Gustavo A. R. Silva wrote:
> In case kfd_get_process_device_data returns null, there are some
> null pointer dereferences in functions kfd_bind_processes_to_device
> and kfd_unbind_processes_from_device.
>
> Fix this by null checking pdd before dereferencing it.
>
> Addresses-Coverity-ID: 1463794 ("Dereference null return value")
> Addresses-Coverity-ID: 1463772 ("Dereference null return value")
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index a22fb071..29d51d5 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -461,6 +461,13 @@ int kfd_bind_processes_to_device(struct kfd_dev *dev)
>   hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>   mutex_lock(>mutex);
>   pdd = kfd_get_process_device_data(dev, p);
> +
> + if (!pdd) {
> + pr_err("Process device data doesn't exist\n");
> + mutex_unlock(>mutex);
> + return -EFAULT;
> + }
> +
>   if (pdd->bound != PDD_BOUND_SUSPENDED) {
>   mutex_unlock(>mutex);
>   continue;
> @@ -501,6 +508,11 @@ void kfd_unbind_processes_from_device(struct kfd_dev 
> *dev)
>   mutex_lock(>mutex);
>   pdd = kfd_get_process_device_data(dev, p);
>  
> + if (!pdd) {
> + mutex_unlock(>mutex);
> + return;
> + }
> +
>   if (pdd->bound == PDD_BOUND)
>   pdd->bound = PDD_BOUND_SUSPENDED;
>   mutex_unlock(>mutex);

Re: [PATCH] drm/amdgpu: use %pap format string for phys_addr_t

[Felix Kuehling]

This commit is Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com>

Thanks,
  Felix


On 2018-01-08 07:53 AM, Arnd Bergmann wrote:
> The newly added get_local_mem_info() function prints a phys_addr_t
> using 0x%llx, which is wrong on most 32-bit systems, as shown by
> this warning:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c: In function 'get_local_mem_info':
> include/linux/kern_levels.h:5:18: error: format '%llx' expects argument of 
> type 'long long unsigned int', but argument 2 has type 'resource_size_t {aka 
> unsigned int}' [-Werror=format=]
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c:297:31: note: format string is 
> defined here
>   pr_debug("Address base: 0x%llx limit 0x%llx public 0x%llx private 0x%llx\n",
>
> Passing the address by reference to the special %pap format string will
> produce the correct output and avoid the warning.
>
> Fixes: 30f1c0421ec5 ("drm/amdgpu: Implement get_local_mem_info")
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 335e454e2ee1..1d605e1c1d66 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -294,8 +294,8 @@ void get_local_mem_info(struct kgd_dev *kgd,
>   }
>   mem_info->vram_width = adev->mc.vram_width;
>  
> - pr_debug("Address base: 0x%llx limit 0x%llx public 0x%llx private 
> 0x%llx\n",
> - adev->mc.aper_base, aper_limit,
> + pr_debug("Address base: %pap limit %pap public 0x%llx private 0x%llx\n",
> + >mc.aper_base, _limit,
>   mem_info->local_mem_size_public,
>   mem_info->local_mem_size_private);
>  

Re: [PATCH] drm/amdgpu: use %pap format string for phys_addr_t

[Felix Kuehling]

This commit is Reviewed-by: Felix Kuehling 

Thanks,
  Felix


On 2018-01-08 07:53 AM, Arnd Bergmann wrote:
> The newly added get_local_mem_info() function prints a phys_addr_t
> using 0x%llx, which is wrong on most 32-bit systems, as shown by
> this warning:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c: In function 'get_local_mem_info':
> include/linux/kern_levels.h:5:18: error: format '%llx' expects argument of 
> type 'long long unsigned int', but argument 2 has type 'resource_size_t {aka 
> unsigned int}' [-Werror=format=]
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c:297:31: note: format string is 
> defined here
>   pr_debug("Address base: 0x%llx limit 0x%llx public 0x%llx private 0x%llx\n",
>
> Passing the address by reference to the special %pap format string will
> produce the correct output and avoid the warning.
>
> Fixes: 30f1c0421ec5 ("drm/amdgpu: Implement get_local_mem_info")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 335e454e2ee1..1d605e1c1d66 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -294,8 +294,8 @@ void get_local_mem_info(struct kgd_dev *kgd,
>   }
>   mem_info->vram_width = adev->mc.vram_width;
>  
> - pr_debug("Address base: 0x%llx limit 0x%llx public 0x%llx private 
> 0x%llx\n",
> - adev->mc.aper_base, aper_limit,
> + pr_debug("Address base: %pap limit %pap public 0x%llx private 0x%llx\n",
> + >mc.aper_base, _limit,
>   mem_info->local_mem_size_public,
>   mem_info->local_mem_size_private);
>  

Re: [PATCH 1/3] drm/amdgpu/display: provide ASSERT macros unconditionally

[Felix Kuehling]

On 2017-11-02 07:26 AM, Arnd Bergmann wrote:
> It seems impossible to build this driver without setting either
> CONFIG_DEBUG_KERNEL or CONFIG_DEBUG_DRIVER:
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h: In function 
> 'set_reg_field_value_ex':
> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h:132:2: error: implicit 
> declaration of function 'ASSERT'; did you mean 'IS_ERR'? 
> [-Werror=implicit-function-declaration]
>
> This moves the ASSERT() macro and related helpers outside of
> the #ifdef to get it to build again.
>
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/gpu/drm/amd/display/dc/os_types.h | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h 
> b/drivers/gpu/drm/amd/display/dc/os_types.h
> index 86170b40b5c5..499bd52004cf 100644
> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
> @@ -61,8 +61,6 @@
>   * general debug capabilities
>   *
>   */
> -#if defined(CONFIG_DEBUG_KERNEL) || defined(CONFIG_DEBUG_DRIVER)
> -
>  #if defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB)
>  #define ASSERT_CRITICAL(expr) do {   \
>   if (WARN_ON(!(expr))) { \
> @@ -75,7 +73,7 @@
>   ; \
>   } \
>  } while (0)
> -#endif
> +#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */

Is this the right comment here? Looks like it should be /*
defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB) */.

Regards,
  Felix

>  
>  #if defined(CONFIG_DEBUG_KERNEL_DC)
>  #define ASSERT(expr) ASSERT_CRITICAL(expr)
> @@ -86,8 +84,6 @@
>  
>  #define BREAK_TO_DEBUGGER() ASSERT(0)
>  
> -#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */
> -
>  #define DC_ERR(...)  do { \
>   dm_error(__VA_ARGS__); \
>   BREAK_TO_DEBUGGER(); \

Re: [PATCH 1/3] drm/amdgpu/display: provide ASSERT macros unconditionally

[Felix Kuehling]

On 2017-11-02 07:26 AM, Arnd Bergmann wrote:
> It seems impossible to build this driver without setting either
> CONFIG_DEBUG_KERNEL or CONFIG_DEBUG_DRIVER:
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h: In function 
> 'set_reg_field_value_ex':
> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h:132:2: error: implicit 
> declaration of function 'ASSERT'; did you mean 'IS_ERR'? 
> [-Werror=implicit-function-declaration]
>
> This moves the ASSERT() macro and related helpers outside of
> the #ifdef to get it to build again.
>
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/gpu/drm/amd/display/dc/os_types.h | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h 
> b/drivers/gpu/drm/amd/display/dc/os_types.h
> index 86170b40b5c5..499bd52004cf 100644
> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
> @@ -61,8 +61,6 @@
>   * general debug capabilities
>   *
>   */
> -#if defined(CONFIG_DEBUG_KERNEL) || defined(CONFIG_DEBUG_DRIVER)
> -
>  #if defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB)
>  #define ASSERT_CRITICAL(expr) do {   \
>   if (WARN_ON(!(expr))) { \
> @@ -75,7 +73,7 @@
>   ; \
>   } \
>  } while (0)
> -#endif
> +#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */

Is this the right comment here? Looks like it should be /*
defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB) */.

Regards,
  Felix

>  
>  #if defined(CONFIG_DEBUG_KERNEL_DC)
>  #define ASSERT(expr) ASSERT_CRITICAL(expr)
> @@ -86,8 +84,6 @@
>  
>  #define BREAK_TO_DEBUGGER() ASSERT(0)
>  
> -#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */
> -
>  #define DC_ERR(...)  do { \
>   dm_error(__VA_ARGS__); \
>   BREAK_TO_DEBUGGER(); \

Re: arch/alpha/include/asm/mmu_context.h:160:2: error: implicit declaration of function 'task_thread_info'

[Felix Kuehling]

Looks like we need to include sched.h before mmu_context.h to make the
Alpha build happy.

Regards,
  Felix


On 2017-09-16 09:50 PM, kbuild test robot wrote:
> Hi Felix,
>
> FYI, the error/warning still remains.
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> master
> head:   2bd6bf03f4c1c59381d62c61d03f6cc3fe71f66e
> commit: 70539bd79500245cbb4c7af00572fcce540d0105 drm/amd: Update MEC HQD 
> loading code for KFD
> date:   5 weeks ago
> config: alpha-allmodconfig (attached as .config)
> compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout 70539bd79500245cbb4c7af00572fcce540d0105
> # save the attached .config to linux build tree
> make.cross ARCH=alpha 
>
> All errors (new ones prefixed by >>):
>
>In file included from include/linux/mmu_context.h:4:0,
> from drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h:29,
> from drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c:23:
>arch/alpha/include/asm/mmu_context.h: In function 'ev5_switch_mm':
>>> arch/alpha/include/asm/mmu_context.h:160:2: error: implicit declaration of 
>>> function 'task_thread_info' [-Werror=implicit-function-declaration]
>  task_thread_info(next)->pcb.asn = mmc & HARDWARE_ASN_MASK;
>  ^~~~
>>> arch/alpha/include/asm/mmu_context.h:160:24: error: invalid type argument 
>>> of '->' (have 'int')
>  task_thread_info(next)->pcb.asn = mmc & HARDWARE_ASN_MASK;
>^~
>arch/alpha/include/asm/mmu_context.h: In function 'init_new_context':
>arch/alpha/include/asm/mmu_context.h:238:24: error: invalid type argument 
> of '->' (have 'int')
>   task_thread_info(tsk)->pcb.ptbr
>^~
>arch/alpha/include/asm/mmu_context.h: In function 'enter_lazy_tlb':
>arch/alpha/include/asm/mmu_context.h:252:23: error: invalid type argument 
> of '->' (have 'int')
>  task_thread_info(tsk)->pcb.ptbr
>   ^~
>cc1: some warnings being treated as errors
>
> vim +/task_thread_info +160 arch/alpha/include/asm/mmu_context.h
>
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  156  
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  157  
> /* Always update the PCB ASN.  Another thread may have allocated
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  158  
>a new mm->context (via flush_tlb_mm) without the ASN serial
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  159  
>number wrapping.  We have no way to detect when this is needed.  */
> 37bfbaf99 include/asm-alpha/mmu_context.h Al Viro2006-01-12 @160  
> task_thread_info(next)->pcb.asn = mmc & HARDWARE_ASN_MASK;
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  161  }
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  162  
>
> :: The code at line 160 was first introduced by commit
> :: 37bfbaf995d2c1f8196ee04c9d6f68258d5ec3e8 [PATCH] alpha: 
> task_thread_info()
>
> :: TO: Al Viro 
> :: CC: Linus Torvalds 
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: arch/alpha/include/asm/mmu_context.h:160:2: error: implicit declaration of function 'task_thread_info'

[Felix Kuehling]

Looks like we need to include sched.h before mmu_context.h to make the
Alpha build happy.

Regards,
  Felix


On 2017-09-16 09:50 PM, kbuild test robot wrote:
> Hi Felix,
>
> FYI, the error/warning still remains.
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> master
> head:   2bd6bf03f4c1c59381d62c61d03f6cc3fe71f66e
> commit: 70539bd79500245cbb4c7af00572fcce540d0105 drm/amd: Update MEC HQD 
> loading code for KFD
> date:   5 weeks ago
> config: alpha-allmodconfig (attached as .config)
> compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout 70539bd79500245cbb4c7af00572fcce540d0105
> # save the attached .config to linux build tree
> make.cross ARCH=alpha 
>
> All errors (new ones prefixed by >>):
>
>In file included from include/linux/mmu_context.h:4:0,
> from drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h:29,
> from drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c:23:
>arch/alpha/include/asm/mmu_context.h: In function 'ev5_switch_mm':
>>> arch/alpha/include/asm/mmu_context.h:160:2: error: implicit declaration of 
>>> function 'task_thread_info' [-Werror=implicit-function-declaration]
>  task_thread_info(next)->pcb.asn = mmc & HARDWARE_ASN_MASK;
>  ^~~~
>>> arch/alpha/include/asm/mmu_context.h:160:24: error: invalid type argument 
>>> of '->' (have 'int')
>  task_thread_info(next)->pcb.asn = mmc & HARDWARE_ASN_MASK;
>^~
>arch/alpha/include/asm/mmu_context.h: In function 'init_new_context':
>arch/alpha/include/asm/mmu_context.h:238:24: error: invalid type argument 
> of '->' (have 'int')
>   task_thread_info(tsk)->pcb.ptbr
>^~
>arch/alpha/include/asm/mmu_context.h: In function 'enter_lazy_tlb':
>arch/alpha/include/asm/mmu_context.h:252:23: error: invalid type argument 
> of '->' (have 'int')
>  task_thread_info(tsk)->pcb.ptbr
>   ^~
>cc1: some warnings being treated as errors
>
> vim +/task_thread_info +160 arch/alpha/include/asm/mmu_context.h
>
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  156  
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  157  
> /* Always update the PCB ASN.  Another thread may have allocated
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  158  
>a new mm->context (via flush_tlb_mm) without the ASN serial
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  159  
>number wrapping.  We have no way to detect when this is needed.  */
> 37bfbaf99 include/asm-alpha/mmu_context.h Al Viro2006-01-12 @160  
> task_thread_info(next)->pcb.asn = mmc & HARDWARE_ASN_MASK;
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  161  }
> ^1da177e4 include/asm-alpha/mmu_context.h Linus Torvalds 2005-04-16  162  
>
> :: The code at line 160 was first introduced by commit
> :: 37bfbaf995d2c1f8196ee04c9d6f68258d5ec3e8 [PATCH] alpha: 
> task_thread_info()
>
> :: TO: Al Viro 
> :: CC: Linus Torvalds 
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [PATCH] lib: Closed hash table with low overhead

[Felix Kuehling]

On 2017-09-15 05:14 PM, Andrew Morton wrote:
> On Mon, 28 Aug 2017 21:53:10 -0400 Felix Kuehling <felix.kuehl...@amd.com> 
> wrote:
>
>> This adds a statically sized closed hash table implementation with
>> low memory and CPU overhead. The API is inspired by kfifo.
>>
>> Storing, retrieving and deleting data does not involve any dynamic
>> memory management, which makes it ideal for use in interrupt context.
>> Static memory usage per entry comprises a 32 or 64 bit hash key, two
>> bits for occupancy tracking and the value size stored in the table.
>> No list heads or pointers are needed. Therefore this data structure
>> should be quite cache-friendly, too.
>>
>> It uses linear probing and lazy deletion. During lookups free space
>> is reclaimed and entries relocated to speed up future lookups.
> I haven't looked at the implementation (yet), but I'm wondering if you
> have identified hash table users (or implementations) elsewhere in the
> kernel which might be migrated to use this?  If so, such conversions
> can be used to determine/demonstrate the desirability of the patch.

I haven't looked into this. I did a quick search for where hash tables
are currently used in the kernel tree. But I'm not sufficiently familiar
with the subsystems to easily decide which ones could benefit from my work.

My implementation has some constraints because the hash table is not
resizable (by design). I think it may be useful for some applications in
drivers, where the amount of data in the hash table is limited by HW
constraints. Data stored in the table should also be quite small. For
larger data structures that need to be allocated with kmalloc, you may
as well use hashtable or rhashtable. To see a performance impact, you'd
need very frequent lookups.

For now I've settled on checking the code into the amdgpu driver so I
can make progress. If someone finds another application for it, it can
be moved to lib/ easily.

Regards,
  Felix

Re: [PATCH] lib: Closed hash table with low overhead

[Felix Kuehling]

On 2017-09-15 05:14 PM, Andrew Morton wrote:
> On Mon, 28 Aug 2017 21:53:10 -0400 Felix Kuehling  
> wrote:
>
>> This adds a statically sized closed hash table implementation with
>> low memory and CPU overhead. The API is inspired by kfifo.
>>
>> Storing, retrieving and deleting data does not involve any dynamic
>> memory management, which makes it ideal for use in interrupt context.
>> Static memory usage per entry comprises a 32 or 64 bit hash key, two
>> bits for occupancy tracking and the value size stored in the table.
>> No list heads or pointers are needed. Therefore this data structure
>> should be quite cache-friendly, too.
>>
>> It uses linear probing and lazy deletion. During lookups free space
>> is reclaimed and entries relocated to speed up future lookups.
> I haven't looked at the implementation (yet), but I'm wondering if you
> have identified hash table users (or implementations) elsewhere in the
> kernel which might be migrated to use this?  If so, such conversions
> can be used to determine/demonstrate the desirability of the patch.

I haven't looked into this. I did a quick search for where hash tables
are currently used in the kernel tree. But I'm not sufficiently familiar
with the subsystems to easily decide which ones could benefit from my work.

My implementation has some constraints because the hash table is not
resizable (by design). I think it may be useful for some applications in
drivers, where the amount of data in the hash table is limited by HW
constraints. Data stored in the table should also be quite small. For
larger data structures that need to be allocated with kmalloc, you may
as well use hashtable or rhashtable. To see a performance impact, you'd
need very frequent lookups.

For now I've settled on checking the code into the amdgpu driver so I
can make progress. If someone finds another application for it, it can
be moved to lib/ easily.

Regards,
  Felix

[PATCH] lib: Closed hash table with low overhead

[Felix Kuehling]

This adds a statically sized closed hash table implementation with
low memory and CPU overhead. The API is inspired by kfifo.

Storing, retrieving and deleting data does not involve any dynamic
memory management, which makes it ideal for use in interrupt context.
Static memory usage per entry comprises a 32 or 64 bit hash key, two
bits for occupancy tracking and the value size stored in the table.
No list heads or pointers are needed. Therefore this data structure
should be quite cache-friendly, too.

It uses linear probing and lazy deletion. During lookups free space
is reclaimed and entries relocated to speed up future lookups.

Signed-off-by: Felix Kuehling <felix.kuehl...@amd.com>
Acked-by: Christian König <christian.koe...@amd.com>
---

This is part of a larger patch series I'm working on to support
demand-paging on AMD Vega10 and similar AMD GPUs. Vega10 generates a
flood of page fault events in its interrupt ring buffer for every
retry. I need a very fast way to discard known page faults in the
interrupt handler to stop processing retry events as early as
possible.

I don't actually need to store any data. All the information I need is
in the hash key itself, which is made up of the fault address and
process ID. I don't need a dynamically resizable data structure
because the hardware will keep retrying. If my hashtable fills up, I
can just discard retry events indiscriminately until space is freed
(by handling older page faults).

In preliminary testing with the builtin self test on a 3.6GHz Core i7,
this code can manage over 120M lookups per second (hits, misses take
about twice as long) with the table 40-50% full. Even 80-90% full it
still remains quite efficient with hits still almost as fast but
misses taking 8x as long as misses.

I'm hoping that people find this useful and consider including it as a
library routine in the kernel.

 include/linux/chash.h | 358 +
 lib/Kconfig   |  24 ++
 lib/Makefile  |   2 +
 lib/chash.c   | 622 ++
 4 files changed, 1006 insertions(+)
 create mode 100644 include/linux/chash.h
 create mode 100644 lib/chash.c

diff --git a/include/linux/chash.h b/include/linux/chash.h
new file mode 100644
index 000..c89b92b
--- /dev/null
+++ b/include/linux/chash.h
@@ -0,0 +1,358 @@
+/*
+ * Copyright 2017 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#ifndef _LINUX_CHASH_H
+#define _LINUX_CHASH_H
+
+#include 
+#include 
+#include 
+#include 
+
+struct __chash_table {
+   u8 bits;
+   u8 key_size;
+   unsigned int value_size;
+   u32 size_mask;
+   unsigned long *occup_bitmap, *valid_bitmap;
+   union {
+   u32 *keys32;
+   u64 *keys64;
+   };
+   u8 *values;
+
+#ifdef CONFIG_CHASH_STATS
+   u64 hits, hits_steps, hits_time_ns;
+   u64 miss, miss_steps, miss_time_ns;
+   u64 relocs, reloc_dist;
+#endif
+};
+
+#define __CHASH_BITMAP_SIZE(bits)  \
+   (((1 << (bits)) + BITS_PER_LONG - 1) / BITS_PER_LONG)
+#define __CHASH_ARRAY_SIZE(bits, size) \
+   size) << (bits)) + sizeof(long) - 1) / sizeof(long))
+
+#define __CHASH_DATA_SIZE(bits, key_size, value_size)  \
+   (__CHASH_BITMAP_SIZE(bits) * 2 +\
+__CHASH_ARRAY_SIZE(bits, key_size) +   \
+__CHASH_ARRAY_SIZE(bits, value_size))
+
+#define STRUCT_CHASH_TABLE(bits, key_size, value_size) \
+   struct {\
+   struct __chash_table table; \
+   unsigned long data  \
+   [__CHASH_DATA_SIZE(bits, key_size, value_size)];\
+   }
+
+/**
+ * struct chash_table - Dynamic

[PATCH] lib: Closed hash table with low overhead

[Felix Kuehling]

This adds a statically sized closed hash table implementation with
low memory and CPU overhead. The API is inspired by kfifo.

Storing, retrieving and deleting data does not involve any dynamic
memory management, which makes it ideal for use in interrupt context.
Static memory usage per entry comprises a 32 or 64 bit hash key, two
bits for occupancy tracking and the value size stored in the table.
No list heads or pointers are needed. Therefore this data structure
should be quite cache-friendly, too.

It uses linear probing and lazy deletion. During lookups free space
is reclaimed and entries relocated to speed up future lookups.

Signed-off-by: Felix Kuehling 
Acked-by: Christian König 
---

This is part of a larger patch series I'm working on to support
demand-paging on AMD Vega10 and similar AMD GPUs. Vega10 generates a
flood of page fault events in its interrupt ring buffer for every
retry. I need a very fast way to discard known page faults in the
interrupt handler to stop processing retry events as early as
possible.

I don't actually need to store any data. All the information I need is
in the hash key itself, which is made up of the fault address and
process ID. I don't need a dynamically resizable data structure
because the hardware will keep retrying. If my hashtable fills up, I
can just discard retry events indiscriminately until space is freed
(by handling older page faults).

In preliminary testing with the builtin self test on a 3.6GHz Core i7,
this code can manage over 120M lookups per second (hits, misses take
about twice as long) with the table 40-50% full. Even 80-90% full it
still remains quite efficient with hits still almost as fast but
misses taking 8x as long as misses.

I'm hoping that people find this useful and consider including it as a
library routine in the kernel.

 include/linux/chash.h | 358 +
 lib/Kconfig   |  24 ++
 lib/Makefile  |   2 +
 lib/chash.c   | 622 ++
 4 files changed, 1006 insertions(+)
 create mode 100644 include/linux/chash.h
 create mode 100644 lib/chash.c

diff --git a/include/linux/chash.h b/include/linux/chash.h
new file mode 100644
index 000..c89b92b
--- /dev/null
+++ b/include/linux/chash.h
@@ -0,0 +1,358 @@
+/*
+ * Copyright 2017 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#ifndef _LINUX_CHASH_H
+#define _LINUX_CHASH_H
+
+#include 
+#include 
+#include 
+#include 
+
+struct __chash_table {
+   u8 bits;
+   u8 key_size;
+   unsigned int value_size;
+   u32 size_mask;
+   unsigned long *occup_bitmap, *valid_bitmap;
+   union {
+   u32 *keys32;
+   u64 *keys64;
+   };
+   u8 *values;
+
+#ifdef CONFIG_CHASH_STATS
+   u64 hits, hits_steps, hits_time_ns;
+   u64 miss, miss_steps, miss_time_ns;
+   u64 relocs, reloc_dist;
+#endif
+};
+
+#define __CHASH_BITMAP_SIZE(bits)  \
+   (((1 << (bits)) + BITS_PER_LONG - 1) / BITS_PER_LONG)
+#define __CHASH_ARRAY_SIZE(bits, size) \
+   size) << (bits)) + sizeof(long) - 1) / sizeof(long))
+
+#define __CHASH_DATA_SIZE(bits, key_size, value_size)  \
+   (__CHASH_BITMAP_SIZE(bits) * 2 +\
+__CHASH_ARRAY_SIZE(bits, key_size) +   \
+__CHASH_ARRAY_SIZE(bits, value_size))
+
+#define STRUCT_CHASH_TABLE(bits, key_size, value_size) \
+   struct {\
+   struct __chash_table table; \
+   unsigned long data  \
+   [__CHASH_DATA_SIZE(bits, key_size, value_size)];\
+   }
+
+/**
+ * struct chash_table - Dynamically allocated closed hash table
+ *
+ * Use this struct for d

Re: [PATCH][drm-next] drm/amdgpu: remove duplicate return statement

[Felix Kuehling]

I must have added that accidentally when cherry-picking an internal
patch for upstreaming. Thanks for catching it.

Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com>

Regards,
  Felix


On 2017-08-23 09:17 AM, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> Remove a redundant identical return statement, it has no use.
>
> Detected by CoverityScan, CID#1454586 ("Structurally dead code")
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> index fb6e5dbd5a03..309f2419c6d8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> @@ -155,7 +155,6 @@ static const struct kfd2kgd_calls kfd2kgd = {
>  struct kfd2kgd_calls *amdgpu_amdkfd_gfx_8_0_get_functions(void)
>  {
>   return (struct kfd2kgd_calls *)
> - return (struct kfd2kgd_calls *)
>  }
>  
>  static inline struct amdgpu_device *get_amdgpu_device(struct kgd_dev *kgd)

Re: [PATCH][drm-next] drm/amdgpu: remove duplicate return statement

[Felix Kuehling]

I must have added that accidentally when cherry-picking an internal
patch for upstreaming. Thanks for catching it.

Reviewed-by: Felix Kuehling 

Regards,
  Felix


On 2017-08-23 09:17 AM, Colin King wrote:
> From: Colin Ian King 
>
> Remove a redundant identical return statement, it has no use.
>
> Detected by CoverityScan, CID#1454586 ("Structurally dead code")
>
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> index fb6e5dbd5a03..309f2419c6d8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> @@ -155,7 +155,6 @@ static const struct kfd2kgd_calls kfd2kgd = {
>  struct kfd2kgd_calls *amdgpu_amdkfd_gfx_8_0_get_functions(void)
>  {
>   return (struct kfd2kgd_calls *)
> - return (struct kfd2kgd_calls *)
>  }
>  
>  static inline struct amdgpu_device *get_amdgpu_device(struct kgd_dev *kgd)

[PATCH] kernel: Export mm_access

[Felix Kuehling]

From: Harish Kasiviswanathan <harish.kasiviswanat...@amd.com>

Exporting mm_access, which is required for implementing accelerated
equivalents of process_vm_readv/process_vm_writev for GPU memory in KFD
(AMD's GPU compute driver). This allows us to apply all the same remote
process memory access policies.

Signed-off-by: Harish Kasiviswanathan <harish.kasiviswanat...@amd.com>
Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com>
---
Current KFD with AMD discrete GPU support is not yet upstream, but we
are working on getting it upstream for 4.13 or 4.14 depending on how we
line up with Dave Airlie's merge windows.

For reference, recent releases of ROCm (AMD's Radeon Open Compute
Platform software stack) with fully open-source user mode and kernel
code can be found on GitHub:
https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver/tree/roc-1.5.0

 kernel/fork.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/fork.c b/kernel/fork.c
index aa1076c..0849896 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1060,6 +1060,7 @@ struct mm_struct *mm_access(struct task_struct
*task, unsigned int mode)

return mm;
 }
+EXPORT_SYMBOL_GPL(mm_access);

 static void complete_vfork_done(struct task_struct *tsk)
 {

[PATCH] kernel: Export mm_access

[Felix Kuehling]

From: Harish Kasiviswanathan 

Exporting mm_access, which is required for implementing accelerated
equivalents of process_vm_readv/process_vm_writev for GPU memory in KFD
(AMD's GPU compute driver). This allows us to apply all the same remote
process memory access policies.

Signed-off-by: Harish Kasiviswanathan 
Reviewed-by: Felix Kuehling 
---
Current KFD with AMD discrete GPU support is not yet upstream, but we
are working on getting it upstream for 4.13 or 4.14 depending on how we
line up with Dave Airlie's merge windows.

For reference, recent releases of ROCm (AMD's Radeon Open Compute
Platform software stack) with fully open-source user mode and kernel
code can be found on GitHub:
https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver/tree/roc-1.5.0

 kernel/fork.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/fork.c b/kernel/fork.c
index aa1076c..0849896 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1060,6 +1060,7 @@ struct mm_struct *mm_access(struct task_struct
*task, unsigned int mode)

return mm;
 }
+EXPORT_SYMBOL_GPL(mm_access);

 static void complete_vfork_done(struct task_struct *tsk)
 {

Re: Enabling peer to peer device transactions for PCIe devices

[Felix Kuehling]

On 16-11-25 12:20 PM, Serguei Sagalovitch wrote:
>
>> A white list may end up being rather complicated if it has to cover
>> different CPU generations and system architectures. I feel this is a
>> decision user space could easily make.
>>
>> Logan
> I agreed that it is better to leave up to user space to check what is
> working
> and what is not. I found that write is practically always working but
> read very
> often not. Also sometimes system BIOS update could fix the issue.
>
But is user mode always aware that P2P is going on or even possible? For
example you may have a library reading a buffer from a file, but it
doesn't necessarily know where that buffer is located (system memory,
VRAM, ...) and it may not know what kind of the device the file is on
(SATA drive, NVMe SSD, ...). The library will never know if all it gets
is a pointer and a file descriptor.

The library ends up calling a read system call. Then it would be up to
the kernel to figure out the most efficient way to read the buffer from
the file. If supported, it could use P2P between a GPU and NVMe where
the NVMe device performs a DMA write to VRAM.

If you put the burden of figuring out the P2P details on user mode code,
I think it will severely limit the use cases that actually take
advantage of it. You also risk a bunch of different implementations that
get it wrong half the time on half the systems out there.

Regards,
  Felix

Re: Enabling peer to peer device transactions for PCIe devices

[Felix Kuehling]

On 16-11-25 12:20 PM, Serguei Sagalovitch wrote:
>
>> A white list may end up being rather complicated if it has to cover
>> different CPU generations and system architectures. I feel this is a
>> decision user space could easily make.
>>
>> Logan
> I agreed that it is better to leave up to user space to check what is
> working
> and what is not. I found that write is practically always working but
> read very
> often not. Also sometimes system BIOS update could fix the issue.
>
But is user mode always aware that P2P is going on or even possible? For
example you may have a library reading a buffer from a file, but it
doesn't necessarily know where that buffer is located (system memory,
VRAM, ...) and it may not know what kind of the device the file is on
(SATA drive, NVMe SSD, ...). The library will never know if all it gets
is a pointer and a file descriptor.

The library ends up calling a read system call. Then it would be up to
the kernel to figure out the most efficient way to read the buffer from
the file. If supported, it could use P2P between a GPU and NVMe where
the NVMe device performs a DMA write to VRAM.

If you put the burden of figuring out the P2P details on user mode code,
I think it will severely limit the use cases that actually take
advantage of it. You also risk a bunch of different implementations that
get it wrong half the time on half the systems out there.

Regards,
  Felix

Re: Enabling peer to peer device transactions for PCIe devices

[Felix Kuehling]

On 16-11-25 03:40 PM, Christian König wrote:
> Am 25.11.2016 um 20:32 schrieb Jason Gunthorpe:
>> This assumes the commands are fairly short lived of course, the
>> expectation of the mmu notifiers is that a flush is reasonably prompt
>
> Correct, this is another problem. GFX command submissions usually
> don't take longer than a few milliseconds, but compute command
> submission can easily take multiple hours.
>
> I can easily imagine what would happen when kswapd is blocked by a GPU
> command submission for an hour or so while the system is under memory
> pressure :)
>
> I'm thinking on this problem for about a year now and going in circles
> for quite a while. So if you have ideas on this even if they sound
> totally crazy, feel free to come up.

Our GPUs (at least starting with VI) support compute-wave-save-restore
and can swap out compute queues with fairly low latency. Yes, there is
some overhead (both memory usage and time), but it's a fairly regular
thing with our hardware scheduler (firmware, actually) when we need to
preempt running compute queues to update runlists or we overcommit the
hardware queue resources.

Regards,
  Felix

Re: Enabling peer to peer device transactions for PCIe devices

[Felix Kuehling]

On 16-11-25 03:40 PM, Christian König wrote:
> Am 25.11.2016 um 20:32 schrieb Jason Gunthorpe:
>> This assumes the commands are fairly short lived of course, the
>> expectation of the mmu notifiers is that a flush is reasonably prompt
>
> Correct, this is another problem. GFX command submissions usually
> don't take longer than a few milliseconds, but compute command
> submission can easily take multiple hours.
>
> I can easily imagine what would happen when kswapd is blocked by a GPU
> command submission for an hour or so while the system is under memory
> pressure :)
>
> I'm thinking on this problem for about a year now and going in circles
> for quite a while. So if you have ideas on this even if they sound
> totally crazy, feel free to come up.

Our GPUs (at least starting with VI) support compute-wave-save-restore
and can swap out compute queues with fairly low latency. Yes, there is
some overhead (both memory usage and time), but it's a fairly regular
thing with our hardware scheduler (firmware, actually) when we need to
preempt running compute queues to update runlists or we overcommit the
hardware queue resources.

Regards,
  Felix

Re: Lockdep incorrectly complaining about circular dependencies involving read-locks

[Felix Kuehling]

On 16-01-21 05:20 PM, Felix Kuehling wrote:
> I'm running into circular lock dependencies reported by lockdep that
> involve read-locks and should not be flagged as deadlocks at all. I
> wrote a very simple test function that demonstrates the problem:
[snip]
> It sets up a circular lock dependency between a mutex and a read-write
> semaphore. However, the semaphore is only ever locked for reading.

The same expirement with a spinlock and an rwlock works correctly. The
difference is that the rwlock allows recursive read-locks, whereas the
rw semaphore does not. Recursive read-locks are not added to the lock
dependency graph at all, hence they don't trip the circular lock checks.

I think this handling of recursive read locks is actually incorrect,
because it fails to notice potential circular deadlocks like this:
> spinlock_t fktest_sp;
> rwlock_t fktest_rw;
>
> spin_lock_init(_sp);
> rwlock_init(_rw);
>
> read_lock(_rw);
> spin_lock(_sp);
> spin_unlock(_sp);
> read_unlock(_rw);
>
> spin_lock(_sp);
> write_lock(_rw);
> write_unlock(_rw);
> spin_unlock(_sp);
In a possible deadlock scenario thread A takes read lock, thread B takes
spin lock, thread A blocks trying to take spin lock, thread B blocks
trying to take write lock.

The read-lock depending on the spinlock is never entered into the
dependency graph. Hence the circular dependency is not seen when the
write lock is taken.

I think the whole handling of read-locks needs to be revamped for proper
detection of circular lock dependencies without false positives or false
negatives (I have demonstrated one example of each with the current
implementation).

A dependency chain must be able to distinguish, whether a lock is taken
for reading or for writing. That means, read locks need their own lock
(sub-)class. A new write dependency (a lock that depends on holding a
write-lock) creates a dependency on both the read and write-lock
classes, because both cases can lead to a potential deadlock. A new read
dependency (a lock that depends on holding a read-lock) only creates a
dependency on the write-lock class, because read locks don't block each
other.

I'm going to prototype my idea. I can use sub-classes for making
separate read and write lock classes. But that means, effectively I only
have half as many usable lock sub-classes left available for nesting
annotations on lock primitives that support read-locking. I hope that's
not a problem.

Feedback welcome.

Regards,
  Felix

-- 
F e l i x   K u e h l i n g
SMTS Software Development Engineer | Vertical Workstation/Compute
1 Commerce Valley Dr. East, Markham, ON L3T 7X6 Canada
(O) +1(289)695-1597
   _ _   _   _   _
  / \   | \ / | |  _  \  \ _  |
 / A \  | \M/ | | |D) )  /|_| |
/_/ \_\ |_| |_| |_/ |__/ \|   facebook.com/AMD | amd.com

Re: Lockdep incorrectly complaining about circular dependencies involving read-locks

[Felix Kuehling]

On 16-01-21 05:20 PM, Felix Kuehling wrote:
> I'm running into circular lock dependencies reported by lockdep that
> involve read-locks and should not be flagged as deadlocks at all. I
> wrote a very simple test function that demonstrates the problem:
[snip]
> It sets up a circular lock dependency between a mutex and a read-write
> semaphore. However, the semaphore is only ever locked for reading.

The same expirement with a spinlock and an rwlock works correctly. The
difference is that the rwlock allows recursive read-locks, whereas the
rw semaphore does not. Recursive read-locks are not added to the lock
dependency graph at all, hence they don't trip the circular lock checks.

I think this handling of recursive read locks is actually incorrect,
because it fails to notice potential circular deadlocks like this:
> spinlock_t fktest_sp;
> rwlock_t fktest_rw;
>
> spin_lock_init(_sp);
> rwlock_init(_rw);
>
> read_lock(_rw);
> spin_lock(_sp);
> spin_unlock(_sp);
> read_unlock(_rw);
>
> spin_lock(_sp);
> write_lock(_rw);
> write_unlock(_rw);
> spin_unlock(_sp);
In a possible deadlock scenario thread A takes read lock, thread B takes
spin lock, thread A blocks trying to take spin lock, thread B blocks
trying to take write lock.

The read-lock depending on the spinlock is never entered into the
dependency graph. Hence the circular dependency is not seen when the
write lock is taken.

I think the whole handling of read-locks needs to be revamped for proper
detection of circular lock dependencies without false positives or false
negatives (I have demonstrated one example of each with the current
implementation).

A dependency chain must be able to distinguish, whether a lock is taken
for reading or for writing. That means, read locks need their own lock
(sub-)class. A new write dependency (a lock that depends on holding a
write-lock) creates a dependency on both the read and write-lock
classes, because both cases can lead to a potential deadlock. A new read
dependency (a lock that depends on holding a read-lock) only creates a
dependency on the write-lock class, because read locks don't block each
other.

I'm going to prototype my idea. I can use sub-classes for making
separate read and write lock classes. But that means, effectively I only
have half as many usable lock sub-classes left available for nesting
annotations on lock primitives that support read-locking. I hope that's
not a problem.

Feedback welcome.

Regards,
  Felix

-- 
F e l i x   K u e h l i n g
SMTS Software Development Engineer | Vertical Workstation/Compute
1 Commerce Valley Dr. East, Markham, ON L3T 7X6 Canada
(O) +1(289)695-1597
   _ _   _   _   _
  / \   | \ / | |  _  \  \ _  |
 / A \  | \M/ | | |D) )  /|_| |
/_/ \_\ |_| |_| |_/ |__/ \|   facebook.com/AMD | amd.com

Lockdep incorrectly complaining about circular dependencies involving read-locks

[Felix Kuehling]

I'm running into circular lock dependencies reported by lockdep that
involve read-locks and should not be flagged as deadlocks at all. I
wrote a very simple test function that demonstrates the problem:

> static void test_lockdep(void)
> {
>   struct mutex fktest_m;
>   struct rw_semaphore fktest_s;
>
>   mutex_init(_m);
>   init_rwsem(_s);
>
>   down_read(_s);
>   mutex_lock(_m);
>   mutex_unlock(_m);
>   up_read(_s);
>
>   mutex_lock(_m);
>   down_read(_s);
>   up_read(_s);
>   mutex_unlock(_m);
>
>   mutex_destroy(_m);
> }

It sets up a circular lock dependency between a mutex and a read-write
semaphore. However, the semaphore is only ever locked for reading. As I
understand it, there is no potential for a deadlock here because
multiple readers don't exclude each other. However, I get this:

> [   10.832547] 
> [   10.834122] ==
> [   10.840655] [ INFO: possible circular locking dependency detected ]
> [   10.847284] 4.4.0-kfd #3 Tainted: GE  
> [   10.852356] ---
> [   10.858989] systemd-udevd/2385 is trying to acquire lock:
> [   10.864695]  (_s){.+.+..}, at: [] 
> test_lockdep+0x9a/0xd4 [amdgpu]
> [   10.873474] 
> [   10.873474] but task is already holding lock:
> [   10.879633]  (_m){+.+...}, at: [] 
> test_lockdep+0x8e/0xd4 [amdgpu]
> [   10.888418] 
> [   10.888418] which lock already depends on the new lock.
> [   10.888418] 
> [   10.897071] 
> [   10.897071] the existing dependency chain (in reverse order) is:
> [   10.904981] 
> -> #1 (_m){+.+...}:
> [   10.909138][] lock_acquire+0x6d/0x90
> [   10.915309][] mutex_lock_nested+0x4a/0x3a0
> [   10.922040][] test_lockdep+0x68/0xd4 [amdgpu]
> [   10.929042][] amdgpu_init+0x9/0x7b [amdgpu]
> [   10.935856][] do_one_initcall+0xc8/0x200
> [   10.942449][] do_init_module+0x56/0x1d8
> [   10.948919][] load_module+0x1b91/0x2460
> [   10.955376][] SyS_finit_module+0x7b/0xa0
> [   10.961961][] entry_SYSCALL_64_fastpath+0x12/0x76
> [   10.969388] 
> -> #0 (_s){.+.+..}:
> [   10.973569][] __lock_acquire+0x100a/0x16b0
> [   10.980315][] lock_acquire+0x6d/0x90
> [   10.986502][] down_read+0x34/0x50
> [   11.002586][] test_lockdep+0x9a/0xd4 [amdgpu]
> [   11.009610][] amdgpu_init+0x9/0x7b [amdgpu]
> [   11.016453][] do_one_initcall+0xc8/0x200
> [   11.023001][] do_init_module+0x56/0x1d8
> [   11.029462][] load_module+0x1b91/0x2460
> [   11.035927][] SyS_finit_module+0x7b/0xa0
> [   11.042478][] entry_SYSCALL_64_fastpath+0x12/0x76
> [   11.049860] 
> [   11.049860] other info that might help us debug this:
> [   11.049860] 
> [   11.058356]  Possible unsafe locking scenario:
> [   11.058356] 
> [   11.064644]CPU0CPU1
> [   11.069436]
> [   11.074229]   lock(_m);
> [   11.077465]lock(_s);
> [   11.083376]lock(_m);
> [   11.089288]   lock(_s);
> [   11.092542] 
> [   11.092542]  *** DEADLOCK ***
> [   11.092542] 
> [   11.098819] 1 lock held by systemd-udevd/2385:
> [   11.103530]  #0:  (_m){+.+...}, at: [] 
> test_lockdep+0x8e/0xd4 [amdgpu]
> [   11.112780] 
> [   11.112780] stack backtrace:
> [   11.117388] CPU: 7 PID: 2385 Comm: systemd-udevd Tainted: GE   
> 4.4.0-kfd #3
> [   11.125840] Hardware name: ASUS All Series/Z97-PRO(Wi-Fi ac)/USB 3.1, BIOS 
> 2401 04/27/2015
> [   11.134593]  82714a90 8808335d7a70 8144e3cb 
> 82714a90
> [   11.142421]  8808335d7ab0 8113ec7a 8808335d7b00 
> 
> [   11.150248]  8808335a9ee0 8808335a9f08 8808335a96c0 
> 8808335a9f08
> [   11.158076] Call Trace:
> [   11.160655]  [] dump_stack+0x44/0x59
> [   11.166105]  [] print_circular_bug+0x1f9/0x207
> [   11.172457]  [] __lock_acquire+0x100a/0x16b0
> [   11.178619]  [] ? mark_held_locks+0x66/0x90
> [   11.184693]  [] ? 0xc0295000
> [   11.190127]  [] lock_acquire+0x6d/0x90
> [   11.195758]  [] ? test_lockdep+0x9a/0xd4 [amdgpu]
> [   11.202376]  [] down_read+0x34/0x50
> [   11.207729]  [] ? test_lockdep+0x9a/0xd4 [amdgpu]
> [   11.214370]  [] test_lockdep+0x9a/0xd4 [amdgpu]
> [   11.220847]  [] amdgpu_init+0x9/0x7b [amdgpu]
> [   11.227096]  [] do_one_initcall+0xc8/0x200
> [   11.233083]  [] ? do_init_module+0x1d/0x1d8
> [   11.239153]  [] ? kmem_cache_alloc+0xbf/0x180
> [   11.245421]  [] do_init_module+0x56/0x1d8
> [   11.251307]  [] load_module+0x1b91/0x2460
> [   11.257196]  [] ? __symbol_put+0x30/0x30
> [   11.262993]  [] ? copy_module_from_fd.isra.61+0xf6/0x150
> [   11.270261]  [] SyS_finit_module+0x7b/0xa0
> [   11.276250]  [] entry_SYSCALL_64_fastpath+0x12/0x76

I confirmed my results with the latest master branch of

Lockdep incorrectly complaining about circular dependencies involving read-locks

[Felix Kuehling]

I'm running into circular lock dependencies reported by lockdep that
involve read-locks and should not be flagged as deadlocks at all. I
wrote a very simple test function that demonstrates the problem:

> static void test_lockdep(void)
> {
>   struct mutex fktest_m;
>   struct rw_semaphore fktest_s;
>
>   mutex_init(_m);
>   init_rwsem(_s);
>
>   down_read(_s);
>   mutex_lock(_m);
>   mutex_unlock(_m);
>   up_read(_s);
>
>   mutex_lock(_m);
>   down_read(_s);
>   up_read(_s);
>   mutex_unlock(_m);
>
>   mutex_destroy(_m);
> }

It sets up a circular lock dependency between a mutex and a read-write
semaphore. However, the semaphore is only ever locked for reading. As I
understand it, there is no potential for a deadlock here because
multiple readers don't exclude each other. However, I get this:

> [   10.832547] 
> [   10.834122] ==
> [   10.840655] [ INFO: possible circular locking dependency detected ]
> [   10.847284] 4.4.0-kfd #3 Tainted: GE  
> [   10.852356] ---
> [   10.858989] systemd-udevd/2385 is trying to acquire lock:
> [   10.864695]  (_s){.+.+..}, at: [] 
> test_lockdep+0x9a/0xd4 [amdgpu]
> [   10.873474] 
> [   10.873474] but task is already holding lock:
> [   10.879633]  (_m){+.+...}, at: [] 
> test_lockdep+0x8e/0xd4 [amdgpu]
> [   10.888418] 
> [   10.888418] which lock already depends on the new lock.
> [   10.888418] 
> [   10.897071] 
> [   10.897071] the existing dependency chain (in reverse order) is:
> [   10.904981] 
> -> #1 (_m){+.+...}:
> [   10.909138][] lock_acquire+0x6d/0x90
> [   10.915309][] mutex_lock_nested+0x4a/0x3a0
> [   10.922040][] test_lockdep+0x68/0xd4 [amdgpu]
> [   10.929042][] amdgpu_init+0x9/0x7b [amdgpu]
> [   10.935856][] do_one_initcall+0xc8/0x200
> [   10.942449][] do_init_module+0x56/0x1d8
> [   10.948919][] load_module+0x1b91/0x2460
> [   10.955376][] SyS_finit_module+0x7b/0xa0
> [   10.961961][] entry_SYSCALL_64_fastpath+0x12/0x76
> [   10.969388] 
> -> #0 (_s){.+.+..}:
> [   10.973569][] __lock_acquire+0x100a/0x16b0
> [   10.980315][] lock_acquire+0x6d/0x90
> [   10.986502][] down_read+0x34/0x50
> [   11.002586][] test_lockdep+0x9a/0xd4 [amdgpu]
> [   11.009610][] amdgpu_init+0x9/0x7b [amdgpu]
> [   11.016453][] do_one_initcall+0xc8/0x200
> [   11.023001][] do_init_module+0x56/0x1d8
> [   11.029462][] load_module+0x1b91/0x2460
> [   11.035927][] SyS_finit_module+0x7b/0xa0
> [   11.042478][] entry_SYSCALL_64_fastpath+0x12/0x76
> [   11.049860] 
> [   11.049860] other info that might help us debug this:
> [   11.049860] 
> [   11.058356]  Possible unsafe locking scenario:
> [   11.058356] 
> [   11.064644]CPU0CPU1
> [   11.069436]
> [   11.074229]   lock(_m);
> [   11.077465]lock(_s);
> [   11.083376]lock(_m);
> [   11.089288]   lock(_s);
> [   11.092542] 
> [   11.092542]  *** DEADLOCK ***
> [   11.092542] 
> [   11.098819] 1 lock held by systemd-udevd/2385:
> [   11.103530]  #0:  (_m){+.+...}, at: [] 
> test_lockdep+0x8e/0xd4 [amdgpu]
> [   11.112780] 
> [   11.112780] stack backtrace:
> [   11.117388] CPU: 7 PID: 2385 Comm: systemd-udevd Tainted: GE   
> 4.4.0-kfd #3
> [   11.125840] Hardware name: ASUS All Series/Z97-PRO(Wi-Fi ac)/USB 3.1, BIOS 
> 2401 04/27/2015
> [   11.134593]  82714a90 8808335d7a70 8144e3cb 
> 82714a90
> [   11.142421]  8808335d7ab0 8113ec7a 8808335d7b00 
> 
> [   11.150248]  8808335a9ee0 8808335a9f08 8808335a96c0 
> 8808335a9f08
> [   11.158076] Call Trace:
> [   11.160655]  [] dump_stack+0x44/0x59
> [   11.166105]  [] print_circular_bug+0x1f9/0x207
> [   11.172457]  [] __lock_acquire+0x100a/0x16b0
> [   11.178619]  [] ? mark_held_locks+0x66/0x90
> [   11.184693]  [] ? 0xc0295000
> [   11.190127]  [] lock_acquire+0x6d/0x90
> [   11.195758]  [] ? test_lockdep+0x9a/0xd4 [amdgpu]
> [   11.202376]  [] down_read+0x34/0x50
> [   11.207729]  [] ? test_lockdep+0x9a/0xd4 [amdgpu]
> [   11.214370]  [] test_lockdep+0x9a/0xd4 [amdgpu]
> [   11.220847]  [] amdgpu_init+0x9/0x7b [amdgpu]
> [   11.227096]  [] do_one_initcall+0xc8/0x200
> [   11.233083]  [] ? do_init_module+0x1d/0x1d8
> [   11.239153]  [] ? kmem_cache_alloc+0xbf/0x180
> [   11.245421]  [] do_init_module+0x56/0x1d8
> [   11.251307]  [] load_module+0x1b91/0x2460
> [   11.257196]  [] ? __symbol_put+0x30/0x30
> [   11.262993]  [] ? copy_module_from_fd.isra.61+0xf6/0x150
> [   11.270261]  [] SyS_finit_module+0x7b/0xa0
> [   11.276250]  [] entry_SYSCALL_64_fastpath+0x12/0x76

I confirmed my results with the latest master branch of