On Mon, 2023-11-06 at 02:59 +0000, Jason-JH Lin (林睿祥) wrote:

On Mon, 2023-11-06 at 01:27 +0000, CK Hu (胡俊光) wrote:

quoted

On Sun, 2023-11-05 at 13:04 +0000, Jason-JH Lin (林睿祥) wrote:

quoted

On Tue, 2023-10-31 at 06:01 +0000, CK Hu (胡俊光) wrote:

quoted

Hi, Jason:

On Mon, 2023-10-23 at 12:45 +0800, Jason-JH.Lin wrote:

quoted

To add secure flow support for mediatek-drm, each crtc have
to
create a secure cmdq mailbox channel. Then cmdq packets with
display HW configuration will be sent to secure cmdq mailbox
channel
and configured in the secure world.

Each crtc have to use secure cmdq interface to configure some
secure
settings for display HW before sending cmdq packets to secure
cmdq
mailbox channel.

If any of fb get from current drm_atomic_state is secure,
then
crtc
will switch to the secure flow to configure display HW.
If all fbs are not secure in current drm_atomic_state, then
crtc
will
switch to the normal flow.

Signed-off-by: Jason-JH.Lin <redacted>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 272
++++++++++++++++++++++-
 drivers/gpu/drm/mediatek/mtk_drm_crtc.h  |   1 +
 drivers/gpu/drm/mediatek/mtk_drm_plane.c |   7 +
 3 files changed, 269 insertions(+), 11 deletions(-)

[snip]

quoted

quoted

quoted

+
+#if IS_REACHABLE(CONFIG_MTK_CMDQ)
+static void mtk_crtc_enable_secure_state(struct drm_crtc
*crtc)
+{
+	enum cmdq_sec_scenario sec_scn = CMDQ_MAX_SEC_COUNT;
+	int i;
+	struct mtk_ddp_comp *ddp_first_comp;
+	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
+	u64 sec_engine = 0; /* for hw engine write output
secure fb */
+	u64 sec_port = 0; /* for larb port read input secure fb
*/
+
+	cmdq_pkt_wfe(&mtk_crtc->sec_cmdq_handle, mtk_crtc-

quoted

cmdq_event,

false);
+
+	ddp_first_comp = mtk_crtc->ddp_comp[0];
+	for (i = 0; i < mtk_crtc->layer_nr; i++)
+		if (mtk_crtc->planes[i].type ==
DRM_PLANE_TYPE_CURSOR)
+			sec_port |=
mtk_ddp_comp_layer_get_sec_port(ddp_first_comp, i);
+
+	if (drm_crtc_index(crtc) == 0)
+		sec_scn = CMDQ_SEC_PRIMARY_DISP;
+	else if (drm_crtc_index(crtc) == 1)
+		sec_scn = CMDQ_SEC_SUB_DISP;
+
+	cmdq_sec_pkt_set_data(&mtk_crtc->sec_cmdq_handle,
sec_engine,
sec_port, sec_scn);

In cmdq driver, sec_engine means engine which need dapc. You
set
0
to
sec_engine, does it mean that no engine is protected by dapc?
If
OVL
is
not protected by dapc, I think we could use cmdq normal thread
to
write
OVL register instead of cmdq secure thread.

We enable DPAC protection for the engine that is able to write
data
to
the DRAM address set on their register, such as WDMA and WROT, to
avoid
their register being set to the normal DRAM address.

We enable larb port protection for the engine that is able to
read
data
from the DRAM address, such as OVL, RDMA and MDP_RDMA, to avoid
secure
DRAM being read by the non-secure larb port. So we don't need
toenable
DAPC for these engines.

No mater DAPC protection or larb port protection, they both need
to
use
sec_engine to tell TEE which engines need to be protected.

But OVL is a special HW engine, it can only set its
DISP_REG_OVL_SECURE
register in [PATCH v2 07/11] to enable its larb port protection,
so
OVL
no need to set the sec_engine. But we'll move that part to the
TEE
secure world, so that means OVL sec_engine will be set here in
the
next
version.

quoted

It's weird that normal world could decide which engine is
protected
by
dapc. If hacker set 0 for output engine, then outout engine
would
not
be protected?

If hacker set 0 for output engine, TEE world will check that
output
engine didn't set sec_engine from normal world by verifying
instruction
where the output engine instruction set the secure handle.

We still need to set sec_engine to check that all the sec_engine
fags
are matched to the scenario and instruction verification in the
secure
world.

So after secure scenario is set, TEE already have a sec_engine
list.
Let's call it TEE sec_engine list. And normal world has another
sec_engine list, let's call it normal sec_engine list. Normal world
pass normal sec_engine list to TEE by cmdq_sec_pkt_set_data() and
TEE
would check normal sec_engine list is identical to TEE sec_engine
list
or not. If TEE already have a TEE sec_engine list, I think it's not
necessary that normal world have another normal sec_engine list. So
drop this normal sec_engine list parameter.

The TEE sec_engine list is align to the sec_engine list in normal
world. I think the sec_engine for DAPC can be dropped and it can be
enabled/disabled by the TEE sec_engine, but the sec_engine for larb
port won't do that verification in the secure world. If sec_engine
for
larb port is not set in normal world, it'll cause iova translation
fault. So we still need this sec_engine for larb port.

In TEE, there is already a TEE sec_engine list, so checking larb port
could use TEE sec_engine list because TEE sec_engine list would be
identical to normal sec_engine list. Why not use TEE sec_engine list to
check larb port? What is the detail of checking larb port? Describe it
if necessary.

Regards,
CK

Regards,
Jason-JH.Lin

quoted

Regards,
CK

quoted

Regards,
Jason-JH.Lin

quoted

Regards,
CK

quoted

+
+	pr_debug("crtc-%d enable secure plane!\n",
drm_crtc_index(crtc));
+}
+#endif

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel