Re: [PATCH 10/10] drm/i915: Disable use of stolen area by User when Intel RST is present

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 22, 2015 at 11:50:53AM +0530, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> 
> The BIOS RapidStartTechnology may corrupt the stolen memory across S3
> suspend due to unalarmed hibernation, in which case we will not be able
> to preserve the User data stored in the stolen region. Hence this patch
> tries to identify presence of the RST device on the ACPI bus, and
> disables use of stolen memory (for persistent data) if found.
> 
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

Hm, could be that acpi mainatainers want to encapsulate this logic within
the acpi subsystem, and only expose a simple helper to drivers like i915.
Please add the same Cc: list to this patch as to the acpi one when
resending, so that acpi maintainers have the full context.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  7 +++++++
>  drivers/gpu/drm/i915/i915_gem.c        |  8 ++++++++
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_acpi.c      | 20 ++++++++++++++++++++
>  4 files changed, 49 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 492878a..d26a8f1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1309,6 +1309,9 @@ struct i915_gem_mm {
>  	 */
>  	bool busy;
>  
> +	/* Intel RapidStart Technology info */
> +	bool nonvolatile_stolen;
> +
>  	/* the indicator for dispatch video commands on two BSD rings */
>  	int bsd_ring_dispatch_index;
>  
> @@ -3417,9 +3420,13 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
>  #ifdef CONFIG_ACPI
>  extern void intel_register_dsm_handler(void);
>  extern void intel_unregister_dsm_handler(void);
> +extern bool intel_detect_acpi_rst(void);
> +extern int match_device(struct device *dev, void* ids);
>  #else
>  static inline void intel_register_dsm_handler(void) { return; }
>  static inline void intel_unregister_dsm_handler(void) { return; }
> +static inline bool intel_detect_acpi_rst(void) { return false; }
> +static int match_device(struct device *dev, void* ids) { return 0; }
>  #endif /* CONFIG_ACPI */
>  
>  /* modesetting */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d27a41e..daca05f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -399,6 +399,7 @@ i915_gem_create(struct drm_file *file,
>  		uint32_t *handle_p)
>  {
>  	struct drm_i915_gem_object *obj;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  	u32 handle;
>  
> @@ -411,6 +412,13 @@ i915_gem_create(struct drm_file *file,
>  
>  	/* Allocate the new object */
>  	if (flags & I915_CREATE_PLACEMENT_STOLEN) {
> +		if (!dev_priv->mm.nonvolatile_stolen) {
> +			/* Stolen may be overwritten by external parties
> +			 * so unsuitable for persistent user data.
> +			 */
> +			return -ENODEV;
> +		}
> +
>  		mutex_lock(&dev->struct_mutex);
>  		obj = i915_gem_object_create_stolen(dev, size);
>  		if (IS_ERR(obj)) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 6d1af9d..98b4998 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -482,6 +482,20 @@ int i915_gem_init_stolen(struct drm_device *dev)
>  	 */
>  	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_usable_size);
>  
> +	/* If the stolen region can be modified behind our backs upon suspend,
> +	 * then we cannot use it to store nonvolatile contents (i.e user data)
> +	 * as it will be corrupted upon resume.
> +	 */
> +	dev_priv->mm.nonvolatile_stolen = true;
> +#ifdef CONFIG_SUSPEND
> +	if (intel_detect_acpi_rst()) {
> +		/* BIOSes using RapidStart Technology have been reported
> +		 * to overwrite stolen across S3, not just S4.
> +		 */
> +		dev_priv->mm.nonvolatile_stolen = false;
> +	}
> +#endif
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
> index eb638a1..5f7713d 100644
> --- a/drivers/gpu/drm/i915/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/intel_acpi.c
> @@ -23,6 +23,11 @@ static const u8 intel_dsm_guid[] = {
>  	0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c
>  };
>  
> +static const struct acpi_device_id irst_ids[] = {
> +	{"INT3392", 0},
> +	{"", 0}
> +};
> +
>  static char *intel_dsm_port_name(u8 id)
>  {
>  	switch (id) {
> @@ -162,3 +167,18 @@ void intel_register_dsm_handler(void)
>  void intel_unregister_dsm_handler(void)
>  {
>  }
> +
> +int match_device(struct device *dev, void* ids)
> +{
> +	if (acpi_match_device(irst_ids, dev))
> +		return 1;
> +
> +	return 0;
> +}
> +bool intel_detect_acpi_rst(void)
> +{
> +	if (bus_for_each_dev(&acpi_bus_type, NULL, NULL, match_device))
> +		return true;;
> +
> +	return false;
> +}
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx