Re: [Intel-gfx] [RFC-v3 03/13] drm/i915/pxp: Implement funcs to create the TEE channel

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

Please not that in the middle section of the review I noticed that we're
creating i915_component which is injected with the send/recv functions
from MEI subsystem, so the component naming conventions seem to be
reversed to me.

Quoting Huang, Sean Z (2020-12-09 09:02:57)
> Implement the funcs to create the TEE channel, so kernel can
> send the TEE commands directly to TEE for creating the arbitrary
> (defualt) session.

This TEE channel should be described in more detail. Now it
is hard to assess if the placement is correct. Is it related to the GT
only, or is it also related to display?

> Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1216,6 +1216,12 @@ struct drm_i915_private {
>         /* Mutex to protect the above hdcp component related values. */
>         struct mutex hdcp_comp_mutex;
>  
> +       struct i915_pxp_comp_master *pxp_tee_master;

The struct and variable names don't relate. No need to add "_master"
here (or "_primary") as we don't seem to have other than one.

	struct i915_pxp_tee_component *pxp_tee_comp;

I can't assess if this belongs to intel_gt or here.

> +       bool pxp_tee_comp_added;

Why can't we just check for non-zero pxp_tee; ?

> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -5,6 +5,7 @@
>  #include "i915_drv.h"
>  #include "intel_pxp.h"
>  #include "intel_pxp_context.h"
> +#include "intel_pxp_tee.h"
>  
>  /* KCR register definitions */
>  #define KCR_INIT            _MMIO(0x320f0)
> @@ -24,6 +25,8 @@ int intel_pxp_init(struct intel_pxp *pxp)
>  
>         intel_uncore_write(gt->uncore, KCR_INIT, KCR_INIT_ALLOW_DISPLAY_ME_WRITES);
>  
> +       intel_pxp_tee_component_init(pxp);

I don't think this is the right location. This is for early hardware
init.

This should be a call named i915_pxp_tee_component_init();

And as it is related to exposing the component it should be much later
in init, probably near the audio component.

> @@ -31,5 +34,7 @@ int intel_pxp_init(struct intel_pxp *pxp)
>  
>  void intel_pxp_uninit(struct intel_pxp *pxp)
>  {
> +       intel_pxp_tee_component_fini(pxp);

Same here.

> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020 Intel Corporation.
> + */
> +
> +#include <linux/component.h>
> +#include "drm/i915_pxp_tee_interface.h"
> +#include "drm/i915_component.h"
> +#include  "i915_drv.h"
> +#include "intel_pxp.h"
> +#include "intel_pxp_context.h"
> +#include "intel_pxp_tee.h"
> +
> +static int intel_pxp_tee_io_message(struct intel_pxp *pxp,

For .c local static functions, the prefixes should be dropped.

This should take struct intel_pxp_tee *tee as first parameter

> +                                   void *msg_in, u32 msg_in_size,

void* is no good, doesn't give impression of unit of size (pages?)

> +                                   void *msg_out, u32 *msg_out_size_ptr,

_ptr is tautology

> +                                   u32 msg_out_buf_size)

We can use the same variable for in and out

static int do_io(struct intel_pxp_tee *tee,
		 u8 *in, size_t in_nbytes,
		 u8 *out, size_t *out_nbytes)

> +{
> +       int ret;
> +       struct intel_gt *gt = container_of(pxp, typeof(*gt), pxp);
> +       struct drm_i915_private *i915 = gt->i915;
> +       struct i915_pxp_comp_master *pxp_tee_master = i915->pxp_tee_master;
> +
> +       if (!pxp_tee_master || !msg_in || !msg_out || !msg_out_size_ptr)
> +               return -EINVAL;

GEM_BUG_ON() should be sufficient.

> +       lockdep_assert_held(&i915->pxp_tee_comp_mutex);
> +
> +       if (drm_debug_enabled(DRM_UT_DRIVER))
> +               print_hex_dump(KERN_DEBUG, "TEE input message binaries:",
> +                              DUMP_PREFIX_OFFSET, 4, 4, msg_in, msg_in_size, true);
> +
> +       ret = pxp_tee_master->ops->send(pxp_tee_master->tee_dev, msg_in, msg_in_size);
> +       if (ret) {
> +               drm_err(&i915->drm, "Failed to send TEE message\n");
> +               return -EFAULT;
> +       }
> +
> +       ret = pxp_tee_master->ops->receive(pxp_tee_master->tee_dev, msg_out, msg_out_buf_size);
> +       if (ret < 0) {
> +               drm_err(&i915->drm, "Failed to receive TEE message\n");
> +               return -EFAULT;
> +       }

Ok, this got confusing. It seems that we're importing the TEE hardware
to use from MEI subsystem? And not exposing it from i915.

I think the whole component mechanism is then the wrong way around. It
should be us importing the tee dev from MEI subsystem to communicate.

Also, why are we having separate send/receive callbacks but we stuff
everything under single function, we should not do that. So this should
be split into send() and recv() functions here.

<SKIPPING REVIEW HERE>

> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_PXP_TEE_H__
> +#define __INTEL_PXP_TEE_H__
> +
> +#include "intel_pxp.h"

We should not do this, but only include the _types.h files required.
But in this case it would only be intel_pxp_tee_types.h as that should
be the parameter.

> +void intel_pxp_tee_component_init(struct intel_pxp *pxp);
> +void intel_pxp_tee_component_fini(struct intel_pxp *pxp);

We should be consistent with <struct>_<verb>, the "component" is extra
here.

void intel_pxp_tee_init(struct intel_pxp_tee *tee);

And same for _fini().

> +
> +#endif /* __INTEL_PXP_TEE_H__ */
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index 55c3b123581b..c1e2a43d2d1e 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -29,6 +29,7 @@
>  enum i915_component_type {
>         I915_COMPONENT_AUDIO = 1,
>         I915_COMPONENT_HDCP,
> +       I915_COMPONENT_PXP

I think I915_COMPONENT_PXP_TEE here, too. Otherwise it's easy to lose
track of what is passed onwards.

> +++ b/include/drm/i915_pxp_tee_interface.h

I think the file should be i915_pxp_tee_component.h for clarity.

> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2020 Intel Corporation
> + *
> + * Authors:
> + * Vitaly Lubart <vitaly.lubart@intel.com>
> + */
> +
> +#ifndef _I915_PXP_TEE_INTERFACE_H_
> +#define _I915_PXP_TEE_INTERFACE_H_

"PXP_TEE_COMPONENT"

> +
> +#include <linux/mutex.h>
> +#include <linux/device.h>
> +
> +/**
> + * struct i915_pxp_component_ops - ops for PXP services.
> + * @owner: Module providing the ops
> + * @send: sends data to PXP
> + * @receive: receives data from PXP
> + */
> +struct i915_pxp_component_ops {

i915_pxp_tee_component_ops

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