From: "Christian König" <deathsimple@vodafone.de>
To: Oded Gabbay <oded.gabbay@amd.com>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()
Date: Mon, 29 Dec 2014 14:45:29 +0100 [thread overview]
Message-ID: <54A15AF9.3070905@vodafone.de> (raw)
In-Reply-To: <54A15597.9020906@amd.com>
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@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/)
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)
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
prev parent reply other threads:[~2014-12-29 13:46 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-29 12:42 [PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl() Oded Gabbay
2014-12-29 12:42 ` [PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style Oded Gabbay
2014-12-29 13:06 ` Christian König
2014-12-29 13:17 ` Oded Gabbay
2014-12-29 13:32 ` Christian König
2014-12-29 13:35 ` Oded Gabbay
2014-12-29 12:42 ` [PATCH 3/3] drm/amdkfd: rewrite kfd_ioctl() according to drm_ioctl() Oded Gabbay
2014-12-29 13:05 ` [PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl() Christian König
2014-12-29 13:22 ` Oded Gabbay
2014-12-29 13:45 ` Christian König [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54A15AF9.3070905@vodafone.de \
--to=deathsimple@vodafone.de \
--cc=dri-devel@lists.freedesktop.org \
--cc=oded.gabbay@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.