diff mbox series

[v5,5/6] drm/i915/pxp: Trigger the global teardown for before suspending

Message ID 20230113011850.1463965-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

Commit Message

Teres Alexis, Alan Previn Jan. 13, 2023, 1:18 a.m. UTC 
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>
---
 drivers/gpu/drm/i915/pxp/intel_pxp.c         | 60 +++++++++++++++++---
 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 |  9 ++-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.h |  5 ++
 5 files changed, 64 insertions(+), 13 deletions(-)

Comments

Rodrigo Vivi Jan. 19, 2023, 7:35 p.m. UTC | #1 
On Thu, Jan 12, 2023 at 05:18: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>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.c         | 60 +++++++++++++++++---
>  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 |  9 ++-
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.h |  5 ++
>  5 files changed, 64 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..f3d9e7989eb7 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -270,6 +270,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 use 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);
> +	}

I really don't see why you need 1 function for totally 2 different cases.
Why not 2 functions then?

> +
> +	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_locked(pxp, true))
> +		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 +335,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 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..d8278c4002e3 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -115,11 +115,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;

o.O

pxp->hw_state_invalidate = restart_arb;

?

or even a better name for the restart_arb to already indicate that is
the hw_state_invalidate ?

>  
>  	/*
>  	 * if we fail to submit the termination there is no point in waiting for
> @@ -167,7 +170,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..4f944b63b5b6 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 restart_arb);
>  #else
>  static inline void intel_pxp_session_management_init(struct intel_pxp *pxp)
>  {
>  }
> +
> +static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
> +{
> +}
>  #endif
>  #endif /* __INTEL_PXP_SESSION_H__ */
> -- 
> 2.39.0
>
Teres Alexis, Alan Previn Jan. 19, 2023, 10:31 p.m. UTC | #2 
Thanks for reviewing - responses below.

On Thu, 2023-01-19 at 14:35 -0500, Vivi, Rodrigo wrote:
> On Thu, Jan 12, 2023 at 05:18: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>
> > 
Alan: [snip]
> >  
> > +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 use 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);
> > +       }
> 
> I really don't see why you need 1 function for totally 2 different cases.
> Why not 2 functions then?
> 

Alan: I don't see why not ;) My goal with above method was was to concentrate the teardown steps in a single function so if future changes are required, we can keep it in this single function entry point. For now i will assume that was a nack so i shall split it on next rev.

> > +
> > +       if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
> > +               return -ETIMEDOUT;
> > +
> > +       return 0;
> > +}
> > +
> > 
Alan: [snip]

> > 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..d8278c4002e3 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > @@ -115,11 +115,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;
> 
> o.O
> 
> pxp->hw_state_invalidate = restart_arb;
Alan: duhhhh... (my bad)
> 
> ?
> 
> or even a better name for the restart_arb to already indicate that is
> the hw_state_invalidate ?
> 
Alan: hmmm... you something mean like:

    hw_state_invalidated = post_invalidation_needs_restart;


Alan: actually i wish we couold redo "hw_state_invalidate" which is currently defined
as a boolean that only means one thing -> teardown and restart. It would be more scalable
if we can replace it with a bitmask of "current + (infered)pending state" with a documented
state-machine with a fixed set of state-transition paths. 

INACTIVE----> STARTING----> ACTIVE ----> TEARDOWN_RESTART--->|
      ^           ^               |                          |
      |           |               |                          V
      |           |<--------------)----------<---------------|
      |                           |
      |                           |-----> TEARDOWN_END---->--|
      |                                                      V
      |<-----------------<----------------<------------------|


However, I didn't do this initially because it would mean a wider set of changes that might
take more time to test and review (downstream customers impacts) but for only 5 states but
where only 2 of em are impacted by this change. For now i shall go with the simpler name change
as you hint above - unless you request this instead.

Alan: [snip]
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index cfc9af8b3d21..f3d9e7989eb7 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -270,6 +270,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 use 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->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_locked(pxp, true))
+		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 +335,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 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..d8278c4002e3 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -115,11 +115,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
@@ -167,7 +170,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..4f944b63b5b6 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 restart_arb);
 #else
 static inline void intel_pxp_session_management_init(struct intel_pxp *pxp)
 {
 }
+
+static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
+{
+}
 #endif
 #endif /* __INTEL_PXP_SESSION_H__ */