[PATCH] drm/i915: Restrict L3 remapping sysfs interface to dwords

[PATCH] drm/i915: Restrict L3 remapping sysfs interface to dwords

[Chris Wilson]

The L3 cache remapping is stored as u32 elements, and we should ensure
that the user only supplies complete slice information(u32).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 57 ++++++++++++++++---------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 034b8abc5062..1e08c5961535 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -144,12 +144,12 @@ static const struct attribute_group media_rc6_attr_group = {
 };
 #endif
 
-static int l3_access_valid(struct drm_i915_private *dev_priv, loff_t offset)
+static int l3_access_valid(struct drm_i915_private *i915, loff_t offset)
 {
-	if (!HAS_L3_DPF(dev_priv))
+	if (!HAS_L3_DPF(i915))
 		return -EPERM;
 
-	if (offset % 4 != 0)
+	if (!IS_ALIGNED(offset, sizeof(u32)))
 		return -EINVAL;
 
 	if (offset >= GEN7_L3LOG_SIZE)
@@ -164,31 +164,28 @@ i915_l3_read(struct file *filp, struct kobject *kobj,
 	     loff_t offset, size_t count)
 {
 	struct device *kdev = kobj_to_dev(kobj);
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct drm_device *dev = &dev_priv->drm;
+	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
 	int slice = (int)(uintptr_t)attr->private;
 	int ret;
 
-	count = round_down(count, 4);
-
-	ret = l3_access_valid(dev_priv, offset);
+	ret = l3_access_valid(i915, offset);
 	if (ret)
 		return ret;
 
+	count = round_down(count, sizeof(u32));
 	count = min_t(size_t, GEN7_L3LOG_SIZE - offset, count);
+	memset(buf, 0, count);
 
-	ret = i915_mutex_lock_interruptible(dev);
+	ret = i915_mutex_lock_interruptible(&i915->drm);
 	if (ret)
 		return ret;
 
-	if (dev_priv->l3_parity.remap_info[slice])
+	if (i915->l3_parity.remap_info[slice])
 		memcpy(buf,
-		       dev_priv->l3_parity.remap_info[slice] + (offset/4),
+		       i915->l3_parity.remap_info[slice] + offset / sizeof(u32),
 		       count);
-	else
-		memset(buf, 0, count);
 
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&i915->drm.struct_mutex);
 
 	return count;
 }
@@ -199,22 +196,24 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
 	      loff_t offset, size_t count)
 {
 	struct device *kdev = kobj_to_dev(kobj);
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct drm_device *dev = &dev_priv->drm;
-	struct i915_gem_context *ctx;
+	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
 	int slice = (int)(uintptr_t)attr->private;
+	struct i915_gem_context *ctx;
 	u32 **remap_info;
 	int ret;
 
-	ret = l3_access_valid(dev_priv, offset);
+	ret = l3_access_valid(i915, offset);
 	if (ret)
 		return ret;
 
-	ret = i915_mutex_lock_interruptible(dev);
+	if (count < sizeof(u32))
+		return -EINVAL;
+
+	ret = i915_mutex_lock_interruptible(&i915->drm);
 	if (ret)
 		return ret;
 
-	remap_info = &dev_priv->l3_parity.remap_info[slice];
+	remap_info = &i915->l3_parity.remap_info[slice];
 	if (!*remap_info) {
 		*remap_info = kzalloc(GEN7_L3LOG_SIZE, GFP_KERNEL);
 		if (!*remap_info) {
@@ -223,20 +222,22 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
 		}
 	}
 
-	/* TODO: Ideally we really want a GPU reset here to make sure errors
+	count = round_down(count, sizeof(u32));
+	memcpy(*remap_info + offset / sizeof(u32), buf, count);
+
+	/* NB: We defer the remapping until we switch to the context */
+	list_for_each_entry(ctx, &i915->contexts.list, link)
+		ctx->remap_slice |= BIT(slice);
+
+	/*
+	 * TODO: Ideally we really want a GPU reset here to make sure errors
 	 * aren't propagated. Since I cannot find a stable way to reset the GPU
 	 * at this point it is left as a TODO.
 	*/
-	memcpy(*remap_info + (offset/4), buf, count);
-
-	/* NB: We defer the remapping until we switch to the context */
-	list_for_each_entry(ctx, &dev_priv->contexts.list, link)
-		ctx->remap_slice |= (1<<slice);
 
 	ret = count;
-
 out:
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&i915->drm.struct_mutex);
 
 	return ret;
 }
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Restrict L3 remapping sysfs interface to dwords

