diff mbox series

[v2,1/3] drm/i915/pxp: Invalidate all PXP fw sessions during teardown

Message ID 20221129004832.3735764-2-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

Alan Previn Nov. 29, 2022, 12:48 a.m. UTC 
A gap was recently discovered where if an application did not
invalidate all of the stream keys (intentionally or not), and the
driver did a full PXP global teardown on the GT subsystem, we
find that future session creation would fail on the security
firmware's side of the equation. i915 is the entity that needs
ensure the sessions' state across both iGT and security firmware
are at a known clean point when performing a full global teardown.

Architecturally speaking, i915 should inspect all active sessions
and submit the invalidate-stream-key PXP command to the security
firmware for each of them. However, for the upstream i915 driver
we only support the arbitration session that can be created
so that will be the only session we will cleanup.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/pxp/intel_pxp.h          |  1 +
 .../drm/i915/pxp/intel_pxp_cmd_interface_42.h | 15 ++++++++
 .../i915/pxp/intel_pxp_cmd_interface_cmn.h    |  3 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  5 +++
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 35 +++++++++++++++++++
 drivers/gpu/drm/i915/pxp/intel_pxp_types.h    |  2 ++
 6 files changed, 61 insertions(+)

Comments

Juston Li Dec. 1, 2022, 1:47 a.m. UTC | #1 
On Mon, 2022-11-28 at 16:48 -0800, Alan Previn wrote:
> A gap was recently discovered where if an application did not
> invalidate all of the stream keys (intentionally or not), and the
> driver did a full PXP global teardown on the GT subsystem, we
> find that future session creation would fail on the security
> firmware's side of the equation. i915 is the entity that needs
> ensure the sessions' state across both iGT and security firmware
> are at a known clean point when performing a full global teardown.
> 
> Architecturally speaking, i915 should inspect all active sessions
> and submit the invalidate-stream-key PXP command to the security
> firmware for each of them. However, for the upstream i915 driver
> we only support the arbitration session that can be created
> so that will be the only session we will cleanup.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.h          |  1 +
>  .../drm/i915/pxp/intel_pxp_cmd_interface_42.h | 15 ++++++++
>  .../i915/pxp/intel_pxp_cmd_interface_cmn.h    |  3 ++
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  5 +++
>  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 35
> +++++++++++++++++++
>  drivers/gpu/drm/i915/pxp/intel_pxp_types.h    |  2 ++
>  6 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 2da309088c6d..bbeb8ed8e211 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -23,6 +23,7 @@ void intel_pxp_init_hw(struct intel_pxp *pxp);
>  void intel_pxp_fini_hw(struct intel_pxp *pxp);
>  
>  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);
>  
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> index 739f9072fa5f..26f7d9f01bf3 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> @@ -12,6 +12,9 @@
>  /* PXP-Opcode for Init Session */
>  #define PXP42_CMDID_INIT_SESSION 0x1e
>  
> +/* PXP-Opcode for Invalidate Stream Key */
> +#define PXP42_CMDID_INVALIDATE_STREAM_KEY 0x00000007
> +
>  /* PXP-Input-Packet: Init Session (Arb-Session) */
>  struct pxp42_create_arb_in {
>         struct pxp_cmd_header header;
> @@ -25,4 +28,16 @@ struct pxp42_create_arb_out {
>         struct pxp_cmd_header header;
>  } __packed;
>  
> +/* PXP-Input-Packet: Invalidate Stream Key */
> +struct pxp42_inv_stream_key_in {
> +       struct pxp_cmd_header header;
> +       u32 rsvd[3];
> +} __packed;
> +
> +/* PXP-Output-Packet: Invalidate Stream Key */
> +struct pxp42_inv_stream_key_out {
> +       struct pxp_cmd_header header;
> +       u32 rsvd;
> +} __packed;
> +
>  #endif /* __INTEL_PXP_FW_INTERFACE_42_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> index c2f23394f9b8..69e34ec49e78 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> @@ -27,6 +27,9 @@ struct pxp_cmd_header {
>         union {
>                 u32 status; /* out */
>                 u32 stream_id; /* in */
> +#define PXP_CMDHDR_EXTDATA_SESSION_VALID GENMASK(0, 0)
> +#define PXP_CMDHDR_EXTDATA_APP_TYPE GENMASK(1, 1)
> +#define PXP_CMDHDR_EXTDATA_SESSION_ID GENMASK(17, 2)
>         };
>         /* Length of the message (excluding the header) */
>         u32 buffer_len;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index 85572360c71a..8eb886d3f2a0 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -91,10 +91,13 @@ static int
> pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
>  {
>         int ret;
>         struct intel_gt *gt = pxp_to_gt(pxp);
> +       u32 active_sip_slots;
>  
>         /* must mark termination in progress calling this function */
>         GEM_WARN_ON(pxp->arb_is_valid);
>  
> +       active_sip_slots = intel_uncore_read(gt->uncore,
> GEN12_KCR_SIP);
> +
>         /* terminate the hw sessions */
>         ret = intel_pxp_terminate_session(pxp, ARB_SESSION);
>         if (ret) {
> @@ -110,6 +113,8 @@ static int
> pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
>  
>         intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);
>  
> +       intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
> +
>         return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index b0c9170b1395..202ea01cbb88 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -309,3 +309,38 @@ int intel_pxp_tee_cmd_create_arb_session(struct
> intel_pxp *pxp,
>  
>         return ret;
>  }
> +
> +void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32
> session_id)
> +{
> +       struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
> +       struct pxp42_inv_stream_key_in msg_in = {0};
> +       struct pxp42_inv_stream_key_out msg_out = {0};
> +       int ret, trials = 0;
> +
> +try_again:
> +       memset(&msg_in, 0, sizeof(msg_in));
> +       memset(&msg_out, 0, sizeof(msg_out));
> +       msg_in.header.api_version = PXP_APIVER(4, 2);
> +       msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY;
> +       msg_in.header.buffer_len = sizeof(msg_in) -
> sizeof(msg_in.header);
> +
> +       msg_in.header.stream_id =
> FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1);
> +       msg_in.header.stream_id |=
> FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0);
> +       msg_in.header.stream_id |=
> FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id);
> +
> +       ret = intel_pxp_tee_io_message(pxp,
> +                                      &msg_in, sizeof(msg_in),
> +                                      &msg_out, sizeof(msg_out),
> +                                      NULL);
> +
> +       /* Cleanup coherency between GT and Firmware is critical, so
> try again if it fails */
> +       if ((ret || msg_out.header.status != 0x0) && ++trials < 3)
> +               goto try_again;
> +
> +       if (ret)
> +               drm_err(&i915->drm, "Failed to send tee msg for inv-
> stream-key-%d, ret=[%d]\n",
> +                       session_id, ret);
> +       else if (msg_out.header.status != 0x0)
> +               drm_warn(&i915->drm, "PXP firmware failed inv-stream-
> key-%d with status 0x%08x\n",
> +                        session_id, msg_out.header.status);
> +}
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> index f74b1e11a505..4d75b06ea4a0 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> @@ -14,6 +14,8 @@
>  struct intel_context;
>  struct i915_pxp_component;
>  
> +#define INTEL_PXP_MAX_HWDRM_SESSIONS 16

