[PATCH 13/83] hsa/radeon: Add 2 new IOCTL to kfd, CREATE_QUEUE and DESTROY_QUEUE
On Sat, 2014-07-12 at 07:42 +1000, Dave Airlie wrote:
> > +/* The 64-bit ABI is the authoritative version. */
> > +#pragma pack(push, 8)
> > +
>
> Don't do this, pad and align things explicitly in structs.
>
> > +struct kfd_ioctl_create_queue_args {
> > + uint64_t ring_base_address; /* to KFD */
> > + uint32_t ring_size; /* to KFD */
> > + uint32_t gpu_id;/* to KFD */
> > + uint32_t queue_type;/* to KFD */
> > + uint32_t queue_percentage; /* to KFD */
> > + uint32_t queue_priority;/* to KFD */
> > + uint64_t write_pointer_address; /* to KFD */
> > + uint64_t read_pointer_address; /* to KFD */
> > +
> > + uint64_t doorbell_address; /* from KFD */
> > + uint32_t queue_id; /* from KFD */
> > +};
> > +
>
> maybe put all the uint64_t at the start, or add explicit padding.
>
> Dave.
Thanks, will be fixed.
Oded
[PATCH 13/83] hsa/radeon: Add 2 new IOCTL to kfd, CREATE_QUEUE and DESTROY_QUEUE
> +/* The 64-bit ABI is the authoritative version. */
> +#pragma pack(push, 8)
> +
Don't do this, pad and align things explicitly in structs.
> +struct kfd_ioctl_create_queue_args {
> + uint64_t ring_base_address; /* to KFD */
> + uint32_t ring_size; /* to KFD */
> + uint32_t gpu_id;/* to KFD */
> + uint32_t queue_type;/* to KFD */
> + uint32_t queue_percentage; /* to KFD */
> + uint32_t queue_priority;/* to KFD */
> + uint64_t write_pointer_address; /* to KFD */
> + uint64_t read_pointer_address; /* to KFD */
> +
> + uint64_t doorbell_address; /* from KFD */
> + uint32_t queue_id; /* from KFD */
> +};
> +
maybe put all the uint64_t at the start, or add explicit padding.
Dave.
[PATCH 13/83] hsa/radeon: Add 2 new IOCTL to kfd, CREATE_QUEUE and DESTROY_QUEUE
On Fri, Jul 11, 2014 at 12:50:13AM +0300, Oded Gabbay wrote:
> This patch adds 2 new IOCTL to kfd driver.
>
> The first IOCTL is KFD_IOC_CREATE_QUEUE that is used by the user-mode
> application to create a compute queue on the GPU.
>
> The second IOCTL is KFD_IOC_DESTROY_QUEUE that is used by the
> user-mode application to destroy an existing compute queue on the GPU.
>
> Signed-off-by: Oded Gabbay
> ---
> drivers/gpu/hsa/radeon/kfd_chardev.c | 155
> ++
> drivers/gpu/hsa/radeon/kfd_doorbell.c | 11 +++
> include/uapi/linux/kfd_ioctl.h| 69 +++
> 3 files changed, 235 insertions(+)
> create mode 100644 include/uapi/linux/kfd_ioctl.h
>
> diff --git a/drivers/gpu/hsa/radeon/kfd_chardev.c
> b/drivers/gpu/hsa/radeon/kfd_chardev.c
> index 0b5bc74..4e7d5d0 100644
> --- a/drivers/gpu/hsa/radeon/kfd_chardev.c
> +++ b/drivers/gpu/hsa/radeon/kfd_chardev.c
> @@ -27,11 +27,13 @@
> #include
> #include
> #include
> +#include
> #include "kfd_priv.h"
> #include "kfd_scheduler.h"
>
> static long kfd_ioctl(struct file *, unsigned int, unsigned long);
> static int kfd_open(struct inode *, struct file *);
> +static int kfd_mmap(struct file *, struct vm_area_struct *);
>
> static const char kfd_dev_name[] = "kfd";
>
> @@ -108,17 +110,170 @@ kfd_open(struct inode *inode, struct file *filep)
> return 0;
> }
>
> +static long
> +kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, void
> __user *arg)
> +{
> + struct kfd_ioctl_create_queue_args args;
> + struct kfd_dev *dev;
> + int err = 0;
> + unsigned int queue_id;
> + struct kfd_queue *queue;
> + struct kfd_process_device *pdd;
> +
> + if (copy_from_user(, arg, sizeof(args)))
> + return -EFAULT;
> +
> + dev = radeon_kfd_device_by_id(args.gpu_id);
> + if (dev == NULL)
> + return -EINVAL;
> +
> + queue = kzalloc(
> + offsetof(struct kfd_queue, scheduler_queue) +
> dev->device_info->scheduler_class->queue_size,
> + GFP_KERNEL);
> +
> + if (!queue)
> + return -ENOMEM;
> +
> + queue->dev = dev;
> +
> + mutex_lock(>mutex);
> +
> + pdd = radeon_kfd_bind_process_to_device(dev, p);
> + if (IS_ERR(pdd) < 0) {
> + err = PTR_ERR(pdd);
> + goto err_bind_pasid;
> + }
> +
> + pr_debug("kfd: creating queue number %d for PASID %d on GPU 0x%x\n",
> + pdd->queue_count,
> + p->pasid,
> + dev->id);
> +
> + if (pdd->queue_count++ == 0) {
> + err =
> dev->device_info->scheduler_class->register_process(dev->scheduler, p,
> >scheduler_process);
> + if (err < 0)
> + goto err_register_process;
> + }
> +
> + if (!radeon_kfd_allocate_queue_id(p, _id))
> + goto err_allocate_queue_id;
> +
> + err = dev->device_info->scheduler_class->create_queue(dev->scheduler,
> pdd->scheduler_process,
> +
> >scheduler_queue,
> + (void __user
> *)args.ring_base_address,
> + args.ring_size,
> + (void __user
> *)args.read_pointer_address,
> + (void __user
> *)args.write_pointer_address,
> +
> radeon_kfd_queue_id_to_doorbell(dev, p, queue_id));
> + if (err)
> + goto err_create_queue;
> +
> + radeon_kfd_install_queue(p, queue_id, queue);
> +
> + args.queue_id = queue_id;
> + args.doorbell_address =
> (uint64_t)(uintptr_t)radeon_kfd_get_doorbell(filep, p, dev, queue_id);
> +
> + if (copy_to_user(arg, , sizeof(args))) {
> + err = -EFAULT;
> + goto err_copy_args_out;
> + }
> +
> + mutex_unlock(>mutex);
> +
> + pr_debug("kfd: queue id %d was created successfully.\n"
> + " ring buffer address == 0x%016llX\n"
> + " read ptr address== 0x%016llX\n"
> + " write ptr address == 0x%016llX\n"
> + " doorbell address== 0x%016llX\n",
> + args.queue_id,
> + args.ring_base_address,
> + args.read_pointer_address,
> + args.write_pointer_address,
> + args.doorbell_address);
> +
> + return 0;
> +
> +err_copy_args_out:
> + dev->device_info->scheduler_class->destroy_queue(dev->scheduler,
> >scheduler_queue);
> +err_create_queue:
> + radeon_kfd_remove_queue(p, queue_id);
> +err_allocate_queue_id:
> + if (--pdd->queue_count == 0) {
> +
> dev->device_info->scheduler_class->deregister_process(dev->scheduler,
> pdd->scheduler_process);
> +
[PATCH 13/83] hsa/radeon: Add 2 new IOCTL to kfd, CREATE_QUEUE and DESTROY_QUEUE
On Fri, Jul 11, 2014 at 12:50:13AM +0300, Oded Gabbay wrote:
> This patch adds 2 new IOCTL to kfd driver.
>
> The first IOCTL is KFD_IOC_CREATE_QUEUE that is used by the user-mode
> application to create a compute queue on the GPU.
>
> The second IOCTL is KFD_IOC_DESTROY_QUEUE that is used by the
> user-mode application to destroy an existing compute queue on the GPU.
>
> Signed-off-by: Oded Gabbay
Coding style need fixing. What is the percent argument ? What is it use
for ?
You need to check range validity of argument provided by userspace. Rules
is never trust userspace. Especialy for things like queue_size which is
use without never being check allowing userspace to send 0 which leads
to broken queue size.
Also out of curiosity what kind of event happens if userspace munmap its
ring buffer before unregistering a queue ?
> ---
> drivers/gpu/hsa/radeon/kfd_chardev.c | 155
> ++
> drivers/gpu/hsa/radeon/kfd_doorbell.c | 11 +++
> include/uapi/linux/kfd_ioctl.h| 69 +++
Again better to create an hsa directory for kfd_ioctl.h
> 3 files changed, 235 insertions(+)
> create mode 100644 include/uapi/linux/kfd_ioctl.h
>
> diff --git a/drivers/gpu/hsa/radeon/kfd_chardev.c
> b/drivers/gpu/hsa/radeon/kfd_chardev.c
> index 0b5bc74..4e7d5d0 100644
> --- a/drivers/gpu/hsa/radeon/kfd_chardev.c
> +++ b/drivers/gpu/hsa/radeon/kfd_chardev.c
> @@ -27,11 +27,13 @@
> #include
> #include
> #include
> +#include
> #include "kfd_priv.h"
> #include "kfd_scheduler.h"
>
> static long kfd_ioctl(struct file *, unsigned int, unsigned long);
> static int kfd_open(struct inode *, struct file *);
> +static int kfd_mmap(struct file *, struct vm_area_struct *);
>
> static const char kfd_dev_name[] = "kfd";
>
> @@ -108,17 +110,170 @@ kfd_open(struct inode *inode, struct file *filep)
> return 0;
> }
>
> +static long
> +kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, void
> __user *arg)
> +{
> + struct kfd_ioctl_create_queue_args args;
> + struct kfd_dev *dev;
> + int err = 0;
> + unsigned int queue_id;
> + struct kfd_queue *queue;
> + struct kfd_process_device *pdd;
> +
> + if (copy_from_user(, arg, sizeof(args)))
> + return -EFAULT;
> +
> + dev = radeon_kfd_device_by_id(args.gpu_id);
> + if (dev == NULL)
> + return -EINVAL;
> +
> + queue = kzalloc(
> + offsetof(struct kfd_queue, scheduler_queue) +
> dev->device_info->scheduler_class->queue_size,
> + GFP_KERNEL);
> +
> + if (!queue)
> + return -ENOMEM;
> +
> + queue->dev = dev;
> +
> + mutex_lock(>mutex);
> +
> + pdd = radeon_kfd_bind_process_to_device(dev, p);
> + if (IS_ERR(pdd) < 0) {
> + err = PTR_ERR(pdd);
> + goto err_bind_pasid;
> + }
> +
> + pr_debug("kfd: creating queue number %d for PASID %d on GPU 0x%x\n",
> + pdd->queue_count,
> + p->pasid,
> + dev->id);
> +
> + if (pdd->queue_count++ == 0) {
> + err =
> dev->device_info->scheduler_class->register_process(dev->scheduler, p,
> >scheduler_process);
> + if (err < 0)
> + goto err_register_process;
> + }
> +
> + if (!radeon_kfd_allocate_queue_id(p, _id))
> + goto err_allocate_queue_id;
> +
> + err = dev->device_info->scheduler_class->create_queue(dev->scheduler,
> pdd->scheduler_process,
> +
> >scheduler_queue,
> + (void __user
> *)args.ring_base_address,
> + args.ring_size,
> + (void __user
> *)args.read_pointer_address,
> + (void __user
> *)args.write_pointer_address,
> +
> radeon_kfd_queue_id_to_doorbell(dev, p, queue_id));
> + if (err)
> + goto err_create_queue;
> +
> + radeon_kfd_install_queue(p, queue_id, queue);
> +
> + args.queue_id = queue_id;
> + args.doorbell_address =
> (uint64_t)(uintptr_t)radeon_kfd_get_doorbell(filep, p, dev, queue_id);
> +
> + if (copy_to_user(arg, , sizeof(args))) {
> + err = -EFAULT;
> + goto err_copy_args_out;
> + }
> +
> + mutex_unlock(>mutex);
> +
> + pr_debug("kfd: queue id %d was created successfully.\n"
> + " ring buffer address == 0x%016llX\n"
> + " read ptr address== 0x%016llX\n"
> + " write ptr address == 0x%016llX\n"
> + " doorbell address== 0x%016llX\n",
> + args.queue_id,
> + args.ring_base_address,
> + args.read_pointer_address,
> +
[PATCH 13/83] hsa/radeon: Add 2 new IOCTL to kfd, CREATE_QUEUE and DESTROY_QUEUE
This patch adds 2 new IOCTL to kfd driver.
The first IOCTL is KFD_IOC_CREATE_QUEUE that is used by the user-mode
application to create a compute queue on the GPU.
The second IOCTL is KFD_IOC_DESTROY_QUEUE that is used by the
user-mode application to destroy an existing compute queue on the GPU.
Signed-off-by: Oded Gabbay
---
drivers/gpu/hsa/radeon/kfd_chardev.c | 155 ++
drivers/gpu/hsa/radeon/kfd_doorbell.c | 11 +++
include/uapi/linux/kfd_ioctl.h| 69 +++
3 files changed, 235 insertions(+)
create mode 100644 include/uapi/linux/kfd_ioctl.h
diff --git a/drivers/gpu/hsa/radeon/kfd_chardev.c
b/drivers/gpu/hsa/radeon/kfd_chardev.c
index 0b5bc74..4e7d5d0 100644
--- a/drivers/gpu/hsa/radeon/kfd_chardev.c
+++ b/drivers/gpu/hsa/radeon/kfd_chardev.c
@@ -27,11 +27,13 @@
#include
#include
#include
+#include
#include "kfd_priv.h"
#include "kfd_scheduler.h"
static long kfd_ioctl(struct file *, unsigned int, unsigned long);
static int kfd_open(struct inode *, struct file *);
+static int kfd_mmap(struct file *, struct vm_area_struct *);
static const char kfd_dev_name[] = "kfd";
@@ -108,17 +110,170 @@ kfd_open(struct inode *inode, struct file *filep)
return 0;
}
+static long
+kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, void __user
*arg)
+{
+ struct kfd_ioctl_create_queue_args args;
+ struct kfd_dev *dev;
+ int err = 0;
+ unsigned int queue_id;
+ struct kfd_queue *queue;
+ struct kfd_process_device *pdd;
+
+ if (copy_from_user(, arg, sizeof(args)))
+ return -EFAULT;
+
+ dev = radeon_kfd_device_by_id(args.gpu_id);
+ if (dev == NULL)
+ return -EINVAL;
+
+ queue = kzalloc(
+ offsetof(struct kfd_queue, scheduler_queue) +
dev->device_info->scheduler_class->queue_size,
+ GFP_KERNEL);
+
+ if (!queue)
+ return -ENOMEM;
+
+ queue->dev = dev;
+
+ mutex_lock(>mutex);
+
+ pdd = radeon_kfd_bind_process_to_device(dev, p);
+ if (IS_ERR(pdd) < 0) {
+ err = PTR_ERR(pdd);
+ goto err_bind_pasid;
+ }
+
+ pr_debug("kfd: creating queue number %d for PASID %d on GPU 0x%x\n",
+ pdd->queue_count,
+ p->pasid,
+ dev->id);
+
+ if (pdd->queue_count++ == 0) {
+ err =
dev->device_info->scheduler_class->register_process(dev->scheduler, p,
>scheduler_process);
+ if (err < 0)
+ goto err_register_process;
+ }
+
+ if (!radeon_kfd_allocate_queue_id(p, _id))
+ goto err_allocate_queue_id;
+
+ err = dev->device_info->scheduler_class->create_queue(dev->scheduler,
pdd->scheduler_process,
+
>scheduler_queue,
+ (void __user
*)args.ring_base_address,
+ args.ring_size,
+ (void __user
*)args.read_pointer_address,
+ (void __user
*)args.write_pointer_address,
+
radeon_kfd_queue_id_to_doorbell(dev, p, queue_id));
+ if (err)
+ goto err_create_queue;
+
+ radeon_kfd_install_queue(p, queue_id, queue);
+
+ args.queue_id = queue_id;
+ args.doorbell_address =
(uint64_t)(uintptr_t)radeon_kfd_get_doorbell(filep, p, dev, queue_id);
+
+ if (copy_to_user(arg, , sizeof(args))) {
+ err = -EFAULT;
+ goto err_copy_args_out;
+ }
+
+ mutex_unlock(>mutex);
+
+ pr_debug("kfd: queue id %d was created successfully.\n"
+" ring buffer address == 0x%016llX\n"
+" read ptr address== 0x%016llX\n"
+" write ptr address == 0x%016llX\n"
+" doorbell address== 0x%016llX\n",
+ args.queue_id,
+ args.ring_base_address,
+ args.read_pointer_address,
+ args.write_pointer_address,
+ args.doorbell_address);
+
+ return 0;
+
+err_copy_args_out:
+ dev->device_info->scheduler_class->destroy_queue(dev->scheduler,
>scheduler_queue);
+err_create_queue:
+ radeon_kfd_remove_queue(p, queue_id);
+err_allocate_queue_id:
+ if (--pdd->queue_count == 0) {
+
dev->device_info->scheduler_class->deregister_process(dev->scheduler,
pdd->scheduler_process);
+ pdd->scheduler_process = NULL;
+ }
+err_register_process:
+err_bind_pasid:
+ kfree(queue);
+ mutex_unlock(>mutex);
+ return err;
+}
+
+static int
+kfd_ioctl_destroy_queue(struct file *filp, struct
