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

Christian König deathsimple at vodafone.de
Mon Dec 29 05:45:29 PST 2014


Am 29.12.2014 um 14:22 schrieb Oded Gabbay:
>
>
> 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 at amd.com>
>>
>> In general sounds like a good idea to me and the patch is "Reviewed-by:
>> Christian König <christian.koenig at 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/)

Ah, yes of course. I simply keep forgetting that we have two device 
nodes for the same physical hardware.

Christian.

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



More information about the dri-devel mailing list