Re: [PATCH v8 07/21] iommu/io-pgtable-arm-v7s: Extend MediaTek 4GB Mode

[Will Deacon <will@kernel.org>]

On Thu, Jul 11, 2019 at 07:53:56PM +0800, Yong Wu wrote:
> On Wed, 2019-07-10 at 15:36 +0100, Will Deacon wrote:
> > On Sat, Jun 29, 2019 at 10:09:13AM +0800, Yong Wu wrote:
> > > MediaTek extend the arm v7s descriptor to support the dram over 4GB.
> > > 
> > > In the mt2712 and mt8173, it's called "4GB mode", the physical address
> > > is from 0x4000_0000 to 0x1_3fff_ffff, but from EMI point of view, it
> > > is remapped to high address from 0x1_0000_0000 to 0x1_ffff_ffff, the
> > > bit32 is always enabled. thus, in the M4U, we always enable the bit9
> > > for all PTEs which means to enable bit32 of physical address.
> > > 
> > > but in mt8183, M4U support the dram from 0x4000_0000 to 0x3_ffff_ffff
> > > which isn't remaped. We extend the PTEs: the bit9 represent bit32 of
> > > PA and the bit4 represent bit33 of PA. Meanwhile the iova still is
> > > 32bits.
> > 
> > What happens if bit4 is set in the pte for mt2712 or mt8173? Perhaps the
> 
> bit4 is ignored in mt2712 and mt8173(No effect).
> 
> > io-pgtable backend should be allowing oas > 32 when
> > IO_PGTABLE_QUIRK_ARM_MTK_4GB is set, and then enforcing that itself.
> 
> About oas, It looks the oas doesn't work in current the v7s. 
> 
> How about I add a new simple preparing patch like this(copy from
> io-pgtable-arm.c)?

This looks like the right sort of idea. Basically, I was thinking that you
can use the oas in conjunction with the quirk to specify whether or not
your two magic bits should be set. You could also then cap the oas using
the size of phys_addr_t to deal with my other comment.

Finally, I was hoping you could drop the |= BIT_ULL(32) and the &=
~BIT_ULL(32) bits of the mtk driver if the pgtable code now accepts higher
addresses. Did that not work out?
> 
> ==========================================
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -495,7 +495,8 @@ static int arm_v7s_map(struct io_pgtable_ops *ops,
> unsigned long iova,
>         if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
>                 return 0;
> 
> -       if (WARN_ON(upper_32_bits(iova) || upper_32_bits(paddr)))
> +       if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
> +                   paddr >= (1ULL << data->iop.cfg.oas)))
>                 return -ERANGE;
> 
> ===============================================
> 
> Then, change the oas in MTK 4GB mode, like this:
> 
> ================================================
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -721,7 +721,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_4GB)))

This should probably still be capped at 34 bits.

> > > +	paddr = pte & mask;
> > > +	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_4GB) {
> > > +		if (pte & ARM_V7S_ATTR_MTK_PA_BIT32)
> > > +			paddr |= BIT_ULL(32);
> > > +		if (pte & ARM_V7S_ATTR_MTK_PA_BIT33)
> > > +			paddr |= BIT_ULL(33);
> > > +	}
> > > +	return paddr;
> > 
> > I think this relies on CONFIG_PHYS_ADDR_T_64BIT, which isn't always set on
> > 32-bit ARM.
> 
> This was discussed at [1]. Robin commented that this is not needed and
> build won't complain about this.

It's not so much the build I was worried about, but more that we'd silently
be doing the wrong thing and I think we can fix that as I mentioned above.

Will