[permalink] [raw]

Subject: Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver

Hi Chun-Kuang,

On Fri, 2020-06-12 at 23:27 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu <[email protected]> 於 2020年6月12日週五上午11:04寫道：
> >
> > Hi Chun-Kuang,
> >
> > [snip]
> > > > > > +/*
> > > > > > + * 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, void *dev_id)
> > > > > > +{
> > > > > > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > > > > > + const struct mtk_device_info **device_info;
> > > > > > + struct mtk_devapc_vio_info *vio_info;
> > > > > > + int slave_type, vio_idx, index;
> > > > > > + const char *vio_master;
> > > > > > + unsigned long flags;
> > > > > > + bool normal;
> > > > > > + u8 perm;
> > > > > > +
> > > > > > + spin_lock_irqsave(&devapc_lock, flags);
> > > > > > +
> > > > > > + device_info = mtk_devapc_ctx->soc->device_info;
> > > > > > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > > > > > + normal = false;
> > > > > > + vio_idx = -1;
> > > > > > + index = -1;
> > > > > > +
> > > > > > + /* There are multiple DEVAPC_PD */
> > > > > > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > > > > > + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> > > > > > + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> > > > > > + &index))
> > > > > > + continue;
> > > > > > +
> > > > > > + /* Ensure that violation info are written before
> > > > > > + * further operations
> > > > > > + */
> > > > > > + smp_mb();
> > > > > > + normal = true;
> > > > > > +
> > > > > > + mask_module_irq(slave_type, vio_idx, true);
> > > > > > +
> > > > > > + if (clear_vio_status(slave_type, vio_idx))
> > > > > > + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> > > > > > + "clear vio status failed",
> > > > > > + "slave_type", slave_type,
> > > > > > + "vio_index", vio_idx);
> > > > > > +
> > > > > > + perm = get_permission(slave_type, index, vio_info->domain_id);
> > > > > > +
> > > > > > + vio_master = mtk_devapc_ctx->soc->master_get
> > > > > > + (vio_info->master_id,
> > > > > > + vio_info->vio_addr,
> > > > > > + slave_type,
> > > > > > + vio_info->shift_sta_bit,
> > > > > > + vio_info->domain_id);
> > > > >
> > > > > Call mt6873_bus_id_to_master() directly. For first patch, make things
> > > > > as simple as possible.
> > > >
> > > > In devapc_violation_irq() function, we use common flow to handle each
> > > > devapc violation on different platforms. The master_get() has different
> > > > implementation on different platforms, that why it called indirectly.
> > > >
> > > > Once we have new platform, we only have to update devapc-mtxxxx.c
> > > > instead of common handler flow.
> > >
> > > You just upstream one SoC now, so I have no information of 2nd SoC.
> > > Without the 2nd SoC, how do we know what is common and what is SoC special?
> > > So the first patch should not consider the things which does not exist yet.
> > >
> > > Regards,
> > > Chun-Kuang.
> > >
> >
> > It has lots of refactoring work need to do if you really want make it
> > "simple". Could I explain more details and let you judge it is simple
> > enough?
>
> Making driver "simple" is very important, so it worth to spend effort
> to make things simple. Everybody could modify this driver, so make
> this driver simple and everybody would join this easily.
>
> > For most MediaTek DEVAPC hw, the violation interrupt handling sequence
> > is shown below.
> >
> > 1. Domain processor receives a interrupt issued by DEVAPC.
> > 2. Software read the violation status and identify it.
> > 3. Software read the debug information which are stored in hw register.
> > a. debug information includes master ID, domain ID, violation
> > address, ...
> > 4. Transfer debug information to human readable strings.
> > 5. Extra handler to dispatch owner directly.
>
> I don't know why need extra handler? What does this extra handler could do?
> If indeed need it, separate extra handler part to an independent patch.

Yes, You are right. I'll make this driver more simple and let everybody
can join it easily. This extra handler may not be necessary for whom try
to enable this driver.

