Re: [Intel-gfx] [PATCH v2 08/16] drm/i915/pxp: Implement arb session teardown

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Huang@freedesktop.org, "Huang, Sean Z" <sean.z.huang@intel.com>
Subject: Re: [Intel-gfx] [PATCH v2 08/16] drm/i915/pxp: Implement arb session teardown
Date: Wed, 03 Mar 2021 22:04:16 +0000	[thread overview]
Message-ID: <161480905671.25897.2439316755371407734@build.alporthouse.com> (raw)
In-Reply-To: <20210301193200.1369-9-daniele.ceraolospurio@intel.com>

Quoting Daniele Ceraolo Spurio (2021-03-01 19:31:52)
> From: "Huang, Sean Z" <sean.z.huang@intel.com>
> 
> Teardown is triggered when the display topology changes and no
> long meets the secure playback requirement, and hardware trashes
> all the encryption keys for display. Additionally, we want to emit a
> teardown operation to make sure we're clean on boot and resume
> 
> v2: emit in the ring, use high prio request (Chris)
> 
> Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/Makefile                |   1 +
>  drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c     | 166 +++++++++++++++++++
>  drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h     |  15 ++
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.c |  38 +++++
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.h |   1 +
>  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c     |   5 +-
>  6 files changed, 225 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
>  create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index d6d510e4875e..8b605f326039 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -273,6 +273,7 @@ i915-y += i915_perf.o
>  # Protected execution platform (PXP) support
>  i915-$(CONFIG_DRM_I915_PXP) += \
>         pxp/intel_pxp.o \
> +       pxp/intel_pxp_cmd.o \
>         pxp/intel_pxp_session.o \
>         pxp/intel_pxp_tee.o
>  
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
> new file mode 100644
> index 000000000000..ffab09839cd3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#include "intel_pxp.h"
> +#include "intel_pxp_session.h"
> +#include "gt/intel_context.h"
> +#include "gt/intel_engine_pm.h"
> +#include "gt/intel_gpu_commands.h"
> +#include "gt/intel_ring.h"
> +
> +#include "i915_trace.h"
> +
> +/* PXP GPU command definitions */
> +
> +/* MI_SET_APPID */
> +#define   MI_SET_APPID_SESSION_ID(x)    ((x) << 0)
> +
> +/* MI_FLUSH_DW */
> +#define   MI_FLUSH_DW_DW0_PROTECTED_MEMORY_ENABLE   BIT(22)
> +
> +/* MI_WAIT */
> +#define   MFX_WAIT_DW0_PXP_SYNC_CONTROL_FLAG BIT(9)
> +#define   MFX_WAIT_DW0_MFX_SYNC_CONTROL_FLAG  BIT(8)

We've been using REG_BIT() for the explicit (u32) casting.

> +/* CRYPTO_KEY_EXCHANGE */
> +#define CRYPTO_KEY_EXCHANGE ((0x3 << 29) | (0x01609 << 16))

#define __INSTR(client) ((client) << INSTR_CLIENT_SHIFT)

#define MI_INSTR(opcode, flags) \
	(__INSTR(INSTR_MI_CLIENT) | (opcode) << 23 | (flags))
#define RC_INTR(foo) (__INSTR(INSTR_RC_CLIENT) | (foo) << 16)

#define CRYPTO_KEY_EXCHANGE RC_INSTR(0x1609)

With a better (foo).