[Tvrtko Ursulin]

On 04/10/2019 11:59, Chris Wilson wrote:
> The L3 cache remapping is stored as u32 elements, and we should ensure
> that the user only supplies complete slice information(u32).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_sysfs.c | 57 ++++++++++++++++---------------
>   1 file changed, 29 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 034b8abc5062..1e08c5961535 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -144,12 +144,12 @@ static const struct attribute_group media_rc6_attr_group = {
>   };
>   #endif
>   
> -static int l3_access_valid(struct drm_i915_private *dev_priv, loff_t offset)
> +static int l3_access_valid(struct drm_i915_private *i915, loff_t offset)
>   {
> -	if (!HAS_L3_DPF(dev_priv))
> +	if (!HAS_L3_DPF(i915))
>   		return -EPERM;
>   
> -	if (offset % 4 != 0)
> +	if (!IS_ALIGNED(offset, sizeof(u32)))
>   		return -EINVAL;
>   
>   	if (offset >= GEN7_L3LOG_SIZE)
> @@ -164,31 +164,28 @@ i915_l3_read(struct file *filp, struct kobject *kobj,
>   	     loff_t offset, size_t count)
>   {
>   	struct device *kdev = kobj_to_dev(kobj);
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct drm_device *dev = &dev_priv->drm;
> +	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
>   	int slice = (int)(uintptr_t)attr->private;
>   	int ret;
>   
> -	count = round_down(count, 4);
> -
> -	ret = l3_access_valid(dev_priv, offset);
> +	ret = l3_access_valid(i915, offset);
>   	if (ret)
>   		return ret;
>   
> +	count = round_down(count, sizeof(u32));
>   	count = min_t(size_t, GEN7_L3LOG_SIZE - offset, count);
> +	memset(buf, 0, count);
>   
> -	ret = i915_mutex_lock_interruptible(dev);
> +	ret = i915_mutex_lock_interruptible(&i915->drm);
>   	if (ret)
>   		return ret;
>   
> -	if (dev_priv->l3_parity.remap_info[slice])
> +	if (i915->l3_parity.remap_info[slice])
>   		memcpy(buf,
> -		       dev_priv->l3_parity.remap_info[slice] + (offset/4),
> +		       i915->l3_parity.remap_info[slice] + offset / sizeof(u32),
>   		       count);
> -	else
> -		memset(buf, 0, count);
>   
> -	mutex_unlock(&dev->struct_mutex);
> +	mutex_unlock(&i915->drm.struct_mutex);
>   
>   	return count;
>   }
> @@ -199,22 +196,24 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
>   	      loff_t offset, size_t count)
>   {
>   	struct device *kdev = kobj_to_dev(kobj);
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct drm_device *dev = &dev_priv->drm;
> -	struct i915_gem_context *ctx;
> +	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
>   	int slice = (int)(uintptr_t)attr->private;
> +	struct i915_gem_context *ctx;
>   	u32 **remap_info;
>   	int ret;
>   
> -	ret = l3_access_valid(dev_priv, offset);
> +	ret = l3_access_valid(i915, offset);
>   	if (ret)
>   		return ret;
>   
> -	ret = i915_mutex_lock_interruptible(dev);
> +	if (count < sizeof(u32))
> +		return -EINVAL;
> +
> +	ret = i915_mutex_lock_interruptible(&i915->drm);
>   	if (ret)
>   		return ret;
>   
> -	remap_info = &dev_priv->l3_parity.remap_info[slice];
> +	remap_info = &i915->l3_parity.remap_info[slice];
>   	if (!*remap_info) {
>   		*remap_info = kzalloc(GEN7_L3LOG_SIZE, GFP_KERNEL);
>   		if (!*remap_info) {
> @@ -223,20 +222,22 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
>   		}
>   	}
>   
> -	/* TODO: Ideally we really want a GPU reset here to make sure errors
> +	count = round_down(count, sizeof(u32));
> +	memcpy(*remap_info + offset / sizeof(u32), buf, count);
> +
> +	/* NB: We defer the remapping until we switch to the context */
> +	list_for_each_entry(ctx, &i915->contexts.list, link)
> +		ctx->remap_slice |= BIT(slice);
> +
> +	/*
> +	 * TODO: Ideally we really want a GPU reset here to make sure errors
>   	 * aren't propagated. Since I cannot find a stable way to reset the GPU
>   	 * at this point it is left as a TODO.
>   	*/
> -	memcpy(*remap_info + (offset/4), buf, count);
> -
> -	/* NB: We defer the remapping until we switch to the context */
> -	list_for_each_entry(ctx, &dev_priv->contexts.list, link)
> -		ctx->remap_slice |= (1<<slice);
>   
>   	ret = count;
> -
>   out:
> -	mutex_unlock(&dev->struct_mutex);
> +	mutex_unlock(&i915->drm.struct_mutex);
>   
>   	return ret;
>   }
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Restrict L3 remapping sysfs interface to dwords

