Message ID | 20220125085634.17972-1-yong.wu@mediatek.com |
---|---|
Headers | show |
Series | MT8195 IOMMU SUPPORT | expand |
Il 25/01/22 09:56, Yong Wu ha scritto: > In the commit 4f956c97d26b ("iommu/mediatek: Move domain_finalise into > attach_device"), I overlooked the sharing pgtable case. > After that commit, the "data" in the mtk_iommu_domain_finalise always is > the data of the current IOMMU HW. Fix this for the sharing pgtable case. > > Only affect mt2712 which is the only SoC that share pgtable currently. > > Fixes: 4f956c97d26b ("iommu/mediatek: Move domain_finalise into attach_device") > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > Add a mutex to protect the data in the structure mtk_iommu_data, > like ->"m4u_group" ->"m4u_dom". For the internal data, we should > protect it in ourselves driver. Add a mutex for this. > This could be a fix for the multi-groups support. > > Fixes: c3045f39244e ("iommu/mediatek: Support for multi domains") > Signed-off-by: Yunfei Wang <yf.wang@mediatek.com> > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 13 +++++++++++-- > drivers/iommu/mtk_iommu.h | 2 ++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index ec2c387abf60..095736bfb7b4 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -464,15 +464,16 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain, > dom->data = data; > } > > + mutex_lock(&data->mutex); > if (!data->m4u_dom) { /* Initialize the M4U HW */ > ret = pm_runtime_resume_and_get(m4udev); > if (ret < 0) > - return ret; > + goto data_unlock; In order to enhance human readability, I would rather propose: goto err_unlock; Apart from this, Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > ret = mtk_iommu_hw_init(data); > if (ret) { > pm_runtime_put(m4udev); > - return ret; > + goto data_unlock; > } > data->m4u_dom = dom; > writel(dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK, > @@ -480,9 +481,14 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain, > > pm_runtime_put(m4udev); > } > + mutex_unlock(&data->mutex); > > mtk_iommu_config(data, dev, true, domid); > return 0; > + > +data_unlock: > + mutex_unlock(&data->mutex); > + return ret; > } > > static void mtk_iommu_detach_device(struct iommu_domain *domain, > @@ -592,6 +598,7 @@ static struct iommu_group *mtk_iommu_device_group(struct device *dev) > if (domid < 0) > return ERR_PTR(domid); > > + mutex_lock(&data->mutex); > group = data->m4u_group[domid]; > if (!group) { > group = iommu_group_alloc(); > @@ -600,6 +607,7 @@ static struct iommu_group *mtk_iommu_device_group(struct device *dev) > } else { > iommu_group_ref_get(group); > } > + mutex_unlock(&data->mutex); > return group; > } > > @@ -874,6 +882,7 @@ static int mtk_iommu_probe(struct platform_device *pdev) > } > > platform_set_drvdata(pdev, data); > + mutex_init(&data->mutex); > > ret = iommu_device_sysfs_add(&data->iommu, dev, NULL, > "mtk-iommu.%pa", &ioaddr); > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h > index f81fa8862ed0..f413546ac6e5 100644 > --- a/drivers/iommu/mtk_iommu.h > +++ b/drivers/iommu/mtk_iommu.h > @@ -80,6 +80,8 @@ struct mtk_iommu_data { > > struct dma_iommu_mapping *mapping; /* For mtk_iommu_v1.c */ > > + struct mutex mutex; /* Protect m4u_group/m4u_dom above */ > + > struct list_head list; > struct mtk_smi_larb_iommu larb_imu[MTK_LARB_NR_MAX]; > }; >
Il 25/01/22 09:56, Yong Wu ha scritto: > Same with the previous patch, add a mutex for the "data" in the > mtk_iommu_domain. Just improve the safety for multi devices > enter attach_device at the same time. We don't get the real issue > for this. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > No need zero for the protect buffer that is only accessed by the IOMMU HW > translation fault happened. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> I would rather keep this a devm_kzalloc instead... the cost is very minimal and this will be handy when new hardware will be introduced, as it may require a bigger buffer: in that case, "older" platforms will use only part of it and we may get garbage data at the end. Regards, Angelo
Il 25/01/22 09:56, Yong Wu ha scritto: > Currently there is only compare_of/release_of/a suspend structure in the > header file. I think it is no need to keep a header file only for these. > Move these into the c file and rm this header file. > > I think there should be a common helper for compare_of and release_of. > There is many copy in drm, it should be another topic. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 25 ++++++++++++++++++++- > drivers/iommu/mtk_iommu.h | 42 ------------------------------------ > drivers/iommu/mtk_iommu_v1.c | 21 +++++++++++++++--- > 3 files changed, 42 insertions(+), 46 deletions(-) > delete mode 100644 drivers/iommu/mtk_iommu.h > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 80c1e5a75868..f88c7bb235bf 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -14,6 +14,7 @@ > #include <linux/io.h> > #include <linux/iommu.h> > #include <linux/iopoll.h> > +#include <linux/io-pgtable.h> > #include <linux/list.h> > #include <linux/mfd/syscon.h> > #include <linux/module.h> > @@ -30,7 +31,7 @@ > #include <asm/barrier.h> > #include <soc/mediatek/smi.h> > > -#include "mtk_iommu.h" > +#include <dt-bindings/memory/mtk-memory-port.h> > > #define REG_MMU_PT_BASE_ADDR 0x000 > #define MMU_PT_ADDR_MASK GENMASK(31, 7) > @@ -166,6 +167,17 @@ struct mtk_iommu_iova_region { > unsigned long long size; > }; > > +struct mtk_iommu_suspend_reg { > + u32 misc_ctrl; > + u32 dcm_dis; > + u32 ctrl_reg; > + u32 int_control0; > + u32 int_main_control; > + u32 ivrp_paddr; > + u32 vld_pa_rng; > + u32 wr_len_ctrl; > +}; > + > struct mtk_iommu_plat_data { > enum mtk_iommu_plat m4u_plat; > u32 flags; > @@ -219,6 +231,17 @@ struct mtk_iommu_domain { > struct mutex mutex; /* Protect "data" in this structure */ > }; > > +/* TODO: A common helper is expected. */ > +static inline int compare_of(struct device *dev, void *data) > +{ > + return dev->of_node == data; > +} > + > +static inline void release_of(struct device *dev, void *data) > +{ > + of_node_put(data); > +} > + Since it's just one line, at this point you should also open-code these, as in you can then remove the two helper functions entirely. So, please do that. > static inline int mtk_iommu_bind(struct device *dev) > { > struct mtk_iommu_data *data = dev_get_drvdata(dev); > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h > deleted file mode 100644 > index d332f9769f83..000000000000 > --- a/drivers/iommu/mtk_iommu.h > +++ /dev/null > @@ -1,42 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0-only */ > -/* > - * Copyright (c) 2015-2016 MediaTek Inc. > - * Author: Honghui Zhang <honghui.zhang@mediatek.com> > - */ > - > -#ifndef _MTK_IOMMU_H_ > -#define _MTK_IOMMU_H_ > - > -#include <linux/device.h> > -#include <linux/io.h> > -#include <linux/io-pgtable.h> > -#include <linux/iommu.h> > -#include <linux/spinlock.h> > -#include <soc/mediatek/smi.h> > -#include <dt-bindings/memory/mtk-memory-port.h> > - > -struct mtk_iommu_suspend_reg { > - union { > - u32 standard_axi_mode;/* v1 */ > - u32 misc_ctrl;/* v2 */ > - }; > - u32 dcm_dis; > - u32 ctrl_reg; > - u32 int_control0; > - u32 int_main_control; > - u32 ivrp_paddr; > - u32 vld_pa_rng; > - u32 wr_len_ctrl; > -}; > - > -static inline int compare_of(struct device *dev, void *data) > -{ > - return dev->of_node == data; > -} > - > -static inline void release_of(struct device *dev, void *data) > -{ > - of_node_put(data); > -} > - > -#endif > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c > index b762a05328d4..23c3bc175153 100644 > --- a/drivers/iommu/mtk_iommu_v1.c > +++ b/drivers/iommu/mtk_iommu_v1.c > @@ -7,7 +7,6 @@ > * > * Based on driver/iommu/mtk_iommu.c > */ > -#include <linux/memblock.h> > #include <linux/bug.h> > #include <linux/clk.h> > #include <linux/component.h> > @@ -28,10 +27,9 @@ > #include <linux/spinlock.h> > #include <asm/barrier.h> > #include <asm/dma-iommu.h> > -#include <linux/init.h> > +#include <dt-bindings/memory/mtk-memory-port.h> > #include <dt-bindings/memory/mt2701-larb-port.h> > #include <soc/mediatek/smi.h> > -#include "mtk_iommu.h" > > #define REG_MMU_PT_BASE_ADDR 0x000 > > @@ -87,6 +85,13 @@ > */ > #define M2701_IOMMU_PGT_SIZE SZ_4M > > +struct mtk_iommu_suspend_reg { > + u32 standard_axi_mode; > + u32 dcm_dis; > + u32 ctrl_reg; > + u32 int_control0; > +}; > + > struct mtk_iommu_data { > void __iomem *base; > int irq; > @@ -110,6 +115,16 @@ struct mtk_iommu_domain { > struct mtk_iommu_data *data; > }; > > +static inline int compare_of(struct device *dev, void *data) > +{ > + return dev->of_node == data; > +} > + > +static inline void release_of(struct device *dev, void *data) > +{ > + of_node_put(data); > +} > + ....And the same comment applies here too. > static inline int mtk_iommu_bind(struct device *dev) > { > struct mtk_iommu_data *data = dev_get_drvdata(dev); >
Il 25/01/22 09:56, Yong Wu ha scritto: > Prepare for supporting multi-banks for the IOMMU HW, No functional change. > > Add a new structure(mtk_iommu_bank_data) for each a bank. Each a bank have > the independent HW base/IRQ/tlb-range ops, and each a bank has its special > iommu-domain(independent pgtable), thus, also move the domain information > into it. > > In previous SoC, we have only one bank which could be treated as bank0( > bankid always is 0 for the previous SoC). > > After adding this structure, the tlb operations and irq could use > bank_data as parameter. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > We preassign some ports in a special bank via the new defined > banks_portmsk. Put it in the plat_data means it is not expected to be > adjusted dynamically. > > If the iommu id in the iommu consumer's dtsi node is inside this > banks_portmsk, then we switch it to this special iommu bank, and > initialise the IOMMU bank HW. > > Each a bank has the independent pgtable(4GB iova range). Each a bank > is a independent iommu domain/group. Currently we don't separate different > iova ranges inside a bank. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 39 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 22586d1aed72..c6de9304bbc6 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -191,6 +191,7 @@ struct mtk_iommu_plat_data { > > u8 banks_num; > bool banks_enable[MTK_IOMMU_BANK_MAX]; > + unsigned int banks_portmsk[MTK_IOMMU_BANK_MAX]; I would prefer to see u32 here instead... but maybe that's just a personal preference, it doesn't really matter. In any case: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > Each bank has some independent registers. thus backup/restore them for > each a bank when suspend and resume. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > Enable the multi-bank functions for infra-iommu. We put PCIE in bank0 > and USB in the last bank(bank4). and we don't use the other banks > currently, disable them. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Thu, 2022-01-27 at 12:08 +0100, AngeloGioacchino Del Regno wrote: > Il 25/01/22 09:56, Yong Wu ha scritto: > > No need zero for the protect buffer that is only accessed by the > > IOMMU HW > > translation fault happened. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > I would rather keep this a devm_kzalloc instead... the cost is very > minimal and > this will be handy when new hardware will be introduced, as it may > require a bigger > buffer: in that case, "older" platforms will use only part of it and > we may get > garbage data at the end. Currently this is to avoid zero 512 bytes for all the platforms. Sorry, I don't understand why it is unnecessary when the new hardware requires a bigger buffer. If the buffer becomes bigger, then clearing it to 0 need more cost. then this patch is more helpful? The content in this buffer is garbage, we won't care about or analyse it. > > Regards, > Angelo
On Wed, Feb 16, 2022 at 2:55 PM Yong Wu <yong.wu@mediatek.com> wrote: > > On Thu, 2022-01-27 at 12:08 +0100, AngeloGioacchino Del Regno wrote: > > Il 25/01/22 09:56, Yong Wu ha scritto: > > > No need zero for the protect buffer that is only accessed by the > > > IOMMU HW > > > translation fault happened. > > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > > > I would rather keep this a devm_kzalloc instead... the cost is very > > minimal and > > this will be handy when new hardware will be introduced, as it may > > require a bigger > > buffer: in that case, "older" platforms will use only part of it and > > we may get > > garbage data at the end. > > Currently this is to avoid zero 512 bytes for all the platforms. > > Sorry, I don't understand why it is unnecessary when the new hardware > requires a bigger buffer. If the buffer becomes bigger, then clearing > it to 0 need more cost. then this patch is more helpful? > > The content in this buffer is garbage, we won't care about or analyse > it. I think we should zero it for security reasons regardless of any other aspects. With this patch it's leaking kernel data to the hardware. At the same time, we're talking here about something executed just 1 time when the driver probes. I don't think the cost would really matter. Best regards, Tomasz
On Wed, 2022-02-16 at 14:59 +0900, Tomasz Figa wrote: > On Wed, Feb 16, 2022 at 2:55 PM Yong Wu <yong.wu@mediatek.com> wrote: > > > > On Thu, 2022-01-27 at 12:08 +0100, AngeloGioacchino Del Regno > > wrote: > > > Il 25/01/22 09:56, Yong Wu ha scritto: > > > > No need zero for the protect buffer that is only accessed by > > > > the > > > > IOMMU HW > > > > translation fault happened. > > > > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > > > > > I would rather keep this a devm_kzalloc instead... the cost is > > > very > > > minimal and > > > this will be handy when new hardware will be introduced, as it > > > may > > > require a bigger > > > buffer: in that case, "older" platforms will use only part of it > > > and > > > we may get > > > garbage data at the end. > > > > Currently this is to avoid zero 512 bytes for all the platforms. > > > > Sorry, I don't understand why it is unnecessary when the new > > hardware > > requires a bigger buffer. If the buffer becomes bigger, then > > clearing > > it to 0 need more cost. then this patch is more helpful? > > > > The content in this buffer is garbage, we won't care about or > > analyse > > it. > > I think we should zero it for security reasons regardless of any > other > aspects. With this patch it's leaking kernel data to the hardware. > > At the same time, we're talking here about something executed just 1 > time when the driver probes. I don't think the cost would really > matter. OK. I will remove this patch in next version. Thanks. > > Best regards, > Tomasz > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek