| Message ID | 1450765253-32104-11-git-send-email-ankitprasad.r.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 22/12/15 06:20, 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> > --- > 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); intel_ prefix? It also looks acpi_match_device already handles !CONFIG_ACPI in include/linux/acpi.h so maybe you don't need this declarations at all. Just define it locally and unconditionally where intel_detect_acpi_rst is. Thing I am not sure about is can the ACPI be compiled in but disabled. acpi=off on the kernel command line suggests it can. What do we want to do in that case? > #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; > +} > Regards, Tvrtko
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> > --- > 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 */ This defeats the purpose of using a generic term. /** 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 hae 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 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); As Tvrtko mentioned this is private to the driver. -Chris
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
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; +}