Re: [PATCH] drm/amdkfd: return negative error code in svm_ioctl()
On 2024-03-25 02:31, Su Hui wrote:
svm_ioctl() should return negative error code in default case.
Fixes: 42de677f7999 ("drm/amdkfd: register svm range")
Signed-off-by: Su Hui
Good catch, ioctl should return -errno. I will apply it to
drm-next.
Reviewed-by: Philip Yang
---
Ps: When I try to compile this file, there is a error :
drivers/gpu/drm/amd/amdkfd/kfd_migrate.c:28:10: fatal error: amdgpu_sync.h:
No such file or directory.
Maybe there are some steps I missed or this place need to be corrected?
Don't know how you compile the driver, amdgpu_sync.h is located
under amdgpu folder, amdkfd/Makefile is included from
amdgpu/Makefile, which set ccflag-y -I correctly.
Regards,
Philip
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index f0f7f48af413..41c376f3fd27 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -4147,7 +4147,7 @@ svm_ioctl(struct kfd_process *p, enum kfd_ioctl_svm_op op, uint64_t start,
r = svm_range_get_attr(p, mm, start, size, nattrs, attrs);
break;
default:
- r = EINVAL;
+ r = -EINVAL;
break;
}
Re: [PATCH] drm/prime: Support page array >= 4GB
On 2023-08-22 05:43, Christian König
wrote:
Am 21.08.23 um 22:02 schrieb Philip Yang:
Without unsigned long typecast, the size
is passed in as zero if page
array size >= 4GB, nr_pages >= 0x10, then sg list
converted will
have the first and the last chunk lost.
Good catch, but I'm not sure if this is enough to make it work.
Additional to that I don't think we have an use case for BOs >
4GiB.
>4GB buffer is normal for compute applications, the issue is
reported by "Maelstrom generated exerciser detects micompares when
GPU accesses larger remote GPU memory." on GFX 9.4.3 APU, which
uses GTT domain to allocate VRAM, and trigger the bug in this drm
prime helper. With this fix, the test passed.
Regards,
Philip
Christian.
Signed-off-by: Philip Yang
---
drivers/gpu/drm/drm_prime.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c
b/drivers/gpu/drm/drm_prime.c
index f924b8b4ab6b..2630ad2e504d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -830,7 +830,7 @@ struct sg_table
*drm_prime_pages_to_sg(struct drm_device *dev,
if (max_segment == 0)
max_segment = UINT_MAX;
err = sg_alloc_table_from_pages_segment(sg, pages,
nr_pages, 0,
- nr_pages << PAGE_SHIFT,
+ (unsigned long)nr_pages <<
PAGE_SHIFT,
max_segment, GFP_KERNEL);
if (err) {
kfree(sg);
[PATCH] drm/prime: Support page array >= 4GB
Without unsigned long typecast, the size is passed in as zero if page
array size >= 4GB, nr_pages >= 0x10, then sg list converted will
have the first and the last chunk lost.
Signed-off-by: Philip Yang
---
drivers/gpu/drm/drm_prime.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index f924b8b4ab6b..2630ad2e504d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -830,7 +830,7 @@ struct sg_table *drm_prime_pages_to_sg(struct drm_device
*dev,
if (max_segment == 0)
max_segment = UINT_MAX;
err = sg_alloc_table_from_pages_segment(sg, pages, nr_pages, 0,
- nr_pages << PAGE_SHIFT,
+ (unsigned long)nr_pages <<
PAGE_SHIFT,
max_segment, GFP_KERNEL);
if (err) {
kfree(sg);
--
2.35.1
Re: Why does kgd2kfd_interrupt() have to schedule work on a specific CPU?
This was to fix application long event wait latency, when app
shader generates lots of event interrupts in short period, the
scheduled work no chance to execute on the same CPU core, this
causes event cannot post/return to app thread which are waiting
the event. To schedule work on the core of same NUMA node is to
optimize cache usage in general.
Regards
Philip
On 2023-06-27 11:42, Alex Deucher
wrote:
+Felix, Philip
On Tue, Jun 27, 2023 at 4:42 AM Philipp Stanner wrote:
Hello folks,
I'm currently trying to learn more about DRM and discovered the
following code sequence:
drivers/gpu/drm/amd/amdkfd/kfd_device.c, Line 824 on 6.4-rc7
static inline void kfd_queue_work(struct workqueue_struct *wq,
struct work_struct *work)
{
int cpu, new_cpu;
cpu = new_cpu = smp_processor_id();
do {
new_cpu = cpumask_next(new_cpu, cpu_online_mask) % nr_cpu_ids;
if (cpu_to_node(new_cpu) == numa_node_id())
break;
} while (cpu != new_cpu);
queue_work_on(new_cpu, wq, work);
}
/* This is called directly from KGD at ISR. */
void kgd2kfd_interrupt(struct kfd_dev *kfd, const void *ih_ring_entry)
{
uint32_t patched_ihre[KFD_MAX_RING_ENTRY_SIZE];
bool is_patched = false;
unsigned long flags;
if (!kfd->init_complete)
return;
if (kfd->device_info.ih_ring_entry_size > sizeof(patched_ihre)) {
dev_err_once(kfd_device, "Ring entry too small\n");
return;
}
spin_lock_irqsave(>interrupt_lock, flags);
if (kfd->interrupts_active
&& interrupt_is_wanted(kfd, ih_ring_entry,
patched_ihre, _patched)
&& enqueue_ih_ring_entry(kfd,
is_patched ? patched_ihre : ih_ring_entry))
kfd_queue_work(kfd->ih_wq, >interrupt_work);
spin_unlock_irqrestore(>interrupt_lock, flags);
}
These functions seem to be exclusively invoked by amdgpu_irq_dispatch()
in amdgpu_irq.c
At first glance it seems to me that it's just a typical scenario taking
place here: Interrupt arises, interrupt submits work to wq, then jumps
back to sleep / former process execution context again.
What I don't understand is why it's apparently important to schedule
the work on a particular CPU.
It seems that the do-while in kfd_queue_work() is searching for a CPU
within the same NUMA-Node. Thus I suspect that this is done because
either
a) performance requires it or
b) the work-function needs access to something that's only available
within the same node.
I suspect there is an interrupt-related reason why that particular work
should be enqueued on a specific CPU. Just by reading the code alone I
can't really figure out why precisely that's necessary, though.
Does someone have any hints for me? :)
Cheers,
Philipp
Re: [PATCH 1/1] drm/amdkfd: Protect the Client whilst it is being operated on
On 2022-03-17 11:13 a.m., Lee Jones wrote: On Thu, 17 Mar 2022, Felix Kuehling wrote: Am 2022-03-17 um 11:00 schrieb Lee Jones: Good afternoon Felix, Thanks for your review. Am 2022-03-17 um 09:16 schrieb Lee Jones: Presently the Client can be freed whilst still in use. Use the already provided lock to prevent this. Cc: Felix Kuehling Cc: Alex Deucher Cc: "Christian König" Cc: "Pan, Xinhui" Cc: David Airlie Cc: Daniel Vetter Cc: amd-...@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: Lee Jones --- drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c index e4beebb1c80a2..3b9ac1e87231f 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c @@ -145,8 +145,11 @@ static int kfd_smi_ev_release(struct inode *inode, struct file *filep) spin_unlock(>smi_lock); synchronize_rcu(); + + spin_lock(>lock); kfifo_free(>fifo); kfree(client); + spin_unlock(>lock); The spin_unlock is after the spinlock data structure has been freed. Good point. If we go forward with this approach the unlock should perhaps be moved to just before the kfree(). There should be no concurrent users here, since we are freeing the data structure. If there still are concurrent users at this point, they will crash anyway. So the locking is unnecessary. The users may well crash, as does the kernel unfortunately. We only get to kfd_smi_ev_release when the file descriptor is closed. User mode has no way to use the client any more at this point. This function also removes the client from the dev->smi_cllients list. So no more events will be added to the client. Therefore it is safe to free the client. If any of the above were not true, it would not be safe to kfree(client). But if it is safe to kfree(client), then there is no need for the locking. I'm not keen to go into too much detail until it's been patched. However, there is a way to free the client while it is still in use. Remember we are multi-threaded. files_struct->count refcount is used to handle this race, as vfs_read/vfs_write takes file refcount and fput calls release only if refcount is 1, to guarantee that read/write from user space is finished here. Another race is driver add_event_to_kfifo while closing the handler. We use rcu_read_lock in add_event_to_kfifo, and kfd_smi_ev_release calls synchronize_rcu to wait for all rcu_read done. So it is safe to call kfifo_free(>fifo) and kfree(client). Regards, Philip
Re: [PATCH 1/1] drm/amdkfd: Replace zero-length array with flexible-array member
On 2022-02-15 7:38 p.m., Felix Kuehling
wrote:
Reference:
https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays
CC: Changcheng Deng
Signed-off-by: Felix Kuehling
Reviewed-by: Philip Yang
---
include/uapi/linux/kfd_ioctl.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 6e4268f5e482..baec5a41de3e 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -673,7 +673,7 @@ struct kfd_ioctl_svm_args {
__u32 op;
__u32 nattr;
/* Variable length array of attributes */
- struct kfd_ioctl_svm_attribute attrs[0];
+ struct kfd_ioctl_svm_attribute attrs[];
};
/**
Re: [Patch v4 23/24] drm/amdkfd: CRIU prepare for svm resume
On 2022-01-10 6:58 p.m., Felix Kuehling
wrote:
On
2022-01-05 9:43 a.m., philip yang wrote:
On 2021-12-22 7:37 p.m., Rajneesh Bhardwaj wrote:
During CRIU restore phase, the VMAs for
the virtual address ranges are
not at their final location yet so in this stage, only cache
the data
required to successfully resume the svm ranges during an
imminent CRIU
resume phase.
Signed-off-by: Rajneesh
Bhardwaj
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 4 +-
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 5 ++
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 99
drivers/gpu/drm/amd/amdkfd/kfd_svm.h | 12 +++
4 files changed, 118 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 916b8d000317..f7aa15b18f95 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -2638,8 +2638,8 @@ static int criu_restore_objects(struct
file *filep,
goto exit;
break;
case KFD_CRIU_OBJECT_TYPE_SVM_RANGE:
- /* TODO: Implement SVM range */
- *priv_offset += sizeof(struct
kfd_criu_svm_range_priv_data);
+ ret = kfd_criu_restore_svm(p, (uint8_t __user
*)args->priv_data,
+ priv_offset,
max_priv_data_size);
if (ret)
goto exit;
break;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 87eb6739a78e..92191c541c29 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -790,6 +790,7 @@ struct svm_range_list {
struct list_head list;
struct work_struct deferred_list_work;
struct list_head deferred_range_list;
+ struct list_head criu_svm_metadata_list;
spinlock_t deferred_list_lock;
atomic_t evicted_ranges;
bool drain_pagefaults;
@@ -1148,6 +1149,10 @@ int kfd_criu_restore_event(struct file
*devkfd,
uint8_t __user *user_priv_data,
uint64_t *priv_data_offset,
uint64_t max_priv_data_size);
+int kfd_criu_restore_svm(struct kfd_process *p,
+ uint8_t __user *user_priv_data,
+ uint64_t *priv_data_offset,
+ uint64_t max_priv_data_size);
/* CRIU - End */
/* Queue Context Management */
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 6d59f1bedcf2..e9f6c63c2a26 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -45,6 +45,14 @@
*/
#define AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING 2000
+struct criu_svm_metadata {
+ struct list_head list;
+ __u64 start_addr;
+ __u64 size;
+ /* Variable length array of attributes */
+ struct kfd_ioctl_svm_attribute attrs[0];
+};
This data structure is struct kfd_criu_svm_range_priv_data plus
list_head, maybe you can add list_head to struct
kfd_criu_svm_range_priv_data and remove this new data structure,
then you can remove extra kzalloc, kfree for each svm object
resume and function svm_criu_prepare_for_resume could be
removed.
Adding list_head to the private structure is a bad idea, because
that structure is copied to/from user mode. Kernel m
Re: [Patch v4 18/24] drm/amdkfd: CRIU checkpoint and restore xnack mode
On 2022-01-10 7:10 p.m., Felix Kuehling
wrote:
On
2022-01-05 10:22 a.m., philip yang wrote:
On 2021-12-22 7:37 p.m., Rajneesh Bhardwaj wrote:
Recoverable page faults are represented
by the xnack mode setting inside
a kfd process and are used to represent the device page
faults. For CR,
we don't consider negative values which are typically used for
querying
the current xnack mode without modifying it.
Signed-off-by: Rajneesh
Bhardwaj
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15
+++
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 1 +
2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 178b0ccfb286..446eb9310915 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1845,6 +1845,11 @@ static int
criu_checkpoint_process(struct kfd_process *p,
memset(_priv, 0, sizeof(process_priv));
process_priv.version = KFD_CRIU_PRIV_VERSION;
+ /* For CR, we don't consider negative xnack mode which is
used for
+ * querying without changing it, here 0 simply means
disabled and 1
+ * means enabled so retry for finding a valid PTE.
+ */
Negative value to query xnack mode is for
kfd_ioctl_set_xnack_mode user space ioctl interface, which is
not used by CRIU, I think this comment is misleading,
+ process_priv.xnack_mode =
p->xnack_enabled ? 1 : 0;
change to process_priv.xnack_enabled
ret =
copy_to_user(user_priv_data + *priv_offset,
_priv, sizeof(process_priv));
@@ -2231,6 +2236,16 @@ static int criu_restore_process(struct
kfd_process *p,
return -EINVAL;
}
+ pr_debug("Setting XNACK mode\n");
+ if (process_priv.xnack_mode &&
!kfd_process_xnack_mode(p, true)) {
+ pr_err("xnack mode cannot be set\n");
+ ret = -EPERM;
+ goto exit;
+ } else {
On GFXv9 GPUs except Aldebaran, this means the process
checkpointed is xnack off, it can restore and resume on GPU with
xnack on, then shader will continue running successfully, but
driver is not guaranteed to map svm ranges on GPU all the time,
if retry fault happens, the shader will not recover. Maybe
change to:
If (KFD_GC_VERSION(dev) != IP_VERSION(9, 4, 2) {
The code here was correct. The xnack mode applies to the whole
process, not just one GPU. The logic for checking the capabilities
of all GPUs is already in kfd_process_xnack_mode. If XNACK cannot
be supported by all GPUs, restoring a non-0 XNACK mode will fail.
Any GPU can run in XNACK-disabled mode. So we don't need any
limitations for process_priv.xnack_enabled == 0.
Yes, the code was correct, for case all GPUs dev->noretry=0
(xnack on), process->xnack_enabled=0, we unmap the queues while
migrating, guarantee to map svm ranges on GPUs then resume queues.
If retry fault happens, we don't recover the fault, report the
fault to user space. That is all correct.
Regards,
Philip
Regards,
Felix
if (process_priv.xnack_enabled !=
kfd_process_xnack_mode(p, true)) {
pr_err("xnack mode cannot be set\n");
ret = -EPERM;
goto exit;
}
}
pr_debug("set xnack mode: %d\n", process_priv.xnack_enabled);
p->xnack_enabled = process_priv.xnack_enabled;
+ pr_debug("set xnack mode:
%d\n", process_priv.xnack_mode);
+ p->xnack_enabled = process_priv.xnack_mode;
Re: [Patch v4 21/24] drm/amdkfd: CRIU Discover svm ranges
On 2021-12-22 7:37 p.m., Rajneesh
Bhardwaj wrote:
A KFD process may contain a number of virtual address ranges for shared
virtual memory management and each such range can have many SVM
attributes spanning across various nodes within the process boundary.
This change reports the total number of such SVM ranges and
their total private data size by extending the PROCESS_INFO op of the the
CRIU IOCTL to discover the svm ranges in the target process and a future
patches brings in the required support for checkpoint and restore for
SVM ranges.
Signed-off-by: Rajneesh Bhardwaj
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 12 +++--
drivers/gpu/drm/amd/amdkfd/kfd_priv.h| 5 +-
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 60
drivers/gpu/drm/amd/amdkfd/kfd_svm.h | 11 +
4 files changed, 82 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 446eb9310915..1c25d5e9067c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -2089,10 +2089,9 @@ static int criu_get_process_object_info(struct kfd_process *p,
uint32_t *num_objects,
uint64_t *objs_priv_size)
{
- int ret;
- uint64_t priv_size;
+ uint64_t queues_priv_data_size, svm_priv_data_size, priv_size;
uint32_t num_queues, num_events, num_svm_ranges;
- uint64_t queues_priv_data_size;
+ int ret;
*num_devices = p->n_pdds;
*num_bos = get_process_num_bos(p);
@@ -2102,7 +2101,10 @@ static int criu_get_process_object_info(struct kfd_process *p,
return ret;
num_events = kfd_get_num_events(p);
- num_svm_ranges = 0; /* TODO: Implement SVM-Ranges */
+
+ ret = svm_range_get_info(p, _svm_ranges, _priv_data_size);
+ if (ret)
+ return ret;
*num_objects = num_queues + num_events + num_svm_ranges;
@@ -2112,7 +2114,7 @@ static int criu_get_process_object_info(struct kfd_process *p,
priv_size += *num_bos * sizeof(struct kfd_criu_bo_priv_data);
priv_size += queues_priv_data_size;
priv_size += num_events * sizeof(struct kfd_criu_event_priv_data);
- /* TODO: Add SVM ranges priv size */
+ priv_size += svm_priv_data_size;
*objs_priv_size = priv_size;
}
return 0;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index d72dda84c18c..87eb6739a78e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -1082,7 +1082,10 @@ enum kfd_criu_object_type {
struct kfd_criu_svm_range_priv_data {
uint32_t object_type;
- uint64_t reserved;
+ uint64_t start_addr;
+ uint64_t size;
+ /* Variable length array of attributes */
+ struct kfd_ioctl_svm_attribute attrs[0];
};
struct kfd_criu_queue_priv_data {
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 7c92116153fe..49e05fb5c898 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -3418,6 +3418,66 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
return 0;
}
+int svm_range_get_info(struct kfd_process *p, uint32_t *num_svm_ranges,
+ uint64_t *svm_priv_data_size)
+{
+ uint64_t total_size, accessibility_size, common_attr_size;
+ int nattr_common = 4, naatr_accessibility = 1;
+ int num_devices = p->n_pdds;
+ struct svm_range_list *svms;
+ struct svm_range *prange;
+ uint32_t count = 0;
+
+ *svm_priv_data_size = 0;
+
+ svms = >svms;
svms is defined as structure inside kfd_process, not pointer, so
>svms will never be NULL.
+ if (!svms)
+ return -EINVAL;
+
+ mutex_lock(>lock);
+ list_for_each_entry(prange, >list, list) {
+ pr_debug("prange: 0x%p start: 0x%lx\t npages: 0x%llx\t end: 0x%llx\n",
+ prange, prange->start, prange->npages,
+ prange->start + prange->npages - 1);
+ count++;
+ }
+ mutex_unlock(>lock);
+
+ *num_svm_ranges = count;
+ /* Only the accessbility attributes need to be queried for all the gpus
+ * individually, remaining ones are spanned across the entire process
+ * regardless of the various gpu nodes. Of the remaining attributes,
+ * KFD_IOCTL_SVM_ATTR_CLR_FLAGS need not be saved.
+ *
+ * KFD_IOCTL_SVM_ATTR_PREFERRED_LOC
+ * KFD_IOCTL_SVM_ATTR_PREFETCH_LOC
+ * KFD_IOCTL_SVM_ATTR_SET_FLAGS
+ * KFD_IOCTL_SVM_ATTR_GRANULARITY
+ *
+ * ** ACCESSBILITY ATTRIBUTES **
+ * (Considered as one, type is altered during query, value is gpuid)
+ * KFD_IOCTL_SVM_ATTR_ACCESS
+ * KFD_IOCTL_SVM_ATTR_ACCESS_IN_PLACE
+ * KFD_IOCTL_SVM_ATTR_NO_ACCESS
+ */
+ if (*num_svm_ranges > 0)
+ {
+ common_attr_size = sizeof(struct kfd_ioctl_svm_attribute) *
+ nattr_common;
+ accessibility_size = sizeof(struct kfd_ioctl_svm_attribute) *
+ naatr_accessibility * num_devices;
+
+ total_size = sizeof(struct kfd_criu_svm_range_priv_data) +
+ common_attr_size + accessibility_size;
+
+ *svm_priv_data_size = *num_svm_ranges * total_size;
+ }
+
+
Re: [Patch v4 18/24] drm/amdkfd: CRIU checkpoint and restore xnack mode
On 2021-12-22 7:37 p.m., Rajneesh
Bhardwaj wrote:
Recoverable page faults are represented by the xnack mode setting inside
a kfd process and are used to represent the device page faults. For CR,
we don't consider negative values which are typically used for querying
the current xnack mode without modifying it.
Signed-off-by: Rajneesh Bhardwaj
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++
drivers/gpu/drm/amd/amdkfd/kfd_priv.h| 1 +
2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 178b0ccfb286..446eb9310915 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1845,6 +1845,11 @@ static int criu_checkpoint_process(struct kfd_process *p,
memset(_priv, 0, sizeof(process_priv));
process_priv.version = KFD_CRIU_PRIV_VERSION;
+ /* For CR, we don't consider negative xnack mode which is used for
+ * querying without changing it, here 0 simply means disabled and 1
+ * means enabled so retry for finding a valid PTE.
+ */
Negative value to query xnack mode is for kfd_ioctl_set_xnack_mode
user space ioctl interface, which is not used by CRIU, I think this
comment is misleading,
+ process_priv.xnack_mode = p->xnack_enabled ? 1 : 0;
change to process_priv.xnack_enabled
ret = copy_to_user(user_priv_data + *priv_offset,
_priv, sizeof(process_priv));
@@ -2231,6 +2236,16 @@ static int criu_restore_process(struct kfd_process *p,
return -EINVAL;
}
+ pr_debug("Setting XNACK mode\n");
+ if (process_priv.xnack_mode && !kfd_process_xnack_mode(p, true)) {
+ pr_err("xnack mode cannot be set\n");
+ ret = -EPERM;
+ goto exit;
+ } else {
On GFXv9 GPUs except Aldebaran, this means the process
checkpointed is xnack off, it can restore and resume on GPU with
xnack on, then shader will continue running successfully, but
driver is not guaranteed to map svm ranges on GPU all the time, if
retry fault happens, the shader will not recover. Maybe change to:
If (KFD_GC_VERSION(dev) != IP_VERSION(9, 4, 2) {
if (process_priv.xnack_enabled != kfd_process_xnack_mode(p,
true)) {
pr_err("xnack mode cannot be set\n");
ret = -EPERM;
goto exit;
}
}
pr_debug("set xnack mode: %d\n", process_priv.xnack_enabled);
p->xnack_enabled = process_priv.xnack_enabled;
+ pr_debug("set xnack mode: %d\n", process_priv.xnack_mode);
+ p->xnack_enabled = process_priv.xnack_mode;
+ }
+
exit:
return ret;
}
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 855c162b85ea..d72dda84c18c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -1057,6 +1057,7 @@ void kfd_process_set_trap_handler(struct qcm_process_device *qpd,
struct kfd_criu_process_priv_data {
uint32_t version;
+ uint32_t xnack_mode;
bool xnack_enabled;
Regards,
Philip
};
struct kfd_criu_device_priv_data {
Re: [Patch v4 21/24] drm/amdkfd: CRIU Discover svm ranges
On 2021-12-22 7:37 p.m., Rajneesh
Bhardwaj wrote:
A KFD process may contain a number of virtual address ranges for shared
virtual memory management and each such range can have many SVM
attributes spanning across various nodes within the process boundary.
This change reports the total number of such SVM ranges and
their total private data size by extending the PROCESS_INFO op of the the
CRIU IOCTL to discover the svm ranges in the target process and a future
patches brings in the required support for checkpoint and restore for
SVM ranges.
Signed-off-by: Rajneesh Bhardwaj
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 12 +++--
drivers/gpu/drm/amd/amdkfd/kfd_priv.h| 5 +-
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 60
drivers/gpu/drm/amd/amdkfd/kfd_svm.h | 11 +
4 files changed, 82 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 446eb9310915..1c25d5e9067c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -2089,10 +2089,9 @@ static int criu_get_process_object_info(struct kfd_process *p,
uint32_t *num_objects,
uint64_t *objs_priv_size)
{
- int ret;
- uint64_t priv_size;
+ uint64_t queues_priv_data_size, svm_priv_data_size, priv_size;
uint32_t num_queues, num_events, num_svm_ranges;
- uint64_t queues_priv_data_size;
+ int ret;
*num_devices = p->n_pdds;
*num_bos = get_process_num_bos(p);
@@ -2102,7 +2101,10 @@ static int criu_get_process_object_info(struct kfd_process *p,
return ret;
num_events = kfd_get_num_events(p);
- num_svm_ranges = 0; /* TODO: Implement SVM-Ranges */
+
+ ret = svm_range_get_info(p, _svm_ranges, _priv_data_size);
+ if (ret)
+ return ret;
*num_objects = num_queues + num_events + num_svm_ranges;
@@ -2112,7 +2114,7 @@ static int criu_get_process_object_info(struct kfd_process *p,
priv_size += *num_bos * sizeof(struct kfd_criu_bo_priv_data);
priv_size += queues_priv_data_size;
priv_size += num_events * sizeof(struct kfd_criu_event_priv_data);
- /* TODO: Add SVM ranges priv size */
+ priv_size += svm_priv_data_size;
*objs_priv_size = priv_size;
}
return 0;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index d72dda84c18c..87eb6739a78e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -1082,7 +1082,10 @@ enum kfd_criu_object_type {
struct kfd_criu_svm_range_priv_data {
uint32_t object_type;
- uint64_t reserved;
+ uint64_t start_addr;
+ uint64_t size;
+ /* Variable length array of attributes */
+ struct kfd_ioctl_svm_attribute attrs[0];
};
struct kfd_criu_queue_priv_data {
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 7c92116153fe..49e05fb5c898 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -3418,6 +3418,66 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
return 0;
}
+int svm_range_get_info(struct kfd_process *p, uint32_t *num_svm_ranges,
+ uint64_t *svm_priv_data_size)
+{
+ uint64_t total_size, accessibility_size, common_attr_size;
+ int nattr_common = 4, naatr_accessibility = 1;
number of SVM range attributes may be increased in future, better
to define MACRO for nattr_common and nattr_accessibility (typo),
then use MACRO in following patches too.
Regards,
Philip
+ int num_devices = p->n_pdds;
+ struct svm_range_list *svms;
+ struct svm_range *prange;
+ uint32_t count = 0;
+
+ *svm_priv_data_size = 0;
+
+ svms = >svms;
+ if (!svms)
+ return -EINVAL;
+
+ mutex_lock(>lock);
+ list_for_each_entry(prange, >list, list) {
+ pr_debug("prange: 0x%p start: 0x%lx\t npages: 0x%llx\t end: 0x%llx\n",
+ prange, prange->start, prange->npages,
+ prange->start + prange->npages - 1);
+ count++;
+ }
+ mutex_unlock(>lock);
+
+ *num_svm_ranges = count;
+ /* Only the accessbility attributes need to be queried for all the gpus
+ * individually, remaining ones are spanned across the entire process
+ * regardless of the various gpu nodes. Of the remaining attributes,
+ * KFD_IOCTL_SVM_ATTR_CLR_FLAGS need not be saved.
+ *
+ * KFD_IOCTL_SVM_ATTR_PREFERRED_LOC
+ * KFD_IOCTL_SVM_ATTR_PREFETCH_LOC
+ * KFD_IOCTL_SVM_ATTR_SET_FLAGS
+ * KFD_IOCTL_SVM_ATTR_GRANULARITY
+ *
+ * ** ACCESSBILITY ATTRIBUTES **
+ * (Considered as one, type is altered during query, value is gpuid)
+ * KFD_IOCTL_SVM_ATTR_ACCESS
+ * KFD_IOCTL_SVM_ATTR_ACCESS_IN_PLACE
+ * KFD_IOCTL_SVM_ATTR_NO_ACCESS
+ */
+ if (*num_svm_ranges > 0)
+ {
+ common_attr_size = sizeof(struct kfd_ioctl_svm_attribute) *
+ nattr_common;
+ accessibility_size = sizeof(struct kfd_ioctl_svm_attribute) *
+ naatr_accessibility * num_devices;
+
+ total_size = sizeof(struct kfd_criu_svm_range_priv_data) +
+
Re: [Patch v4 23/24] drm/amdkfd: CRIU prepare for svm resume
On 2021-12-22 7:37 p.m., Rajneesh
Bhardwaj wrote:
During CRIU restore phase, the VMAs for the virtual address ranges are
not at their final location yet so in this stage, only cache the data
required to successfully resume the svm ranges during an imminent CRIU
resume phase.
Signed-off-by: Rajneesh Bhardwaj
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 4 +-
drivers/gpu/drm/amd/amdkfd/kfd_priv.h| 5 ++
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 99
drivers/gpu/drm/amd/amdkfd/kfd_svm.h | 12 +++
4 files changed, 118 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 916b8d000317..f7aa15b18f95 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -2638,8 +2638,8 @@ static int criu_restore_objects(struct file *filep,
goto exit;
break;
case KFD_CRIU_OBJECT_TYPE_SVM_RANGE:
- /* TODO: Implement SVM range */
- *priv_offset += sizeof(struct kfd_criu_svm_range_priv_data);
+ ret = kfd_criu_restore_svm(p, (uint8_t __user *)args->priv_data,
+ priv_offset, max_priv_data_size);
if (ret)
goto exit;
break;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 87eb6739a78e..92191c541c29 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -790,6 +790,7 @@ struct svm_range_list {
struct list_head list;
struct work_struct deferred_list_work;
struct list_head deferred_range_list;
+ struct list_headcriu_svm_metadata_list;
spinlock_t deferred_list_lock;
atomic_t evicted_ranges;
booldrain_pagefaults;
@@ -1148,6 +1149,10 @@ int kfd_criu_restore_event(struct file *devkfd,
uint8_t __user *user_priv_data,
uint64_t *priv_data_offset,
uint64_t max_priv_data_size);
+int kfd_criu_restore_svm(struct kfd_process *p,
+ uint8_t __user *user_priv_data,
+ uint64_t *priv_data_offset,
+ uint64_t max_priv_data_size);
/* CRIU - End */
/* Queue Context Management */
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 6d59f1bedcf2..e9f6c63c2a26 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -45,6 +45,14 @@
*/
#define AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING 2000
+struct criu_svm_metadata {
+ struct list_head list;
+ __u64 start_addr;
+ __u64 size;
+ /* Variable length array of attributes */
+ struct kfd_ioctl_svm_attribute attrs[0];
+};
This data structure is struct kfd_criu_svm_range_priv_data plus
list_head, maybe you can add list_head to struct
kfd_criu_svm_range_priv_data and remove this new data structure,
then you can remove extra kzalloc, kfree for each svm object resume
and function svm_criu_prepare_for_resume could be removed.
+
static void svm_range_evict_svm_bo_worker(struct work_struct *work);
static bool
svm_range_cpu_invalidate_pagetables(struct mmu_interval_notifier *mni,
@@ -2753,6 +2761,7 @@ int svm_range_list_init(struct kfd_process *p)
INIT_DELAYED_WORK(>restore_work, svm_range_restore_work);
INIT_WORK(>deferred_list_work, svm_range_deferred_list_work);
INIT_LIST_HEAD(>deferred_range_list);
+ INIT_LIST_HEAD(>criu_svm_metadata_list);
spin_lock_init(>deferred_list_lock);
for (i = 0; i < p->n_pdds; i++)
@@ -3418,6 +3427,96 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
return 0;
}
+int svm_criu_prepare_for_resume(struct kfd_process *p,
+struct kfd_criu_svm_range_priv_data *svm_priv)
+{
+ int nattr_common = 4, nattr_accessibility = 1;
+ struct criu_svm_metadata *criu_svm_md = NULL;
+ uint64_t svm_attrs_size, svm_object_md_size;
+ struct svm_range_list *svms = >svms;
+ int num_devices = p->n_pdds;
+ int i, ret = 0;
+
+ svm_attrs_size = sizeof(struct kfd_ioctl_svm_attribute) *
+ (nattr_common + nattr_accessibility * num_devices);
+ svm_object_md_size = sizeof(struct criu_svm_metadata) + svm_attrs_size;
+
+ criu_svm_md = kzalloc(svm_object_md_size, GFP_KERNEL);
+ if (!criu_svm_md) {
+ pr_err("failed to allocate memory to store svm metadata\n");
+ ret = -ENOMEM;
+ goto exit;
+ }
+
+ criu_svm_md->start_addr = svm_priv->start_addr;
+ criu_svm_md->size = svm_priv->size;
+ for (i = 0; i < svm_attrs_size; i++)
for (i = 0; i < nattr_common + nattr_accessibility *
num_devices
; i++)
This function and for loop is not needed if you remove struct
criu_svm_metadata.
Regards,
Philip
+ {
+ criu_svm_md->attrs[i].type = svm_priv->attrs[i].type;
+ criu_svm_md->attrs[i].value = svm_priv->attrs[i].value;
+ }
+
+ list_add_tail(_svm_md->list, >criu_svm_metadata_list);
+
+
+exit:
+ return ret;
+}
+
+int kfd_criu_restore_svm(struct kfd_process *p,
+ uint8_t __user *user_priv_ptr,
+ uint64_t *priv_data_offset,
Re: [PATCH] drm/amdkfd: use max() and min() to make code cleaner
On 2021-12-15 3:52 a.m.,
cgel@gmail.com wrote:
From: Changcheng Deng
Use max() and min() in order to make code cleaner.
Reported-by: Zeal Robot
Signed-off-by: Changcheng Deng
Reviewed-by: Philip Yang
Applied, thanks.
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 7e92dcea4ce8..c6d3555b5be6 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2254,8 +2254,8 @@ svm_range_cpu_invalidate_pagetables(struct mmu_interval_notifier *mni,
start = mni->interval_tree.start;
last = mni->interval_tree.last;
- start = (start > range->start ? start : range->start) >> PAGE_SHIFT;
- last = (last < (range->end - 1) ? last : range->end - 1) >> PAGE_SHIFT;
+ start = max(start, range->start) >> PAGE_SHIFT;
+ last = min(last, range->end - 1) >> PAGE_SHIFT;
pr_debug("[0x%lx 0x%lx] range[0x%lx 0x%lx] notifier[0x%lx 0x%lx] %d\n",
start, last, range->start >> PAGE_SHIFT,
(range->end - 1) >> PAGE_SHIFT,
Re: [PATCH] drm/amdkfd: Fix a wild pointer dereference in svm_range_add()
On 2021-11-30 6:26 a.m., Zhou Qingyang
wrote:
In svm_range_add(), the return value of svm_range_new() is assigned
to prange and >insert_list is used in list_add(). There is a
a dereference of >insert_list in list_add(), which could lead
to a wild pointer dereference on failure of vm_range_new() if
CONFIG_DEBUG_LIST is unset in .config file.
Fix this bug by adding a check of prange.
This bug was found by a static analyzer. The analysis employs
differential checking to identify inconsistent security operations
(e.g., checks or kfrees) between two code paths and confirms that the
inconsistent operations are not recovered in the current function or
the callers, so they constitute bugs.
Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.
Builds with CONFIG_DRM_AMDGPU=m, CONFIG_HSA_AMD=y, and
CONFIG_HSA_AMD_SVM=y show no new warnings, and our static analyzer no
longer warns about this code.
Fixes: 42de677f7999 ("drm/amdkfd: register svm range")
Signed-off-by: Zhou Qingyang
Reviewed-by: Philip Yang
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 58b89b53ebe6..e40c2211901d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2940,6 +2940,9 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
if (left) {
prange = svm_range_new(svms, last - left + 1, last);
+ if (!prange)
+ return -ENOMEM;
+
list_add(>insert_list, insert_list);
list_add(>update_list, update_list);
}
Re: drm/amdkfd: implement counters for vm fault and migration
On 2021-07-06 9:08 p.m., Felix Kuehling
wrote:
Am 2021-07-06 um 11:36 a.m. schrieb Colin Ian King:
Hi,
Static analysis with Coverity on linux-next has found a potential null
pointer dereference in function svm_range_restore_pages in
drivers/gpu/drm/amd/amdkfd/kfd_svm.c from the following commit:
commit d4ebc2007040a0aff01bfe1b194085d3867328fd
Author: Philip Yang
Date: Tue Jun 22 00:12:32 2021 -0400
drm/amdkfd: implement counters for vm fault and migration
The analysis is as follows:
Thanks. Philip, see inline ...
2397 int
2398 svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
2399uint64_t addr)
2400{
2401struct mm_struct *mm = NULL;
2402struct svm_range_list *svms;
2403struct svm_range *prange;
2404struct kfd_process *p;
2405uint64_t timestamp;
2406int32_t best_loc;
2407int32_t gpuidx = MAX_GPU_INSTANCE;
2408bool write_locked = false;
2409int r = 0;
2410
1. Condition !(adev->kfd.dev->pgmap.type != 0), taking false branch.
2411if (!KFD_IS_SVM_API_SUPPORTED(adev->kfd.dev)) {
2412pr_debug("device does not support SVM\n");
2413return -EFAULT;
2414}
2415
2416p = kfd_lookup_process_by_pasid(pasid);
2. Condition !p, taking false branch.
2417if (!p) {
2418pr_debug("kfd process not founded pasid 0x%x\n", pasid);
2419return -ESRCH;
2420}
3. Condition !p->xnack_enabled, taking false branch.
2421if (!p->xnack_enabled) {
2422pr_debug("XNACK not enabled for pasid 0x%x\n", pasid);
2423return -EFAULT;
2424}
2425svms = >svms;
2426
4. Condition 0 /* __builtin_types_compatible_p() */, taking false
branch.
5. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
6. Falling through to end of if statement.
7. Condition !!branch, taking false branch.
8. Condition ({...; !!branch;}), taking false branch.
2427pr_debug("restoring svms 0x%p fault address 0x%llx\n", svms,
addr);
2428
2429mm = get_task_mm(p->lead_thread);
9. Condition !mm, taking false branch.
2430if (!mm) {
2431pr_debug("svms 0x%p failed to get mm\n", svms);
2432r = -ESRCH;
2433goto out;
2434}
2435
2436mmap_read_lock(mm);
2437retry_write_locked:
2438mutex_lock(>lock);
2439prange = svm_range_from_addr(svms, addr, NULL);
10. Condition !prange, taking true branch.
18. Condition !prange, taking true branch.
2440if (!prange) {
11. Condition 0 /* __builtin_types_compatible_p() */, taking false
branch.
12. Condition 1 /* __builtin_types_compatible_p() */, taking true
branch.
13. Falling through to end of if statement.
14. Condition !!branch, taking false branch.
15. Condition ({...; !!branch;}), taking false branch.
19. Condition 0 /* __builtin_types_compatible_p() */, taking false
branch.
20. Condition 1 /* __builtin_types_compatible_p() */, taking true
branch.
21. Falling through to end of if statement.
22. Condition !!branch, taking false branch.
23. Condition ({...; !!branch;}), taking false branch.
2441pr_debug("failed to find prange svms 0x%p address
[0x%llx]\n",
2442 svms, addr);
16. Condition !write_locked, taking true branch.
24. Condition !write_locked, taking false branch.
2443if (!write_locked) {
2444/* Need the write lock to create new range
with MMU notifier.
2445 * Also flush pending deferred work to make
sure the interval
2446 * tree is up to date before we add a new range
2447 */
2448mutex_unlock(>lock);
2449mmap_read_unlock(mm);
2450mmap_write_lock(mm);
2451write_locked = true;
17. Jumping to label retry_write_locked.
2452goto retry_write_locked;
2453}
2454prange = svm_range_create_unregistered_range(adev,
p, mm, addr);
25. Condition !prange, taking true branch.
26. var_compare_op: Comparing prange to null implies that prange
might be null.
2455if (!prange) {
27. Condition 0 /* __builtin_types_compatible_p() */, taking false
branch.
28. Condition 1 /* __builtin_types_compatible_p() */, taking true
branch.
29. Falling through to end of if statement.
30. Condition !!branch, taking false branch.
31. Condition ({...; !!branch;}), taking false branch.
2456pr_debug("failed to create unreg
Re: drm/amdkfd: implement counters for vm fault and migration
On 2021-07-06 11:36 a.m., Colin Ian
King wrote:
Hi,
Static analysis with Coverity on linux-next has found a potential null
pointer dereference in function svm_range_restore_pages in
drivers/gpu/drm/amd/amdkfd/kfd_svm.c from the following commit:
commit d4ebc2007040a0aff01bfe1b194085d3867328fd
Author: Philip Yang
Date: Tue Jun 22 00:12:32 2021 -0400
drm/amdkfd: implement counters for vm fault and migration
The analysis is as follows:
2397 int
2398 svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
2399uint64_t addr)
2400{
2401struct mm_struct *mm = NULL;
2402struct svm_range_list *svms;
2403struct svm_range *prange;
2404struct kfd_process *p;
2405uint64_t timestamp;
2406int32_t best_loc;
2407int32_t gpuidx = MAX_GPU_INSTANCE;
2408bool write_locked = false;
2409int r = 0;
2410
1. Condition !(adev->kfd.dev->pgmap.type != 0), taking false branch.
2411if (!KFD_IS_SVM_API_SUPPORTED(adev->kfd.dev)) {
2412pr_debug("device does not support SVM\n");
2413return -EFAULT;
2414}
2415
2416p = kfd_lookup_process_by_pasid(pasid);
2. Condition !p, taking false branch.
2417if (!p) {
2418pr_debug("kfd process not founded pasid 0x%x\n", pasid);
2419return -ESRCH;
2420}
3. Condition !p->xnack_enabled, taking false branch.
2421if (!p->xnack_enabled) {
2422pr_debug("XNACK not enabled for pasid 0x%x\n", pasid);
2423return -EFAULT;
2424}
2425svms = >svms;
2426
4. Condition 0 /* __builtin_types_compatible_p() */, taking false
branch.
5. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
6. Falling through to end of if statement.
7. Condition !!branch, taking false branch.
8. Condition ({...; !!branch;}), taking false branch.
2427pr_debug("restoring svms 0x%p fault address 0x%llx\n", svms,
addr);
2428
2429mm = get_task_mm(p->lead_thread);
9. Condition !mm, taking false branch.
2430if (!mm) {
2431pr_debug("svms 0x%p failed to get mm\n", svms);
2432r = -ESRCH;
2433goto out;
2434}
2435
2436mmap_read_lock(mm);
2437retry_write_locked:
2438mutex_lock(>lock);
2439prange = svm_range_from_addr(svms, addr, NULL);
10. Condition !prange, taking true branch.
18. Condition !prange, taking true branch.
2440if (!prange) {
11. Condition 0 /* __builtin_types_compatible_p() */, taking false
branch.
12. Condition 1 /* __builtin_types_compatible_p() */, taking true
branch.
13. Falling through to end of if statement.
14. Condition !!branch, taking false branch.
15. Condition ({...; !!branch;}), taking false branch.
19. Condition 0 /* __builtin_types_compatible_p() */, taking false
branch.
20. Condition 1 /* __builtin_types_compatible_p() */, taking true
branch.
21. Falling through to end of if statement.
22. Condition !!branch, taking false branch.
23. Condition ({...; !!branch;}), taking false branch.
2441pr_debug("failed to find prange svms 0x%p address
[0x%llx]\n",
2442 svms, addr);
16. Condition !write_locked, taking true branch.
24. Condition !write_locked, taking false branch.
2443if (!write_locked) {
2444/* Need the write lock to create new range
with MMU notifier.
2445 * Also flush pending deferred work to make
sure the interval
2446 * tree is up to date before we add a new range
2447 */
2448mutex_unlock(>lock);
2449mmap_read_unlock(mm);
2450mmap_write_lock(mm);
2451write_locked = true;
17. Jumping to label retry_write_locked.
2452goto retry_write_locked;
2453}
2454prange = svm_range_create_unregistered_range(adev,
p, mm, addr);
25. Condition !prange, taking true branch.
26. var_compare_op: Comparing prange to null implies that prange
might be null.
2455if (!prange) {
27. Condition 0 /* __builtin_types_compatible_p() */, taking false
branch.
28. Condition 1 /* __builtin_types_compatible_p() */, taking true
branch.
29. Falling through to end of if statement.
30. Condition !!branch, taking false branch.
31. Condition ({...; !!branch;}), taking false branch.
2456pr_debug("failed to create unregistered
range svms 0x%p address [0x%llx]\n",
2457 svms, addr);
2458
Re: [PATCH v2 1/3] drm/amdkfd: Use drm_priv to pass VM from KFD to amdgpu
On 2021-04-14 10:40 p.m., Felix
Kuehling wrote:
amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu needs the drm_priv to allow mmap
to access the BO through the corresponding file descriptor. The VM can
also be extracted from drm_priv, so drm_priv can replace the vm parameter
in the kfd2kgd interface.
This series is Reviewed-by: Philip Yang
Signed-off-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h| 14 ++--
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 69 +++
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 8 +--
.../drm/amd/amdkfd/kfd_device_queue_manager.c | 6 +-
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 +-
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 23 +++
6 files changed, 67 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 5ffb07b02810..0d59bebd92af 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -236,20 +236,20 @@ uint8_t amdgpu_amdkfd_get_xgmi_hops_count(struct kgd_dev *dst, struct kgd_dev *s
/* GPUVM API */
int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
struct file *filp, u32 pasid,
- void **vm, void **process_info,
+ void **process_info,
struct dma_fence **ef);
-void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm);
-uint64_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
+void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *drm_priv);
+uint64_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *drm_priv);
int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
struct kgd_dev *kgd, uint64_t va, uint64_t size,
- void *vm, struct kgd_mem **mem,
+ void *drm_priv, struct kgd_mem **mem,
uint64_t *offset, uint32_t flags);
int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
struct kgd_dev *kgd, struct kgd_mem *mem, uint64_t *size);
int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
- struct kgd_dev *kgd, struct kgd_mem *mem, void *vm);
+ struct kgd_dev *kgd, struct kgd_mem *mem, void *drm_priv);
int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
- struct kgd_dev *kgd, struct kgd_mem *mem, void *vm);
+ struct kgd_dev *kgd, struct kgd_mem *mem, void *drm_priv);
int amdgpu_amdkfd_gpuvm_sync_memory(
struct kgd_dev *kgd, struct kgd_mem *mem, bool intr);
int amdgpu_amdkfd_gpuvm_map_gtt_bo_to_kernel(struct kgd_dev *kgd,
@@ -260,7 +260,7 @@ int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct kgd_dev *kgd,
struct kfd_vm_fault_info *info);
int amdgpu_amdkfd_gpuvm_import_dmabuf(struct kgd_dev *kgd,
struct dma_buf *dmabuf,
- uint64_t va, void *vm,
+ uint64_t va, void *drm_priv,
struct kgd_mem **mem, uint64_t *size,
uint64_t *mmap_offset);
int amdgpu_amdkfd_get_tile_config(struct kgd_dev *kgd,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 7d4118c8128a..dc86faa03b88 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -948,6 +948,13 @@ static int process_update_pds(struct amdkfd_process_info *process_info,
return 0;
}
+static struct amdgpu_vm *drm_priv_to_vm(struct drm_file *drm_priv)
+{
+ struct amdgpu_fpriv *fpriv = drm_priv->driver_priv;
+
+ return >vm;
+}
+
static int init_kfd_vm(struct amdgpu_vm *vm, void **process_info,
struct dma_fence **ef)
{
@@ -1036,15 +1043,19 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void **process_info,
int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
struct file *filp, u32 pasid,
- void **vm, void **process_info,
+ void **process_info,
struct dma_fence **ef)
{
struct amdgpu_device *adev = get_amdgpu_device(kgd);
- struct drm_file *drm_priv = filp->private_data;
- struct amdgpu_fpriv *drv_priv = drm_priv->driver_priv;
- struct amdgpu_vm *avm = _priv->vm;
+ struct amdgpu_fpriv *drv_priv;
+ struct amdgpu_vm *avm;
int ret;
+ ret = amdgpu_file_to_fpriv(filp, _priv);
+ if (ret)
+ return ret;
+ avm = _priv->vm;
+
/* Already a compute VM? */
if (avm->process_info)
return -EINVAL;
@@ -1059,7 +1070,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
if (ret)
return ret;
- *vm = (void *)avm;
+ amdgpu_vm_set_task_info(avm);
return 0;
}
@@ -1100,15 +,17 @@ void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
}
}
-void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm)
+void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *drm_priv)
{
struct amdgpu_device *adev = get_amdgpu_device(kgd);
- struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
+ struct amdgpu_vm *avm;
- if (WARN_ON(!kgd || !vm))
+ if (WARN_ON(!kgd || !drm_priv))
return;
- pr_debug("Releasing pr
Re: [PATCH 4/4] drm/amdgpu: Remove verify_access shortcut for KFD BOs
On 2021-04-07 7:12 p.m., Felix Kuehling
wrote:
This shortcut is no longer needed with access managed progerly by KFD.
Reviewed-by: Philip Yang
Signed-off-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 936b3cfdde55..4947dfe9aa70 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -165,13 +165,6 @@ static int amdgpu_verify_access(struct ttm_buffer_object *bo, struct file *filp)
{
struct amdgpu_bo *abo = ttm_to_amdgpu_bo(bo);
- /*
- * Don't verify access for KFD BOs. They don't have a GEM
- * object associated with them.
- */
- if (abo->kfd_bo)
- return 0;
-
if (amdgpu_ttm_tt_get_usermm(bo->ttm))
return -EPERM;
return drm_vma_node_verify_access(>tbo.base.vma_node,
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/4] drm/amdkfd: Allow access for mmapping KFD BOs
On 2021-04-07 7:12 p.m., Felix Kuehling
wrote:
DRM allows access automatically when it creates a GEM handle for a BO.
KFD BOs don't have GEM handles, so KFD needs to manage access manually.
After reading drm vma manager, I understand it uses rbtree to
store all GPU drm file handle when calling drm_vma_node_allow,
drm_vma_node_is_allowed/drm_vma_node_verify_access is checked when
creating mapping, I don't understand how application uses this,
but seems we need call drm_vma_node_allow when
amdgpu_amdkfd_gpuvm_map_memory_to_gpu, to add mapping GPUs drm
file handle to vma manager.
Regards,
Philip
Signed-off-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h| 3 ++-
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 19 ++-
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 8 +---
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 7 ---
4 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 0d59bebd92af..7c8c5e469707 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -245,7 +245,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
void *drm_priv, struct kgd_mem **mem,
uint64_t *offset, uint32_t flags);
int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
- struct kgd_dev *kgd, struct kgd_mem *mem, uint64_t *size);
+ struct kgd_dev *kgd, struct kgd_mem *mem, void *drm_priv,
+ uint64_t *size);
int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
struct kgd_dev *kgd, struct kgd_mem *mem, void *drm_priv);
int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 95442bcd60fb..e7d61ec966b6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1232,6 +1232,12 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
domain_string(alloc_domain), ret);
goto err_bo_create;
}
+ ret = drm_vma_node_allow(>vma_node, drm_priv);
+ if (ret) {
+ pr_debug("Failed to allow vma node access. ret %d\n",
+ ret);
pr_debug("Failed to allow vma node access. ret %d\n", ret);
It's within 80 columns.
Philip
+ goto err_node_allow;
+ }
bo = gem_to_amdgpu_bo(gobj);
if (bo_type == ttm_bo_type_sg) {
bo->tbo.sg = sg;
@@ -1261,6 +1267,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
allocate_init_user_pages_failed:
remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
+ drm_vma_node_revoke(>vma_node, drm_priv);
+err_node_allow:
amdgpu_bo_unref();
/* Don't unreserve system mem limit twice */
goto err_reserve_limit;
@@ -1278,7 +1286,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
}
int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
- struct kgd_dev *kgd, struct kgd_mem *mem, uint64_t *size)
+ struct kgd_dev *kgd, struct kgd_mem *mem, void *drm_priv,
+ uint64_t *size)
{
struct amdkfd_process_info *process_info = mem->process_info;
unsigned long bo_size = mem->bo->tbo.base.size;
@@ -1355,6 +1364,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
}
/* Free the BO*/
+ drm_vma_node_revoke(>bo->tbo.base.vma_node, drm_priv);
drm_gem_object_put(>bo->tbo.base);
mutex_destroy(>lock);
kfree(mem);
@@ -1666,6 +1676,7 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct kgd_dev *kgd,
struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
struct drm_gem_object *obj;
struct amdgpu_bo *bo;
+ int ret;
if (dma_buf->ops != _dmabuf_ops)
/* Can't handle non-graphics buffers */
@@ -1686,6 +1697,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct kgd_dev *kgd,
if (!*mem)
return -ENOMEM;
+ ret = drm_vma_node_allow(>vma_node, drm_priv);
+ if (ret) {
+ kfree(mem);
+ return ret;
+ }
+
if (size)
*size = amdgpu_bo_size(bo);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 43de260b2230..8fc18de7cff4 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1328,7 +1328,8 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
return 0;
err_free:
- amdgpu_amdkfd_gpuvm_free_memory_of_gpu(dev->kgd, (struct kgd_mem *)mem, NULL);
+ amdgpu_amdkfd_gpuvm_free_memory_of_gpu(dev->kgd, (struct kgd_mem *)mem,
+ pdd->vm, NULL);
err_unlock:
mutex_unlock(>mutex);
return err;
@@ -1365,7 +1366,7 @@ static int kfd_ioctl_free_memory_of_gpu(struct file *filep,
}
ret = amdgpu_amdkfd_gpuvm_free_memory_of_gpu(dev->kgd,
- (struct kgd_mem *)mem, );
+ (struct kgd_mem *)mem, pdd->vm, );
/* If freeing the buffer failed, leave the handle in place for
* clean-up during process tear-down.
@@ -1721,7 +1722,8 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
return 0;
Re: [PATCH 2/4] drm/amdkfd: Use drm_priv to pass VM from KFD to amdgpu
On 2021-04-07 7:12 p.m., Felix Kuehling
wrote:
amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu needs the drm_priv to allow mmap
to access the BO through the corresponding file descriptor.
Signed-off-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h| 14 ++--
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 69 +++
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 +-
3 files changed, 50 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 5ffb07b02810..0d59bebd92af 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -236,20 +236,20 @@ uint8_t amdgpu_amdkfd_get_xgmi_hops_count(struct kgd_dev *dst, struct kgd_dev *s
/* GPUVM API */
int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
struct file *filp, u32 pasid,
- void **vm, void **process_info,
+ void **process_info,
struct dma_fence **ef);
-void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm);
-uint64_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
+void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *drm_priv);
+uint64_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *drm_priv);
int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
struct kgd_dev *kgd, uint64_t va, uint64_t size,
- void *vm, struct kgd_mem **mem,
+ void *drm_priv, struct kgd_mem **mem,
uint64_t *offset, uint32_t flags);
int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
struct kgd_dev *kgd, struct kgd_mem *mem, uint64_t *size);
int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
- struct kgd_dev *kgd, struct kgd_mem *mem, void *vm);
+ struct kgd_dev *kgd, struct kgd_mem *mem, void *drm_priv);
int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
- struct kgd_dev *kgd, struct kgd_mem *mem, void *vm);
+ struct kgd_dev *kgd, struct kgd_mem *mem, void *drm_priv);
int amdgpu_amdkfd_gpuvm_sync_memory(
struct kgd_dev *kgd, struct kgd_mem *mem, bool intr);
int amdgpu_amdkfd_gpuvm_map_gtt_bo_to_kernel(struct kgd_dev *kgd,
@@ -260,7 +260,7 @@ int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct kgd_dev *kgd,
struct kfd_vm_fault_info *info);
int amdgpu_amdkfd_gpuvm_import_dmabuf(struct kgd_dev *kgd,
struct dma_buf *dmabuf,
- uint64_t va, void *vm,
+ uint64_t va, void *drm_priv,
struct kgd_mem **mem, uint64_t *size,
uint64_t *mmap_offset);
int amdgpu_amdkfd_get_tile_config(struct kgd_dev *kgd,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 36012229ccc1..95442bcd60fb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -951,6 +951,13 @@ static int process_update_pds(struct amdkfd_process_info *process_info,
return 0;
}
+static struct amdgpu_vm *drm_priv_to_vm(struct drm_file *drm_priv)
+{
+ struct amdgpu_fpriv *fpriv = drm_priv->driver_priv;
+
+ return >vm;
+}
+
static int init_kfd_vm(struct amdgpu_vm *vm, void **process_info,
struct dma_fence **ef)
{
@@ -1039,15 +1046,19 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void **process_info,
int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
struct file *filp, u32 pasid,
- void **vm, void **process_info,
+ void **process_info,
struct dma_fence **ef)
{
struct amdgpu_device *adev = get_amdgpu_device(kgd);
- struct drm_file *drm_priv = filp->private_data;
- struct amdgpu_fpriv *drv_priv = drm_priv->driver_priv;
- struct amdgpu_vm *avm = _priv->vm;
+ struct amdgpu_fpriv *drv_priv;
+ struct amdgpu_vm *avm;
int ret;
+ ret = amdgpu_file_to_fpriv(filp, _priv);
+ if (ret)
+ return ret;
+ avm = _priv->vm;
+
/* Already a compute VM? */
if (avm->process_info)
return -EINVAL;
@@ -1062,7 +1073,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
if (ret)
return ret;
- *vm = (void *)avm;
+ amdgpu_vm_set_task_info(avm);
return 0;
}
@@ -1103,15 +1114,17 @@ void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
}
}
-void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm)
+void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *drm_priv)
{
struct amdgpu_device *adev = get_amdgpu_device(kgd);
- struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
+ struct amdgpu_vm *avm;
- if (WARN_ON(!kgd || !vm))
+ if (WARN_ON(!kgd || !drm_priv))
return;
- pr_debug("Releasing process vm %p\n", vm);
+ avm = drm_priv_to_vm(drm_priv);
+
+ pr_debug("Releasing process vm %p\n", avm);
/* The original pasid of amdgpu vm has already been
* released during making a amdgpu vm to a compute vm
@@ -1122,9 +1135,9 @@ void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm)
amdgpu_vm_release_compute(adev, avm);
}
-uint64_t
Re: [PATCH 1/4] drm/amdkfd: Remove legacy code not acquiring VMs
On 2021-04-07 7:12 p.m., Felix Kuehling
wrote:
ROCm user mode has acquired VMs from DRM file descriptors for as long
as it supported the upstream KFD. Legacy code to support older versions
of ROCm is not needed any more.
Reviewed-by: Philip Yang
Signed-off-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h| 4 --
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 50 ---
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 27 --
3 files changed, 10 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 14f68c028126..5ffb07b02810 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -234,14 +234,10 @@ 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, 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, u32 pasid,
void **vm, void **process_info,
struct dma_fence **ef);
-void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm);
uint64_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e93850f2f3b1..36012229ccc1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1037,41 +1037,6 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void **process_info,
return ret;
}
-int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, u32 pasid,
- void **vm, void **process_info,
- struct dma_fence **ef)
-{
- struct amdgpu_device *adev = get_amdgpu_device(kgd);
- struct amdgpu_vm *new_vm;
- int ret;
-
- new_vm = kzalloc(sizeof(*new_vm), GFP_KERNEL);
- if (!new_vm)
- return -ENOMEM;
-
- /* Initialize AMDGPU part of the VM */
- ret = amdgpu_vm_init(adev, new_vm, AMDGPU_VM_CONTEXT_COMPUTE, pasid);
- if (ret) {
- pr_err("Failed init vm ret %d\n", ret);
- goto amdgpu_vm_init_fail;
- }
-
- /* Initialize KFD part of the VM and process info */
- ret = init_kfd_vm(new_vm, process_info, ef);
- if (ret)
- goto init_kfd_vm_fail;
-
- *vm = (void *) new_vm;
-
- return 0;
-
-init_kfd_vm_fail:
- amdgpu_vm_fini(adev, new_vm);
-amdgpu_vm_init_fail:
- kfree(new_vm);
- return ret;
-}
-
int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
struct file *filp, u32 pasid,
void **vm, void **process_info,
@@ -1138,21 +1103,6 @@ void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
}
}
-void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm)
-{
- struct amdgpu_device *adev = get_amdgpu_device(kgd);
- struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
-
- if (WARN_ON(!kgd || !vm))
- return;
-
- pr_debug("Destroying process vm %p\n", vm);
-
- /* Release the VM context */
- amdgpu_vm_fini(adev, avm);
- kfree(vm);
-}
-
void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm)
{
struct amdgpu_device *adev = get_amdgpu_device(kgd);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index d4241d29ea94..d97e330a5022 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -935,9 +935,6 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
pdd->dev->kgd, pdd->vm);
fput(pdd->drm_file);
}
- else if (pdd->vm)
- amdgpu_amdkfd_gpuvm_destroy_process_vm(
-pdd->dev->kgd, pdd->vm);
if (pdd->qpd.cwsr_kaddr && !pdd->qpd.cwsr_base)
free_pages((unsigned long)pdd->qpd.cwsr_kaddr,
@@ -1375,19 +1372,18 @@ int kfd_process_device_init_vm(struct kfd_process_device *pdd,
struct kfd_dev *dev;
int ret;
+ if (!drm_file)
+ return -EINVAL;
+
if (pdd->vm)
- return drm_file ? -EBUSY : 0;
+ return -EBUSY;
p = pdd->process;
dev = pdd->dev;
- if (drm_file)
- ret = amdgpu_amdkfd_gpuvm_acquire_process_vm(
- dev->kgd, drm_file, p->pasid,
- >vm, >kgd_process_info, >ef);
- else
- ret = amdgpu_amdkfd_gpuvm_create_process_vm(dev->kgd, p->pasid,
- >vm, >kgd_process_info, >ef);
+ ret = amdgpu_amdkfd_gpuvm_acquire_process_vm(
+ dev->kgd, drm_file, p->pasid,
+ >vm, >kgd_process_info, >ef);
if (ret) {
pr_err("Failed to create process VM object\n");
return ret;
@@ -1409,8 +1405,6 @@ int kfd_process_device_init_vm(struct kfd_process_device *pdd,
err_init_cwsr:
err_reserve_ib_mem:
kfd_process_device_f
Re: [PATCH 1/1] drm/amdkfd: Fix recursive lock warnings
On 2021-02-16 3:22 p.m., Felix Kuehling
wrote:
memalloc_nofs_save/restore are no longer sufficient to prevent recursive
lock warnings when holding locks that can be taken in MMU notifiers. Use
memalloc_noreclaim_save/restore instead.
Fixes: f920e413ff9c ("mm: track mmu notifiers in fs_reclaim_acquire/release")
CC: Daniel Vetter
Signed-off-by: Felix Kuehling
Reviewed-by: Philip Yang
---
drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
index 16262e5d93f5..7351dd195274 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
@@ -243,11 +243,11 @@ get_sh_mem_bases_nybble_64(struct kfd_process_device *pdd)
static inline void dqm_lock(struct device_queue_manager *dqm)
{
mutex_lock(>lock_hidden);
- dqm->saved_flags = memalloc_nofs_save();
+ dqm->saved_flags = memalloc_noreclaim_save();
}
static inline void dqm_unlock(struct device_queue_manager *dqm)
{
- memalloc_nofs_restore(dqm->saved_flags);
+ memalloc_noreclaim_restore(dqm->saved_flags);
mutex_unlock(>lock_hidden);
}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/amdkfd: Cleanup kfd_process if init_cwsr_apu fails
Reviewed-by: Philip Yang for the series.
On 2021-02-12 1:40 a.m., Felix Kuehling
wrote:
If init_cwsr_apu fails, we currently leave the kfd_process structure in
place anyway. The next kfd_open will then succeed, using the existing
kfd_process structure. Fix that by cleaning up the kfd_process after a
failure in init_cwsr_apu.
Signed-off-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 145cd0a17d50..a4d7682289bb 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -775,10 +775,8 @@ struct kfd_process *kfd_create_process(struct file *filep)
goto out;
ret = kfd_process_init_cwsr_apu(process, filep);
- if (ret) {
- process = ERR_PTR(ret);
- goto out;
- }
+ if (ret)
+ goto out_destroy;
if (!procfs.kobj)
goto out;
@@ -826,6 +824,14 @@ struct kfd_process *kfd_create_process(struct file *filep)
mutex_unlock(_processes_mutex);
return process;
+
+out_destroy:
+ hash_del_rcu(>kfd_processes);
+ mutex_unlock(_processes_mutex);
+ synchronize_srcu(_processes_srcu);
+ /* kfd_process_free_notifier will trigger the cleanup */
+ mmu_notifier_put(>mmu_notifier);
+ return ERR_PTR(ret);
}
struct kfd_process *kfd_get_process(const struct task_struct *thread)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/ttm: fix pipelined gutting for evictions
On 2020-07-23 7:02 p.m., Felix Kuehling wrote:
Am 2020-07-23 um 5:00 a.m. schrieb Christian König:
We can't pipeline that during eviction because the memory needs
to be available immediately.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index bc2230ecb7e3..122040056a07 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -651,8 +651,16 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
placement.num_busy_placement = 0;
bdev->driver->evict_flags(bo, );
- if (!placement.num_placement && !placement.num_busy_placement)
- return ttm_bo_pipeline_gutting(bo);
+ if (!placement.num_placement && !placement.num_busy_placement) {
+ ttm_bo_wait(bo, false, false);
+
+ ttm_tt_destroy(bo->ttm);
+
+ memset(>mem, 0, sizeof(bo->mem));
Where does the memory in the bo->mem (ttm_mem_reg) get destroyed? It
doesn't get attached to a ghost BO in this case, so someone will have to
call ttm_bo_mem_put explicitly before you wipe out bo->mem.
After migrating to ram,
svm_range_bo_unref-->amdgpu_unref_bo->ttm_bo_put->ttm_bo_release calls
ttm_bo_mem_put.
vram is already freed before we signal fence, right?
Regards,
Philip
Regards,
Felix
+ bo->mem.mem_type = TTM_PL_SYSTEM;
+ bo->ttm = NULL;
+ return 0;
+ }
evict_mem = bo->mem;
evict_mem.mm_node = NULL;
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 12/14] drm/amdgpu: Use mmu_interval_notifier instead of hmm_mirror
I test v3 and it works fine.
Regards,
Philip
On 2019-11-12 3:22 p.m., Jason Gunthorpe wrote:
From: Jason Gunthorpe
Convert the collision-retry lock around hmm_range_fault to use the one now
provided by the mmu_interval notifier.
Although this driver does not seem to use the collision retry lock that
hmm provides correctly, it can still be converted over to use the
mmu_interval_notifier api instead of hmm_mirror without too much trouble.
This also deletes another place where a driver is associating additional
data (struct amdgpu_mn) with a mmu_struct.
Signed-off-by: Philip Yang
Reviewed-by: Philip Yang
Tested-by: Philip Yang
Signed-off-by: Jason Gunthorpe
---
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 4 +
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c| 14 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c| 148 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.h| 49 --
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 116 --
5 files changed, 94 insertions(+), 237 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 47700302a08b7f..1bcedb9b477dce 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1738,6 +1738,10 @@ static int update_invalid_user_pages(struct
amdkfd_process_info *process_info,
return ret;
}
+ /*
+* FIXME: Cannot ignore the return code, must hold
+* notifier_lock
+*/
amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
/* Mark the BO as valid unless it was invalidated
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 82823d9a8ba887..22c989bca7514c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -603,8 +603,6 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
e->tv.num_shared = 2;
amdgpu_bo_list_get_list(p->bo_list, >validated);
- if (p->bo_list->first_userptr != p->bo_list->num_entries)
- p->mn = amdgpu_mn_get(p->adev, AMDGPU_MN_TYPE_GFX);
INIT_LIST_HEAD();
amdgpu_vm_get_pd_bo(>vm, >validated, >vm_pd);
@@ -1287,11 +1285,11 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
if (r)
goto error_unlock;
- /* No memory allocation is allowed while holding the mn lock.
-* p->mn is hold until amdgpu_cs_submit is finished and fence is added
-* to BOs.
+ /* No memory allocation is allowed while holding the notifier lock.
+* The lock is held until amdgpu_cs_submit is finished and fence is
+* added to BOs.
*/
- amdgpu_mn_lock(p->mn);
+ mutex_lock(>adev->notifier_lock);
/* If userptr are invalidated after amdgpu_cs_parser_bos(), return
* -EAGAIN, drmIoctl in libdrm will restart the amdgpu_cs_ioctl.
@@ -1334,13 +1332,13 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
amdgpu_vm_move_to_lru_tail(p->adev, >vm);
ttm_eu_fence_buffer_objects(>ticket, >validated, p->fence);
- amdgpu_mn_unlock(p->mn);
+ mutex_unlock(>adev->notifier_lock);
return 0;
error_abort:
drm_sched_job_cleanup(>base);
- amdgpu_mn_unlock(p->mn);
+ mutex_unlock(>adev->notifier_lock);
error_unlock:
amdgpu_job_free(job);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
index 9fe1c31ce17a30..828b5167ff128f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
@@ -50,28 +50,6 @@
#include "amdgpu.h"
#include "amdgpu_amdkfd.h"
-/**
- * amdgpu_mn_lock - take the write side lock for this notifier
- *
- * @mn: our notifier
- */
-void amdgpu_mn_lock(struct amdgpu_mn *mn)
-{
- if (mn)
- down_write(>lock);
-}
-
-/**
- * amdgpu_mn_unlock - drop the write side lock for this notifier
- *
- * @mn: our notifier
- */
-void amdgpu_mn_unlock(struct amdgpu_mn *mn)
-{
- if (mn)
- up_write(>lock);
-}
-
/**
* amdgpu_mn_invalidate_gfx - callback to notify about mm change
*
@@ -94,6 +72,9 @@ static bool amdgpu_mn_invalidate_gfx(struct
mmu_interval_notifier *mni,
return false;
mutex_lock(>notifier_lock);
+
+ mmu_interval_set_seq(mni, cur_seq);
+
r = dma_resv_wait_timeout_rcu(bo->tbo.base.resv, true, false,
MAX_SCHEDULE_TIMEOUT);
mutex_unlock(>notifier_lock);
@@ -127,6 +108,9 @@ static bool amdgpu_mn_invalidate_hsa(struct
mmu_interval_notifier *mni,
return false;
mutex_lock(>notifier_lock);
+
+ mmu_interval_s