> >
> > What we really care is which master violates the rules, and which slave
> > had been accessed unexpectedly.
> >
> > Here are platform specific information:
> > 1. Slaves layout (platform devices)
> > 2. hw register layout which are stored violation information
> > 3. Master ID mapping table
> > 4. Domain ID mapping table
> >
> > Hope these steps could help you understand what is common and what is
> > SoC specific. If you want to see the 2nd SoC's driver, I can also send
> > it for you to take a look.
>
> Please upstream 2nd SoC's driver, so I could review common part and
> SoC specific part.
>
> Regards,
> Chun-Kuang.
>
> >
> > Thanks,
> > Neal
> >
> > > >
> > > > >
> > > > > > +
> > > > > > + if (!vio_master) {
> > > > > > + pr_warn(PFX "master_get failed\n");
> > > > > > + vio_master = "UNKNOWN_MASTER";
> > > > > > + }
> > > > > > +
> > > > > > + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> > > > > > + "Violation", "slave_type", slave_type,
> > > > > > + "sys_index",
> > > > > > + device_info[slave_type][index].sys_index,
> > > > > > + "ctrl_index",
> > > > > > + device_info[slave_type][index].ctrl_index,
> > > > > > + "vio_index",
> > > > > > + device_info[slave_type][index].vio_index);
> > > > > > +
> > > > > > + pr_info(PFX "%s %s %s %s\n",
> > > > > > + "Violation - master:", vio_master,
> > > > > > + "access violation slave:",
> > > > > > + device_info[slave_type][index].device);
> > > > > > +
> > > > > > + devapc_vio_reason(perm);
> > > > > > +
> > > > > > + devapc_extra_handler(slave_type, vio_master, vio_idx,
> > > > > > + vio_info->vio_addr);
> > > > > > +
> > > > > > + mask_module_irq(slave_type, vio_idx, false);
> > > > > > + }
> > > > > > +
> > > > > > + if (normal) {
> > > > > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > > > > + return IRQ_HANDLED;
> > > > > > + }
> > > > > > +
> > > > > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > > > > + return IRQ_HANDLED;
> > > > > > +}
> > > > > > +
> > > >
> > > > [snip]
> > > >
> > > >
> >

2020-06-15 02:46:48

by Neal Liu

[permalink] [raw]

Subject: Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver

Hi Chun-Kuang,

On Sun, 2020-06-14 at 11:26 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu <[email protected]> 於 2020年6月9日週二下午6:25寫道：
> >
> > MT6873 bus frabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violations are 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 devapc-mt6873 driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <[email protected]>
> > ---
>
> [snip]
>
> > +
> > + /* 50 */
> > + {-1, -1, 50, "OOB_way_en", true},
> > + {-1, -1, 51, "OOB_way_en", true},
> > + {-1, -1, 52, "OOB_way_en", true},
> > + {-1, -1, 53, "OOB_way_en", true},
> > + {-1, -1, 54, "OOB_way_en", true},
> > + {-1, -1, 55, "OOB_way_en", true},
> > + {-1, -1, 56, "Decode_error", true},
> > + {-1, -1, 57, "Decode_error", true},
> > + {-1, -1, 58, "DISP_PWM", false},
> > + {-1, -1, 59, "IMP_IIC_WRAP", false},
> > +
> > + /* 60 */
> > + {-1, -1, 60, "DEVICE_APC_PERI_PAR__AO", false},
> > + {-1, -1, 61, "DEVICE_APC_PERI_PAR_PDN", false},
>
> You does not process the item whose enable_vio_irq is false, so I
> think you should remove these items and remove enable_vio_irq because
> the rest item's enable_vio_irq would always be true.

In some users, they can decide which slaves they want to enable or
disable violation irq in different product. We remain this property for
compatibility.

