Re: [PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()

On 12/29/2014 03:05 PM, Christian König wrote:

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:

...

This patch moves the copy_to_user() and copy_from_user() calls from the different ioctl functions in amdkfd to the general kfd_ioctl() function, as this is a common code for all ioctls.

This was done according to example taken from drm_ioctl.c

Signed-off-by: Oded Gabbay oded.gabbay@amd.com

In general sounds like a good idea to me and the patch is "Reviewed-by: Christian König christian.koenig@amd.com" for now.

What really questions me is why we need all this code duplication and can't reuse the DRM infrastructure for this. But that's more a problem in the long term.

Do you mean registering as DRM IOCTL ? Or do you mean something else ?

If it is the former, than I think the main problem is that we use different devices (/dev/kfd vs. /dev/dri/)

If it is the latter, could you give me more specifics ?

Oded

Regards, Christian.

...

---

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 +++++++++++++++---------------- 1 file changed, 117 insertions(+), 117 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c index 7d4974b..5460ad2 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c @@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, struct file *filep) return 0; } -static long kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,

•                void __user *arg)
  

+static int kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,

•                void *data)
  

  {

• struct kfd_ioctl_get_version_args args;

• struct kfd_ioctl_get_version_args *args = data; int err = 0;

• args.major_version = KFD_IOCTL_MAJOR_VERSION;
• args.minor_version = KFD_IOCTL_MINOR_VERSION;
• if (copy_to_user(arg, &args, sizeof(args)))

•    err = -EFAULT;
  

• args->major_version = KFD_IOCTL_MAJOR_VERSION;
• args->minor_version = KFD_IOCTL_MINOR_VERSION; return err; }

@@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct queue_properties *q_properties, return 0; } -static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,

•                void __user *arg)
  

+static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,

•                void *data)
  

  {

• struct kfd_ioctl_create_queue_args args;

• struct kfd_ioctl_create_queue_args *args = data; struct kfd_dev *dev; int err = 0; unsigned int queue_id;

@@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, memset(&q_properties, 0, sizeof(struct queue_properties));

• if (copy_from_user(&args, arg, sizeof(args)))

•    return -EFAULT;
  

• pr_debug("kfd: creating queue ioctl\n");
  

• err = set_queue_properties_from_user(&q_properties, &args);

• err = set_queue_properties_from_user(&q_properties, args); if (err) return err;

• dev = kfd_device_by_id(args.gpu_id);

• dev = kfd_device_by_id(args->gpu_id); if (dev == NULL) return -EINVAL;

@@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, pdd = kfd_bind_process_to_device(dev, p); if (IS_ERR(pdd)) {

•    err = PTR_ERR(pdd);
  

•    err = -ESRCH;
      goto err_bind_process;
  }
  

@@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, if (err != 0) goto err_create_queue;

• args.queue_id = queue_id;

• args->queue_id = queue_id; /* Return gpu_id as doorbell offset for mmap usage */

• args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
• if (copy_to_user(arg, &args, sizeof(args))) {

•    err = -EFAULT;
  

•    goto err_copy_args_out;
  

• }

• args->doorbell_offset = args->gpu_id << PAGE_SHIFT; mutex_unlock(&p->mutex);

• pr_debug("kfd: queue id %d was created successfully\n", args.queue_id);

• pr_debug("kfd: queue id %d was created successfully\n", args->queue_id); pr_debug("ring buffer address == 0x%016llX\n",

•        args.ring_base_address);
  

•        args->ring_base_address);
  pr_debug("read ptr address    == 0x%016llX\n",
  

•        args.read_pointer_address);
  

•        args->read_pointer_address);
  pr_debug("write ptr address   == 0x%016llX\n",
  

•        args.write_pointer_address);
  

•        args->write_pointer_address);
  return 0;
  

-err_copy_args_out:

• pqm_destroy_queue(&p->pqm, queue_id); err_create_queue: err_bind_process: mutex_unlock(&p->mutex);

