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

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: ankitprasad.r.sharma@intel.com, intel-gfx@lists.freedesktop.org
Cc: akash.goel@intel.com, shashidhar.hiremath@intel.com
Subject: Re: [PATCH 10/10] drm/i915: Disable use of stolen area by User when Intel RST is present
Date: Fri, 19 Feb 2016 10:34:18 +0000	[thread overview]
Message-ID: <56C6EFAA.4020608@linux.intel.com> (raw)
In-Reply-To: <1455864697-32140-11-git-send-email-ankitprasad.r.sharma@intel.com>


On 19/02/16 06:51, 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.
>
> v2: Updated comment, updated/corrected new functions private to driver
> (Chris/Tvrtko)
>
> v3: Disabling stolen by default, wait till required acpi changes to
> detect device presence are pulled in (Ankit)
>
> v4: Enabled stolen by default as required acpi changes are merged
> (Ankit)
>
> v5: renamed variable, is IS_ENABLED() in place of #ifdef, use char*
> instead of structures (Lukas)
>

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

Regards,

Tvrtko

> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> ---
>   drivers/gpu/drm/i915/i915_drv.h        | 11 +++++++++++
>   drivers/gpu/drm/i915/i915_gem.c        |  8 ++++++++
>   drivers/gpu/drm/i915/i915_gem_stolen.c | 12 ++++++++++++
>   drivers/gpu/drm/i915/intel_acpi.c      |  7 +++++++
>   4 files changed, 38 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 16f2f94..75e6935 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1349,6 +1349,16 @@ struct i915_gem_mm {
>   	 */
>   	bool busy;
>
> +	/**
> +	 * Stolen will be lost upon hibernate (as the memory is unpowered).
> +	 * Across resume, we expect stolen to be intact - however, it may
> +	 * also be utililised by third parties (e.g. Intel RapidStart
> +	 * Technology) and if so we have to assume that any data stored in
> +	 * stolen across resume is lost and we set this flag to indicate that
> +	 * the stolen memory is volatile.
> +	 */
> +	bool volatile_stolen;
> +
>   	/* the indicator for dispatch video commands on two BSD rings */
>   	unsigned int bsd_ring_dispatch_index;
>
> @@ -3465,6 +3475,7 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
>   #endif
>
>   /* intel_acpi.c */
> +bool intel_detect_acpi_rst(void);
>   #ifdef CONFIG_ACPI
>   extern void intel_register_dsm_handler(void);
>   extern void intel_unregister_dsm_handler(void);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 587beea..8e5fce4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -396,8 +396,16 @@ static struct drm_i915_gem_object *
>   i915_gem_alloc_object_stolen(struct drm_device *dev, size_t size)
>   {
>   	struct drm_i915_gem_object *obj;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>   	int ret;
>
> +	if (dev_priv->mm.volatile_stolen) {
> +		/* Stolen may be overwritten by external parties
> +		 * so unsuitable for persistent user data.
> +		 */
> +		return ERR_PTR(-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 335a1ef..88ee036 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -482,6 +482,18 @@ 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.volatile_stolen = false;
> +	if (IS_ENABLED(CONFIG_SUSPEND)) {
> +		/* BIOSes using RapidStart Technology have been reported
> +		 * to overwrite stolen across S3, not just S4.
> +		 */
> +		dev_priv->mm.volatile_stolen = intel_detect_acpi_rst();
> +	}
> +
>   	return 0;
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
> index eb638a1..05fd67f 100644
> --- a/drivers/gpu/drm/i915/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/intel_acpi.c
> @@ -23,6 +23,8 @@ static const u8 intel_dsm_guid[] = {
>   	0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c
>   };
>
> +static const char *irst_id = "INT3392";
> +
>   static char *intel_dsm_port_name(u8 id)
>   {
>   	switch (id) {
> @@ -162,3 +164,8 @@ void intel_register_dsm_handler(void)
>   void intel_unregister_dsm_handler(void)
>   {
>   }
> +
> +bool intel_detect_acpi_rst(void)
> +{
> +	return acpi_dev_present(irst_id);
> +}
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx