<pre>
On Tue, 2023-10-31 at 06:01 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
>
> On Mon, 2023-10-23 at 12:45 +0800, Jason-JH.Lin wrote:
> > 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 <jason-jh.lin@mediatek.com>
> > ---
> > 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(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index b6fa4ad2f94d..6c2cf339b923 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -56,6 +56,11 @@ struct mtk_drm_crtc {
> > u32cmdq_event;
> > u32cmdq_vblank_cnt;
> > wait_queue_head_tcb_blocking_queue;
> > +
> > +struct cmdq_clientsec_cmdq_client;
> > +struct cmdq_pktsec_cmdq_handle;
> > +boolsec_cmdq_working;
> > +wait_queue_head_tsec_cb_blocking_queue;
> > #endif
> >
> > struct device*mmsys_dev;
> > @@ -67,6 +72,7 @@ struct mtk_drm_crtc {
> > /* lock for display hardware access */
> > struct mutexhw_lock;
> > boolconfig_updating;
> > +boolsec_on;
> > };
> >
> > struct mtk_crtc_state {
> > @@ -109,6 +115,154 @@ static void mtk_drm_finish_page_flip(struct
> > mtk_drm_crtc *mtk_crtc)
> > }
> > }
> >
> > +void mtk_crtc_disable_secure_state(struct drm_crtc *crtc)
> > +{
> > +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > +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 */
> > +
> > +mutex_lock(&mtk_crtc->hw_lock);
> > +
> > +if (!mtk_crtc->sec_cmdq_client.chan) {
> > +pr_err("crtc-%d secure mbox channel is NULL\n",
> > drm_crtc_index(crtc));
> > +goto err;
> > +}
> > +
> > +if (!mtk_crtc->sec_on) {
> > +pr_debug("crtc-%d is already disabled!\n",
> > drm_crtc_index(crtc));
> > +goto err;
> > +}
> > +
> > +mbox_flush(mtk_crtc->sec_cmdq_client.chan, 0);
> > +mtk_crtc->sec_cmdq_handle.cmd_buf_size = 0;
> > +
> > +if (mtk_crtc->sec_cmdq_handle.sec_data) {
> > +struct cmdq_sec_data *sec_data;
> > +
> > +sec_data = mtk_crtc->sec_cmdq_handle.sec_data;
> > +sec_data->addrMetadataCount = 0;
> > +sec_data->addrMetadatas = (uintptr_t)NULL;
> > +}
> > +
> > +/*
> > + * Secure path only support DL mode, so we just wait
> > + * the first path frame done here
> > + */
> > +cmdq_pkt_wfe(&mtk_crtc->sec_cmdq_handle, mtk_crtc->cmdq_event,
> > false);
> > +
> > +ddp_first_comp = mtk_crtc->ddp_comp[0];
> > +for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > +struct drm_plane *plane = &mtk_crtc->planes[i];
> > +
> > +sec_port |=
> > mtk_ddp_comp_layer_get_sec_port(ddp_first_comp, i);
>
> sec_port is useless, so remove it.
>
> > +
> > +/* make sure secure layer off before switching secure
> > state */
> > +if (!mtk_plane_fb_is_secure(plane->state->fb)) {
> > +struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> > +
> > +plane_state->pending.enable = false;
> > +mtk_ddp_comp_layer_config(ddp_first_comp, i,
> > plane_state,
> > + &mtk_crtc-
> > > sec_cmdq_handle);
> >
> > +}
> > +}
> > +
> > +/* Disable secure path */
> > +if (drm_crtc_index(crtc) == 0)
> > +sec_scn = CMDQ_SEC_PRIMARY_DISP_DISABLE;
> > +else if (drm_crtc_index(crtc) == 1)
> > +sec_scn = CMDQ_SEC_SUB_DISP_DISABLE;
> > +
> > +cmdq_sec_pkt_set_data(&mtk_crtc->sec_cmdq_handle, sec_engine,
> > sec_engine, sec_scn);
> > +
> > +cmdq_pkt_finalize(&mtk_crtc->sec_cmdq_handle);
> > +dma_sync_single_for_device(mtk_crtc->sec_cmdq_client.chan-
> > > mbox->dev,
> >
> > + mtk_crtc->sec_cmdq_handle.pa_base,
> > + mtk_crtc-
> > > sec_cmdq_handle.cmd_buf_size,
> >
> > + DMA_TO_DEVICE);
> > +
> > +mtk_crtc->sec_cmdq_working = true;
> > +mbox_send_message(mtk_crtc->sec_cmdq_client.chan, &mtk_crtc-
> > > sec_cmdq_handle);
> >
> > +mbox_client_txdone(mtk_crtc->sec_cmdq_client.chan, 0);
> > +
> > +// Wait for sec state to be disabled by cmdq
> > +wait_event_timeout(mtk_crtc->sec_cb_blocking_queue,
> > + !mtk_crtc->sec_cmdq_working,
> > + msecs_to_jiffies(500));
> > +
> > +mtk_crtc->sec_on = false;
> > +pr_debug("crtc-%d disable secure plane!\n",
> > drm_crtc_index(crtc));
> > +
> > +err:
> > +mutex_unlock(&mtk_crtc->hw_lock);
> > +#endif
> > +}
> > +
> > +#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->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.

> 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.

Regards,
Jason-JH.Lin

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

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->