Re: [Intel-gfx] [PATCH v6 5/6] drm/i915/pxp: Trigger the global teardown for before suspending

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
> > > 
>