All of lore.kernel.org
 help / color / mirror / Atom feed
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

  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.