> +
> +/* stall until prior PXP and MFX/HCP/HUC objects are cmopleted */
> +#define MFX_WAIT_PXP \
> +       MFX_WAIT | \
> +       MFX_WAIT_DW0_PXP_SYNC_CONTROL_FLAG | \
> +       MFX_WAIT_DW0_MFX_SYNC_CONTROL_FLAG;
> +
> +static u32 *pxp_emit_session_selection(u32 *cs, u32 idx)
> +{
> +       *cs++ = MFX_WAIT_PXP;

One day someone will proofread bspec.

> +       /* pxp off */
> +       *cs++ = MI_FLUSH_DW;
> +       *cs++ = 0;
> +       *cs++ = 0;

Hmm. Can the immediate data be dropped? TIL.

> +       /* select session */
> +       *cs++ = MI_SET_APPID | MI_SET_APPID_SESSION_ID(idx);
> +
> +       *cs++ = MFX_WAIT_PXP;
> +
> +       /* pxp on */
> +       *cs++ = MI_FLUSH_DW | MI_FLUSH_DW_DW0_PROTECTED_MEMORY_ENABLE;
> +       *cs++ = 0;
> +       *cs++ = 0;

Bspec says "after completion of the flush...", which says to me we
should not initiate the wait until after the flush, so we would need a
post-sync op here to stall the CS (or else we may complete the wait
before the operation is begun). I don't see any programming notes to
that effect, so could just be my paranoia from handling atomics.

> +       *cs++ = MFX_WAIT_PXP;

Fwiw, the bspec language would seem to imply that nothing should happen
with this wait at this point. Perhaps more reason to make the pxp-on
MI_FLUSH_DW be synchronous. (Though we will have a sync point at the
breadcrumb, so meh.)

> +
> +       return cs;
> +}
> +
> +static u32 *pxp_emit_inline_termination(u32 *cs)
> +{
> +       /* session inline termination */
> +       *cs++ = CRYPTO_KEY_EXCHANGE;
> +       *cs++ = 0;
> +
> +       return cs;
> +}
> +
> +static u32 *pxp_emit_wait(u32 *cs)
> +{
> +       /* wait for cmds to go through */
> +       *cs++ = MFX_WAIT_PXP;
> +       *cs++ = 0;
> +
> +       return cs;
> +}
> +
> +/*
> + * if we ever need to terminate more than one session, we can submit multiple
> + * selections and terminations back-to-back with a single wait at the end
> + */
> +#define SELECTION_LEN 10
> +#define TERMINATION_LEN 2
> +#define WAIT_LEN 2

Dwords lengths ok.

