On Mon, 2022-11-21 at 19:21 +0000, Teres Alexis, Alan Previn wrote:
>
>
> On Mon, 2022-11-21 at 10:39 -0800, Juston Li wrote:
> > On Thu, 2022-11-17 at 16:36 -0800, Alan Previn wrote:
> > > A driver bug was recently discovered where the security firmware
> > > was
> > > receiving internal HW signals indicating that session key
> > > expirations
> > > had occurred. Architecturally, the firmware was expecting a
> > > response
> > > from the GuC to acknowledge the event with the firmware side.
> > > However the OS was in a suspended state and GuC had been reset.
> > > Internal specifications actually required the driver to ensure
> > > that all active sessions be properly cleaned up in such cases
> > > where
> > > the system is suspended and the GuC potentially unable to
> > > respond.
> > >
> > > This patch adds the global teardown code in i915's
> > > suspend_prepare
> > > code path.
> > >
> Alan:[snip]
>
> > > +void intel_pxp_end(struct intel_pxp *pxp)
> > > +{
> > > + struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
> > > + intel_wakeref_t wakeref;
> > > +
> > > + if (!intel_pxp_is_enabled(pxp))
> > > + return;
> > > +
> > > + wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> > > +
> > > + mutex_lock(&pxp->arb_mutex);
> > > +
> > > + if (__pxp_global_teardown_locked(pxp, true))
> > > + drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end
> > > timed
> > > out\n");
> > > +
> > > + mutex_unlock(&pxp->arb_mutex);
> > > +
> > > + intel_pxp_fini_hw(pxp);
> >
> > Is intel_pxp_suspend() still needed then if we already fini_hw()
> > here
> > and mark invalidation in intel_pxp_terminate()?
> >
>
> Good catch - looks like we might not need intel_pxp_suspend. But I'll
> verify that for you.
Actually, might need to be careful here. If system aborts suspend or
fails to suspend for any reason, suspend_prepare()->intel_pxp_fini_hw()
might have been called but not suspend().
Correct me if I'm wrong, but in that case I don't think resume() will
be called and thus intel_pxp_init_hw().
For some background, there were some issues with PXP ending up in a bad
state when some other driver caused suspend to fail or user
closed/opened lid quickly and aborted suspend.
> Also, looks like i forgot to include a non-CONFIG_DRM_I915_PXP
> version of intel_pxp_end which was causing the build
> failure. Will resend.
>
> Btw, thanks for reviewing this Juston, i had cc'd you because of the
> impact to suspend-resume flows and I believe you
> have had prior experience debugging issues with that and runtime-
> suspend/resume. Do you any other issues with this
> change?
Np, thanks for the patches!
The only other concern I had that's more of a downstream issue is we
ended up using hw_state_invalidated to block PXP ioctl ops during
teardown to prevent further PXP ioctls triggering pxp_start and another
termination queued.
I don't recall if I sent you this patch on our tree:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3207105
I think this could happen in suspend now too, if app sends PXP ops
while suspend termination is in progress.
Juston