This isn't need anymore, otherwise

Reviewed-by: Juston Li <justonli@chromium.org>

> +
>  /**
>   * struct intel_pxp - pxp state
>   */
Alan Previn Dec. 1, 2022, 6:07 p.m. UTC | #2 
On Wed, 2022-11-30 at 17:47 -0800, Juston Li wrote:
> On Mon, 2022-11-28 at 16:48 -0800, Alan Previn wrote:
> > 
> > 
> > 
Alan:[snip]
> > +#define INTEL_PXP_MAX_HWDRM_SESSIONS 16
> 
> This isn't need anymore, otherwise
> 
> Reviewed-by: Juston Li <justonli@chromium.org>
> 
Alan: Thanks - yeah - will respin a final with that fix. I also will need to link from this patch to the mei patches
that needs the fixes. (or include in this series).

> > +
> >  /**
> >   * struct intel_pxp - pxp state
> >   */
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 2da309088c6d..bbeb8ed8e211 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -23,6 +23,7 @@  void intel_pxp_init_hw(struct intel_pxp *pxp);
 void intel_pxp_fini_hw(struct intel_pxp *pxp);
 
 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);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
index 739f9072fa5f..26f7d9f01bf3 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
@@ -12,6 +12,9 @@ 
 /* PXP-Opcode for Init Session */
 #define PXP42_CMDID_INIT_SESSION 0x1e
 
+/* PXP-Opcode for Invalidate Stream Key */
+#define PXP42_CMDID_INVALIDATE_STREAM_KEY 0x00000007
+
 /* PXP-Input-Packet: Init Session (Arb-Session) */
 struct pxp42_create_arb_in {
 	struct pxp_cmd_header header;
@@ -25,4 +28,16 @@  struct pxp42_create_arb_out {
 	struct pxp_cmd_header header;
 } __packed;
 
+/* PXP-Input-Packet: Invalidate Stream Key */
+struct pxp42_inv_stream_key_in {
+	struct pxp_cmd_header header;
+	u32 rsvd[3];
+} __packed;
+
+/* PXP-Output-Packet: Invalidate Stream Key */
+struct pxp42_inv_stream_key_out {
+	struct pxp_cmd_header header;
+	u32 rsvd;
+} __packed;
+
 #endif /* __INTEL_PXP_FW_INTERFACE_42_H__ */
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
index c2f23394f9b8..69e34ec49e78 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
@@ -27,6 +27,9 @@  struct pxp_cmd_header {
 	union {
 		u32 status; /* out */
 		u32 stream_id; /* in */
+#define PXP_CMDHDR_EXTDATA_SESSION_VALID GENMASK(0, 0)
+#define PXP_CMDHDR_EXTDATA_APP_TYPE GENMASK(1, 1)
+#define PXP_CMDHDR_EXTDATA_SESSION_ID GENMASK(17, 2)
 	};
 	/* Length of the message (excluding the header) */
 	u32 buffer_len;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index 85572360c71a..8eb886d3f2a0 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -91,10 +91,13 @@  static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
 {
 	int ret;
 	struct intel_gt *gt = pxp_to_gt(pxp);
+	u32 active_sip_slots;
 
 	/* must mark termination in progress calling this function */
 	GEM_WARN_ON(pxp->arb_is_valid);
 
+	active_sip_slots = intel_uncore_read(gt->uncore, GEN12_KCR_SIP);
+
 	/* terminate the hw sessions */
 	ret = intel_pxp_terminate_session(pxp, ARB_SESSION);
 	if (ret) {
@@ -110,6 +113,8 @@  static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
 
 	intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);
 
+	intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index b0c9170b1395..202ea01cbb88 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -309,3 +309,38 @@  int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp,
 
 	return ret;
 }
+
+void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id)
+{
+	struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
+	struct pxp42_inv_stream_key_in msg_in = {0};
+	struct pxp42_inv_stream_key_out msg_out = {0};
+	int ret, trials = 0;
+
+try_again:
+	memset(&msg_in, 0, sizeof(msg_in));
+	memset(&msg_out, 0, sizeof(msg_out));
+	msg_in.header.api_version = PXP_APIVER(4, 2);
+	msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY;
+	msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
+
+	msg_in.header.stream_id = FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1);
+	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0);
+	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id);
+
+	ret = intel_pxp_tee_io_message(pxp,
+				       &msg_in, sizeof(msg_in),
+				       &msg_out, sizeof(msg_out),
+				       NULL);
+
+	/* Cleanup coherency between GT and Firmware is critical, so try again if it fails */
+	if ((ret || msg_out.header.status != 0x0) && ++trials < 3)
+		goto try_again;
+
+	if (ret)
+		drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%d, ret=[%d]\n",
+			session_id, ret);
+	else if (msg_out.header.status != 0x0)
+		drm_warn(&i915->drm, "PXP firmware failed inv-stream-key-%d with status 0x%08x\n",
+			 session_id, msg_out.header.status);
+}
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
index f74b1e11a505..4d75b06ea4a0 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
@@ -14,6 +14,8 @@ 
 struct intel_context;
 struct i915_pxp_component;
 
+#define INTEL_PXP_MAX_HWDRM_SESSIONS 16
+
 /**
  * struct intel_pxp - pxp state
  */