Re: [RFC 04/14] drm/i915/pxp: allocate a vcs context for pxp usage

[Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>]

On 2/6/2021 4:49 AM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2021-02-06 02:09:15)
> > The context is required to send the session termination commands to the
> > VCS, which will be implemented in a follow-up patch. We can also use the
> > presence of the context as a check of pxp initialization completion.
> >
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/Makefile              |  4 ++
> >   drivers/gpu/drm/i915/gt/intel_gt.c         |  5 ++
> >   drivers/gpu/drm/i915/gt/intel_gt_types.h   |  3 ++
> >   drivers/gpu/drm/i915/pxp/intel_pxp.c       | 61 ++++++++++++++++++++++
> >   drivers/gpu/drm/i915/pxp/intel_pxp.h       | 35 +++++++++++++
> >   drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 15 ++++++
> >   6 files changed, 123 insertions(+)
> >   create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp.c
> >   create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp.h
> >   create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index ce01634d4ea7..e2677e8c03e8 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -268,6 +268,10 @@ i915-y += \
> >   
> >   i915-y += i915_perf.o
> >   
> > +# Protected execution platform (PXP) support
> > +i915-$(CONFIG_DRM_I915_PXP) += \
> > +       pxp/intel_pxp.o
> > +
> >   # Post-mortem debug and GPU hang state capture
> >   i915-$(CONFIG_DRM_I915_CAPTURE_ERROR) += i915_gpu_error.o
> >   i915-$(CONFIG_DRM_I915_SELFTEST) += \
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index ca76f93bc03d..daf61db620d6 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -20,6 +20,7 @@
> >   #include "intel_uncore.h"
> >   #include "intel_pm.h"
> >   #include "shmem_utils.h"
> > +#include "pxp/intel_pxp.h"
> >   
> >   void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
> >   {
> > @@ -624,6 +625,8 @@ int intel_gt_init(struct intel_gt *gt)
> >          if (err)
> >                  goto err_gt;
> >   
> > +       intel_pxp_init(&gt->pxp);
> > +
> >          goto out_fw;
> >   err_gt:
> >          __intel_gt_disable(gt);
> > @@ -658,6 +661,8 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
> >   
> >          intel_rps_driver_unregister(&gt->rps);
> >   
> > +       intel_pxp_fini(&gt->pxp);
> > +
> >          /*
> >           * Upon unregistering the device to prevent any new users, cancel
> >           * all in-flight requests so that we can quickly unbind the active
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index 626af37c7790..324d267eee15 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > @@ -23,6 +23,7 @@
> >   #include "intel_rc6_types.h"
> >   #include "intel_rps_types.h"
> >   #include "intel_wakeref.h"
> > +#include "pxp/intel_pxp_types.h"
> >   
> >   struct drm_i915_private;
> >   struct i915_ggtt;
> > @@ -145,6 +146,8 @@ struct intel_gt {
> >                  /* Slice/subslice/EU info */
> >                  struct sseu_dev_info sseu;
> >          } info;
> > +
> > +       struct intel_pxp pxp;
> >   };
> >   
> >   enum intel_gt_scratch_field {
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > new file mode 100644
> > index 000000000000..4ddc8a71a3e7
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > @@ -0,0 +1,61 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright(c) 2020 Intel Corporation.
> > + */
> > +#include "intel_pxp.h"
> > +#include "gt/intel_context.h"
> > +#include "i915_drv.h"
> > +
> > +static int create_vcs_context(struct intel_pxp *pxp)
> > +{
> > +       struct intel_gt *gt = pxp_to_gt(pxp);
> > +       struct intel_context *ce = NULL;
> > +       int i;
> > +
> > +       /*
> > +        * Find the first VCS engine present. We're guaranteed there is one
> > +        * if we're in this function due to the check in has_pxp
> > +        */
> > +       for (i = 0; i < I915_MAX_VCS && !ce; i++)
> > +               if (HAS_ENGINE(gt, _VCS(i)))
> > +                       ce = intel_context_create(gt->engine[_VCS(i)]);
> Just wondering if
>
> struct intel_engine_cs **vcs_engines = gt->engine_class[CLASS_VIDEO_DECODE];
>
> for (i = 0; i < ARRAY_SIZE(gt->engine_class[CLASS_VIDEO_DECODE]); i++) {
> 	if (!vcs_engines[i])
> 		continue;
>
> 	ce = intel_context_create(vcs_engines[i]);
> 	break;
> }
>
> is a better iterator as it only checks one place of truth about whether
> or not the engine exists.
>
> for_each_engine_class(engine, gt, class, i)
>
> A couple of places could use that.
>
> > +       if (IS_ERR(ce)) {
> > +               drm_err(&gt->i915->drm, "failed to create VCS ctx for PXP\n");
> > +               return PTR_ERR(ce);
> Is the lack of this feature enough to prevent module loading? Surely
> userspace will notice and report the lack of the feature?

It's not, and we don't fail the load on this error, it's just used by 
intel_pxp_init() to abort PXP enabling.

> > +       }
> > +
> > +       pxp->ce = ce;
> Is protected context then implicitly tried to one engine? i.e.
> userspace has to use the same engine as we control invalidation?
> Otherwise, everytime we use pxp->ce we must impose barriers across all
> gt->vcs.

It's not tied. I considered using a virtual engine, but that needs some 
proper testing so better as a follow up step after we have the tests in 
place.
Regarding the barriers, I don't think they'd be very helpful. The 
teardown process includes an MMIO write after this context completes, 
followed by waiting for the irq and then re-creating the session. We 
could halt submission of all contexts marked as protected while the 
teardown is in progress, but any buffer that was written to using the 
old keys is now garbage. The spec says that userspace can detect a 
session being re-created but I'm not sure how, let me check with 
architecture (I was told that the behavior in the original patch series 
was agreed on by all parties so I just stuck to that).

Daniele

> > +
> > +       return 0;
> > +}
> > +
> > +static void destroy_vcs_context(struct intel_pxp *pxp)
> > +{
> > +       intel_context_put(fetch_and_zero(&pxp->ce));
> > +}
> > +
> > +void intel_pxp_init(struct intel_pxp *pxp)
> > +{
> > +       struct intel_gt *gt = pxp_to_gt(pxp);
> > +       int ret;
> > +
> > +       if (!HAS_PXP(gt->i915))
> > +               return;
> > +
> > +       ret = create_vcs_context(pxp);
> > +       if (ret)
> > +               return;
> > +
> > +       drm_info(&gt->i915->drm, "Protected Xe Path (PXP) protected content support initialized\n");
> > +
> > +       return;
> > +}
> > +
> > +void intel_pxp_fini(struct intel_pxp *pxp)
> > +{
> > +       if (!intel_pxp_is_enabled(pxp))
> > +               return;
> > +
> > +       destroy_vcs_context(pxp);
> > +}
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > new file mode 100644
> > index 000000000000..e2acd06402cd
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> > + */
> > +
> > +#ifndef __INTEL_PXP_H__
> > +#define __INTEL_PXP_H__
> > +
> > +#include "gt/intel_gt_types.h"
> > +#include "intel_pxp_types.h"
> > +
> > +static inline struct intel_gt *pxp_to_gt(struct intel_pxp *pxp)
> > +{
> > +       return container_of(pxp, struct intel_gt, pxp);
> > +}
> > +
> > +static inline bool intel_pxp_is_enabled(struct intel_pxp *pxp)
> const struct intel_pxp *pxp
> -Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx