Re: [PATCH v8 3/3] iommu/mediatek: Allow page table PA up to 35bit
> Single memory zone feature will remove ZONE_DMA32 and ZONE_DMA. So add
> the quirk IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT to let level 1 and level 2
> pgtable support at most 35bit PA.
>
> Signed-off-by: Ning Li
> Signed-off-by: Yunfei Wang
Reviewed-by: Miles Chen
> ---
> drivers/iommu/mtk_iommu.c | 14 +-
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 3d62399e8865..4dbc33758711 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -138,6 +138,7 @@
> /* PM and clock always on. e.g. infra iommu */
> #define PM_CLK_AOBIT(15)
> #define IFA_IOMMU_PCIE_SUPPORT BIT(16)
> +#define PGTABLE_PA_35_EN BIT(17)
>
> #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask) \
> pdata)->flags) & (mask)) == (_x))
> @@ -240,6 +241,7 @@ struct mtk_iommu_data {
> struct mtk_iommu_domain {
> struct io_pgtable_cfg cfg;
> struct io_pgtable_ops *iop;
> + u32 ttbr;
>
> struct mtk_iommu_bank_data *bank;
> struct iommu_domain domain;
> @@ -596,6 +598,9 @@ static int mtk_iommu_domain_finalise(struct
> mtk_iommu_domain *dom,
> .iommu_dev = data->dev,
> };
>
> + if (MTK_IOMMU_HAS_FLAG(data->plat_data, PGTABLE_PA_35_EN))
> + dom->cfg.quirks |= IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT;
> +
> if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE))
> dom->cfg.oas = data->enable_4GB ? 33 : 32;
> else
> @@ -684,8 +689,8 @@ static int mtk_iommu_attach_device(struct iommu_domain
> *domain,
> goto err_unlock;
> }
> bank->m4u_dom = dom;
> - writel(dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK,
> -bank->base + REG_MMU_PT_BASE_ADDR);
> + bank->m4u_dom->ttbr = MTK_IOMMU_ADDR(dom->cfg.arm_v7s_cfg.ttbr);
> + writel(bank->m4u_dom->ttbr, data->base + REG_MMU_PT_BASE_ADDR);
>
> pm_runtime_put(m4udev);
> }
> @@ -1366,8 +1371,7 @@ static int __maybe_unused
> mtk_iommu_runtime_resume(struct device *dev)
> writel_relaxed(reg->int_control[i], base +
> REG_MMU_INT_CONTROL0);
> writel_relaxed(reg->int_main_control[i], base +
> REG_MMU_INT_MAIN_CONTROL);
> writel_relaxed(reg->ivrp_paddr[i], base + REG_MMU_IVRP_PADDR);
> - writel(m4u_dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK,
> -base + REG_MMU_PT_BASE_ADDR);
> + writel(m4u_dom->ttbr, base + REG_MMU_PT_BASE_ADDR);
> } while (++i < data->plat_data->banks_num);
>
> /*
> @@ -1401,7 +1405,7 @@ static const struct mtk_iommu_plat_data mt2712_data = {
> static const struct mtk_iommu_plat_data mt6779_data = {
> .m4u_plat = M4U_MT6779,
> .flags = HAS_SUB_COMM_2BITS | OUT_ORDER_WR_EN | WR_THROT_EN |
> - MTK_IOMMU_TYPE_MM,
> + MTK_IOMMU_TYPE_MM | PGTABLE_PA_35_EN,
> .inv_sel_reg = REG_MMU_INV_SEL_GEN2,
> .banks_num= 1,
> .banks_enable = {true},
> --
> 2.18.0
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 2/3] iommu/mediatek: Rename MTK_IOMMU_TLB_ADDR to MTK_IOMMU_ADDR
> Rename MTK_IOMMU_TLB_ADDR to MTK_IOMMU_ADDR, and update MTK_IOMMU_ADDR > definition for better generality. > > Signed-off-by: Ning Li > Signed-off-by: Yunfei Wang Reviewed-by: Miles Chen ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 4/6] arm64: dts: mediatek: mt2712e: Add mediatek, infracfg phandle for IOMMU
> The IOMMU driver now looks for the "mediatek,infracfg" phandle as a
> new way to retrieve a syscon to that:
> even though the old way is retained, it has been deprecated and the
> driver will write a message in kmsg advertising to use the phandle
> way instead.
>
> For this reason, assign the right phandle to mediatek,infracfg in
> the iommu node.
>
> Signed-off-by: AngeloGioacchino Del Regno
>
Reviewed-by: Miles Chen
> ---
> arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> index 623eb3beabf2..4797537cb368 100644
> --- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> @@ -329,6 +329,7 @@ iommu0: iommu@10205000 {
> interrupts = ;
> clocks = <&infracfg CLK_INFRA_M4U>;
> clock-names = "bclk";
> + mediatek,infracfg = <&infracfg>;
> mediatek,larbs = <&larb0>, <&larb1>, <&larb2>,
><&larb3>, <&larb6>;
> #iommu-cells = <1>;
> @@ -346,6 +347,7 @@ iommu1: iommu@1020a000 {
> interrupts = ;
> clocks = <&infracfg CLK_INFRA_M4U>;
> clock-names = "bclk";
> + mediatek,infracfg = <&infracfg>;
> mediatek,larbs = <&larb4>, <&larb5>, <&larb7>;
> #iommu-cells = <1>;
> };
> --
> 2.35.1
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/6] arm64: dts: mediatek: mt8173: Add mediatek, infracfg phandle for IOMMU
> The IOMMU driver now looks for the "mediatek,infracfg" phandle as a
> new way to retrieve a syscon to that:
> even though the old way is retained, it has been deprecated and the
> driver will write a message in kmsg advertising to use the phandle
> way instead.
>
> For this reason, assign the right phandle to mediatek,infracfg in
> the iommu node.
>
> Signed-off-by: AngeloGioacchino Del Regno
>
Reviewed-by: Miles Chen
> ---
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index 40d7b47fc52e..825a3c670373 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -588,6 +588,7 @@ iommu: iommu@10205000 {
> interrupts = ;
> clocks = <&infracfg CLK_INFRA_M4U>;
> clock-names = "bclk";
> + mediatek,infracfg = <&infracfg>;
> mediatek,larbs = <&larb0>, <&larb1>, <&larb2>,
><&larb3>, <&larb4>, <&larb5>;
> #iommu-cells = <1>;
> --
> 2.35.1
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to infracfg
> This driver will get support for more SoCs and the list of infracfg > compatibles is expected to grow: in order to prevent getting this > situation out of control and see a long list of compatible strings, > add support to retrieve a handle to infracfg's regmap through a > new "mediatek,infracfg" phandle. I also like the idea of using mediatek,infracfg instead of a list of platform compatible strings. Reviewed-by: Miles Chen > > In order to keep retrocompatibility with older devicetrees, the old > way is kept in place. > > Signed-off-by: AngeloGioacchino Del Regno > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Fix race condition during iova_domain initialization
Hi YF,
>When many devices share the same iova domain, iommu_dma_init_domain()
>may be called at the same time. The checking of iovad->start_pfn will
>all get false in iommu_dma_init_domain() and both enter init_iova_domain()
>to do iovad initialization.
After reading this patch.
It means that we use iovad->start_pfn as a key to tell if an iovad is already
initialized,
but we do not protect iovad->start_pfn from concurrent access.
So what's happening is like:
cpu0cpu1
of_dma_configure_id() of_dma_configure_id()
arch_setup_dma_ops() arch_setup_dma_ops()
iommu_setup_dma_ops() iommu_setup_dma_ops()
init_iova_domain() init_iova_domain()
if (iovad->start_pfn) { if (iovad->start_pfn) {
} }
init_iova_domain()init_iova_domain()
init_iova_domain() is called at the same time.
>Fix this by protecting init_iova_domain() with iommu_dma_cookie->mutex.
Reviewed-by: Miles Chen
>Exception backtrace:
>rb_insert_color(param1=0xFF80CD2BDB40, param3=1) + 64
>init_iova_domain() + 180
>iommu_setup_dma_ops() + 260
>arch_setup_dma_ops() + 132
>of_dma_configure_id() + 468
>platform_dma_configure() + 32
>really_probe() + 1168
>driver_probe_device() + 268
>__device_attach_driver() + 524
>__device_attach() + 524
>bus_probe_device() + 64
>deferred_probe_work_func() + 260
>process_one_work() + 580
>worker_thread() + 1076
>kthread() + 332
>ret_from_fork() + 16
>
>Signed-off-by: Ning Li
>Signed-off-by: Yunfei Wang
>---
> drivers/iommu/dma-iommu.c | 17 +
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>index 09f6e1c0f9c0..b38c5041eeab 100644
>--- a/drivers/iommu/dma-iommu.c
>+++ b/drivers/iommu/dma-iommu.c
>@@ -63,6 +63,7 @@ struct iommu_dma_cookie {
>
> /* Domain for flush queue callback; NULL if flush queue not in use */
> struct iommu_domain *fq_domain;
>+ struct mutexmutex;
> };
>
> static DEFINE_STATIC_KEY_FALSE(iommu_deferred_attach_enabled);
>@@ -309,6 +310,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
> if (!domain->iova_cookie)
> return -ENOMEM;
>
>+ mutex_init(&domain->iova_cookie->mutex);
> return 0;
> }
>
>@@ -549,26 +551,33 @@ static int iommu_dma_init_domain(struct iommu_domain
>*domain, dma_addr_t base,
> }
>
> /* start_pfn is always nonzero for an already-initialised domain */
>+ mutex_lock(&cookie->mutex);
>
> if (iovad->start_pfn) {
> if (1UL << order != iovad->granule ||
> base_pfn != iovad->start_pfn) {
> pr_warn("Incompatible range for DMA domain\n");
>- return -EFAULT;
>+ ret = -EFAULT;
>+ goto done_unlock;
> }
>
>- return 0;
>+ ret = 0;
>+ goto done_unlock;
> }
>
> init_iova_domain(iovad, 1UL << order, base_pfn);
> ret = iova_domain_init_rcaches(iovad);
> if (ret)
>- return ret;
>+ goto done_unlock;
>
> /* If the FQ fails we can simply fall back to strict mode */
> if (domain->type == IOMMU_DOMAIN_DMA_FQ && iommu_dma_init_fq(domain))
> domain->type = IOMMU_DOMAIN_DMA;
>
>- return iova_reserve_iommu_regions(dev, domain);
>+ ret = iova_reserve_iommu_regions(dev, domain);
>+
>+done_unlock:
>+ mutex_unlock(&cookie->mutex);
>+ return ret;
> }
>
> /**
>--
>2.18.0
>
>
>___
>Linux-mediatek mailing list
>linux-media...@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/2] iommu/io-pgtable-arm-v7s: Add a quirk to allow pgtable PA up to 35bit
Hi Yunfei, > The calling to kmem_cache_alloc for level 2 pgtable allocation may run > in atomic context, and it fails sometimes when DMA32 zone runs out of > memory. > > Since Mediatek IOMMU hardware support at most 35bit PA in pgtable, > so add a quirk to allow the PA of pgtables support up to bit35. > > Signed-off-by: Ning Li > Signed-off-by: Yunfei Wang > --- > drivers/iommu/io-pgtable-arm-v7s.c | 56 ++ > include/linux/io-pgtable.h | 15 +--- > 2 files changed, 52 insertions(+), 19 deletions(-) > > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c > b/drivers/iommu/io-pgtable-arm-v7s.c ...snip... > + gfp_t gfp_l1 = __GFP_ZERO | ARM_V7S_TABLE_GFP_DMA; > struct io_pgtable_cfg *cfg = &data->iop.cfg; > struct device *dev = cfg->iommu_dev; > phys_addr_t phys; > @@ -241,9 +251,11 @@ static void *__arm_v7s_alloc_table(int lvl, gfp_t gfp, > size_t size = ARM_V7S_TABLE_SIZE(lvl, cfg); > void *table = NULL; > > + if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT) > + gfp_l1 = __GFP_ZERO; __GFP_ZERO is an action modifier, if we do not want ARM_V7S_TABLE_GFP_DMA (GFP_DMA/GFP_DMA32), use gfp_l1 = (GFP_KERNEL | __GFP_ZERO) thanks, Miles > + > if (lvl == 1) > - table = (void *)__get_free_pages( > - __GFP_ZERO | ARM_V7S_TABLE_GFP_DMA, get_order(size)); > + table = (void *)__get_free_pages(gfp_l1, get_order(size)); > else if (lvl == 2) > table = kmem_cache_zalloc(data->l2_tables, gfp); > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Fix iova map result check bug
> The data type of the return value of the iommu_map_sg_atomic
> is ssize_t, but the data type of iova size is size_t,
> e.g. one is int while the other is unsigned int.
>
> When iommu_map_sg_atomic return value is compared with iova size,
> it will force the signed int to be converted to unsigned int, if
> iova map fails and iommu_map_sg_atomic return error code is less
> than 0, then (ret < iova_len) is false, which will to cause not
> do free iova, and the master can still successfully get the iova
> of map fail, which is not expected.
>
> Therefore, we need to check the return value of iommu_map_sg_atomic
> in two cases according to whether it is less than 0.
>
> Fixes: ad8f36e4b6b1 ("iommu: return full error code from
> iommu_map_sg[_atomic]()")
> Signed-off-by: Yunfei Wang
Yes, we have to make sure ssize_t >= 0 before comparing ssize_t and size_t.
Reviewed-by: Miles Chen
>
> Cc: # 5.15.*
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v3] iommu/mediatek: Fix NULL pointer dereference when printing dev_name
When larbdev is NULL (in the case I hit, the node is incorrectly set
iommus = <&iommu NUM>), it will cause device_link_add() fail and
kernel crashes when we try to print dev_name(larbdev).
Let's fail the probe if a larbdev is NULL to avoid invalid inputs from
dts.
It should work for normal correct setting and avoid the crash caused
by my incorrect setting.
Error log:
[ 18.189042][ T301] Unable to handle kernel NULL pointer dereference at
virtual address 0050
...
[ 18.344519][ T301] pstate: a045 (NzCv daif +PAN -UAO)
[ 18.345213][ T301] pc : mtk_iommu_probe_device+0xf8/0x118 [mtk_iommu]
[ 18.346050][ T301] lr : mtk_iommu_probe_device+0xd0/0x118 [mtk_iommu]
[ 18.346884][ T301] sp : ffc00a5635e0
[ 18.347392][ T301] x29: ffc00a5635e0 x28: ffd44a46c1d8
[ 18.348156][ T301] x27: ff80c39a8000 x26: ffd44a80cc38
[ 18.348917][ T301] x25: x24: ffd44a80cc38
[ 18.349677][ T301] x23: ffd44e4da4c6 x22: ffd44a80cc38
[ 18.350438][ T301] x21: ff80cecd1880 x20:
[ 18.351198][ T301] x19: ff80c439f010 x18: ffc00a50d0c0
[ 18.351959][ T301] x17: x16: 0004
[ 18.352719][ T301] x15: 0004 x14: ffd44eb5d420
[ 18.353480][ T301] x13: 0ad2 x12: 0003
[ 18.354241][ T301] x11: fad2 x10: c000fad2
[ 18.355003][ T301] x9 : a0d288d8d7142d00 x8 : a0d288d8d7142d00
[ 18.355763][ T301] x7 : ffd44c2bc640 x6 :
[ 18.356524][ T301] x5 : 0080 x4 : 0001
[ 18.357284][ T301] x3 : x2 : 0005
[ 18.358045][ T301] x1 : x0 :
[ 18.360208][ T301] Hardware name: MT6873 (DT)
[ 18.360771][ T301] Call trace:
[ 18.361168][ T301] dump_backtrace+0xf8/0x1f0
[ 18.361737][ T301] dump_stack_lvl+0xa8/0x11c
[ 18.362305][ T301] dump_stack+0x1c/0x2c
[ 18.362816][ T301] mrdump_common_die+0x184/0x40c [mrdump]
[ 18.363575][ T301] ipanic_die+0x24/0x38 [mrdump]
[ 18.364230][ T301] atomic_notifier_call_chain+0x128/0x2b8
[ 18.364937][ T301] die+0x16c/0x568
[ 18.365394][ T301] __do_kernel_fault+0x1e8/0x214
[ 18.365402][ T301] do_page_fault+0xb8/0x678
[ 18.366934][ T301] do_translation_fault+0x48/0x64
[ 18.368645][ T301] do_mem_abort+0x68/0x148
[ 18.368652][ T301] el1_abort+0x40/0x64
[ 18.368660][ T301] el1h_64_sync_handler+0x54/0x88
[ 18.368668][ T301] el1h_64_sync+0x68/0x6c
[ 18.368673][ T301] mtk_iommu_probe_device+0xf8/0x118 [mtk_iommu]
...
Cc: Robin Murphy
Cc: Yong Wu
Reported-by: kernel test robot
Fixes: 635319a4a744 ("media: iommu/mediatek: Add device_link between the
consumer and the larb devices")
Signed-off-by: Miles Chen
---
Change since v2
probe fail if larbdev is NULL so we do not have to worry about release logic
Change since v1
fix a build warning reported by kernel test robot
https://lore.kernel.org/lkml/202204231446.iykdz674-...@intel.com/
---
drivers/iommu/mtk_iommu.c| 6 ++
drivers/iommu/mtk_iommu_v1.c | 7 +++
2 files changed, 13 insertions(+)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 6fd75a60abd6..155acfbce44f 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -572,6 +572,9 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
* All the ports in each a device should be in the same larbs.
*/
larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ if (larbid >= MTK_LARB_NR_MAX)
+ return ERR_PTR(-EINVAL);
+
for (i = 1; i < fwspec->num_ids; i++) {
larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
if (larbid != larbidx) {
@@ -581,6 +584,9 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
}
}
larbdev = data->larb_imu[larbid].dev;
+ if (!larbdev)
+ return ERR_PTR(-EINVAL);
+
link = device_link_add(dev, larbdev,
DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
if (!link)
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index ecff800656e6..74563f689fbd 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -80,6 +80,7 @@
/* MTK generation one iommu HW only support 4K size mapping */
#define MT2701_IOMMU_PAGE_SHIFT12
#define MT2701_IOMMU_PAGE_SIZE (1UL << MT2701_IOMMU_PAGE_SHIFT)
+#define MT2701_LARB_NR_MAX 3
/*
* MTK m4u support 4GB iova address space, and only support 4K page
@@ -457,6 +458,9 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
/* Link the consumer device with the smi-larb device(supplier) */
larbid = mt2701_m4u_to_larb(fwspec->ids[0]);
+ if (larbid >= MT2701_LARB_NR_MAX)
+ return ERR_PTR(-EINVA
Re: [PATCH 1/2] iommu/io-pgtable-arm-v7s: Add a quirk to support TTBR up to 35bit for MediaTek
Hi YF,
> The calling to kmem_cache_alloc for level 2 page table allocation may
> run in atomic context, and it fails sometimes when DMA32 zone runs out
> of memory.
>
> Since Mediatek IOMMU hardware support at most 35bit PA in page table,
s/Mediatek/MediaTek/
s/support/supports/
> so add a quirk to allow the PA of level 2 pgtable support bit35.
35bits PA, right?
>
>
...snip...
>
> phys = virt_to_phys(table);
> - if (phys != (arm_v7s_iopte)phys) {
> + if (phys != (arm_v7s_iopte)phys &&
> + !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT)) {
I have one question while reading this.
If IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT is set, it means that the phys can be up
to 35 bits.
In aarch64, kmalloc() could return up to 52 bits PA (e.g., ARM64_PA_BITS_52=y)
How do we guarantee that phys is safe (<= 35 bits) in this case?
For example:
When IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT is set, the platform guarantees its PAs
are at most
35 bits?
Thanks,
Miles
> /* Doesn't fit in PTE */
> dev_err(dev, "Page table does not fit in PTE: %pa", &phys);
> goto out_free;
> @@ -457,9 +464,14 @@ static arm_v7s_iopte arm_v7s_install_table(arm_v7s_iopte
> *table,
> arm_v7s_iopte curr,
> struct io_pgtable_cfg *cfg)
> {
> + phys_addr_t phys = virt_to_phys(table);
> arm_v7s_iopte old, new;
>
> - new = virt_to_phys(table) | ARM_V7S_PTE_TYPE_TABLE;
> + new = phys | ARM_V7S_PTE_TYPE_TABLE;
> +
> + if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT)
> + new = to_iopte_mtk(phys, new, cfg);
> +
> if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
> new |= ARM_V7S_ATTR_NS_TABLE;
>
> @@ -778,6 +790,7 @@ static phys_addr_t arm_v7s_iova_to_phys(struct
> io_pgtable_ops *ops,
> static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
> void *cookie)
> {
> + slab_flags_t slab_flag = ARM_V7S_TABLE_SLAB_FLAGS;
> struct arm_v7s_io_pgtable *data;
>
> if (cfg->ias > (arm_v7s_is_mtk_enabled(cfg) ? 34 : ARM_V7S_ADDR_BITS))
> @@ -788,7 +801,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct
> io_pgtable_cfg *cfg,
>
> if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
> IO_PGTABLE_QUIRK_NO_PERMS |
> - IO_PGTABLE_QUIRK_ARM_MTK_EXT))
> + IO_PGTABLE_QUIRK_ARM_MTK_EXT |
> + IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT))
> return NULL;
>
> /* If ARM_MTK_4GB is enabled, the NO_PERMS is also expected. */
> @@ -801,10 +815,12 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct
> io_pgtable_cfg *cfg,
> return NULL;
>
> spin_lock_init(&data->split_lock);
> + if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT)
> + slab_flag = 0;
> data->l2_tables = kmem_cache_create("io-pgtable_armv7s_l2",
> ARM_V7S_TABLE_SIZE(2, cfg),
> ARM_V7S_TABLE_SIZE(2, cfg),
> - ARM_V7S_TABLE_SLAB_FLAGS, NULL);
> + slab_flag, NULL);
> if (!data->l2_tables)
> goto out_free_data;
>
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index 86af6f0a00a2..7ed15ad4710c 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -74,17 +74,22 @@ struct io_pgtable_cfg {
>* to support up to 35 bits PA where the bit32, bit33 and bit34 are
>* encoded in the bit9, bit4 and bit5 of the PTE respectively.
>*
> + * IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT: (ARM v7s format) MediaTek IOMMUs
> + * extend the translation table support up to 35 bits PA, the
> + * encoding format is same with IO_PGTABLE_QUIRK_ARM_MTK_EXT.
> + *
>* IO_PGTABLE_QUIRK_ARM_TTBR1: (ARM LPAE format) Configure the table
>* for use in the upper half of a split address space.
>*
>* IO_PGTABLE_QUIRK_ARM_OUTER_WBWA: Override the outer-cacheability
>* attributes set in the TCR for a non-coherent page-table walker.
>*/
> - #define IO_PGTABLE_QUIRK_ARM_NS BIT(0)
> - #define IO_PGTABLE_QUIRK_NO_PERMS BIT(1)
> - #define IO_PGTABLE_QUIRK_ARM_MTK_EXTBIT(3)
> - #define IO_PGTABLE_QUIRK_ARM_TTBR1 BIT(5)
> - #define IO_PGTABLE_QUIRK_ARM_OUTER_WBWA BIT(6)
> + #define IO_PGTABLE_QUIRK_ARM_NS BIT(0)
> + #define IO_PGTABLE_QUIRK_NO_PERMS BIT(1)
> + #define IO_PGTABLE_QUIRK_ARM_MTK_EXTBIT(3)
> + #define IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT BIT(4)
> + #define IO_PGTABLE_QUIRK_ARM_TTBR1 BIT(5)
> + #define IO_PGTABLE_QUIRK_ARM_OUTER_WBWA
Re: [PATCH v2] iommu/mediatek: fix NULL pointer dereference when printing dev_name
Hi Yong,
>On Mon, 2022-04-25 at 11:03 +0100, Robin Murphy wrote:
>> On 2022-04-25 09:24, Miles Chen via iommu wrote:
>> > When larbdev is NULL (in the case I hit, the node is incorrectly
>> > set
>> > iommus = <&iommu NUM>), it will cause device_link_add() fail and
>>
>> Until the MT8195 infra MMU support lands, is there ever a case where
>> it's actually valid for larbdev to be NULL? If not, I think it would
>> be
>> a lot clearer to explicitly fail the probe here, rather than
>> silently
>> continue and risk fatal errors, hangs, or other weird behaviour if
>> there's no guarantee that the correct LARB is powered up (plus then
>> the
>> release callbacks wouldn't need to worry about it either).
>
>Yes. It should return fail for this case. This issue only happens when
>the dts parameters doesn't respect the definition from the binding[1].
>
>Locally Miles tested with a internal definition that have not send
>upstream to get this KE. In this case, I'm not sure if we should
>request the user use the right ID in dts. Anyway I have no objection to
>modifying this, then something like this: (Avoid invalid input from
>dtb)
>
>@@ -790,6 +790,8 @@ static struct iommu_device
>*mtk_iommu_probe_device(struct device *dev)
>* All the ports in each a device should be in the same larbs.
>*/
> larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
>+ if (larbid >= MTK_LARB_NR_MAX)
>+ return ERR_PTR(-EINVAL);
> for (i = 1; i < fwspec->num_ids; i++) {
> larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
> if (larbid != larbidx) {
>@@ -799,6 +801,8 @@ static struct iommu_device
>*mtk_iommu_probe_device(struct device *dev)
> }
> }
> larbdev = data->larb_imu[larbid].dev;
>+ if (!larbdev)
>+ return ERR_PTR(-EINVAL);
> link = device_link_add(dev, larbdev,
> DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
> if (!link)
Thanks for guilding me, I will put this in patch v2.
Thanks,
Miles
>
>
>[1]
>https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml#L116
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/mediatek: fix NULL pointer dereference when printing dev_name
hi Robin,
>> -link = device_link_add(dev, larbdev,
>> - DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
>> -if (!link)
>> -dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
>> +if (larbdev) {
>
>Until the MT8195 infra MMU support lands, is there ever a case where
>it's actually valid for larbdev to be NULL? If not, I think it would be
>a lot clearer to explicitly fail the probe here, rather than silently
>continue and risk fatal errors, hangs, or other weird behaviour if
>there's no guarantee that the correct LARB is powered up (plus then the
>release callbacks wouldn't need to worry about it either).
Thanks, I will do probe fail in patch v3 and remove the release modification.
thanks,
Miles
>
>Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] iommu/mediatek: fix NULL pointer dereference when printing dev_name
When larbdev is NULL (in the case I hit, the node is incorrectly set
iommus = <&iommu NUM>), it will cause device_link_add() fail and
kernel crashes when we try to print dev_name(larbdev).
Fix it by adding a NULL pointer check before
device_link_add/device_link_remove.
It should work for normal correct setting and avoid the crash caused
by my incorrect setting.
Error log:
[ 18.189042][ T301] Unable to handle kernel NULL pointer dereference at
virtual address 0050
[ 18.190247][ T301] Mem abort info:
[ 18.190255][ T301] ESR = 0x9605
[ 18.190263][ T301] EC = 0x25: DABT (current EL), IL = 32 bits
[ 18.192142][ T301] SET = 0, FnV = 0
[ 18.192151][ T301] EA = 0, S1PTW = 0
[ 18.194710][ T301] FSC = 0x05: level 1 translation fault
[ 18.195424][ T301] Data abort info:
[ 18.195888][ T301] ISV = 0, ISS = 0x0005
[ 18.196500][ T301] CM = 0, WnR = 0
[ 18.196977][ T301] user pgtable: 4k pages, 39-bit VAs, pgdp=000104f9e000
[ 18.197889][ T301] [0050] pgd=,
p4d=, pud=
[ 18.199220][ T301] Internal error: Oops: 9605 [#1] PREEMPT SMP
[ 18.343152][ T301] Kernel Offset: 0x144408 from 0xffc00800
[ 18.343988][ T301] PHYS_OFFSET: 0x4000
[ 18.344519][ T301] pstate: a045 (NzCv daif +PAN -UAO)
[ 18.345213][ T301] pc : mtk_iommu_probe_device+0xf8/0x118 [mtk_iommu]
[ 18.346050][ T301] lr : mtk_iommu_probe_device+0xd0/0x118 [mtk_iommu]
[ 18.346884][ T301] sp : ffc00a5635e0
[ 18.347392][ T301] x29: ffc00a5635e0 x28: ffd44a46c1d8
[ 18.348156][ T301] x27: ff80c39a8000 x26: ffd44a80cc38
[ 18.348917][ T301] x25: x24: ffd44a80cc38
[ 18.349677][ T301] x23: ffd44e4da4c6 x22: ffd44a80cc38
[ 18.350438][ T301] x21: ff80cecd1880 x20:
[ 18.351198][ T301] x19: ff80c439f010 x18: ffc00a50d0c0
[ 18.351959][ T301] x17: x16: 0004
[ 18.352719][ T301] x15: 0004 x14: ffd44eb5d420
[ 18.353480][ T301] x13: 0ad2 x12: 0003
[ 18.354241][ T301] x11: fad2 x10: c000fad2
[ 18.355003][ T301] x9 : a0d288d8d7142d00 x8 : a0d288d8d7142d00
[ 18.355763][ T301] x7 : ffd44c2bc640 x6 :
[ 18.356524][ T301] x5 : 0080 x4 : 0001
[ 18.357284][ T301] x3 : x2 : 0005
[ 18.358045][ T301] x1 : x0 :
[ 18.360208][ T301] Hardware name: MT6873 (DT)
[ 18.360771][ T301] Call trace:
[ 18.361168][ T301] dump_backtrace+0xf8/0x1f0
[ 18.361737][ T301] dump_stack_lvl+0xa8/0x11c
[ 18.362305][ T301] dump_stack+0x1c/0x2c
[ 18.362816][ T301] mrdump_common_die+0x184/0x40c [mrdump]
[ 18.363575][ T301] ipanic_die+0x24/0x38 [mrdump]
[ 18.364230][ T301] atomic_notifier_call_chain+0x128/0x2b8
[ 18.364937][ T301] die+0x16c/0x568
[ 18.365394][ T301] __do_kernel_fault+0x1e8/0x214
[ 18.365402][ T301] do_page_fault+0xb8/0x678
[ 18.366934][ T301] do_translation_fault+0x48/0x64
[ 18.368645][ T301] do_mem_abort+0x68/0x148
[ 18.368652][ T301] el1_abort+0x40/0x64
[ 18.368660][ T301] el1h_64_sync_handler+0x54/0x88
[ 18.368668][ T301] el1h_64_sync+0x68/0x6c
[ 18.368673][ T301] mtk_iommu_probe_device+0xf8/0x118 [mtk_iommu]
[ 18.369840][ T301] __iommu_probe_device+0x12c/0x358
[ 18.370880][ T301] iommu_probe_device+0x3c/0x31c
[ 18.372026][ T301] of_iommu_configure+0x200/0x274
[ 18.373587][ T301] of_dma_configure_id+0x1b8/0x230
[ 18.375200][ T301] platform_dma_configure+0x24/0x3c
[ 18.376456][ T301] really_probe+0x110/0x504
[ 18.376464][ T301] __driver_probe_device+0xb4/0x188
[ 18.376472][ T301] driver_probe_device+0x5c/0x2b8
[ 18.376481][ T301] __driver_attach+0x338/0x42c
[ 18.377992][ T301] bus_add_driver+0x218/0x4c8
[ 18.379389][ T301] driver_register+0x84/0x17c
[ 18.380580][ T301] __platform_driver_register+0x28/0x38
...
Reported-by: kernel test robot
Fixes: 635319a4a744 ("media: iommu/mediatek: Add device_link between the
consumer and the larb devices")
Signed-off-by: Miles Chen
---
Change since v1
fix a build warning reported by kernel test robot
https://lore.kernel.org/lkml/202204231446.iykdz674-...@intel.com/
---
drivers/iommu/mtk_iommu.c| 13 -
drivers/iommu/mtk_iommu_v1.c | 13 -
2 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 6fd75a60abd6..03e0133f346a 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -581,10 +581,12 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
}
}
larbdev = data->larb_imu[larbid].dev;
- link = device_link_add(dev, larbdev,
- DL_FLAG_PM_RUNTIME | DL_FLAG_STATEL
Re: [PATCH] iommu/mediatek: fix NULL pointer dereference when printing dev_name
>Hi Miles,
>
>Thank you for the patch! Perhaps something to improve:
>
>[auto build test WARNING on joro-iommu/next]
>[also build test WARNING on v5.18-rc3 next-20220422]
>[If your patch is applied to the wrong git tree, kindly drop us a note.
>And when submitting patch, we suggest to use '--base' as documented in
>https://git-scm.com/docs/git-format-patch]
>
>url:
>https://github.com/intel-lab-lkp/linux/commits/Miles-Chen/iommu-mediatek-fix-NULL-pointer-dereference-when-printing-dev_name/20220423-070605
>base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
>config: hexagon-randconfig-r041-20220422
>(https://download.01.org/0day-ci/archive/20220423/202204231446.iykdz674-...@intel.com/config)
>compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project
>5bd87350a5ae429baf8f373cb226a57b62f87280)
>reproduce (this is a W=1 build):
>wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
>chmod +x ~/bin/make.cross
>#
> https://github.com/intel-lab-lkp/linux/commit/85771767e503ca60069fe4e6ec2ddb80c7f9bafa
>git remote add linux-review https://github.com/intel-lab-lkp/linux
>git fetch --no-tags linux-review
> Miles-Chen/iommu-mediatek-fix-NULL-pointer-dereference-when-printing-dev_name/20220423-070605
>git checkout 85771767e503ca60069fe4e6ec2ddb80c7f9bafa
># save the config file
>mkdir build_dir && cp config build_dir/.config
>COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
> O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/iommu/
>
>If you fix the issue, kindly add following tag as appropriate
>Reported-by: kernel test robot
>
>All warnings (new ones prefixed by >>):
>
>>> drivers/iommu/mtk_iommu.c:605:6: warning: variable 'larbdev' is
>>> uninitialized when used here [-Wuninitialized]
> if (larbdev) {
> ^~~
> drivers/iommu/mtk_iommu.c:597:24: note: initialize the variable 'larbdev'
> to silence this warning
> struct device *larbdev;
> ^
> = NULL
> 1 warning generated.
Thanks for catching this, I will fix this in next version.
thanks,
Miles
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/mediatek: fix NULL pointer dereference when printing dev_name
When larbdev is NULL (in the case I hit, the node is incorrectly set
iommus = <&iommu NUM>), it will cause device_link_add() fail and
the kernel crashes when we try to print dev_name(larbdev).
Fix it by adding a NULL pointer check before
device_link_add/device_link_remove.
It should work for normal correct setting and avoid the crash caused
by my incorrect setting.
Error log:
[ 18.189042][ T301] Unable to handle kernel NULL pointer dereference at
virtual address 0050
[ 18.190247][ T301] Mem abort info:
[ 18.190255][ T301] ESR = 0x9605
[ 18.190263][ T301] EC = 0x25: DABT (current EL), IL = 32 bits
[ 18.192142][ T301] SET = 0, FnV = 0
[ 18.192151][ T301] EA = 0, S1PTW = 0
[ 18.194710][ T301] FSC = 0x05: level 1 translation fault
[ 18.195424][ T301] Data abort info:
[ 18.195888][ T301] ISV = 0, ISS = 0x0005
[ 18.196500][ T301] CM = 0, WnR = 0
[ 18.196977][ T301] user pgtable: 4k pages, 39-bit VAs, pgdp=000104f9e000
[ 18.197889][ T301] [0050] pgd=,
p4d=, pud=
[ 18.199220][ T301] Internal error: Oops: 9605 [#1] PREEMPT SMP
[ 18.343152][ T301] Kernel Offset: 0x144408 from 0xffc00800
[ 18.343988][ T301] PHYS_OFFSET: 0x4000
[ 18.344519][ T301] pstate: a045 (NzCv daif +PAN -UAO)
[ 18.345213][ T301] pc : mtk_iommu_probe_device+0xf8/0x118 [mtk_iommu]
[ 18.346050][ T301] lr : mtk_iommu_probe_device+0xd0/0x118 [mtk_iommu]
[ 18.346884][ T301] sp : ffc00a5635e0
[ 18.347392][ T301] x29: ffc00a5635e0 x28: ffd44a46c1d8
[ 18.348156][ T301] x27: ff80c39a8000 x26: ffd44a80cc38
[ 18.348917][ T301] x25: x24: ffd44a80cc38
[ 18.349677][ T301] x23: ffd44e4da4c6 x22: ffd44a80cc38
[ 18.350438][ T301] x21: ff80cecd1880 x20:
[ 18.351198][ T301] x19: ff80c439f010 x18: ffc00a50d0c0
[ 18.351959][ T301] x17: x16: 0004
[ 18.352719][ T301] x15: 0004 x14: ffd44eb5d420
[ 18.353480][ T301] x13: 0ad2 x12: 0003
[ 18.354241][ T301] x11: fad2 x10: c000fad2
[ 18.355003][ T301] x9 : a0d288d8d7142d00 x8 : a0d288d8d7142d00
[ 18.355763][ T301] x7 : ffd44c2bc640 x6 :
[ 18.356524][ T301] x5 : 0080 x4 : 0001
[ 18.357284][ T301] x3 : x2 : 0005
[ 18.358045][ T301] x1 : x0 :
[ 18.360208][ T301] Hardware name: MT6873 (DT)
[ 18.360771][ T301] Call trace:
[ 18.361168][ T301] dump_backtrace+0xf8/0x1f0
[ 18.361737][ T301] dump_stack_lvl+0xa8/0x11c
[ 18.362305][ T301] dump_stack+0x1c/0x2c
[ 18.362816][ T301] mrdump_common_die+0x184/0x40c [mrdump]
[ 18.363575][ T301] ipanic_die+0x24/0x38 [mrdump]
[ 18.364230][ T301] atomic_notifier_call_chain+0x128/0x2b8
[ 18.364937][ T301] die+0x16c/0x568
[ 18.365394][ T301] __do_kernel_fault+0x1e8/0x214
[ 18.365402][ T301] do_page_fault+0xb8/0x678
[ 18.366934][ T301] do_translation_fault+0x48/0x64
[ 18.368645][ T301] do_mem_abort+0x68/0x148
[ 18.368652][ T301] el1_abort+0x40/0x64
[ 18.368660][ T301] el1h_64_sync_handler+0x54/0x88
[ 18.368668][ T301] el1h_64_sync+0x68/0x6c
[ 18.368673][ T301] mtk_iommu_probe_device+0xf8/0x118 [mtk_iommu]
[ 18.369840][ T301] __iommu_probe_device+0x12c/0x358
[ 18.370880][ T301] iommu_probe_device+0x3c/0x31c
[ 18.372026][ T301] of_iommu_configure+0x200/0x274
[ 18.373587][ T301] of_dma_configure_id+0x1b8/0x230
[ 18.375200][ T301] platform_dma_configure+0x24/0x3c
[ 18.376456][ T301] really_probe+0x110/0x504
[ 18.376464][ T301] __driver_probe_device+0xb4/0x188
[ 18.376472][ T301] driver_probe_device+0x5c/0x2b8
[ 18.376481][ T301] __driver_attach+0x338/0x42c
[ 18.377992][ T301] bus_add_driver+0x218/0x4c8
[ 18.379389][ T301] driver_register+0x84/0x17c
[ 18.380580][ T301] __platform_driver_register+0x28/0x38
...
Fixes: 635319a4a744 ("media: iommu/mediatek: Add device_link between the
consumer and the larb devices")
Signed-off-by: Miles Chen
---
drivers/iommu/mtk_iommu.c| 16 ++--
drivers/iommu/mtk_iommu_v1.c | 16 ++--
2 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 6fd75a60abd6..1405502118ca 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -581,10 +581,12 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
}
}
larbdev = data->larb_imu[larbid].dev;
- link = device_link_add(dev, larbdev,
- DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
- if (!link)
- dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
+ if (larbdev) {
+ link = device_link_add(de
Re: [PATCH] iommu/iova: Improve 32-bit free space estimate
Hi Joerg, Robin, > Applied without stable tag for now. If needed, please consider > re-sending it for stable when this patch is merged upstream. > Yeah, having figured out the history, I ended up with the opinion that > it was a missed corner-case optimisation opportunity, rather than an > actual error with respect to intent or implementation, so I > intentionally left that out. Plus figuring out an exact Fixes tag might > be tricky - as above I reckon it probably only started to become > significant somwehere around 5.11 or so. > > All of these various levels of retry mechanisms are only a best-effort > thing, and ultimately if you're making large allocations from a small > space there are always going to be *some* circumstances that still > manage to defeat them. Over time, we've made them try harder, but that > fact that we haven't yet made them try hard enough to work well for a > particular use-case does not constitute a bug. However as Joerg says, > anyone's welcome to make a case to Greg to backport a mainline commit if > it's a low-risk change with significant benefit to real-world stable > kernel users. Got it, thank you. We will try to push to the android LTS trees we need. Thanks, Miles > > Thanks all! > > Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/iova: Improve 32-bit free space estimate
Hi Robin, > For various reasons based on the allocator behaviour and typical > use-cases at the time, when the max32_alloc_size optimisation was > introduced it seemed reasonable to couple the reset of the tracked > size to the update of cached32_node upon freeing a relevant IOVA. > However, since subsequent optimisations focused on helping genuine > 32-bit devices make best use of even more limited address spaces, it > is now a lot more likely for cached32_node to be anywhere in a "full" > 32-bit address space, and as such more likely for space to become > available from IOVAs below that node being freed. > > At this point, the short-cut in __cached_rbnode_delete_update() really > doesn't hold up any more, and we need to fix the logic to reliably > provide the expected behaviour. We still want cached32_node to only move > upwards, but we should reset the allocation size if *any* 32-bit space > has become available. > > Reported-by: Yunfei Wang > Signed-off-by: Robin Murphy Would you mind adding: Cc: to this path? I checked and I think the patch can be applied to 5.4 and later. thanks, Miles ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/iova: Improve 32-bit free space estimate
> For various reasons based on the allocator behaviour and typical > use-cases at the time, when the max32_alloc_size optimisation was > introduced it seemed reasonable to couple the reset of the tracked > size to the update of cached32_node upon freeing a relevant IOVA. > However, since subsequent optimisations focused on helping genuine > 32-bit devices make best use of even more limited address spaces, it > is now a lot more likely for cached32_node to be anywhere in a "full" > 32-bit address space, and as such more likely for space to become > available from IOVAs below that node being freed. > > At this point, the short-cut in __cached_rbnode_delete_update() really > doesn't hold up any more, and we need to fix the logic to reliably > provide the expected behaviour. We still want cached32_node to only move > upwards, but we should reset the allocation size if *any* 32-bit space > has become available. > > Reported-by: Yunfei Wang > Signed-off-by: Robin Murphy Reviewed-by: Miles Chen ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/iova: Reset max32_alloc_size after cleaning
>> If no cached iova is freed, resetting max32_alloc_size before
>> the retry allocation only give us a retry. Is it possible that
>> other users free their iovas during the additional retry?
>
> No, it's not possible, since everyone's serialised by iova_rbtree_lock.
> If the caches were already empty and the retry gets the lock first, it
> will still fail again - forcing a reset of max32_alloc_size only means
> it has to take the slow path to that failure. If another caller *did*
> manage to get in and free something between free_global_cached_iovas()
> dropping the lock and alloc_iova() re-taking it, then that would have
> legitimately reset max32_alloc_size anyway.
Thanks for your explanation.
YF showed me some numbers yesterday and maybe we can have a further
discussion in that test case. (It looks like that some iovas are freed but
their pfn_lo(s) are less than cached_iova->pfn_lo, so max32_alloc_size is not
reset. So there are enought free iovas but the allocation still failed)
__cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
{
struct iova *cached_iova;
cached_iova = to_iova(iovad->cached32_node);
if (free == cached_iova ||
(free->pfn_hi < iovad->dma_32bit_pfn &&
free->pfn_lo >= cached_iova->pfn_lo)) {
iovad->cached32_node = rb_next(&free->node);
iovad->max32_alloc_size = iovad->dma_32bit_pfn;
}
...
}
Hi YF,
Could your share your observation of the iova allocation failure you hit?
Thanks,
Miles
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/iova: Reset max32_alloc_size after cleaning
Hi Yunfei, >> Since __alloc_and_insert_iova_range fail will set the current alloc >> iova size to max32_alloc_size (iovad->max32_alloc_size = size), >> when the retry is executed into the __alloc_and_insert_iova_range >> function, the retry action will be blocked by the check condition >> (size >= iovad->max32_alloc_size) and goto iova32_full directly, >> causes the action of retry regular alloc iova in >> __alloc_and_insert_iova_range to not actually be executed. >> >> Based on the above, so need reset max32_alloc_size before retry alloc >> iova when alloc iova fail, that is set the initial dma_32bit_pfn value >> of iovad to max32_alloc_size, so that the action of retry alloc iova >> in __alloc_and_insert_iova_range can be executed. > > Have you observed this making any difference in practice? > > Given that both free_cpu_cached_iovas() and free_global_cached_iovas() > call iova_magazine_free_pfns(), which calls remove_iova(), which calls > __cached_rbnode_delete_update(), I'm thinking no... > > Robin. > Like Robin pointed out, if some cached iovas are freed by free_global_cached_iovas()/free_cpu_cached_iovas(), the max32_alloc_size should be reset to iovad->dma_32bit_pfn. If no cached iova is freed, resetting max32_alloc_size before the retry allocation only give us a retry. Is it possible that other users free their iovas during the additional retry? alloc_iova_fast() retry: alloc_iova() // failed, iovad->max32_alloc_size = size free_cpu_cached_iovas() iova_magazine_free_pfns() remove_iova() __cached_rbnode_delete_update() iovad->max32_alloc_size = iovad->dma_32bit_pfn // reset free_global_cached_iovas() iova_magazine_free_pfns() remove_iova() __cached_rbnode_delete_update() iovad->max32_alloc_size = iovad->dma_32bit_pfn // reset goto retry; thanks, Miles ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