[Patchwork]

== Series Details ==

Series: drm/i915: Restrict L3 remapping sysfs interface to dwords
URL   : https://patchwork.freedesktop.org/series/67588/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7005 -> Patchwork_14666
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_14666 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_14666, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14666/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_14666:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_execlists:
    - fi-cfl-8700k:       [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7005/fi-cfl-8700k/igt@i915_selftest@live_execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14666/fi-cfl-8700k/igt@i915_selftest@live_execlists.html

  
Known issues
------------

  Here are the changes found in Patchwork_14666 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       [PASS][3] -> [INCOMPLETE][4] ([fdo#107718])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7005/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14666/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-icl-u3:          [PASS][5] -> [DMESG-WARN][6] ([fdo#107724]) +3 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7005/fi-icl-u3/igt@gem_exec_suspend@basic-s4-devices.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14666/fi-icl-u3/igt@gem_exec_suspend@basic-s4-devices.html

  
#### Possible fixes ####

  * igt@gem_flink_basic@double-flink:
    - fi-icl-u3:          [DMESG-WARN][7] ([fdo#107724]) -> [PASS][8] +2 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7005/fi-icl-u3/igt@gem_flink_basic@double-flink.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14666/fi-icl-u3/igt@gem_flink_basic@double-flink.html

  * igt@i915_selftest@live_execlists:
    - fi-icl-u2:          [DMESG-FAIL][9] -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7005/fi-icl-u2/igt@i915_selftest@live_execlists.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14666/fi-icl-u2/igt@i915_selftest@live_execlists.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-icl-u2:          [FAIL][11] ([fdo#109635 ]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7005/fi-icl-u2/igt@kms_chamelium@hdmi-crc-fast.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14666/fi-icl-u2/igt@kms_chamelium@hdmi-crc-fast.html

  
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109635 ]: https://bugs.freedesktop.org/show_bug.cgi?id=109635 


Participating hosts (51 -> 44)
------------------------------

  Additional (1): fi-icl-u4 
  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-bsw-n3050 fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7005 -> Patchwork_14666

  CI-20190529: 20190529
  CI_DRM_7005: 86a485c2df9dee0d2c6dad4c6444facce79ca469 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5211: 1601e1571eb0f29a06b64494040b3ea7859a650f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14666: f5ff687aad3bd662d37769a3c7fa28008ff4f61d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f5ff687aad3b drm/i915: Restrict L3 remapping sysfs interface to dwords

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14666/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx