Re: [Intel-gfx] [PATCH v5 4/6] drm/i915/pxp: Invalidate all PXP fw sessions during teardown

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

Thanks Rodrigo for the ack.

On Thu, 2023-01-19 at 14:28 -0500, Vivi, Rodrigo wrote:
> On Thu, Jan 12, 2023 at 05:18:48PM -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>
> > Reviewed-by: Juston Li <justonli@chromium.org>
> 
> Acked-by: Rodrigo Vivi <rodrigo.vivi@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  |  2 ++
> >  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 35 +++++++++++++++++++
> >  5 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > index 04440fada711..9658d3005222 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > @@ -24,6 +24,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 ae413580b81a..74ed7e16e481 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > @@ -110,6 +110,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 bef6d7f8ac55..9e247f38f3bd 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > @@ -311,3 +311,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->ctrl_gt->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);
> > +}
> > -- 
> > 2.39.0
> >