On Thu, Aug 22, 2019 at 11:57:11AM +0100, Robin Murphy wrote:

On 2019-08-22 11:17 am, Will Deacon wrote:

quoted

On Thu, Aug 22, 2019 at 11:08:58AM +0100, Robin Murphy wrote:

quoted

On 2019-08-22 9:56 am, Yong Wu wrote:

quoted

On Wed, 2019-08-21 at 16:24 +0100, Will Deacon wrote:

quoted

On Wed, Aug 21, 2019 at 09:53:12PM +0800, Yong Wu wrote:

quoted

MediaTek extend the arm v7s descriptor to support up to 34 bits PA where
the bit32 and bit33 are encoded in the bit9 and bit4 of the PTE
respectively. Meanwhile the iova still is 32bits.

Regarding whether the pagetable address could be over 4GB, the mt8183
support it while the previous mt8173 don't, thus keep it as is.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
   drivers/iommu/io-pgtable-arm-v7s.c | 32 +++++++++++++++++++++++++-------
   include/linux/io-pgtable.h         |  7 +++----
   2 files changed, 28 insertions(+), 11 deletions(-)

[...]

quoted

@@ -731,7 +747,9 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
   {
   	struct arm_v7s_io_pgtable *data;
-	if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
+	if (cfg->ias > ARM_V7S_ADDR_BITS ||
+	    (cfg->oas > ARM_V7S_ADDR_BITS &&
+	     !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)))

Please can you instead change arm_v7s_alloc_pgtable() so that it allows an
ias of up to 34 when the IO_PGTABLE_QUIRK_ARM_MTK_EXT is set?

Here I only simply skip the oas checking for our case. then which way do
your prefer?  something like you commented before:?


	if (cfg->ias > ARM_V7S_ADDR_BITS)
		return NULL;

	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT) {
		if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
			cfg->oas = min(cfg->oas, ARM_V7S_ADDR_BITS);
		else if (cfg->oas > 34)
			return NULL;
	} else if (cfg->oas > ARM_V7S_ADDR_BITS) {
		return NULL;
	}

All it should take is something like:

	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)
		max_oas = 34;
	else
		max_oas = 32;
	if (cfg->oas > max_oas)
		return NULL;

or even just:

	if (cfg->oas > 32 ||
	    (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT && cfg->oas > 34))
		return NULL;

(and if we prefer the latter style, perhaps we could introduce some kind of
"is_mtk_4gb()" helper to save on verbosity)

I wondered the same thing, but another place we'd want the check is in
iopte_to_paddr() which probably needs the PHYS_ADDR_T check to avoid GCC
warnings, although I didn't try it.

I'm pretty sure I confirmed that "paddr |= BIT_ULL(32)" doesn't warn when
phys_addt_t is 32-bit - it's well-defined unsigned integer truncation after
all, and if GCC starts warning about all the valid no-op code it optimises
away then it's going to run up against IS_ENABLED() first and foremost ;)

You're quite right, although we live in a world where GCC shouts at us about
missing comments in switch statements so I think my worry was justified!

quoted

So if we did:

static bool cfg_mtk_ext_enabled(struct io_pgtable_cfg *cfg)
{
	return IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) &&
	       cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT;
}

Then I suppose we could do this in _alloc():

	if (cfg->oas > cfg_mtk_ext_enabled(cfg) ? 34 : ARM_V7S_ADDR_BITS)
		return NULL;

^^ Apparantly, I left the bracketting here as an exercise to the reader.

quoted

and then this in iopte_to_paddr():

	[...]

	paddr = pte & mask;
	if (!cfg_mtk_ext_enabled(cfg))
		return paddr;

	if (pte & ARM_V7S_ATTR_MTK_PA_BIT32)
		paddr |= ...

	[...]

What do you reckon?

Yeah, that's the general shape of things I was picturing - I'm not that
fussed about the PHYS_ADDR_T_64BIT thing, especially if it's wrapped up in
just one place, so if you do want to keep it as belt-and-braces I'll just
consider it a slight code size optimisation for 32-bit builds.

Ok, great. Yong Wu -- are you ok respinning with the above + missing
brackets?

Will