On Wed, Jul 28, 2021 at 07:01:00PM -0700, Daniele Ceraolo Spurio wrote:
> From: "Huang, Sean Z" <sean.z.huang@xxxxxxxxx>
>
> The HW will generate a teardown interrupt when session termination is
> required, which requires i915 to submit a terminating batch. Once the HW
> is done with the termination it will generate another interrupt, at
> which point it is safe to re-create the session.
>
> Since the termination and re-creation flow is something we want to
> trigger from the driver as well, use a common work function that can be
> called both from the irq handler and from the driver set-up flows, which
> has the addded benefit of allowing us to skip any extra locks because
> the work itself serializes the operations.
>
> v2: use struct completion instead of bool (Chris)
> v3: drop locks, clean up functions and improve comments (Chris),
> move to common work function.
> v4: improve comments, simplify wait logic (Rodrigo)
> v5: unconditionally set interrupts, rename state_attacked var (Rodrigo)
>
> Signed-off-by: Huang, Sean Z <sean.z.huang@xxxxxxxxx>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 7 ++
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 66 +++++++++++--
> drivers/gpu/drm/i915/pxp/intel_pxp.h | 8 ++
> drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 99 ++++++++++++++++++++
> drivers/gpu/drm/i915/pxp/intel_pxp_irq.h | 32 +++++++
> drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 54 ++++++++++-
> drivers/gpu/drm/i915/pxp/intel_pxp_session.h | 5 +-
> drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 8 +-
> drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 18 ++++
> 11 files changed, 283 insertions(+), 16 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e13bc803e5ce..5dcdf5942d32 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -279,6 +279,7 @@ i915-y += i915_perf.o
> i915-$(CONFIG_DRM_I915_PXP) += \
> pxp/intel_pxp.o \
> pxp/intel_pxp_cmd.o \
> + pxp/intel_pxp_irq.o \
> pxp/intel_pxp_session.o \
> pxp/intel_pxp_tee.o
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> index b2de83be4d97..699a74582d32 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> @@ -13,6 +13,7 @@
> #include "intel_lrc_reg.h"
> #include "intel_uncore.h"
> #include "intel_rps.h"
> +#include "pxp/intel_pxp_irq.h"
>
> static void guc_irq_handler(struct intel_guc *guc, u16 iir)
> {
> @@ -64,6 +65,9 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance,
> if (instance == OTHER_GTPM_INSTANCE)
> return gen11_rps_irq_handler(>->rps, iir);
>
> + if (instance == OTHER_KCR_INSTANCE)
> + return intel_pxp_irq_handler(>->pxp, iir);
> +
> WARN_ONCE(1, "unhandled other interrupt instance=0x%x, iir=0x%x\n",
> instance, iir);
> }
> @@ -196,6 +200,9 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> intel_uncore_write(uncore, GEN11_GPM_WGBOXPERF_INTR_MASK, ~0);
> intel_uncore_write(uncore, GEN11_GUC_SG_INTR_ENABLE, 0);
> intel_uncore_write(uncore, GEN11_GUC_SG_INTR_MASK, ~0);
> +
> + intel_uncore_write(uncore, GEN11_CRYPTO_RSVD_INTR_ENABLE, 0);
> + intel_uncore_write(uncore, GEN11_CRYPTO_RSVD_INTR_MASK, ~0);
> }
>
> void gen11_gt_irq_postinstall(struct intel_gt *gt)
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 70eed4fe3fe3..1a2a71916dfc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8086,6 +8086,7 @@ enum {
> /* irq instances for OTHER_CLASS */
> #define OTHER_GUC_INSTANCE 0
> #define OTHER_GTPM_INSTANCE 1
> +#define OTHER_KCR_INSTANCE 4
>
> #define GEN11_INTR_IDENTITY_REG(x) _MMIO(0x190060 + ((x) * 4))
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 26176d43a02d..b0c7edc10cc3 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -2,7 +2,9 @@
> /*
> * Copyright(c) 2020 Intel Corporation.
> */
> +#include <linux/workqueue.h>
> #include "intel_pxp.h"
> +#include "intel_pxp_irq.h"
> #include "intel_pxp_session.h"
> #include "intel_pxp_tee.h"
> #include "gt/intel_context.h"
> @@ -68,6 +70,16 @@ void intel_pxp_init(struct intel_pxp *pxp)
>
> mutex_init(&pxp->tee_mutex);
>
> + /*
> + * we'll use the completion to check if there is a termination pending,
> + * so we start it as completed and we reinit it when a termination
> + * is triggered.
> + */
> + init_completion(&pxp->termination);
> + complete_all(&pxp->termination);
> +
> + INIT_WORK(&pxp->session_work, intel_pxp_session_work);
> +
> ret = create_vcs_context(pxp);
> if (ret)
> return;
> @@ -96,19 +108,61 @@ void intel_pxp_fini(struct intel_pxp *pxp)
> destroy_vcs_context(pxp);
> }
>
> -void intel_pxp_init_hw(struct intel_pxp *pxp)
> +void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp)
> {
> - int ret;
> + pxp->arb_is_valid = false;
> + reinit_completion(&pxp->termination);
> +}
> +
> +static void intel_pxp_queue_termination(struct intel_pxp *pxp)
> +{
> + struct intel_gt *gt = pxp_to_gt(pxp);
> +
> + /*
> + * We want to get the same effect as if we received a termination
> + * interrupt, so just pretend that we did.
> + */
> + spin_lock_irq(>->irq_lock);
> + intel_pxp_mark_termination_in_progress(pxp);
> + pxp->session_events |= PXP_TERMINATION_REQUEST;
> + queue_work(system_unbound_wq, &pxp->session_work);
> + spin_unlock_irq(>->irq_lock);
> +}
>
> +/*
> + * the arb session is restarted from the irq work when we receive the
> + * termination completion interrupt
> + */
> +int intel_pxp_wait_for_arb_start(struct intel_pxp *pxp)
> +{
> + if (!intel_pxp_is_enabled(pxp))
> + return 0;
> +
> + if (!wait_for_completion_timeout(&pxp->termination,
> + msecs_to_jiffies(100)))
> + return -ETIMEDOUT;
> +
> + if (!pxp->arb_is_valid)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +void intel_pxp_init_hw(struct intel_pxp *pxp)
> +{
> kcr_pxp_enable(pxp_to_gt(pxp));
> + intel_pxp_irq_enable(pxp);
>
> - /* always emit a full termination to clean the state */
> - ret = intel_pxp_terminate_arb_session_and_global(pxp);
> - if (!ret)
> - intel_pxp_create_arb_session(pxp);
> + /*
> + * the session could've been attacked while we weren't loaded, so
> + * handle it as if it was and re-create it.
> + */
> + intel_pxp_queue_termination(pxp);
> }
>
> void intel_pxp_fini_hw(struct intel_pxp *pxp)
> {
> kcr_pxp_disable(pxp_to_gt(pxp));
> +
> + intel_pxp_irq_disable(pxp);
> }
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 8eeb65af78b1..074b3b980957 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -30,6 +30,9 @@ void intel_pxp_fini(struct intel_pxp *pxp);
>
> 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);
> +int intel_pxp_wait_for_arb_start(struct intel_pxp *pxp);
> #else
> static inline void intel_pxp_init(struct intel_pxp *pxp)
> {
> @@ -38,6 +41,11 @@ static inline void intel_pxp_init(struct intel_pxp *pxp)
> static inline void intel_pxp_fini(struct intel_pxp *pxp)
> {
> }
> +
> +static inline int intel_pxp_wait_for_arb_start(struct intel_pxp *pxp)
> +{
> + return 0;
> +}
> #endif
>
> #endif /* __INTEL_PXP_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> new file mode 100644
> index 000000000000..46eca1e81b9b
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020 Intel Corporation.
> + */
> +#include <linux/workqueue.h>
> +#include "intel_pxp.h"
> +#include "intel_pxp_irq.h"
> +#include "intel_pxp_session.h"
> +#include "gt/intel_gt_irq.h"
> +#include "i915_irq.h"
> +#include "i915_reg.h"
> +
> +/**
> + * intel_pxp_irq_handler - Handles PXP interrupts.
> + * @pxp: pointer to pxp struct
> + * @iir: interrupt vector
> + */
> +void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> +{
> + struct intel_gt *gt = pxp_to_gt(pxp);
> +
> + if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp)))
> + return;
> +
> + lockdep_assert_held(>->irq_lock);
> +
> + if (unlikely(!iir))
> + return;
> +
> + if (iir & (GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT |
> + GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT)) {
> + /* immediately mark PXP as inactive on termination */
> + intel_pxp_mark_termination_in_progress(pxp);
> + pxp->session_events |= PXP_TERMINATION_REQUEST;
> + }
> +
> + if (iir & GEN12_DISPLAY_STATE_RESET_COMPLETE_INTERRUPT)
> + pxp->session_events |= PXP_TERMINATION_COMPLETE;
> +
> + if (pxp->session_events)
> + queue_work(system_unbound_wq, &pxp->session_work);
> +}
> +
> +static inline void __pxp_set_interrupts(struct intel_gt *gt, u32 interrupts)
> +{
> + struct intel_uncore *uncore = gt->uncore;
> + const u32 mask = interrupts << 16;
> +
> + intel_uncore_write(uncore, GEN11_CRYPTO_RSVD_INTR_ENABLE, mask);
> + intel_uncore_write(uncore, GEN11_CRYPTO_RSVD_INTR_MASK, ~mask);
> +}
> +
> +static inline void pxp_irq_reset(struct intel_gt *gt)
> +{
> + spin_lock_irq(>->irq_lock);
> + gen11_gt_reset_one_iir(gt, 0, GEN11_KCR);
> + spin_unlock_irq(>->irq_lock);
> +}
> +
> +void intel_pxp_irq_enable(struct intel_pxp *pxp)
> +{
> + struct intel_gt *gt = pxp_to_gt(pxp);
> +
> + spin_lock_irq(>->irq_lock);
> +
> + if (!pxp->irq_enabled)
> + WARN_ON_ONCE(gen11_gt_reset_one_iir(gt, 0, GEN11_KCR));
> +
> + __pxp_set_interrupts(gt, GEN12_PXP_INTERRUPTS);
> + pxp->irq_enabled = true;
> +
> + spin_unlock_irq(>->irq_lock);
> +}
> +
> +void intel_pxp_irq_disable(struct intel_pxp *pxp)
> +{
> + struct intel_gt *gt = pxp_to_gt(pxp);
> +
> + /*
> + * We always need to submit a global termination when we re-enable the
> + * interrupts, so there is no need to make sure that the session state
> + * makes sense at the end of this function. Just make sure this is not
> + * called in a path were the driver consider the session as valid and
> + * doesn't call a termination on restart.
> + */
> + GEM_WARN_ON(intel_pxp_is_active(pxp));
> +
> + spin_lock_irq(>->irq_lock);
> +
> + pxp->irq_enabled = false;
> + __pxp_set_interrupts(gt, 0);
> +
> + spin_unlock_irq(>->irq_lock);
> + intel_synchronize_irq(gt->i915);
> +
> + pxp_irq_reset(gt);
> +
> + flush_work(&pxp->session_work);
> +}
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> new file mode 100644
> index 000000000000..8b5793654844
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_PXP_IRQ_H__
> +#define __INTEL_PXP_IRQ_H__
> +
> +#include <linux/types.h>
> +
> +struct intel_pxp;
> +
> +#define GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT BIT(1)
> +#define GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT BIT(2)
> +#define GEN12_DISPLAY_STATE_RESET_COMPLETE_INTERRUPT BIT(3)
> +
> +#define GEN12_PXP_INTERRUPTS \
> + (GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT | \
> + GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT | \
> + GEN12_DISPLAY_STATE_RESET_COMPLETE_INTERRUPT)
> +
> +#ifdef CONFIG_DRM_I915_PXP
> +void intel_pxp_irq_enable(struct intel_pxp *pxp);
> +void intel_pxp_irq_disable(struct intel_pxp *pxp);
> +void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir);
> +#else
> +static inline void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> +{
> +}
> +#endif
> +
> +#endif /* __INTEL_PXP_IRQ_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index b8e24adeb1f3..67c30e534d50 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -48,7 +48,7 @@ static int pxp_wait_for_session_state(struct intel_pxp *pxp, u32 id, bool in_pla
> return ret;
> }
>
> -int intel_pxp_create_arb_session(struct intel_pxp *pxp)
> +static int pxp_create_arb_session(struct intel_pxp *pxp)
> {
> struct intel_gt *gt = pxp_to_gt(pxp);
> int ret;
> @@ -77,12 +77,13 @@ int intel_pxp_create_arb_session(struct intel_pxp *pxp)
> return 0;
> }
>
> -int intel_pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
> +static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
> {
> int ret;
> struct intel_gt *gt = pxp_to_gt(pxp);
>
> - pxp->arb_is_valid = false;
> + /* must mark termination in progress calling this function */
> + GEM_WARN_ON(pxp->arb_is_valid);
>
> /* terminate the hw sessions */
> ret = intel_pxp_terminate_session(pxp, ARB_SESSION);
> @@ -101,3 +102,50 @@ int intel_pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
>
> return ret;
> }
> +
> +static void pxp_terminate(struct intel_pxp *pxp)
> +{
> + int ret;
> +
> + pxp->hw_state_invalidated = true;
> +
> + /*
> + * if we fail to submit the termination there is no point in waiting for
> + * it to complete. PXP will be marked as non-active until the next
> + * termination is issued.
> + */
> + ret = pxp_terminate_arb_session_and_global(pxp);
> + if (ret)
> + complete_all(&pxp->termination);
> +}
> +
> +static void pxp_terminate_complete(struct intel_pxp *pxp)
> +{
> + /* Re-create the arb session after teardown handle complete */
> + if (fetch_and_zero(&pxp->hw_state_invalidated))
> + pxp_create_arb_session(pxp);
> +
> + complete_all(&pxp->termination);
> +}
> +
> +void intel_pxp_session_work(struct work_struct *work)
> +{
> + struct intel_pxp *pxp = container_of(work, typeof(*pxp), session_work);
> + struct intel_gt *gt = pxp_to_gt(pxp);
> + u32 events = 0;
> +
> + spin_lock_irq(>->irq_lock);
> + events = fetch_and_zero(&pxp->session_events);
> + spin_unlock_irq(>->irq_lock);
> +
> + if (!events)
> + return;
> +
> + if (events & PXP_TERMINATION_REQUEST) {
> + events &= ~PXP_TERMINATION_COMPLETE;
> + pxp_terminate(pxp);
> + }
> +
> + if (events & PXP_TERMINATION_COMPLETE)
> + pxp_terminate_complete(pxp);
> +}
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> index 7354314b1cc4..ba4c9d2b94b7 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> @@ -8,9 +8,8 @@
>
> #include <linux/types.h>
>
> -struct intel_pxp;
> +struct work_struct;
>
> -int intel_pxp_create_arb_session(struct intel_pxp *pxp);
> -int intel_pxp_terminate_arb_session_and_global(struct intel_pxp *pxp);
> +void intel_pxp_session_work(struct work_struct *work);
>
> #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 3662bf67407a..7693540dc1f9 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -80,6 +80,7 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
> {
> struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
> struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
> + int ret;
>
> mutex_lock(&pxp->tee_mutex);
> pxp->pxp_component = data;
> @@ -88,15 +89,14 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
>
> /* the component is required to fully start the PXP HW */
> intel_pxp_init_hw(pxp);
> -
> - if (!pxp->arb_is_valid) {
> + ret = intel_pxp_wait_for_arb_start(pxp);
> + if (ret) {
> drm_err(&i915->drm, "Failed to create arb session during bind\n");
> intel_pxp_fini_hw(pxp);
> pxp->pxp_component = NULL;
> - return -EIO;
> }
>
> - return 0;
> + return ret;
> }
>
> static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> index a4797a98c1f9..475e3312c287 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> @@ -6,8 +6,10 @@
> #ifndef __INTEL_PXP_TYPES_H__
> #define __INTEL_PXP_TYPES_H__
>
> +#include <linux/completion.h>
> #include <linux/mutex.h>
> #include <linux/types.h>
> +#include <linux/workqueue.h>
>
> struct intel_context;
> struct i915_pxp_component;
> @@ -25,6 +27,22 @@ struct intel_pxp {
> bool arb_is_valid;
>
> struct mutex tee_mutex; /* protects the tee channel binding */
> +
> + /*
> + * If the HW perceives an attack on the integrity of the encryption it
> + * will invalidate the keys and expect SW to re-initialize the session.
> + * We keep track of this state to make sure we only re-start the arb
> + * session when required.
> + */
Please convert these into kerneldoc, pull them somewhere into driver docs,
and then document the locking rules for all the members here. At least
where relevant. Even if the locking rules are "only &session_work mutates
this completion" or whatever the rules are.
Yeah I know this is very late, but I've read too much i915-gem/gt/core
locking code, and it's way too much incomprehensible overall. And probably
worse, not documented at all. So we need to step up here.
-Daniel
> + bool hw_state_invalidated;
> +
> + bool irq_enabled;
> + struct completion termination;
> +
> + struct work_struct session_work;
> + u32 session_events; /* protected with gt->irq_lock */
> +#define PXP_TERMINATION_REQUEST BIT(0)
> +#define PXP_TERMINATION_COMPLETE BIT(1)
> };
>
> #endif /* __INTEL_PXP_TYPES_H__ */
> --
> 2.32.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