> +#define __SESSION_TERMINATION_LEN (SELECTION_LEN + TERMINATION_LEN)
> +#define SESSION_TERMINATION_LEN(x) (__SESSION_TERMINATION_LEN * (x) + WAIT_LEN)
> +
> +static struct i915_request *pxp_request_create(struct intel_context *ce)
> +{
> +       struct i915_request *rq;
> +
> +       intel_context_enter(ce);
> +       rq = __i915_request_create(ce, GFP_KERNEL);
> +       intel_context_exit(ce);
> +
> +       return rq;
> +}
> +
> +static void pxp_request_commit(struct i915_request *rq)
> +{
> +       struct i915_sched_attr attr = { .priority = I915_PRIORITY_MAX };
> +
> +       trace_i915_request_add(rq);
> +       __i915_request_commit(rq);
> +       __i915_request_queue(rq, &attr);
> +}
> +
> +int intel_pxp_submit_session_termination(struct intel_pxp *pxp, u32 id)
> +{
> +       struct i915_request *rq;
> +       struct intel_context *ce = pxp->ce;
> +       u32 *cs;
> +       int err;
> +
> +       if (!intel_pxp_is_enabled(pxp))
> +               return 0;
> +
> +       intel_engine_pm_get(ce->engine);

As you are not using the engine->kernel_context, you can leave this out
and the normal intel_context_enter() acquire of the wakeref will cover
you.

You could then use the normal i915_request_create(), but we would still
have the custom i915_request_add, which would absorb the mutex_unlock so
would not look so unbalanced.

> +       mutex_lock(&ce->timeline->mutex);
> +
> +       rq = pxp_request_create(ce);
> +       if (IS_ERR(rq)) {
> +               mutex_unlock(&ce->timeline->mutex);
> +               err = PTR_ERR(rq);
> +               goto out_pm;
> +       }
> +
> +       if (ce->engine->emit_init_breadcrumb) {
> +               err = ce->engine->emit_init_breadcrumb(rq);
> +               if (err)
> +                       goto out_rq;
> +       }
> +
> +       cs = intel_ring_begin(rq, SESSION_TERMINATION_LEN(1));
> +       if (IS_ERR(cs)) {
> +               err = PTR_ERR(cs);
> +               goto out_rq;
> +       }
> +
> +       cs = pxp_emit_session_selection(cs, id);
> +       cs = pxp_emit_inline_termination(cs);

I would wrap this pair with

	cs = pxp_emit_session_termination(cs, id);

so the correspondence with SESSION_TERMINATION_LEN() is clearer and I'd
be tempted then to add the WAIT_LEN explicitly.

> +       cs = pxp_emit_wait(cs);
> +
> +       intel_ring_advance(rq, cs);

Fwiw, this would be a good excuse to decouple invalidate/flush from the
breadcrumbs. Later, much later.

> +
> +out_rq:
> +       i915_request_get(rq);
> +
> +       if (unlikely(err))
> +               i915_request_set_error_once(rq, err);
> +
> +       pxp_request_commit(rq);
> +
> +       mutex_unlock(&ce->timeline->mutex);
> +
> +       if (!err && i915_request_wait(rq, 0, HZ / 5) < 0)
> +               err = -ETIME;

"intel_pxp_submit_session_termination" does not imply synchronous
behaviour to me.

intel_pxp_terminate_session() ?

> +
> +       i915_request_put(rq);
> +
> +out_pm:
> +       intel_engine_pm_put(ce->engine);
> +
> +       return err;
> +}
> +
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h
> new file mode 100644
> index 000000000000..7c33b66f0812
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_PXP_CMD_H__
> +#define __INTEL_PXP_CMD_H__
> +
> +#include <linux/types.h>
> +
> +struct intel_pxp;
> +
> +int intel_pxp_submit_session_termination(struct intel_pxp *pxp, u32 idx);
> +
> +#endif /* __INTEL_PXP_CMD_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index 6abc59a63e51..ddbfac75686a 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -7,6 +7,7 @@
>  #include "i915_drv.h"
>  
>  #include "intel_pxp.h"
> +#include "intel_pxp_cmd.h"
>  #include "intel_pxp_session.h"
>  #include "intel_pxp_tee.h"
>  #include "intel_pxp_types.h"
> @@ -15,6 +16,9 @@
>  
>  #define GEN12_KCR_SIP _MMIO(0x32260) /* KCR hwdrm session in play 0-31 */
>  
> +/* PXP global terminate register for session termination */
> +#define PXP_GLOBAL_TERMINATE _MMIO(0x320f8)
> +
>  static bool intel_pxp_session_is_in_play(struct intel_pxp *pxp, u32 id)
>  {
>         struct intel_gt *gt = pxp_to_gt(pxp);
> @@ -80,3 +84,37 @@ int intel_pxp_create_arb_session(struct intel_pxp *pxp)
>  
>         return 0;
>  }
> +
> +/**
> + * intel_pxp_arb_terminate_session_with_global_terminate - Terminate the arb hw

terminate_session_and_global ? Just to remove the repetition?

> + * session.
> + * @pxp: pointer to pxp struct.
> + *
> + * Return: 0 if terminate is successful, error code otherwise
> + */
> +int intel_pxp_arb_terminate_session_with_global_terminate(struct intel_pxp *pxp)
> +{
> +       int ret;
> +       struct intel_gt *gt = pxp_to_gt(pxp);
> +
> +       lockdep_assert_held(&pxp->mutex);
> +
> +       pxp->arb_is_in_play = false;
> +
> +       /* terminate the hw sessions */
> +       ret = intel_pxp_submit_session_termination(pxp, ARB_SESSION);
> +       if (ret) {
> +               drm_err(&gt->i915->drm, "Failed to submit session termination\n");
> +               return ret;
> +       }

Should arb_is_in_play not be updated until after successful termination?

> +       ret = pxp_wait_for_session_state(pxp, ARB_SESSION, false);
> +       if (ret) {
> +               drm_err(&gt->i915->drm, "Session state did not clear\n");
> +               return ret;
> +       }
> +
> +       intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);

Are we happy that this is instantaneous and doesn't need an ack/wait?

> +       return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> index 6fc4a2370c44..07c97df7a509 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> @@ -12,5 +12,6 @@ struct intel_pxp;
>  
>  bool intel_pxp_arb_session_is_in_play(struct intel_pxp *pxp);
>  int intel_pxp_create_arb_session(struct intel_pxp *pxp);
> +int intel_pxp_arb_terminate_session_with_global_terminate(struct intel_pxp *pxp);
>  
>  #endif /* __INTEL_PXP_SESSION_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index dc3850b372c5..fd9a69248dd8 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -99,7 +99,10 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
>         mutex_lock(&pxp->mutex);
>  
>         /* Create arb session only if tee is ready, during system boot or sleep/resume */
> -       if (!intel_pxp_arb_session_is_in_play(pxp))
> +       if (intel_pxp_arb_session_is_in_play(pxp))
> +               ret = intel_pxp_arb_terminate_session_with_global_terminate(pxp);
> +
> +       if (!ret)
>                 ret = intel_pxp_create_arb_session(pxp);
>  
>         mutex_unlock(&pxp->mutex);
> -- 
> 2.29.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx