Hi Chun-Kuang,
On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu <neal.liu@xxxxxxxxxxxx> 於 2020年7月21日 週二 下午12:00寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@xxxxxxxxxxxx>
> > ---
>
> [snip]
>
> > +
> > +static u32 get_shift_group(struct mtk_devapc_context *ctx, u32 vio_idx)
>
> vio_idx is useless, so remove it.
Okay, I'll remove it in next patch.
>
> > +{
> > + u32 vio_shift_sta;
> > + void __iomem *reg;
> > +
> > + reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > + vio_shift_sta = readl(reg);
> > +
> > + if (vio_shift_sta)
> > + return __ffs(vio_shift_sta);
> > +
> > + return 31;
> > +}
> > +
>
> [snip]
>
> > +
> > +/*
> > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > + * debug information.
> > + */
> > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > +{
> > + u32 shift_bit;
> > +
> > + if (check_vio_mask(ctx, vio_idx))
> > + return false;
> > +
> > + if (!check_vio_status(ctx, vio_idx))
> > + return false;
> > +
> > + shift_bit = get_shift_group(ctx, vio_idx);
> > +
> > + if (sync_vio_dbg(ctx, shift_bit))
> > + return false;
> > +
> > + devapc_extract_vio_dbg(ctx);
>
> I think get_shift_group(), sync_vio_dbg(), and
> devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> loop in devapc_violation_irq()) because these three function is not
> related to vio_idx.
> Another question: when multiple vio_idx violation occur, vio_addr is
> related to which one vio_idx? The latest happened one?
>
Actually, it's related to vio_idx. But we don't use it directly on these
function. I think below snip code might be better way to understand it.
for (...)
{
check_vio_mask()
check_vio_status()
// if get vio_idx, mask it temporarily
mask_module_irq(true)
clear_vio_status()
// dump violation info
get_shift_group()
sync_vio_dbg()
devapc_extract_vio_dbg()
// unmask
mask_module_irq(false)
}
About your question, vio_addr would be the first one.
> > +
> > + return true;
> > +}
> > +
> > +/*
> > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > + * violation information including which master violates
> > + * access slave.
> > + */
> > +static irqreturn_t devapc_violation_irq(int irq_number,
> > + struct mtk_devapc_context *ctx)
> > +{
> > + u32 vio_idx;
> > +
> > + for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > + if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > + continue;
> > +
> > + /* Ensure that violation info are written before
> > + * further operations
> > + */
> > + smp_mb();
> > +
> > + /*
> > + * Mask slave's irq before clearing vio status.
> > + * Must do it to avoid nested interrupt and prevent
> > + * unexpected behavior.
> > + */
> > + mask_module_irq(ctx, vio_idx, true);
> > +
> > + clear_vio_status(ctx, vio_idx);
> > +
> > + mask_module_irq(ctx, vio_idx, false);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * start_devapc - initialize devapc status and start receiving interrupt
> > + * while devapc violation is triggered.
> > + */
> > +static int start_devapc(struct mtk_devapc_context *ctx)
> > +{
> > + void __iomem *pd_vio_shift_sta_reg;
> > + void __iomem *pd_apc_con_reg;
> > + u32 vio_shift_sta;
> > + u32 vio_idx;
> > +
> > + pd_apc_con_reg = ctx->devapc_pd_base + ctx->offset->apc_con;
> > + pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > + if (!pd_apc_con_reg || !pd_vio_shift_sta_reg)
> > + return -EINVAL;
> > +
> > + /* Clear devapc violation status */
> > + writel(BIT(31), pd_apc_con_reg);
> > +
> > + /* Clear violation shift status */
> > + vio_shift_sta = readl(pd_vio_shift_sta_reg);
> > + if (vio_shift_sta)
> > + writel(vio_shift_sta, pd_vio_shift_sta_reg);
> > +
> > + /* Clear slave violation status */
> > + for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > + clear_vio_status(ctx, vio_idx);
> > + mask_module_irq(ctx, vio_idx, false);
> > + }
> > +
>
> Why do you clear these? After power on hardware, I think these
> register status are correct. If the default value of these register
> are not correct, add a comment for this.
>
The register default value would be correct after power on.
But there are many things have to do before kernel driver probe.
During that time, devapc register status might be changed. But we are
focusing on handling violation after driver probe instead.
So clearing all reg status to make it as initial state.
> Regards,
> Chun-Kuang.
>
> > + return 0;
> > +}
> > +
