| Message ID | 20230124053150.3543928-6-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pxp: Add missing cleanup steps for PXP global-teardown | expand |
On Mon, Jan 23, 2023 at 09:31:49PM -0800, Alan Previn wrote: > A driver bug was recently discovered where the security firmware was > receiving internal HW signals indicating that session key expirations > had occurred. Architecturally, the firmware was expecting a response > from the GuC to acknowledge the event with the firmware side. > However the OS was in a suspended state and GuC had been reset. > > Internal specifications actually required the driver to ensure > that all active sessions be properly cleaned up in such cases where > the system is suspended and the GuC potentially unable to respond. > > This patch adds the global teardown code in i915's suspend_prepare > code path. > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > Reviewed-by: Juston Li <justonli@chromium.org> it should probably contain some revision history above so the reviewer can know what changed from the time he reviewed and now. it looks we have another function that I don't remember... Juston, could you please confirm your review stands in this version? if so feel free to add my ack with that. > --- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 65 +++++++++++++++++--- > drivers/gpu/drm/i915/pxp/intel_pxp.h | 1 + > drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 2 +- > drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 6 +- > drivers/gpu/drm/i915/pxp/intel_pxp_session.h | 5 ++ > 5 files changed, 66 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c > index cfc9af8b3d21..9d4c7724e98e 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -270,6 +270,60 @@ static bool pxp_component_bound(struct intel_pxp *pxp) > return bound; > } > > +static int __pxp_global_teardown_final(struct intel_pxp *pxp) > +{ > + if (!pxp->arb_is_valid) > + return 0; > + /* > + * To ensure synchronous and coherent session teardown completion > + * in response to suspend or shutdown triggers, don't use a worker. > + */ > + intel_pxp_mark_termination_in_progress(pxp); > + intel_pxp_terminate(pxp, false); > + > + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250))) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +static int __pxp_global_teardown_restart(struct intel_pxp *pxp) > +{ > + if (pxp->arb_is_valid) > + return 0; > + /* > + * The arb-session is currently inactive and we are doing a reset and restart > + * due to a runtime event. Use the worker that was designed for this. > + */ > + pxp_queue_termination(pxp); > + > + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250))) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +void intel_pxp_end(struct intel_pxp *pxp) > +{ > + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; > + intel_wakeref_t wakeref; > + > + if (!intel_pxp_is_enabled(pxp)) > + return; > + > + wakeref = intel_runtime_pm_get(&i915->runtime_pm); > + > + mutex_lock(&pxp->arb_mutex); > + > + if (__pxp_global_teardown_final(pxp)) > + drm_dbg(&i915->drm, "PXP end timed out\n"); > + > + mutex_unlock(&pxp->arb_mutex); > + > + intel_pxp_fini_hw(pxp); > + intel_runtime_pm_put(&i915->runtime_pm, wakeref); > +} > + > /* > * the arb session is restarted from the irq work when we receive the > * termination completion interrupt > @@ -286,16 +340,9 @@ int intel_pxp_start(struct intel_pxp *pxp) > > mutex_lock(&pxp->arb_mutex); > > - if (pxp->arb_is_valid) > - goto unlock; > - > - pxp_queue_termination(pxp); > - > - if (!wait_for_completion_timeout(&pxp->termination, > - msecs_to_jiffies(250))) { > - ret = -ETIMEDOUT; > + ret = __pxp_global_teardown_restart(pxp); > + if (ret) > goto unlock; > - } > > /* make sure the compiler doesn't optimize the double access */ > barrier(); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h > index 9658d3005222..3ded0890cd27 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -27,6 +27,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp); > void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id); > > int intel_pxp_start(struct intel_pxp *pxp); > +void intel_pxp_end(struct intel_pxp *pxp); > > int intel_pxp_key_check(struct intel_pxp *pxp, > struct drm_i915_gem_object *obj, > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > index 892d39cc61c1..e427464aa131 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > @@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp) > if (!intel_pxp_is_enabled(pxp)) > return; > > - pxp->arb_is_valid = false; > + intel_pxp_end(pxp); > > intel_pxp_invalidate(pxp); > } > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > index 74ed7e16e481..448cacb0465d 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > @@ -115,11 +115,11 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp) > return ret; > } > > -static void pxp_terminate(struct intel_pxp *pxp) > +void intel_pxp_terminate(struct intel_pxp *pxp, bool post_invalidation_needs_restart) > { > int ret; > > - pxp->hw_state_invalidated = true; > + pxp->hw_state_invalidated = post_invalidation_needs_restart; > > /* > * if we fail to submit the termination there is no point in waiting for > @@ -167,7 +167,7 @@ static void pxp_session_work(struct work_struct *work) > > if (events & PXP_TERMINATION_REQUEST) { > events &= ~PXP_TERMINATION_COMPLETE; > - pxp_terminate(pxp); > + intel_pxp_terminate(pxp, true); > } > > if (events & PXP_TERMINATION_COMPLETE) > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > index 903ac52cffa1..ba5788127109 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > @@ -12,9 +12,14 @@ struct intel_pxp; > > #ifdef CONFIG_DRM_I915_PXP > void intel_pxp_session_management_init(struct intel_pxp *pxp); > +void intel_pxp_terminate(struct intel_pxp *pxp, bool post_invalidation_needs_restart); > #else > static inline void intel_pxp_session_management_init(struct intel_pxp *pxp) > { > } > + > +static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool post_invalidation_needs_restart) > +{ > +} > #endif > #endif /* __INTEL_PXP_SESSION_H__ */ > -- > 2.39.0 >
On Tue, 2023-01-24 at 10:17 -0500, Rodrigo Vivi wrote: > On Mon, Jan 23, 2023 at 09:31:49PM -0800, Alan Previn wrote: > > A driver bug was recently discovered where the security firmware > > was > > receiving internal HW signals indicating that session key > > expirations > > had occurred. Architecturally, the firmware was expecting a > > response > > from the GuC to acknowledge the event with the firmware side. > > However the OS was in a suspended state and GuC had been reset. > > > > Internal specifications actually required the driver to ensure > > that all active sessions be properly cleaned up in such cases where > > the system is suspended and the GuC potentially unable to respond. > > > > This patch adds the global teardown code in i915's suspend_prepare > > code path. > > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > > Reviewed-by: Juston Li <justonli@chromium.org> > > it should probably contain some revision history above so the > reviewer can know what changed from the time he reviewed and now. > > it looks we have another function that I don't remember... > Juston, could you please confirm your review stands in this version? > if so feel free to add my ack with that. Sure, v6 is: Reviewed-by: Juston Li <justonli@chromium.org> Appreciate the reviews/revisions Rodrigo and Alan! > > > --- > > drivers/gpu/drm/i915/pxp/intel_pxp.c | 65 > > +++++++++++++++++--- > > drivers/gpu/drm/i915/pxp/intel_pxp.h | 1 + > > drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 2 +- > > drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 6 +- > > drivers/gpu/drm/i915/pxp/intel_pxp_session.h | 5 ++ > > 5 files changed, 66 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c > > b/drivers/gpu/drm/i915/pxp/intel_pxp.c > > index cfc9af8b3d21..9d4c7724e98e 100644 > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > > @@ -270,6 +270,60 @@ static bool pxp_component_bound(struct > > intel_pxp *pxp) > > return bound; > > } > > > > +static int __pxp_global_teardown_final(struct intel_pxp *pxp) > > +{ > > + if (!pxp->arb_is_valid) > > + return 0; > > + /* > > + * To ensure synchronous and coherent session teardown > > completion > > + * in response to suspend or shutdown triggers, don't use a > > worker. > > + */ > > + intel_pxp_mark_termination_in_progress(pxp); > > + intel_pxp_terminate(pxp, false); > > + > > + if (!wait_for_completion_timeout(&pxp->termination, > > msecs_to_jiffies(250))) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > + > > +static int __pxp_global_teardown_restart(struct intel_pxp *pxp) > > +{ > > + if (pxp->arb_is_valid) > > + return 0; > > + /* > > + * The arb-session is currently inactive and we are doing a > > reset and restart > > + * due to a runtime event. Use the worker that was designed > > for this. > > + */ > > + pxp_queue_termination(pxp); > > + > > + if (!wait_for_completion_timeout(&pxp->termination, > > msecs_to_jiffies(250))) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > + > > +void intel_pxp_end(struct intel_pxp *pxp) > > +{ > > + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; > > + intel_wakeref_t wakeref; > > + > > + if (!intel_pxp_is_enabled(pxp)) > > + return; > > + > > + wakeref = intel_runtime_pm_get(&i915->runtime_pm); > > + > > + mutex_lock(&pxp->arb_mutex); > > + > > + if (__pxp_global_teardown_final(pxp)) > > + drm_dbg(&i915->drm, "PXP end timed out\n"); > > + > > + mutex_unlock(&pxp->arb_mutex); > > + > > + intel_pxp_fini_hw(pxp); > > + intel_runtime_pm_put(&i915->runtime_pm, wakeref); > > +} > > + > > /* > > * the arb session is restarted from the irq work when we receive > > the > > * termination completion interrupt > > @@ -286,16 +340,9 @@ int intel_pxp_start(struct intel_pxp *pxp) > > > > mutex_lock(&pxp->arb_mutex); > > > > - if (pxp->arb_is_valid) > > - goto unlock; > > - > > - pxp_queue_termination(pxp); > > - > > - if (!wait_for_completion_timeout(&pxp->termination, > > - msecs_to_jiffies(250))) { > > - ret = -ETIMEDOUT; > > + ret = __pxp_global_teardown_restart(pxp); > > + if (ret) > > goto unlock; > > - } > > > > /* make sure the compiler doesn't optimize the double > > access */ > > barrier(); > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h > > b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > index 9658d3005222..3ded0890cd27 100644 > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > @@ -27,6 +27,7 @@ void > > intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp); > > void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 > > arb_session_id); > > > > int intel_pxp_start(struct intel_pxp *pxp); > > +void intel_pxp_end(struct intel_pxp *pxp); > > > > int intel_pxp_key_check(struct intel_pxp *pxp, > > struct drm_i915_gem_object *obj, > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > > b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > > index 892d39cc61c1..e427464aa131 100644 > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > > @@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp > > *pxp) > > if (!intel_pxp_is_enabled(pxp)) > > return; > > > > - pxp->arb_is_valid = false; > > + intel_pxp_end(pxp); > > > > intel_pxp_invalidate(pxp); > > } > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > > b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > > index 74ed7e16e481..448cacb0465d 100644 > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > > @@ -115,11 +115,11 @@ static int > > pxp_terminate_arb_session_and_global(struct intel_pxp *pxp) > > return ret; > > } > > > > -static void pxp_terminate(struct intel_pxp *pxp) > > +void intel_pxp_terminate(struct intel_pxp *pxp, bool > > post_invalidation_needs_restart) > > { > > int ret; > > > > - pxp->hw_state_invalidated = true; > > + pxp->hw_state_invalidated = > > post_invalidation_needs_restart; > > > > /* > > * if we fail to submit the termination there is no point > > in waiting for > > @@ -167,7 +167,7 @@ static void pxp_session_work(struct work_struct > > *work) > > > > if (events & PXP_TERMINATION_REQUEST) { > > events &= ~PXP_TERMINATION_COMPLETE; > > - pxp_terminate(pxp); > > + intel_pxp_terminate(pxp, true); > > } > > > > if (events & PXP_TERMINATION_COMPLETE) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > > b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > > index 903ac52cffa1..ba5788127109 100644 > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > > @@ -12,9 +12,14 @@ struct intel_pxp; > > > > #ifdef CONFIG_DRM_I915_PXP > > void intel_pxp_session_management_init(struct intel_pxp *pxp); > > +void intel_pxp_terminate(struct intel_pxp *pxp, bool > > post_invalidation_needs_restart); > > #else > > static inline void intel_pxp_session_management_init(struct > > intel_pxp *pxp) > > { > > } > > + > > +static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool > > post_invalidation_needs_restart) > > +{ > > +} > > #endif > > #endif /* __INTEL_PXP_SESSION_H__ */ > > -- > > 2.39.0 > >
On Tue, Jan 24, 2023 at 10:08:52AM -0800, Juston Li wrote: > On Tue, 2023-01-24 at 10:17 -0500, Rodrigo Vivi wrote: > > On Mon, Jan 23, 2023 at 09:31:49PM -0800, Alan Previn wrote: > > > A driver bug was recently discovered where the security firmware > > > was > > > receiving internal HW signals indicating that session key > > > expirations > > > had occurred. Architecturally, the firmware was expecting a > > > response > > > from the GuC to acknowledge the event with the firmware side. > > > However the OS was in a suspended state and GuC had been reset. > > > > > > Internal specifications actually required the driver to ensure > > > that all active sessions be properly cleaned up in such cases where > > > the system is suspended and the GuC potentially unable to respond. > > > > > > This patch adds the global teardown code in i915's suspend_prepare > > > code path. > > > > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > > > Reviewed-by: Juston Li <justonli@chromium.org> > > > > it should probably contain some revision history above so the > > reviewer can know what changed from the time he reviewed and now. > > > > it looks we have another function that I don't remember... > > Juston, could you please confirm your review stands in this version? > > if so feel free to add my ack with that. > > Sure, v6 is: > Reviewed-by: Juston Li <justonli@chromium.org> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Appreciate the reviews/revisions Rodrigo and Alan! > > > > > > --- > > > drivers/gpu/drm/i915/pxp/intel_pxp.c | 65 > > > +++++++++++++++++--- > > > drivers/gpu/drm/i915/pxp/intel_pxp.h | 1 + > > > drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 2 +- > > > drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 6 +- > > > drivers/gpu/drm/i915/pxp/intel_pxp_session.h | 5 ++ > > > 5 files changed, 66 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c > > > b/drivers/gpu/drm/i915/pxp/intel_pxp.c > > > index cfc9af8b3d21..9d4c7724e98e 100644 > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > > > @@ -270,6 +270,60 @@ static bool pxp_component_bound(struct > > > intel_pxp *pxp) > > > return bound; > > > } > > > > > > +static int __pxp_global_teardown_final(struct intel_pxp *pxp) > > > +{ > > > + if (!pxp->arb_is_valid) > > > + return 0; > > > + /* > > > + * To ensure synchronous and coherent session teardown > > > completion > > > + * in response to suspend or shutdown triggers, don't use a > > > worker. > > > + */ > > > + intel_pxp_mark_termination_in_progress(pxp); > > > + intel_pxp_terminate(pxp, false); > > > + > > > + if (!wait_for_completion_timeout(&pxp->termination, > > > msecs_to_jiffies(250))) > > > + return -ETIMEDOUT; > > > + > > > + return 0; > > > +} > > > + > > > +static int __pxp_global_teardown_restart(struct intel_pxp *pxp) > > > +{ > > > + if (pxp->arb_is_valid) > > > + return 0; > > > + /* > > > + * The arb-session is currently inactive and we are doing a > > > reset and restart > > > + * due to a runtime event. Use the worker that was designed > > > for this. > > > + */ > > > + pxp_queue_termination(pxp); > > > + > > > + if (!wait_for_completion_timeout(&pxp->termination, > > > msecs_to_jiffies(250))) > > > + return -ETIMEDOUT; > > > + > > > + return 0; > > > +} > > > + > > > +void intel_pxp_end(struct intel_pxp *pxp) > > > +{ > > > + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; > > > + intel_wakeref_t wakeref; > > > + > > > + if (!intel_pxp_is_enabled(pxp)) > > > + return; > > > + > > > + wakeref = intel_runtime_pm_get(&i915->runtime_pm); > > > + > > > + mutex_lock(&pxp->arb_mutex); > > > + > > > + if (__pxp_global_teardown_final(pxp)) > > > + drm_dbg(&i915->drm, "PXP end timed out\n"); > > > + > > > + mutex_unlock(&pxp->arb_mutex); > > > + > > > + intel_pxp_fini_hw(pxp); > > > + intel_runtime_pm_put(&i915->runtime_pm, wakeref); > > > +} > > > + > > > /* > > > * the arb session is restarted from the irq work when we receive > > > the > > > * termination completion interrupt > > > @@ -286,16 +340,9 @@ int intel_pxp_start(struct intel_pxp *pxp) > > > > > > mutex_lock(&pxp->arb_mutex); > > > > > > - if (pxp->arb_is_valid) > > > - goto unlock; > > > - > > > - pxp_queue_termination(pxp); > > > - > > > - if (!wait_for_completion_timeout(&pxp->termination, > > > - msecs_to_jiffies(250))) { > > > - ret = -ETIMEDOUT; > > > + ret = __pxp_global_teardown_restart(pxp); > > > + if (ret) > > > goto unlock; > > > - } > > > > > > /* make sure the compiler doesn't optimize the double > > > access */ > > > barrier(); > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > index 9658d3005222..3ded0890cd27 100644 > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > @@ -27,6 +27,7 @@ void > > > intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp); > > > void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 > > > arb_session_id); > > > > > > int intel_pxp_start(struct intel_pxp *pxp); > > > +void intel_pxp_end(struct intel_pxp *pxp); > > > > > > int intel_pxp_key_check(struct intel_pxp *pxp, > > > struct drm_i915_gem_object *obj, > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > > > b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > > > index 892d39cc61c1..e427464aa131 100644 > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > > > @@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp > > > *pxp) > > > if (!intel_pxp_is_enabled(pxp)) > > > return; > > > > > > - pxp->arb_is_valid = false; > > > + intel_pxp_end(pxp); > > > > > > intel_pxp_invalidate(pxp); > > > } > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > > > b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > > > index 74ed7e16e481..448cacb0465d 100644 > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > > > @@ -115,11 +115,11 @@ static int > > > pxp_terminate_arb_session_and_global(struct intel_pxp *pxp) > > > return ret; > > > } > > > > > > -static void pxp_terminate(struct intel_pxp *pxp) > > > +void intel_pxp_terminate(struct intel_pxp *pxp, bool > > > post_invalidation_needs_restart) > > > { > > > int ret; > > > > > > - pxp->hw_state_invalidated = true; > > > + pxp->hw_state_invalidated = > > > post_invalidation_needs_restart; > > > > > > /* > > > * if we fail to submit the termination there is no point > > > in waiting for > > > @@ -167,7 +167,7 @@ static void pxp_session_work(struct work_struct > > > *work) > > > > > > if (events & PXP_TERMINATION_REQUEST) { > > > events &= ~PXP_TERMINATION_COMPLETE; > > > - pxp_terminate(pxp); > > > + intel_pxp_terminate(pxp, true); > > > } > > > > > > if (events & PXP_TERMINATION_COMPLETE) > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > > > b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > > > index 903ac52cffa1..ba5788127109 100644 > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h > > > @@ -12,9 +12,14 @@ struct intel_pxp; > > > > > > #ifdef CONFIG_DRM_I915_PXP > > > void intel_pxp_session_management_init(struct intel_pxp *pxp); > > > +void intel_pxp_terminate(struct intel_pxp *pxp, bool > > > post_invalidation_needs_restart); > > > #else > > > static inline void intel_pxp_session_management_init(struct > > > intel_pxp *pxp) > > > { > > > } > > > + > > > +static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool > > > post_invalidation_needs_restart) > > > +{ > > > +} > > > #endif > > > #endif /* __INTEL_PXP_SESSION_H__ */ > > > -- > > > 2.39.0 > > > >
On Tue, 2023-01-24 at 13:43 -0500, Vivi, Rodrigo wrote: > On Tue, Jan 24, 2023 at 10:08:52AM -0800, Juston Li wrote: > > On Tue, 2023-01-24 at 10:17 -0500, Rodrigo Vivi wrote: > > > On Mon, Jan 23, 2023 at 09:31:49PM -0800, Alan Previn wrote: > > > > A driver bug was recently discovered where the security firmware > > > > was > > > > receiving internal HW signals indicating that session key > > > > expirations > > > > had occurred. Architecturally, the firmware was expecting a > > > > response > > > > from the GuC to acknowledge the event with the firmware side. > > > > However the OS was in a suspended state and GuC had been reset. > > > > > > > > Internal specifications actually required the driver to ensure > > > > that all active sessions be properly cleaned up in such cases where > > > > the system is suspended and the GuC potentially unable to respond. > > > > > > > > This patch adds the global teardown code in i915's suspend_prepare > > > > code path. > > > > > > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > > > > Reviewed-by: Juston Li <justonli@chromium.org> > > > > > > it should probably contain some revision history above so the > > > reviewer can know what changed from the time he reviewed and now. > > > > > > it looks we have another function that I don't remember... > > > Juston, could you please confirm your review stands in this version? > > > if so feel free to add my ack with that. > > > > Sure, v6 is: > > Reviewed-by: Juston Li <justonli@chromium.org> > > > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Thanks guys - will respin the final one (i hope) with tweaks on patch #2. ...alan
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c index cfc9af8b3d21..9d4c7724e98e 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c @@ -270,6 +270,60 @@ static bool pxp_component_bound(struct intel_pxp *pxp) return bound; } +static int __pxp_global_teardown_final(struct intel_pxp *pxp) +{ + if (!pxp->arb_is_valid) + return 0; + /* + * To ensure synchronous and coherent session teardown completion + * in response to suspend or shutdown triggers, don't use a worker. + */ + intel_pxp_mark_termination_in_progress(pxp); + intel_pxp_terminate(pxp, false); + + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250))) + return -ETIMEDOUT; + + return 0; +} + +static int __pxp_global_teardown_restart(struct intel_pxp *pxp) +{ + if (pxp->arb_is_valid) + return 0; + /* + * The arb-session is currently inactive and we are doing a reset and restart + * due to a runtime event. Use the worker that was designed for this. + */ + pxp_queue_termination(pxp); + + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250))) + return -ETIMEDOUT; + + return 0; +} + +void intel_pxp_end(struct intel_pxp *pxp) +{ + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; + intel_wakeref_t wakeref; + + if (!intel_pxp_is_enabled(pxp)) + return; + + wakeref = intel_runtime_pm_get(&i915->runtime_pm); + + mutex_lock(&pxp->arb_mutex); + + if (__pxp_global_teardown_final(pxp)) + drm_dbg(&i915->drm, "PXP end timed out\n"); + + mutex_unlock(&pxp->arb_mutex); + + intel_pxp_fini_hw(pxp); + intel_runtime_pm_put(&i915->runtime_pm, wakeref); +} + /* * the arb session is restarted from the irq work when we receive the * termination completion interrupt @@ -286,16 +340,9 @@ int intel_pxp_start(struct intel_pxp *pxp) mutex_lock(&pxp->arb_mutex); - if (pxp->arb_is_valid) - goto unlock; - - pxp_queue_termination(pxp); - - if (!wait_for_completion_timeout(&pxp->termination, - msecs_to_jiffies(250))) { - ret = -ETIMEDOUT; + ret = __pxp_global_teardown_restart(pxp); + if (ret) goto unlock; - } /* make sure the compiler doesn't optimize the double access */ barrier(); diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h index 9658d3005222..3ded0890cd27 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h @@ -27,6 +27,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp); void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id); int intel_pxp_start(struct intel_pxp *pxp); +void intel_pxp_end(struct intel_pxp *pxp); int intel_pxp_key_check(struct intel_pxp *pxp, struct drm_i915_gem_object *obj, diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c index 892d39cc61c1..e427464aa131 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c @@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp) if (!intel_pxp_is_enabled(pxp)) return; - pxp->arb_is_valid = false; + intel_pxp_end(pxp); intel_pxp_invalidate(pxp); } diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c index 74ed7e16e481..448cacb0465d 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c @@ -115,11 +115,11 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp) return ret; } -static void pxp_terminate(struct intel_pxp *pxp) +void intel_pxp_terminate(struct intel_pxp *pxp, bool post_invalidation_needs_restart) { int ret; - pxp->hw_state_invalidated = true; + pxp->hw_state_invalidated = post_invalidation_needs_restart; /* * if we fail to submit the termination there is no point in waiting for @@ -167,7 +167,7 @@ static void pxp_session_work(struct work_struct *work) if (events & PXP_TERMINATION_REQUEST) { events &= ~PXP_TERMINATION_COMPLETE; - pxp_terminate(pxp); + intel_pxp_terminate(pxp, true); } if (events & PXP_TERMINATION_COMPLETE) diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h index 903ac52cffa1..ba5788127109 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h @@ -12,9 +12,14 @@ struct intel_pxp; #ifdef CONFIG_DRM_I915_PXP void intel_pxp_session_management_init(struct intel_pxp *pxp); +void intel_pxp_terminate(struct intel_pxp *pxp, bool post_invalidation_needs_restart); #else static inline void intel_pxp_session_management_init(struct intel_pxp *pxp) { } + +static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool post_invalidation_needs_restart) +{ +} #endif #endif /* __INTEL_PXP_SESSION_H__ */