[Intel-gfx] [PATCH v7 12/12] drm/i915: add support for perf configuration queries

[Lionel Landwerlin]

On 09/07/2019 13:07, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-07-09 10:32:08)
>> +static int query_perf_config_data(struct drm_i915_private *i915,
>> +                                 struct drm_i915_query_item *query_item,
>> +                                 bool use_uuid)
>> +{
>> +       struct drm_i915_query_perf_config __user *user_query_config_ptr =
>> +               u64_to_user_ptr(query_item->data_ptr);
>> +       struct drm_i915_perf_oa_config __user *user_config_ptr =
>> +               u64_to_user_ptr(query_item->data_ptr +
>> +                               sizeof(struct drm_i915_query_perf_config));
>> +       struct drm_i915_perf_oa_config user_config;
>> +       struct i915_oa_config *oa_config = NULL;
>> +       u32 flags, total_size;
>> +       int ret;
>> +
>> +       if (!i915->perf.initialized)
>> +               return -ENODEV;
>> +
>> +       total_size = sizeof(struct drm_i915_query_perf_config) +
>> +               sizeof(struct drm_i915_perf_oa_config);
>> +
>> +       if (query_item->length == 0)
>> +               return total_size;
>> +
>> +       if (query_item->length < total_size) {
>> +               DRM_DEBUG("Invalid query config data item size=%u expected=%u\n",
>> +                         query_item->length, total_size);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!access_ok(user_query_config_ptr, total_size))
>> +               return -EFAULT;
>> +
>> +       if (__get_user(flags, &user_query_config_ptr->flags))
>> +               return -EFAULT;
>> +
>> +       if (flags != 0)
>> +               return -EINVAL;
>> +
>> +       ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (use_uuid) {
>> +               char uuid[UUID_STRING_LEN + 1] = { 0, };
>> +               struct i915_oa_config *tmp;
>> +               int id;
>> +
>> +               BUILD_BUG_ON(sizeof(user_query_config_ptr->uuid) >= sizeof(uuid));
>> +
>> +               if (__copy_from_user(uuid, user_query_config_ptr->uuid,
>> +                                    sizeof(user_query_config_ptr->uuid))) {
> __copy_from_user() from inside a mutex brings back nightmares. I think
> you are ok, but you are tarnishing this lock with mmap_sem, which is
> huge and may bite in future. And it looks superfluous, no? You could do
> the copy form user before taking the lock, and then once you have the
> config pinned, you can drop the lock again.
> -Chris
>
Sounds good, updating.


-Lionel