Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

On Tue, 2020-08-04 at 00:13 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@xxxxxxxxxxxx> 於 2020年8月3日 週一 上午11:32寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <neal.liu@xxxxxxxxxxxx> 於 2020年7月31日 週五 上午10:44寫道：
> > > >
> > > > Hi Chun-Kuang,
> > > >
> > > >
> > > > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > > > > Hi, Neal:
> > > > >
> > > > > Neal Liu <neal.liu@xxxxxxxxxxxx> 於 2020年7月29日 週三 下午4:29寫道：
> > > > > >
> > > > > > 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]
> > > > >
> > > > > > +
> > > > > > +/*
> > > > > > + * devapc_extract_vio_dbg - extract full violation information after doing
> > > > > > + *                          shift mechanism.
> > > > > > + */
> > > > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > > > > +{
> > > > > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > > > +       struct mtk_devapc_vio_info *vio_info;
> > > > > > +       void __iomem *vio_dbg0_reg;
> > > > > > +       void __iomem *vio_dbg1_reg;
> > > > > > +       u32 dbg0;
> > > > > > +
> > > > > > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > > > > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > > > > +
> > > > > > +       vio_dbgs = ctx->vio_dbgs;
> > > > > > +       vio_info = ctx->vio_info;
> > > > > > +
> > > > > > +       /* Starts to extract violation information */
> > > > > > +       dbg0 = readl(vio_dbg0_reg);
> > > > > > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > > > > > +
> > > > > > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > > > > > +                             vio_dbgs->mstid.start;
> > > > >
> > > > > What is master_id? How could we use it to debug? For example, if we
> > > > > get a master_id = 1, what should we do for this?
> > > > >
> > > > > > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > > > +                             vio_dbgs->dmnid.start;
> > > > >
> > > > > What is domain_id? How could we use it to debug? For example, if we
> > > > > get a domain_id = 2, what should we do for this?
> > > > >
> > > >
> > > > master_id and domain_id belongs our bus side-band signal info. It can
> > > > help us to find the violation master.
> > >
> > > Does 'violation master' means the hardware could access the protected
> > > register? (ex. CPU, GCE, ...) If so, I think it's better to add
> > > comment to explain how to map (master_id, domain_id) to a hardware
> > > (maybe the device in device tree) because every body does not know
> > > what the number means. Don't try to translate the number to a string
> > > because this would cost much time to do this. Just print a number and
> > > we could find out the master by the comment.
> >
> > 'violation master' means the master which violates the permission
> > control. For example, if we set permission 'Secure R/W only' as CPU to
> > spi register. When violation is triggered, it means CPU access spi
> > register through normal world instead of secure world, which is not
> > allowed.
> >
> > 'master_id' cannot use the simple comments to describe which master it
> > is. It depends on violation slaves. For example, if there are two
> > violations:
> > 1. CPU access spi reg
> > 2. CPU access timer reg
> > It might be different 'master_id' for CPU on these two cases.
> > I would prefer to remain the id number if translate to a string is a bad
> > idea.
> > Thanks !
> 
> It seams that master_id and domain_id does not help for debug. When we
> get master_id = 1 and domain_id = 2, we don't know what it mean. I
> think we just need violation address because we could find the driver
> that write this address and the bug would be inside this driver. So
> need not to process master_id and domain_id.
> 

Actually, it does help us for debug. violation master is not CPU only.
It might be any other master in our SoC. So the bug might not be inside
the kernel driver.
I'll prefer to remain this information.
Thanks !

> Regards,
> Chun-Kuang.
> 
> >
> > >
> > > >
> > > > > > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > > > > +                           vio_dbgs->vio_w.start) == 1;
> > > > > > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > > > > +                         vio_dbgs->vio_r.start) == 1;
> > > > > > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > > > > +                                 vio_dbgs->addr_h.start;
> > > > >
> > > > > What is vio_addr_high? As I know all register address are 32 bits, is
> > > > > vio_addr_high the address above 32 bits?
> > > >
> > > > Yes, you are right. In MT6779, all register base are 32 bits. We can
> > > > ignore this info for current driver. I'll update on next patch.
> > > > Thanks !
> > >
> > > Such a strange hardware, all register is 32 bits but it has a
> > > vio_addr_high in its register. OK, just drop this.
> > >
> > > >
> > > > >
> > > > > > +
> > > > > > +       devapc_vio_info_print(ctx);
> > > > > > +}
> > > > > > +
> > > > >