>
> > +};
> > +
> > +static struct mtk_device_num mtk6873_devices_num[] = {
> > + {SLAVE_TYPE_INFRA, VIO_SLAVE_NUM_INFRA},
> > + {SLAVE_TYPE_PERI, VIO_SLAVE_NUM_PERI},
> > + {SLAVE_TYPE_PERI2, VIO_SLAVE_NUM_PERI2},
> > + {SLAVE_TYPE_PERI_PAR, VIO_SLAVE_NUM_PERI_PAR},
> > +};
> > +
> > +static struct PERIAXI_ID_INFO peri_mi_id_to_master[] = {
> > + {"THERM2", { 0, 0, 0 } },
> > + {"SPM", { 0, 1, 0 } },
> > + {"CCU", { 0, 0, 1 } },
> > + {"THERM", { 0, 1, 1 } },
> > + {"SPM_DRAMC", { 1, 1, 0 } },
>
> The bits { 1, 1, 0 } equal to a number 0x3, I thiink you should use a
> number instead of bits and everything would be more easy.

We would like to keep it because the bit value contains more than 0/1,
it might be '2' in some cases. '2' means it can be 0 or 1. This totally
by hardware design & implementation.

> > +};
> > +
>
> [snip]
>
> > +
> > +/*
> > + * mtk_devapc_vio_check - check violation shift status is raised or not.
> > + *
> > + * Returns the value of violation shift status reg
> > + */
> > +static void mtk_devapc_vio_check(int slave_type, int *shift_bit)
> > +{
> > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > + struct mtk_devapc_vio_info *vio_info;
> > + u32 vio_shift_sta;
> > + int i;
> > +
> > + if (slave_type >= slave_type_num) {
>
> This never happen, so remove it.

Indeed, thanks

>
> > + pr_err(PFX "%s: param check failed, %s:0x%x\n",
> > + __func__, "slave_type", slave_type);
> > + return;
> > + }
> > +
> > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > + vio_shift_sta = readl(mtk_devapc_pd_get(slave_type, VIO_SHIFT_STA, 0));
> > +
> > + if (!vio_shift_sta) {
> > + pr_info(PFX "violation is triggered before. %s:0x%x\n",
> > + "shift_bit", *shift_bit);
> > +
> > + } else if (vio_shift_sta & (0x1UL << *shift_bit)) {
> > + pr_debug(PFX "%s: 0x%x is matched with %s:%d\n",
> > + "vio_shift_sta", vio_shift_sta,
> > + "shift_bit", *shift_bit);
> > +
> > + } else {
> > + pr_info(PFX "%s: 0x%x is not matched with %s:%d\n",
> > + "vio_shift_sta", vio_shift_sta,
> > + "shift_bit", *shift_bit);
> > +
> > + for (i = 0; i < MOD_NO_IN_1_DEVAPC * 2; i++) {
> > + if (vio_shift_sta & (0x1 << i)) {
> > + *shift_bit = i;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + vio_info->shift_sta_bit = *shift_bit;
> > +}
> > +
> > +static void devapc_extract_vio_dbg(int slave_type)
> > +{
> > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > + void __iomem *vio_dbg0_reg, *vio_dbg1_reg, *vio_dbg2_reg;
> > + const struct mtk_infra_vio_dbg_desc *vio_dbgs;
> > + struct mtk_devapc_vio_info *vio_info;
> > + u32 dbg0;
> > +
> > + if (slave_type >= slave_type_num) {
>
> Ditto.

Indeed, thanks

>
> Regards,
> Chun-Kuang.
>
> > + pr_err(PFX "%s: param check failed, %s:0x%x\n",
> > + __func__, "slave_type", slave_type);
> > + return;
> > + }
> > +
> > + vio_dbg0_reg = mtk_devapc_pd_get(slave_type, VIO_DBG0, 0);
> > + vio_dbg1_reg = mtk_devapc_pd_get(slave_type, VIO_DBG1, 0);
> > + vio_dbg2_reg = mtk_devapc_pd_get(slave_type, VIO_DBG2, 0);
> > +
> > + vio_dbgs = mtk_devapc_ctx->soc->vio_dbgs;
> > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > +
> > + /* Extract violation information */
> > + dbg0 = readl(vio_dbg0_reg);
> > + vio_info->master_id = readl(vio_dbg1_reg);
> > + vio_info->vio_addr = readl(vio_dbg2_reg);
> > +
> > + vio_info->domain_id = (dbg0 & vio_dbgs->vio_dbg_dmnid)
> > + >> vio_dbgs->vio_dbg_dmnid_start_bit;
> > + vio_info->write = ((dbg0 & vio_dbgs->vio_dbg_w_vio)
> > + >> vio_dbgs->vio_dbg_w_vio_start_bit) == 1;
> > + vio_info->read = ((dbg0 & vio_dbgs->vio_dbg_r_vio)
> > + >> vio_dbgs->vio_dbg_r_vio_start_bit) == 1;
> > + vio_info->vio_addr_high = (dbg0 & vio_dbgs->vio_addr_high)
> > + >> vio_dbgs->vio_addr_high_start_bit;
> > +
> > + devapc_vio_info_print();
> > +}
> > +
> > +/*

2020-06-15 14:17:09

by Chun-Kuang Hu

[permalink] [raw]

Subject: Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver

Hi, Neal:

Neal Liu <[email protected]> 於 2020年6月15日週一上午10:43寫道：
>
> Hi Chun-Kuang,
>
>
> On Sun, 2020-06-14 at 11:26 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[email protected]> 於 2020年6月9日週二下午6:25寫道：
> > >
> > > MT6873 bus frabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violations are 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 devapc-mt6873 driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu <[email protected]>
> > > ---
> >
> > [snip]
> >
> > > +
> > > + /* 50 */
> > > + {-1, -1, 50, "OOB_way_en", true},
> > > + {-1, -1, 51, "OOB_way_en", true},
> > > + {-1, -1, 52, "OOB_way_en", true},
> > > + {-1, -1, 53, "OOB_way_en", true},
> > > + {-1, -1, 54, "OOB_way_en", true},
> > > + {-1, -1, 55, "OOB_way_en", true},
> > > + {-1, -1, 56, "Decode_error", true},
> > > + {-1, -1, 57, "Decode_error", true},
> > > + {-1, -1, 58, "DISP_PWM", false},
> > > + {-1, -1, 59, "IMP_IIC_WRAP", false},
> > > +
> > > + /* 60 */
> > > + {-1, -1, 60, "DEVICE_APC_PERI_PAR__AO", false},
> > > + {-1, -1, 61, "DEVICE_APC_PERI_PAR_PDN", false},
> >
> > You does not process the item whose enable_vio_irq is false, so I
> > think you should remove these items and remove enable_vio_irq because
> > the rest item's enable_vio_irq would always be true.
>
> In some users, they can decide which slaves they want to enable or
> disable violation irq in different product. We remain this property for
> compatibility.

I think in upstream version, you could still remove enable_vio_irq and
process all items. For downstream case, users could remove items they
does not interest in.

>
> >
> > > +};
> > > +
> > > +static struct mtk_device_num mtk6873_devices_num[] = {
> > > + {SLAVE_TYPE_INFRA, VIO_SLAVE_NUM_INFRA},
> > > + {SLAVE_TYPE_PERI, VIO_SLAVE_NUM_PERI},
> > > + {SLAVE_TYPE_PERI2, VIO_SLAVE_NUM_PERI2},
> > > + {SLAVE_TYPE_PERI_PAR, VIO_SLAVE_NUM_PERI_PAR},
> > > +};
> > > +
> > > +static struct PERIAXI_ID_INFO peri_mi_id_to_master[] = {
> > > + {"THERM2", { 0, 0, 0 } },
> > > + {"SPM", { 0, 1, 0 } },
> > > + {"CCU", { 0, 0, 1 } },
> > > + {"THERM", { 0, 1, 1 } },
> > > + {"SPM_DRAMC", { 1, 1, 0 } },
> >
> > The bits { 1, 1, 0 } equal to a number 0x3, I thiink you should use a
> > number instead of bits and everything would be more easy.
>
> We would like to keep it because the bit value contains more than 0/1,
> it might be '2' in some cases. '2' means it can be 0 or 1. This totally
> by hardware design & implementation.

Upstream the patch that has dont-care-bits, otherwise, use a number for this.
Even there is dont-care-bits, I have a better way to process it. Here
is an example that has dont-care-bits:

+ {"Tinysys", { 0, 1, 0, 0, 2, 2, 2, 2, 2, 2, 0, 0, 0, 0 } },

I could use a { value, mask } pair for this case,

value = 0x0002; /* value for care bits */
mask = 0x3c02; /* mask for care bits */

So the compare statement would be

if ((bus_id & mask) == value)

So you could get rid of the second for-loop and reduce the processing
time in irq handler.

Regards,
Chun-Kuang.

2020-06-15 14:20:22

by Chun-Kuang Hu

[permalink] [raw]

Subject: Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver

Chun-Kuang Hu <[email protected]> 於 2020年6月15日週一下午10:14寫道：
>
> Hi, Neal:
>
> Neal Liu <[email protected]> 於 2020年6月15日週一上午10:43寫道：
> >
> > Hi Chun-Kuang,
> >
> >
> > On Sun, 2020-06-14 at 11:26 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <[email protected]> 於 2020年6月9日週二下午6:25寫道：
> > > >
> > > > MT6873 bus frabric provides TrustZone security support and data
> > > > protection to prevent slaves from being accessed by unexpected
> > > > masters.
> > > > The security violations are 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 devapc-mt6873 driver. The violation
> > > > information is printed in order to find the murderer.
> > > >
> > > > Signed-off-by: Neal Liu <[email protected]>
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > + /* 50 */
> > > > + {-1, -1, 50, "OOB_way_en", true},
> > > > + {-1, -1, 51, "OOB_way_en", true},
> > > > + {-1, -1, 52, "OOB_way_en", true},
> > > > + {-1, -1, 53, "OOB_way_en", true},
> > > > + {-1, -1, 54, "OOB_way_en", true},
> > > > + {-1, -1, 55, "OOB_way_en", true},
> > > > + {-1, -1, 56, "Decode_error", true},
> > > > + {-1, -1, 57, "Decode_error", true},
> > > > + {-1, -1, 58, "DISP_PWM", false},
> > > > + {-1, -1, 59, "IMP_IIC_WRAP", false},
> > > > +
> > > > + /* 60 */
> > > > + {-1, -1, 60, "DEVICE_APC_PERI_PAR__AO", false},
> > > > + {-1, -1, 61, "DEVICE_APC_PERI_PAR_PDN", false},
> > >
> > > You does not process the item whose enable_vio_irq is false, so I
> > > think you should remove these items and remove enable_vio_irq because
> > > the rest item's enable_vio_irq would always be true.
> >
> > In some users, they can decide which slaves they want to enable or
> > disable violation irq in different product. We remain this property for
> > compatibility.
>
> I think in upstream version, you could still remove enable_vio_irq and
> process all items. For downstream case, users could remove items they
> does not interest in.
>
> >
> > >
> > > > +};
> > > > +
> > > > +static struct mtk_device_num mtk6873_devices_num[] = {
> > > > + {SLAVE_TYPE_INFRA, VIO_SLAVE_NUM_INFRA},
> > > > + {SLAVE_TYPE_PERI, VIO_SLAVE_NUM_PERI},
> > > > + {SLAVE_TYPE_PERI2, VIO_SLAVE_NUM_PERI2},
> > > > + {SLAVE_TYPE_PERI_PAR, VIO_SLAVE_NUM_PERI_PAR},
> > > > +};
> > > > +
> > > > +static struct PERIAXI_ID_INFO peri_mi_id_to_master[] = {
> > > > + {"THERM2", { 0, 0, 0 } },
> > > > + {"SPM", { 0, 1, 0 } },
> > > > + {"CCU", { 0, 0, 1 } },
> > > > + {"THERM", { 0, 1, 1 } },
> > > > + {"SPM_DRAMC", { 1, 1, 0 } },
> > >
> > > The bits { 1, 1, 0 } equal to a number 0x3, I thiink you should use a
> > > number instead of bits and everything would be more easy.
> >
> > We would like to keep it because the bit value contains more than 0/1,
> > it might be '2' in some cases. '2' means it can be 0 or 1. This totally
> > by hardware design & implementation.
>
> Upstream the patch that has dont-care-bits, otherwise, use a number for this.
> Even there is dont-care-bits, I have a better way to process it. Here
> is an example that has dont-care-bits:
>
> + {"Tinysys", { 0, 1, 0, 0, 2, 2, 2, 2, 2, 2, 0, 0, 0, 0 } },
>
> I could use a { value, mask } pair for this case,
>
> value = 0x0002; /* value for care bits */
> mask = 0x3c02; /* mask for care bits */

Sorry, this would be

mask = 0x3c0f; /* mask for care bits */

>
> So the compare statement would be
>
> if ((bus_id & mask) == value)
>
> So you could get rid of the second for-loop and reduce the processing
> time in irq handler.
>
> Regards,
> Chun-Kuang.

2020-06-15 15:54:16

by Chun-Kuang Hu

[permalink] [raw]

On Tue, 2020-06-09 at 11:32 -0600, Rob Herring wrote:
> On Tue, Jun 09, 2020 at 06:24:19PM +0800, Neal Liu wrote:
> > These patch series introduce a MediaTek MT6873 devapc driver.
> >
> > MT6873 bus frabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violations are 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 devapc-mt6873 driver. The violation
> > information is printed in order to find the murderer.
>
> There's also a proposed driver with similar functionality[1]. Come up
> with a common solution.
>
> Rob
>
> [1] https://lore.kernel.org/linux-arm-kernel/[email protected]/

Actually, Mediatek devapc HW do the similar things. But the real
"firewall" functionality is implemented in TrustZone instread of REE.
This devapc-mt6873 driver is mainly handled the violation.

Bus firewall framework seems not cover this parts.