| Message ID | 20221118003631.1523764-3-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 Thu, 2022-11-17 at 16:36 -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> > --- > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > @@ -14,7 +14,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); > } We discovered that depending on the runtime-pm behavior of the platform, this patch uncovers a hang on the mei component driver (when calling the mei-pxp's interface for sending messages to the firmware from within the suspend-prepare callstack). As soon as that mei patch is fit for publication we shall either merge the patches into a single series or provide a link from here. ...alan
On Thu, 2022-11-17 at 16:36 -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> > --- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 60 +++++++++++++++++- > -- > drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 + > drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 2 +- > drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 9 ++- > 4 files changed, 60 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c > b/drivers/gpu/drm/i915/pxp/intel_pxp.c > index 5efe61f67546..659410ae1b89 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -198,6 +198,55 @@ static bool pxp_component_bound(struct intel_pxp > *pxp) > return bound; > } > > +static int __pxp_global_teardown_locked(struct intel_pxp *pxp, bool > terminate_for_cleanup) > +{ > + if (terminate_for_cleanup) { > + if (!pxp->arb_is_valid) > + return 0; > + /* > + * To ensure synchronous and coherent session > teardown completion > + * in response to suspend or shutdown triggers, don't > user a worker. > + */ > + intel_pxp_mark_termination_in_progress(pxp); > + intel_pxp_terminate(pxp, false); > + } else { > + if (pxp->arb_is_valid) > + return 0; > + /* > + * If we are not in final termination, and the arb- > session is currently > + * inactive, we are doing a reset and restart due to > some 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_to_gt(pxp)->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_locked(pxp, true)) > + drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end timed > out\n"); > + > + mutex_unlock(&pxp->arb_mutex); > + > + intel_pxp_fini_hw(pxp); Is intel_pxp_suspend() still needed then if we already fini_hw() here and mark invalidation in intel_pxp_terminate()? > + intel_runtime_pm_put(&i915->runtime_pm, wakeref); > +} > + > /* > * the arb session is restarted from the irq work when we receive > the > * termination completion interrupt > @@ -214,16 +263,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_locked(pxp, false); > + 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 6ba8fa5bfea0..d001828b3372 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -26,6 +26,8 @@ void intel_pxp_mark_termination_in_progress(struct > intel_pxp *pxp); > void intel_pxp_tee_end_all_fw_sessions(struct intel_pxp *pxp, u32 > sessions_mask); > > int intel_pxp_start(struct intel_pxp *pxp); > +void intel_pxp_end(struct intel_pxp *pxp); > +void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb); > > 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 6a7d4e2ee138..36af52c28e63 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > @@ -14,7 +14,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 85e404b5ad0e..fdf30554d80f 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > @@ -118,11 +118,14 @@ 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 restart_arb) > { > int ret; > > - pxp->hw_state_invalidated = true; > + if (restart_arb) > + pxp->hw_state_invalidated = true; > + else > + pxp->hw_state_invalidated = false; > > /* > * if we fail to submit the termination there is no point in > waiting for > @@ -170,7 +173,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)
On Mon, 2022-11-21 at 10:39 -0800, Juston Li wrote: > On Thu, 2022-11-17 at 16:36 -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. > > Alan:[snip] > > +void intel_pxp_end(struct intel_pxp *pxp) > > +{ > > + struct drm_i915_private *i915 = pxp_to_gt(pxp)->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_locked(pxp, true)) > > + drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end timed > > out\n"); > > + > > + mutex_unlock(&pxp->arb_mutex); > > + > > + intel_pxp_fini_hw(pxp); > > Is intel_pxp_suspend() still needed then if we already fini_hw() here > and mark invalidation in intel_pxp_terminate()? > Good catch - looks like we might not need intel_pxp_suspend. But I'll verify that for you. Also, looks like i forgot to include a non-CONFIG_DRM_I915_PXP version of intel_pxp_end which was causing the build failure. Will resend. Btw, thanks for reviewing this Juston, i had cc'd you because of the impact to suspend-resume flows and I believe you have had prior experience debugging issues with that and runtime-suspend/resume. Do you any other issues with this change? ...alan
On Mon, 2022-11-21 at 19:21 +0000, Teres Alexis, Alan Previn wrote: > > > On Mon, 2022-11-21 at 10:39 -0800, Juston Li wrote: > > On Thu, 2022-11-17 at 16:36 -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. > > > > Alan:[snip] > > > > +void intel_pxp_end(struct intel_pxp *pxp) > > > +{ > > > + struct drm_i915_private *i915 = pxp_to_gt(pxp)->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_locked(pxp, true)) > > > + drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end > > > timed > > > out\n"); > > > + > > > + mutex_unlock(&pxp->arb_mutex); > > > + > > > + intel_pxp_fini_hw(pxp); > > > > Is intel_pxp_suspend() still needed then if we already fini_hw() > > here > > and mark invalidation in intel_pxp_terminate()? > > > > Good catch - looks like we might not need intel_pxp_suspend. But I'll > verify that for you. Actually, might need to be careful here. If system aborts suspend or fails to suspend for any reason, suspend_prepare()->intel_pxp_fini_hw() might have been called but not suspend(). Correct me if I'm wrong, but in that case I don't think resume() will be called and thus intel_pxp_init_hw(). For some background, there were some issues with PXP ending up in a bad state when some other driver caused suspend to fail or user closed/opened lid quickly and aborted suspend. > Also, looks like i forgot to include a non-CONFIG_DRM_I915_PXP > version of intel_pxp_end which was causing the build > failure. Will resend. > > Btw, thanks for reviewing this Juston, i had cc'd you because of the > impact to suspend-resume flows and I believe you > have had prior experience debugging issues with that and runtime- > suspend/resume. Do you any other issues with this > change? Np, thanks for the patches! The only other concern I had that's more of a downstream issue is we ended up using hw_state_invalidated to block PXP ioctl ops during teardown to prevent further PXP ioctls triggering pxp_start and another termination queued. I don't recall if I sent you this patch on our tree: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3207105 I think this could happen in suspend now too, if app sends PXP ops while suspend termination is in progress. Juston
On Mon, 2022-11-21 at 14:14 -0800, Juston Li wrote: > > Alan:[snip] > > > > > > +void intel_pxp_end(struct intel_pxp *pxp) > > > > +{ > > > > + struct drm_i915_private *i915 = pxp_to_gt(pxp)->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_locked(pxp, true)) > > > > + drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end > > > > timed > > > > out\n"); > > > > + > > > > + mutex_unlock(&pxp->arb_mutex); > > > > + > > > > + intel_pxp_fini_hw(pxp); > > > > > > Is intel_pxp_suspend() still needed then if we already fini_hw() > > > here > > > and mark invalidation in intel_pxp_terminate()? > > > > > > > Good catch - looks like we might not need intel_pxp_suspend. But I'll > > verify that for you. > > Actually, might need to be careful here. If system aborts suspend or > fails to suspend for any reason, suspend_prepare()->intel_pxp_fini_hw() > might have been called but not suspend(). > > Correct me if I'm wrong, but in that case I don't think resume() will > be called and thus intel_pxp_init_hw(). > > For some background, there were some issues with PXP ending up in a bad > state when some other driver caused suspend to fail or user > closed/opened lid quickly and aborted suspend. > > Yeah, i only now notice that we although we define an i915 callback for 'struct dev_pm_ops.prepare' we don't provide one for 'struct dev_pm_ops.complete' which are the two opposing sides of suspend-vs-resume phases - meaning we really should be doing the intel_pxp_init_hw() in the "complete" callback. I will go ahead and propose this change (considering this is the last part of resume, I'm hoping it will not cause any regression) and should address this concern and fix a "lack of symmetry" > > Also, looks like i forgot to include a non-CONFIG_DRM_I915_PXP > > version of intel_pxp_end which was causing the build > > failure. Will resend. > > > > Btw, thanks for reviewing this Juston, i had cc'd you because of the > > impact to suspend-resume flows and I believe you > > have had prior experience debugging issues with that and runtime- > > suspend/resume. Do you any other issues with this > > change? > > Np, thanks for the patches! > > The only other concern I had that's more of a downstream issue is we > ended up using hw_state_invalidated to block PXP ioctl ops during > teardown to prevent further PXP ioctls triggering pxp_start and another > termination queued. > I don't recall if I sent you this patch on our tree: > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3207105 > I think this could happen in suspend now too, if app sends PXP ops > while suspend termination is in progress. > > Juston Yes we did receive this patch.
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c index 5efe61f67546..659410ae1b89 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c @@ -198,6 +198,55 @@ static bool pxp_component_bound(struct intel_pxp *pxp) return bound; } +static int __pxp_global_teardown_locked(struct intel_pxp *pxp, bool terminate_for_cleanup) +{ + if (terminate_for_cleanup) { + if (!pxp->arb_is_valid) + return 0; + /* + * To ensure synchronous and coherent session teardown completion + * in response to suspend or shutdown triggers, don't user a worker. + */ + intel_pxp_mark_termination_in_progress(pxp); + intel_pxp_terminate(pxp, false); + } else { + if (pxp->arb_is_valid) + return 0; + /* + * If we are not in final termination, and the arb-session is currently + * inactive, we are doing a reset and restart due to some 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_to_gt(pxp)->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_locked(pxp, true)) + drm_dbg(&(pxp_to_gt(pxp))->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 @@ -214,16 +263,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_locked(pxp, false); + 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 6ba8fa5bfea0..d001828b3372 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h @@ -26,6 +26,8 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp); void intel_pxp_tee_end_all_fw_sessions(struct intel_pxp *pxp, u32 sessions_mask); int intel_pxp_start(struct intel_pxp *pxp); +void intel_pxp_end(struct intel_pxp *pxp); +void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb); 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 6a7d4e2ee138..36af52c28e63 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c @@ -14,7 +14,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 85e404b5ad0e..fdf30554d80f 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c @@ -118,11 +118,14 @@ 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 restart_arb) { int ret; - pxp->hw_state_invalidated = true; + if (restart_arb) + pxp->hw_state_invalidated = true; + else + pxp->hw_state_invalidated = false; /* * if we fail to submit the termination there is no point in waiting for @@ -170,7 +173,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)
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> --- drivers/gpu/drm/i915/pxp/intel_pxp.c | 60 +++++++++++++++++--- drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 + drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 2 +- drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 9 ++- 4 files changed, 60 insertions(+), 13 deletions(-)