Hi Chun-Kuang,

On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:

Hi, Neal:

Neal Liu [off-list ref] 於 2020年7月31日 週五 上午10:44寫道：

quoted

Hi Chun-Kuang,


On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:

quoted

Hi, Neal:

Neal Liu [off-list ref] 於 2020年7月29日 週三 下午4:29寫道：

quoted

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

[snip]

quoted

+
+static int get_shift_group(struct mtk_devapc_context *ctx)
+{
+       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 -EIO;
+}

get_shift_group() is a small function, I would like to merge this
function into sync_vio_dbg() to make code more simple.

This function have a specific functionality. And it would make this
driver more readability. I would like to keep it as a function. Is that
okay for you?

After merge, the function would be:

static bool sync_min_shift_group_vio_dbg(struct mtk_devapc_context *ctx)
{
 int min_shift_group;
 int ret;
 u32 val;

 /* find the minimum shift group which has violation */
 val = readl(ctx->devapc_pd_base + ctx->offset->vio_shift_sta);
 if (!val)
    return false;

 min_shift_group = __ffs(val);

 /* Assign the group to sync */
 writel(0x1 << min_shift_group, ctx->devapc_pd_base +
ctx->offset->vio_shift_sel);

 /* Start syncing */
 writel(0x1, ctx->devapc_pd_base + ctx->offset->vio_shift_con);

 ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
     PHY_DEVAPC_TIMEOUT);
 if (ret) {
  dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
  return false;
 }

 /* Stop syncing */
 writel(0x0, ctx->devapc_pd_base + ctx->offset->vio_shift_con);

 /* ? */
 writel(0x1 << min_shift_group, ctx->devapc_pd_base +
ctx->offset->vio_shift_sta);

 return true;
}

The whole function is to sync min_shift_group violation info, I don't
know why separate any part to an independent function? Any function
call would cause penalty on CPU performance, so I does not like to
break this function. After good comment, I think every body could
understand the function of each register.
After the merge, the code would be so simple as:

while(sync_min_shift_group_vio_dbg(ctx))
    devapc_extract_vio_dbg(ctx);

Okay, this looks good to me. I'll apply this on next patch.
Thanks !

quoted

quoted

quoted

+

[snip]

quoted

+
+#define PHY_DEVAPC_TIMEOUT     0x10000
+
+/*
+ * sync_vio_dbg - do "shift" mechansim" to get full violation information.
+ *                shift mechanism is depends on devapc hardware design.
+ *                Mediatek devapc set multiple slaves as a group. When violation
+ *                is triggered, violation info is kept inside devapc hardware.
+ *                Driver should do shift mechansim to "shift" full violation
+ *                info to VIO_DBGs registers.
+ *
+ */
+static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
+{
+       void __iomem *pd_vio_shift_sta_reg;
+       void __iomem *pd_vio_shift_sel_reg;
+       void __iomem *pd_vio_shift_con_reg;
+       int ret;
+       u32 val;
+
+       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
+       pd_vio_shift_sel_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sel;
+       pd_vio_shift_con_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_con;
+
+       /* Enable shift mechansim */
+       writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
+       writel(0x1, pd_vio_shift_con_reg);
+
+       ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
+                                PHY_DEVAPC_TIMEOUT);
+       if (ret)
+               dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
+
+       /* Disable shift mechanism */
+       writel(0x0, pd_vio_shift_con_reg);
+       writel(0x0, pd_vio_shift_sel_reg);
+       writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
+
+       return ret;
+}
+

[snip]

quoted

+
+/*
+ * 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?

quoted

+       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 !

quoted

quoted

quoted

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

quoted

quoted

quoted

+
+       devapc_vio_info_print(ctx);
+}
+

[snip]

quoted

+
+/*
+ * 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;
+
+       /*
+        * Mask slave's irq before clearing vio status.
+        * Must do it to avoid nested interrupt and prevent
+        * unexpected behavior.
+        */
+       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
+               mask_module_irq(ctx, vio_idx, true);

I would like to rewrite this for-loop as below to prevent too many
function call in irq handler.

