On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> The GSC notifies us of a proxy request via the HECI2 interrupt. The
alan:snip
> @@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> u32 irqs = GT_RENDER_USER_INTERRUPT;
> u32 guc_mask = intel_uc_wants_guc(>->uc) ? GUC_INTR_GUC2HOST : 0;
> u32 gsc_mask = 0;
> + u32 heci_mask = 0;
alan: nit: should we call this heci2_mask - uniquely identifiable from legacy.
alan:snip
> - else if (HAS_HECI_GSC(gt->i915))
> + heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/
alan: does this GSC_IRQ_INTF macro still make sense for this newer platforms with GSC-CS / HECI2?
i dont think i see other bit definitions for this mask register, so might else well just define it as BIT14?
alan:snip
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index 4aecb5a7b631..da11ce5ca99e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -1577,6 +1577,7 @@
>
> #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + ((x) * 4))
> #define GEN11_CSME (31)
> +#define GEN12_HECI_2 (30)
alan: I dont see this being used anywhere - should remove this.
> +#define GEN11_HECI2_RSVD_INTR_MASK _MMIO(0x1900e4)
alan: GEN11? don't you mean GEN12?
> +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir)
> +{
> + struct intel_gt *gt = gsc_uc_to_gt(gsc);
> +
> + if (unlikely(!iir))
> + return;
> +
> + lockdep_assert_held(gt->irq_lock);
> +
> + if (!gsc->proxy.component) {
> + gt_err(gt, "GSC proxy irq received without the component being bound!\n");
alan: So although proxy init is only a one-time thing (even surviving suspend-resume), we
know that proxy runtime irqs could happen anytime (depending on other gpu-security-related
system interactions). However, would the component driver be bound/unbound through a
suspend-resume cycle? If yes, then would this check fail if an IRQ for a proxy request
came too early after a resume cycle. If yes, then should we not do somethign here to ensure that
when the component gets bound later, we know there is something waiting to be processed?
(in bind, if proxy-init was done before, but we have outstanding IRQs, then we can trigger
the worker from there, i.e. the bind func?)
alan:snip
> static int i915_gsc_proxy_component_bind(struct device *i915_kdev,
> struct device *tee_kdev, void *data)
> {
> struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
> - struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc;
> + struct intel_gt *gt = i915->media_gt;
> + struct intel_gsc_uc *gsc = >->uc.gsc;
> + intel_wakeref_t wakeref;
> +
> + /* enable HECI2 IRQs */
> + with_intel_runtime_pm(&i915->runtime_pm, wakeref)
> + intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE),
> + 0, HECI_H_CSR_IE);
alan: i notice we don't seem to care about potentially re-writing a '1' into reset
if it was midst reset when we read. Shouldn't we also protect against that here?
alan:snip
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> index 023bded10dde..a2a0813b8a76 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> @@ -23,6 +23,9 @@ struct intel_gsc_uc {
> /* for delayed load and proxy handling */
> struct workqueue_struct *wq;
> struct work_struct work;
> + u32 gsc_work_actions; /* protected by gt->irq_lock */
> +#define GSC_ACTION_FW_LOAD BIT(0)
> +#define GSC_ACTION_SW_PROXY BIT(1
>
alan: perhaps its time to have a substruct for "proxy_worker" and include
'wq' and 'work' in additional to actions?
> )
>
> struct {
> struct i915_gsc_proxy_component *component;