@@ -297,99 +284,90 @@ err_bind_process: } static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,

•                void __user *arg)
  

•                void *data)
  

  { int retval;

• struct kfd_ioctl_destroy_queue_args args;
• if (copy_from_user(&args, arg, sizeof(args)))

•    return -EFAULT;
  

• struct kfd_ioctl_destroy_queue_args *args = data; pr_debug("kfd: destroying queue id %d for PASID %d\n",

•            args.queue_id,
  

•            args->queue_id,
              p->pasid);
  mutex_lock(&p->mutex);
  

• retval = pqm_destroy_queue(&p->pqm, args.queue_id);

• retval = pqm_destroy_queue(&p->pqm, args->queue_id); mutex_unlock(&p->mutex); return retval; } static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,

•                void __user *arg)
  

•                void *data)
  

  { int retval;

• struct kfd_ioctl_update_queue_args args;

• struct kfd_ioctl_update_queue_args *args = data; struct queue_properties properties;

• if (copy_from_user(&args, arg, sizeof(args)))

•    return -EFAULT;
  

• if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {

• if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) { pr_err("kfd: queue percentage must be between 0 to

KFD_MAX_QUEUE_PERCENTAGE\n"); return -EINVAL; }

• if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {

• if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) { pr_err("kfd: queue priority must be between 0 to

KFD_MAX_QUEUE_PRIORITY\n"); return -EINVAL; }

• if ((args.ring_base_address) &&

• if ((args->ring_base_address) && (!access_ok(VERIFY_WRITE,

•        (const void __user *) args.ring_base_address,
  

•        (const void __user *) args->ring_base_address,
          sizeof(uint64_t)))) {
      pr_err("kfd: can't access ring base address\n");
      return -EFAULT;
  }
  

• if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {

• if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) { pr_err("kfd: ring size must be a power of 2 or 0\n"); return -EINVAL; }

• properties.queue_address = args.ring_base_address;
• properties.queue_size = args.ring_size;
• properties.queue_percent = args.queue_percentage;
• properties.priority = args.queue_priority;

• properties.queue_address = args->ring_base_address;
• properties.queue_size = args->ring_size;
• properties.queue_percent = args->queue_percentage;
• properties.priority = args->queue_priority; pr_debug("kfd: updating queue id %d for PASID %d\n",

•        args.queue_id, p->pasid);
  

•        args->queue_id, p->pasid);
  mutex_lock(&p->mutex);
  

• retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);

• retval = pqm_update_queue(&p->pqm, args->queue_id, &properties); mutex_unlock(&p->mutex); return retval; }

-static long kfd_ioctl_set_memory_policy(struct file *filep,

•            struct kfd_process *p, void __user *arg)
  

+static int kfd_ioctl_set_memory_policy(struct file *filep,

•                struct kfd_process *p, void *data)
  

  {

• struct kfd_ioctl_set_memory_policy_args args;

• struct kfd_ioctl_set_memory_policy_args *args = data; struct kfd_dev *dev; int err = 0; struct kfd_process_device *pdd; enum cache_policy default_policy, alternate_policy;

• if (copy_from_user(&args, arg, sizeof(args)))

•    return -EFAULT;
  

• if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT

•    && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
  

• if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT

•    && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
      return -EINVAL;
  }
  

• if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT

•    && args.alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
  

• if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT

•    && args->alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
      return -EINVAL;
  }
  

• dev = kfd_device_by_id(args.gpu_id);

• dev = kfd_device_by_id(args->gpu_id); if (dev == NULL) return -EINVAL;

@@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct file *filep, pdd = kfd_bind_process_to_device(dev, p); if (IS_ERR(pdd)) {

•    err = PTR_ERR(pdd);
  

•    err = -ESRCH;
      goto out;
  }
  

• default_policy = (args.default_policy == KFD_IOC_CACHE_POLICY_COHERENT)

• default_policy = (args->default_policy == KFD_IOC_CACHE_POLICY_COHERENT) ? cache_policy_coherent : cache_policy_noncoherent; alternate_policy =

•    (args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
  

•    (args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
         ? cache_policy_coherent : cache_policy_noncoherent;
  if (!dev->dqm->set_cache_memory_policy(dev->dqm,
              &pdd->qpd,
              default_policy,
              alternate_policy,
  

•            (void __user *)args.alternate_aperture_base,
  

•            args.alternate_aperture_size))
  

•            (void __user *)args->alternate_aperture_base,
  

•            args->alternate_aperture_size))
      err = -EINVAL;
  

  out:

@@ -422,53 +400,44 @@ out: return err; } -static long kfd_ioctl_get_clock_counters(struct file *filep,

•            struct kfd_process *p, void __user *arg)
  

+static int kfd_ioctl_get_clock_counters(struct file *filep,

•            struct kfd_process *p, void *data)
  

  {

• struct kfd_ioctl_get_clock_counters_args args;

• struct kfd_ioctl_get_clock_counters_args *args = data; struct kfd_dev *dev; struct timespec time;

• if (copy_from_user(&args, arg, sizeof(args)))

•    return -EFAULT;
  

• dev = kfd_device_by_id(args.gpu_id);

• dev = kfd_device_by_id(args->gpu_id); if (dev == NULL) return -EINVAL; /* Reading GPU clock counter from KGD */

• args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);

• args->gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd); /* No access to rdtsc. Using raw monotonic time */ getrawmonotonic(&time);

