From: Robin Murphy <robin.murphy@arm.com>
To: Stephan Gerhold <stephan@gerhold.net>, Will Deacon <will@kernel.org>
Cc: iommu@lists.linux-foundation.org, kernel-team@android.com,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 8/8] iommu/io-pgtable-arm: Prepare for TTBR1 usage
Date: Wed, 19 Feb 2020 17:51:14 +0000 [thread overview]
Message-ID: <615b5077-4cd0-d5b0-5147-202f8c0771c7@arm.com> (raw)
In-Reply-To: <20200219122711.GA176090@gerhold.net>
On 19/02/2020 12:27 pm, Stephan Gerhold wrote:
> Hi Will, Hi Robin,
>
> On Fri, Jan 10, 2020 at 03:28:52PM +0000, Will Deacon wrote:
>> From: Robin Murphy <robin.murphy@arm.com>
>>
>> Now that we can correctly extract top-level indices without relying on
>> the remaining upper bits being zero, the only remaining impediments to
>> using a given table for TTBR1 are the address validation on map/unmap
>> and the awkward TCR translation granule format. Add a quirk so that we
>> can do the right thing at those points.
>>
>> Tested-by: Jordan Crouse <jcrouse@codeaurora.org>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> Signed-off-by: Will Deacon <will@kernel.org>
>> ---
>> drivers/iommu/io-pgtable-arm.c | 25 +++++++++++++++++++------
>> include/linux/io-pgtable.h | 4 ++++
>> 2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
>> index 846963c87e0f..5e966ef0bacf 100644
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -104,6 +104,10 @@
>> #define ARM_LPAE_TCR_TG0_64K 1
>> #define ARM_LPAE_TCR_TG0_16K 2
>>
>> +#define ARM_LPAE_TCR_TG1_16K 1
>> +#define ARM_LPAE_TCR_TG1_4K 2
>> +#define ARM_LPAE_TCR_TG1_64K 3
>> +
>> #define ARM_LPAE_TCR_SH_NS 0
>> #define ARM_LPAE_TCR_SH_OS 2
>> #define ARM_LPAE_TCR_SH_IS 3
>> @@ -463,6 +467,7 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>> arm_lpae_iopte *ptep = data->pgd;
>> int ret, lvl = data->start_level;
>> arm_lpae_iopte prot;
>> + long iaext = (long)iova >> cfg->ias;
>>
>> /* If no access, then nothing to do */
>> if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
>> @@ -471,7 +476,9 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>> if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size))
>> return -EINVAL;
>>
>> - if (WARN_ON(iova >> data->iop.cfg.ias || paddr >> data->iop.cfg.oas))
>> + if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1)
>> + iaext = ~iaext;
>> + if (WARN_ON(iaext || paddr >> cfg->oas))
>> return -ERANGE;
>>
>> prot = arm_lpae_prot_to_pte(data, iommu_prot);
>> @@ -637,11 +644,14 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
>> struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
>> struct io_pgtable_cfg *cfg = &data->iop.cfg;
>> arm_lpae_iopte *ptep = data->pgd;
>> + long iaext = (long)iova >> cfg->ias;
>>
>> if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size))
>> return 0;
>>
>> - if (WARN_ON(iova >> data->iop.cfg.ias))
>> + if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1)
>> + iaext = ~iaext;
>> + if (WARN_ON(iaext))
>> return 0;
>>
>> return __arm_lpae_unmap(data, gather, iova, size, data->start_level, ptep);
>
> This part of the patch seems to break io-pgtable-arm.c on ARM32 to some
> extent. I have a device using qcom_iommu that could normally run ARM64,
> but I'm unable to because of outdated (signed) firmware. :(
> So it's running a lot of code that would normally run only on ARM64.
>
> It used to work quite well but now qcom-venus fails to probe on:
> if (WARN_ON(iaext || paddr >> cfg->oas))
> (I added some debug prints for clarity.)
>
> qcom-venus 1d00000.video-codec: Adding to iommu group 0
> arm-lpae io-pgtable: quirks: 0x0
> arm-lpae io-pgtable: arm_lpae_map: iova: 0xdd800000, paddr: 0xebe3f000, iaext: 0xffffffff, paddr >> oas: 0x0
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 954 at drivers/iommu/io-pgtable-arm.c:487 arm_lpae_map+0xe4/0x510
> Hardware name: Generic DT based system
> ...
> [<c04bafb0>] (arm_lpae_map) from [<c04bcd6c>] (qcom_iommu_map+0x50/0x78)
> [<c04bcd6c>] (qcom_iommu_map) from [<c04b7290>] (__iommu_map+0xe8/0x1cc)
> [<c04b7290>] (__iommu_map) from [<c04b7bbc>] (__iommu_map_sg+0xa8/0x118)
> [<c04b7bbc>] (__iommu_map_sg) from [<c04b7c64>] (iommu_map_sg_atomic+0x18/0x20)
> [<c04b7c64>] (iommu_map_sg_atomic) from [<c04b94f8>] (iommu_dma_alloc+0x4dc/0x5a0)
> [<c04b94f8>] (iommu_dma_alloc) from [<c0196a50>] (dma_alloc_attrs+0x104/0x114)
> [<c0196a50>] (dma_alloc_attrs) from [<bf302c60>] (venus_hfi_create+0xa4/0x260 [venus_core])
> [<bf302c60>] (venus_hfi_create [venus_core]) from [<bf2fe6cc>] (venus_probe+0x1e4/0x328 [venus_core])
> [<bf2fe6cc>] (venus_probe [venus_core]) from [<c04c8eb4>] (platform_drv_probe+0x48/0x98)
> ...
> Exception stack(0xc2587fa8 to 0xc2587ff0)
> 7fa0: b6f3dc70 b5051010 b5051010 0017388c b6e645b0 b6e645b0
> 7fc0: b6f3dc70 b5051010 00020000 00000080 b6e30790 be84ea54 0046ac91 00000000
> 7fe0: b6e75f1c be84e940 b6e5fb51 b6eec8a4
> ---[ end trace 2a0ed284f6d82f16 ]---
> qcom-venus: probe of 1d00000.video-codec failed with error -12
>
> Note that iaext = 0xffffffff != 0.
> It seems to be some int/long size problem that only happens with larger
> iova addresses on 32-bit systems.
>
> Without the (long) cast for iova everything is working correctly again:
>
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 983b08477e64..f7ecc763c706 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -468,7 +468,7 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
> arm_lpae_iopte *ptep = data->pgd;
> int ret, lvl = data->start_level;
> arm_lpae_iopte prot;
> - long iaext = (long)iova >> cfg->ias;
> + long iaext = iova >> cfg->ias;
>
> /* If no access, then nothing to do */
> if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> @@ -645,7 +645,7 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
> struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> struct io_pgtable_cfg *cfg = &data->iop.cfg;
> arm_lpae_iopte *ptep = data->pgd;
> - long iaext = (long)iova >> cfg->ias;
> + long iaext = iova >> cfg->ias;
>
> if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size))
> return 0;
>
> But I'm not sure if this is really the correct "fix"...
No, that would end up breaking the TTBR1 case. The quirk isn't intended
to be supported on 32-bit platforms, but somehow it's slipped through
that this sign-extension still affects them (and goes wrong to boot),
sorry about that.
I think quick fixes would be either to replace the "(long)" cast with
"(s64)(u64)", or to just elide the iaext check altogether when 32-bit
longs make it impossible to pass an out-of-range IOVA anyway. I'll have
a think and see what works out nicest.
> This problem can be also easily reproduced by enabling
> IOMMU_IO_PGTABLE_LPAE_SELFTEST on ARM32.
> (Shouldn't there be some system that runs these automatically? ;))
>
> Without this patch all of them are passing:
>
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 48
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 48
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 48
> arm-lpae io-pgtable: selftest: completed with 18 PASS 0 FAIL
Hmm, we probably shouldn't be running IAS > 32 tests on 32-bit, given
that such configs are effectively impossible to use.
Robin.
>
> But with this patch 6 of them are failing (with a similar warning as
> I posted above):
>
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 32
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 1 at drivers/iommu/io-pgtable-arm.c:482 arm_lpae_map+0xa4/0x4e4
> Hardware name: Generic DT based system
> [<c01104ac>] (unwind_backtrace) from [<c010c0b4>] (show_stack+0x10/0x14)
> [<c010c0b4>] (show_stack) from [<c06cfb34>] (dump_stack+0x90/0xa4)
> [<c06cfb34>] (dump_stack) from [<c012d76c>] (__warn+0xb8/0xd4)
> [<c012d76c>] (__warn) from [<c012d7ec>] (warn_slowpath_fmt+0x64/0xbc)
> [<c012d7ec>] (warn_slowpath_fmt) from [<c04baf70>] (arm_lpae_map+0xa4/0x4e4)
> [<c04baf70>] (arm_lpae_map) from [<c0a1de3c>] (arm_lpae_do_selftests+0x234/0x688)
> [<c0a1de3c>] (arm_lpae_do_selftests) from [<c0102788>] (do_one_initcall+0x50/0x1b8)
> [<c0102788>] (do_one_initcall) from [<c0a0148c>] (kernel_init_freeable+0x1ec/0x25c)
> [<c0a0148c>] (kernel_init_freeable) from [<c06e6dc4>] (kernel_init+0x8/0x110)
> [<c06e6dc4>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
> Exception stack(0xee867fb0 to 0xee867ff8)
> 7fa0: 00000000 00000000 00000000 00000000
> 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 89e831c50a111e7c ]---
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 1 at drivers/iommu/io-pgtable-arm.c:1182 arm_lpae_do_selftests+0x4b8/0x688
> selftest: test failed for fmt idx 0
> Hardware name: Generic DT based system
> [<c01104ac>] (unwind_backtrace) from [<c010c0b4>] (show_stack+0x10/0x14)
> [<c010c0b4>] (show_stack) from [<c06cfb34>] (dump_stack+0x90/0xa4)
> [<c06cfb34>] (dump_stack) from [<c012d76c>] (__warn+0xb8/0xd4)
> [<c012d76c>] (__warn) from [<c012d820>] (warn_slowpath_fmt+0x98/0xbc)
> [<c012d820>] (warn_slowpath_fmt) from [<c0a1e0c0>] (arm_lpae_do_selftests+0x4b8/0x688)
> [<c0a1e0c0>] (arm_lpae_do_selftests) from [<c0102788>] (do_one_initcall+0x50/0x1b8)
> [<c0102788>] (do_one_initcall) from [<c0a0148c>] (kernel_init_freeable+0x1ec/0x25c)
> [<c0a0148c>] (kernel_init_freeable) from [<c06e6dc4>] (kernel_init+0x8/0x110)
> [<c06e6dc4>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
> Exception stack(0xee867fb0 to 0xee867ff8)
> 7fa0: 00000000 00000000 00000000 00000000
> 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 89e831c50a111e7d ]---
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 32-bit
> arm-lpae io-pgtable: data: 3 levels, 0x20 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 36
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 36-bit
> arm-lpae io-pgtable: data: 3 levels, 0x200 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 40
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 40-bit
> arm-lpae io-pgtable: data: 4 levels, 0x10 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 42
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 42-bit
> arm-lpae io-pgtable: data: 4 levels, 0x40 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 44
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 44-bit
> arm-lpae io-pgtable: data: 4 levels, 0x100 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 48
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 48-bit
> arm-lpae io-pgtable: data: 4 levels, 0x1000 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 48
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 48
> arm-lpae io-pgtable: selftest: completed with 12 PASS 6 FAIL
>
> Any suggestions how to fix this properly?
>
> Thanks,
> Stephan
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID
From: Robin Murphy <robin.murphy@arm.com>
To: Stephan Gerhold <stephan@gerhold.net>, Will Deacon <will@kernel.org>
Cc: iommu@lists.linux-foundation.org, kernel-team@android.com,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 8/8] iommu/io-pgtable-arm: Prepare for TTBR1 usage
Date: Wed, 19 Feb 2020 17:51:14 +0000 [thread overview]
Message-ID: <615b5077-4cd0-d5b0-5147-202f8c0771c7@arm.com> (raw)
In-Reply-To: <20200219122711.GA176090@gerhold.net>
On 19/02/2020 12:27 pm, Stephan Gerhold wrote:
> Hi Will, Hi Robin,
>
> On Fri, Jan 10, 2020 at 03:28:52PM +0000, Will Deacon wrote:
>> From: Robin Murphy <robin.murphy@arm.com>
>>
>> Now that we can correctly extract top-level indices without relying on
>> the remaining upper bits being zero, the only remaining impediments to
>> using a given table for TTBR1 are the address validation on map/unmap
>> and the awkward TCR translation granule format. Add a quirk so that we
>> can do the right thing at those points.
>>
>> Tested-by: Jordan Crouse <jcrouse@codeaurora.org>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> Signed-off-by: Will Deacon <will@kernel.org>
>> ---
>> drivers/iommu/io-pgtable-arm.c | 25 +++++++++++++++++++------
>> include/linux/io-pgtable.h | 4 ++++
>> 2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
>> index 846963c87e0f..5e966ef0bacf 100644
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -104,6 +104,10 @@
>> #define ARM_LPAE_TCR_TG0_64K 1
>> #define ARM_LPAE_TCR_TG0_16K 2
>>
>> +#define ARM_LPAE_TCR_TG1_16K 1
>> +#define ARM_LPAE_TCR_TG1_4K 2
>> +#define ARM_LPAE_TCR_TG1_64K 3
>> +
>> #define ARM_LPAE_TCR_SH_NS 0
>> #define ARM_LPAE_TCR_SH_OS 2
>> #define ARM_LPAE_TCR_SH_IS 3
>> @@ -463,6 +467,7 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>> arm_lpae_iopte *ptep = data->pgd;
>> int ret, lvl = data->start_level;
>> arm_lpae_iopte prot;
>> + long iaext = (long)iova >> cfg->ias;
>>
>> /* If no access, then nothing to do */
>> if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
>> @@ -471,7 +476,9 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>> if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size))
>> return -EINVAL;
>>
>> - if (WARN_ON(iova >> data->iop.cfg.ias || paddr >> data->iop.cfg.oas))
>> + if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1)
>> + iaext = ~iaext;
>> + if (WARN_ON(iaext || paddr >> cfg->oas))
>> return -ERANGE;
>>
>> prot = arm_lpae_prot_to_pte(data, iommu_prot);
>> @@ -637,11 +644,14 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
>> struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
>> struct io_pgtable_cfg *cfg = &data->iop.cfg;
>> arm_lpae_iopte *ptep = data->pgd;
>> + long iaext = (long)iova >> cfg->ias;
>>
>> if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size))
>> return 0;
>>
>> - if (WARN_ON(iova >> data->iop.cfg.ias))
>> + if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1)
>> + iaext = ~iaext;
>> + if (WARN_ON(iaext))
>> return 0;
>>
>> return __arm_lpae_unmap(data, gather, iova, size, data->start_level, ptep);
>
> This part of the patch seems to break io-pgtable-arm.c on ARM32 to some
> extent. I have a device using qcom_iommu that could normally run ARM64,
> but I'm unable to because of outdated (signed) firmware. :(
> So it's running a lot of code that would normally run only on ARM64.
>
> It used to work quite well but now qcom-venus fails to probe on:
> if (WARN_ON(iaext || paddr >> cfg->oas))
> (I added some debug prints for clarity.)
>
> qcom-venus 1d00000.video-codec: Adding to iommu group 0
> arm-lpae io-pgtable: quirks: 0x0
> arm-lpae io-pgtable: arm_lpae_map: iova: 0xdd800000, paddr: 0xebe3f000, iaext: 0xffffffff, paddr >> oas: 0x0
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 954 at drivers/iommu/io-pgtable-arm.c:487 arm_lpae_map+0xe4/0x510
> Hardware name: Generic DT based system
> ...
> [<c04bafb0>] (arm_lpae_map) from [<c04bcd6c>] (qcom_iommu_map+0x50/0x78)
> [<c04bcd6c>] (qcom_iommu_map) from [<c04b7290>] (__iommu_map+0xe8/0x1cc)
> [<c04b7290>] (__iommu_map) from [<c04b7bbc>] (__iommu_map_sg+0xa8/0x118)
> [<c04b7bbc>] (__iommu_map_sg) from [<c04b7c64>] (iommu_map_sg_atomic+0x18/0x20)
> [<c04b7c64>] (iommu_map_sg_atomic) from [<c04b94f8>] (iommu_dma_alloc+0x4dc/0x5a0)
> [<c04b94f8>] (iommu_dma_alloc) from [<c0196a50>] (dma_alloc_attrs+0x104/0x114)
> [<c0196a50>] (dma_alloc_attrs) from [<bf302c60>] (venus_hfi_create+0xa4/0x260 [venus_core])
> [<bf302c60>] (venus_hfi_create [venus_core]) from [<bf2fe6cc>] (venus_probe+0x1e4/0x328 [venus_core])
> [<bf2fe6cc>] (venus_probe [venus_core]) from [<c04c8eb4>] (platform_drv_probe+0x48/0x98)
> ...
> Exception stack(0xc2587fa8 to 0xc2587ff0)
> 7fa0: b6f3dc70 b5051010 b5051010 0017388c b6e645b0 b6e645b0
> 7fc0: b6f3dc70 b5051010 00020000 00000080 b6e30790 be84ea54 0046ac91 00000000
> 7fe0: b6e75f1c be84e940 b6e5fb51 b6eec8a4
> ---[ end trace 2a0ed284f6d82f16 ]---
> qcom-venus: probe of 1d00000.video-codec failed with error -12
>
> Note that iaext = 0xffffffff != 0.
> It seems to be some int/long size problem that only happens with larger
> iova addresses on 32-bit systems.
>
> Without the (long) cast for iova everything is working correctly again:
>
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 983b08477e64..f7ecc763c706 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -468,7 +468,7 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
> arm_lpae_iopte *ptep = data->pgd;
> int ret, lvl = data->start_level;
> arm_lpae_iopte prot;
> - long iaext = (long)iova >> cfg->ias;
> + long iaext = iova >> cfg->ias;
>
> /* If no access, then nothing to do */
> if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> @@ -645,7 +645,7 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
> struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> struct io_pgtable_cfg *cfg = &data->iop.cfg;
> arm_lpae_iopte *ptep = data->pgd;
> - long iaext = (long)iova >> cfg->ias;
> + long iaext = iova >> cfg->ias;
>
> if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size))
> return 0;
>
> But I'm not sure if this is really the correct "fix"...
No, that would end up breaking the TTBR1 case. The quirk isn't intended
to be supported on 32-bit platforms, but somehow it's slipped through
that this sign-extension still affects them (and goes wrong to boot),
sorry about that.
I think quick fixes would be either to replace the "(long)" cast with
"(s64)(u64)", or to just elide the iaext check altogether when 32-bit
longs make it impossible to pass an out-of-range IOVA anyway. I'll have
a think and see what works out nicest.
> This problem can be also easily reproduced by enabling
> IOMMU_IO_PGTABLE_LPAE_SELFTEST on ARM32.
> (Shouldn't there be some system that runs these automatically? ;))
>
> Without this patch all of them are passing:
>
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 48
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 48
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 48
> arm-lpae io-pgtable: selftest: completed with 18 PASS 0 FAIL
Hmm, we probably shouldn't be running IAS > 32 tests on 32-bit, given
that such configs are effectively impossible to use.
Robin.
>
> But with this patch 6 of them are failing (with a similar warning as
> I posted above):
>
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 32
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 1 at drivers/iommu/io-pgtable-arm.c:482 arm_lpae_map+0xa4/0x4e4
> Hardware name: Generic DT based system
> [<c01104ac>] (unwind_backtrace) from [<c010c0b4>] (show_stack+0x10/0x14)
> [<c010c0b4>] (show_stack) from [<c06cfb34>] (dump_stack+0x90/0xa4)
> [<c06cfb34>] (dump_stack) from [<c012d76c>] (__warn+0xb8/0xd4)
> [<c012d76c>] (__warn) from [<c012d7ec>] (warn_slowpath_fmt+0x64/0xbc)
> [<c012d7ec>] (warn_slowpath_fmt) from [<c04baf70>] (arm_lpae_map+0xa4/0x4e4)
> [<c04baf70>] (arm_lpae_map) from [<c0a1de3c>] (arm_lpae_do_selftests+0x234/0x688)
> [<c0a1de3c>] (arm_lpae_do_selftests) from [<c0102788>] (do_one_initcall+0x50/0x1b8)
> [<c0102788>] (do_one_initcall) from [<c0a0148c>] (kernel_init_freeable+0x1ec/0x25c)
> [<c0a0148c>] (kernel_init_freeable) from [<c06e6dc4>] (kernel_init+0x8/0x110)
> [<c06e6dc4>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
> Exception stack(0xee867fb0 to 0xee867ff8)
> 7fa0: 00000000 00000000 00000000 00000000
> 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 89e831c50a111e7c ]---
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 1 at drivers/iommu/io-pgtable-arm.c:1182 arm_lpae_do_selftests+0x4b8/0x688
> selftest: test failed for fmt idx 0
> Hardware name: Generic DT based system
> [<c01104ac>] (unwind_backtrace) from [<c010c0b4>] (show_stack+0x10/0x14)
> [<c010c0b4>] (show_stack) from [<c06cfb34>] (dump_stack+0x90/0xa4)
> [<c06cfb34>] (dump_stack) from [<c012d76c>] (__warn+0xb8/0xd4)
> [<c012d76c>] (__warn) from [<c012d820>] (warn_slowpath_fmt+0x98/0xbc)
> [<c012d820>] (warn_slowpath_fmt) from [<c0a1e0c0>] (arm_lpae_do_selftests+0x4b8/0x688)
> [<c0a1e0c0>] (arm_lpae_do_selftests) from [<c0102788>] (do_one_initcall+0x50/0x1b8)
> [<c0102788>] (do_one_initcall) from [<c0a0148c>] (kernel_init_freeable+0x1ec/0x25c)
> [<c0a0148c>] (kernel_init_freeable) from [<c06e6dc4>] (kernel_init+0x8/0x110)
> [<c06e6dc4>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
> Exception stack(0xee867fb0 to 0xee867ff8)
> 7fa0: 00000000 00000000 00000000 00000000
> 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 89e831c50a111e7d ]---
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 32-bit
> arm-lpae io-pgtable: data: 3 levels, 0x20 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 36
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 36-bit
> arm-lpae io-pgtable: data: 3 levels, 0x200 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 40
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 40-bit
> arm-lpae io-pgtable: data: 4 levels, 0x10 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 42
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 42-bit
> arm-lpae io-pgtable: data: 4 levels, 0x40 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 44
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 44-bit
> arm-lpae io-pgtable: data: 4 levels, 0x100 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 48
> [warning as above]
> arm-lpae io-pgtable: cfg: pgsize_bitmap 0x40201000, ias 48-bit
> arm-lpae io-pgtable: data: 4 levels, 0x1000 pgd_size, 12 pg_shift, 9 bits_per_level, pgd @ (ptrval)
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x02004000, IAS 48
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 32
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 36
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 40
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 42
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 44
> arm-lpae io-pgtable: selftest: pgsize_bitmap 0x20010000, IAS 48
> arm-lpae io-pgtable: selftest: completed with 12 PASS 6 FAIL
>
> Any suggestions how to fix this properly?
>
> Thanks,
> Stephan
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-02-19 17:51 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-10 15:28 [PATCH 0/8] Finish off the split page table prep work Will Deacon
2020-01-10 15:28 ` Will Deacon
2020-01-10 15:28 ` [PATCH 1/8] iommu/io-pgtable-arm: Rationalise TTBRn handling Will Deacon
2020-01-10 15:28 ` Will Deacon
2020-01-10 15:28 ` [PATCH 2/8] iommu/io-pgtable-arm: Support non-coherent stage-2 page tables Will Deacon
2020-01-10 15:28 ` Will Deacon
2020-01-10 15:28 ` [PATCH 3/8] iommu/io-pgtable-arm: Ensure non-cacheable mappings are Outer Shareable Will Deacon
2020-01-10 15:28 ` Will Deacon
2020-01-10 15:28 ` [PATCH 4/8] iommu/io-pgtable-arm: Ensure ARM_64_LPAE_S2_TCR_RES1 is unsigned Will Deacon
2020-01-10 15:28 ` Will Deacon
2020-01-10 15:28 ` [PATCH 5/8] iommu/io-pgtable-arm: Rationalise TCR handling Will Deacon
2020-01-10 15:28 ` Will Deacon
2020-01-10 15:28 ` [PATCH 6/8] iommu/arm-smmu: Rename public #defines under ARM_SMMU_ namespace Will Deacon
2020-01-10 15:28 ` Will Deacon
2020-01-10 15:28 ` [PATCH 7/8] iommu/io-pgtable-arm: Rationalise VTCR handling Will Deacon
2020-01-10 15:28 ` Will Deacon
2020-01-10 15:28 ` [PATCH 8/8] iommu/io-pgtable-arm: Prepare for TTBR1 usage Will Deacon
2020-01-10 15:28 ` Will Deacon
2020-02-19 12:27 ` Stephan Gerhold
2020-02-19 12:27 ` Stephan Gerhold
2020-02-19 17:51 ` Robin Murphy [this message]
2020-02-19 17:51 ` Robin Murphy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=615b5077-4cd0-d5b0-5147-202f8c0771c7@arm.com \
--to=robin.murphy@arm.com \
--cc=iommu@lists.linux-foundation.org \
--cc=kernel-team@android.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=stephan@gerhold.net \
--cc=will@kernel.org \
--subject='Re: [PATCH 8/8] iommu/io-pgtable-arm: Prepare for TTBR1 usage' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.