for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->vio_idx_num); i++)
    writel(0xffffffff, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);

This idea is okay for me. Is there any macro to replace 0xffffffff?

GENMASK(31, 0);

quoted

quoted

reg  = readl(ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
reg |= 1 << (ctx->vio_idx_num - 32 * i + 1) - 1;
writel(reg, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);

Are you trying to clear the bits which over vio_idx_num?
If yes, I think the second line should be:
reg &= 1 << (ctx->vio_idx_num - 32 * i) - 1;

For example, if vio_idx_num is 40:
after for loop:
vio_mask0 = 0xffffffff;
vio_mask1 = 0xffffffff;

when vio_idx_num is 40, VIO_MOD_TO_REG_IND(ctx->vio_idx_num) is 1, so
after for-loop:
vio_mask0 = 0xffffffff;

And the code after for-loop just to do this:
vio_mask1 = 0xff;

quoted

reg = readl(vio_mask1);
reg &= (1 << 8) - 1; (which is 0x000000ff)
reg will be 0xff
writel(reg, vio_mask1);

Does it make sense?

Actually, it is okay to overwrite the unused register bits.
So it's no matter to do this step.

OK, the code would be

for (i = 0; i <= VIO_MOD_TO_REG_IND(ctx->vio_idx_num - 1); i++)
    writel(GENMASK(31, 0), ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);

Yes, this is okay for me.
Thanks !

Regards,
Chun-Kuang.

quoted

quoted

quoted

+
+       devapc_dump_vio_dbg(ctx);
+
+       /*
+        * Ensure that violation info are written
+        * before further operations
+        */
+       smp_mb();
+
+       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);
+       }

Ditto for this for-loop.

Ditto.

quoted

quoted

+
+       return IRQ_HANDLED;
+}
+

[snip]

quoted

+
+static int mtk_devapc_probe(struct platform_device *pdev)
+{
+       struct device_node *node = pdev->dev.of_node;
+       struct mtk_devapc_context *ctx;
+       struct clk *devapc_infra_clk;
+       u32 devapc_irq;
+       int ret;
+
+       if (IS_ERR(node))
+               return -ENODEV;
+
+       ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+       if (!ctx)
+               return -ENOMEM;
+
+       ctx = (struct mtk_devapc_context *)of_device_get_match_data(&pdev->dev);
+       ctx->dev = &pdev->dev;
+
+       ctx->vio_info = devm_kzalloc(&pdev->dev,
+                                    sizeof(struct mtk_devapc_vio_info),
+                                    GFP_KERNEL);
+       if (!ctx->vio_info)
+               return -ENOMEM;
+
+       ctx->devapc_pd_base = of_iomap(node, 0);
+       if (!ctx->devapc_pd_base)
+               return -EINVAL;
+
+       devapc_irq = irq_of_parse_and_map(node, 0);
+       if (!devapc_irq)
+               return -EINVAL;
+
+       devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
+       if (IS_ERR(devapc_infra_clk))
+               return -EINVAL;
+
+       if (clk_prepare_enable(devapc_infra_clk))
+               return -EINVAL;
+
+       ret = devm_request_irq(&pdev->dev, devapc_irq,
+                              (irq_handler_t)devapc_violation_irq,
+                              IRQF_TRIGGER_NONE, "devapc", ctx);
+       if (ret)

You should clk_disable_unprepare(devapc_infra_clk);

Yes, I miss this part. Thanks for your remind.
I'll update it on next patch.

quoted

quoted

+               return ret;
+
+       start_devapc(ctx);
+
+       return 0;
+}
+
+static int mtk_devapc_remove(struct platform_device *dev)
+{

Ditto.

Ditto.

quoted

Regards,
Chun-Kuang.

quoted

+       return 0;
+}
+
+static struct platform_driver mtk_devapc_driver = {
+       .probe = mtk_devapc_probe,
+       .remove = mtk_devapc_remove,
+       .driver = {
+               .name = KBUILD_MODNAME,
+               .of_match_table = mtk_devapc_dt_match,
+       },
+};
+
+module_platform_driver(mtk_devapc_driver);
+

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