• args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);

• args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time); get_monotonic_boottime(&time);

• args.system_clock_counter = (uint64_t)timespec_to_ns(&time);

• args->system_clock_counter = (uint64_t)timespec_to_ns(&time); /* Since the counter is in nano-seconds we use 1GHz frequency */

• args.system_clock_freq = 1000000000;
• if (copy_to_user(arg, &args, sizeof(args)))

•    return -EFAULT;
  

• args->system_clock_freq = 1000000000; return 0; } static int kfd_ioctl_get_process_apertures(struct file *filp,

•            struct kfd_process *p, void __user *arg)
  

•            struct kfd_process *p, void *data)
  

  {

• struct kfd_ioctl_get_process_apertures_args args;

• struct kfd_ioctl_get_process_apertures_args *args = data; struct kfd_process_device_apertures *pAperture; struct kfd_process_device *pdd; dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);

• if (copy_from_user(&args, arg, sizeof(args)))

•    return -EFAULT;
  

• args.num_of_nodes = 0;

• args->num_of_nodes = 0; mutex_lock(&p->mutex);

@@ -477,7 +446,8 @@ static int kfd_ioctl_get_process_apertures(struct file *filp, /* Run over all pdd of the process */ pdd = kfd_get_first_process_device_data(p); do {

•        pAperture = &args.process_apertures[args.num_of_nodes];
  

•        pAperture =
  

•            &args->process_apertures[args->num_of_nodes];
          pAperture->gpu_id = pdd->dev->id;
          pAperture->lds_base = pdd->lds_base;
          pAperture->lds_limit = pdd->lds_limit;
  

@@ -487,7 +457,7 @@ static int kfd_ioctl_get_process_apertures(struct file *filp, pAperture->scratch_limit = pdd->scratch_limit; dev_dbg(kfd_device,

•            "node id %u\n", args.num_of_nodes);
  

•            "node id %u\n", args->num_of_nodes);
          dev_dbg(kfd_device,
              "gpu id %u\n", pdd->dev->id);
          dev_dbg(kfd_device,
  

@@ -503,23 +473,23 @@ static int kfd_ioctl_get_process_apertures(struct file *filp, dev_dbg(kfd_device, "scratch_limit %llX\n", pdd->scratch_limit);

•        args.num_of_nodes++;
  

•        args->num_of_nodes++;
      } while ((pdd = kfd_get_next_process_device_data(p, pdd)) != NULL &&
  

•            (args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
  

•            (args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
  }
  mutex_unlock(&p->mutex);
  

• if (copy_to_user(arg, &args, sizeof(args)))

•    return -EFAULT;
  

• return 0;
  

  } static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) { struct kfd_process *process;
• long err = -EINVAL;

• char stack_kdata[128];
• char *kdata = NULL;
• unsigned int usize, asize;
• int retcode = -EINVAL; dev_dbg(kfd_device, "ioctl cmd 0x%x (#%d), arg 0x%lx\n",

@@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) if (IS_ERR(process)) return PTR_ERR(process);

• if (cmd & (IOC_IN | IOC_OUT)) {

•    if (asize <= sizeof(stack_kdata)) {
  

•        kdata = stack_kdata;
  

•    } else {
  

•        kdata = kmalloc(asize, GFP_KERNEL);
  

•        if (!kdata) {
  

•            retcode = -ENOMEM;
  

•            goto err_i1;
  

•        }
  

•    }
  

•    if (asize > usize)
  

•        memset(kdata + usize, 0, asize - usize);
  

• }
• if (cmd & IOC_IN) {

•    if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
  

•        retcode = -EFAULT;
  

•        goto err_i1;
  

•    }
  

• } else if (cmd & IOC_OUT) {

•    memset(kdata, 0, usize);
  

• }

• switch (cmd) {
  case KFD_IOC_GET_VERSION:
  

•    err = kfd_ioctl_get_version(filep, process, (void __user *)arg);
  

•    retcode = kfd_ioctl_get_version(filep, process, kdata);
      break;
  case KFD_IOC_CREATE_QUEUE:
  

•    err = kfd_ioctl_create_queue(filep, process,
  

•                    (void __user *)arg);
  

•    retcode = kfd_ioctl_create_queue(filep, process,
  

•                    kdata);
      break;
  case KFD_IOC_DESTROY_QUEUE:
  

•    err = kfd_ioctl_destroy_queue(filep, process,
  

•                    (void __user *)arg);
  

•    retcode = kfd_ioctl_destroy_queue(filep, process,
  

•                    kdata);
      break;
  case KFD_IOC_SET_MEMORY_POLICY:
  

•    err = kfd_ioctl_set_memory_policy(filep, process,
  

•                    (void __user *)arg);
  

•    retcode = kfd_ioctl_set_memory_policy(filep, process,
  

•                    kdata);
      break;
  case KFD_IOC_GET_CLOCK_COUNTERS:
  

•    err = kfd_ioctl_get_clock_counters(filep, process,
  

•                    (void __user *)arg);
  

•    retcode = kfd_ioctl_get_clock_counters(filep, process,
  

•                    kdata);
      break;
  case KFD_IOC_GET_PROCESS_APERTURES:
  

•    err = kfd_ioctl_get_process_apertures(filep, process,
  

•                    (void __user *)arg);
  

•    retcode = kfd_ioctl_get_process_apertures(filep, process,
  

•                    kdata);
      break;
  case KFD_IOC_UPDATE_QUEUE:
  

•    err = kfd_ioctl_update_queue(filep, process,
  

•                    (void __user *)arg);
  

•    retcode = kfd_ioctl_update_queue(filep, process,
  

•                    kdata);
      break;
  default:
  

•    dev_err(kfd_device,
  

•    dev_dbg(kfd_device,
          "unknown ioctl cmd 0x%x, arg 0x%lx)\n",
          cmd, arg);
  

•    err = -EINVAL;
  

•    retcode = -EINVAL;
      break;
  }
  

• if (err < 0)

•    dev_err(kfd_device,
  

•        "ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
  

•        err, cmd, _IOC_NR(cmd));
  

• if (cmd & IOC_OUT)

•    if (copy_to_user((void __user *)arg, kdata, usize) != 0)
  

•        retcode = -EFAULT;
  

• return err;

+err_i1:

• if (kdata != stack_kdata)

•    kfree(kdata);
  

• if (retcode)

•    dev_dbg(kfd_device, "ret = %d\n", retcode);
  

• return retcode; } static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)