Date:   Thu, 1 Sep 2022 02:15:01 +0200
From:   Janne Grunau <j@...nau.net>
To:     Robin Murphy <robin.murphy@....com>
Cc:     iommu@...ts.linux-foundation.org,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        asahi@...ts.linux.dev, Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Hector Martin <marcan@...can.st>,
        Joerg Roedel <joro@...tes.org>,
        Sven Peter <sven@...npeter.dev>, Will Deacon <will@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/5] iommu/io-pgtable: Move Apple DART support to its
 own file

On 2022-06-27 16:09:23 +0100, Robin Murphy wrote:
> On 2022-06-21 08:18, Janne Grunau wrote:
> > The pte format used by the DARTs found in the Apple M1 (t8103) is not
> > fully compatible with io-pgtable-arm. The 24 MSB are used for subpage
> > protection (mapping only parts of page) and conflict with the address
> > mask. In addition bit 1 is not available for tagging entries but disables
> > subpage protection. Subpage protection could be useful to support a CPU
> > granule of 4k with the fixed IOMMU page size of 16k.
> > 
> > The DARTs found on Apple M1 Pro/Max/Ultra use another different pte
> > format which is even less compatible. To support an output address size
> > of 42 bit the address is shifted down by 4. Subpage protection is
> > mandatory and bit 1 signifies uncached mappings used by the display
> > controller.
> > 
> > It would be advantageous to share code for all known Apple DART
> > variants to support common features. The page table allocator for DARTs
> > is less complex since it uses a two levels of translation table without
> > support for huge pages.
> > 
> > Signed-off-by: Janne Grunau <j@...nau.net>
> > 
> > ---
> > 
> > Changes in v3:
> > - move APPLE_DART to its own io-pgtable implementation, copied from
> >    io-pgtable-arm and simplified
> > 
> > Changes in v2:
> > - add APPLE_DART2 io-pgtable format
> > 
> >   MAINTAINERS                     |   1 +
> >   drivers/iommu/Kconfig           |   1 -
> >   drivers/iommu/Makefile          |   2 +-
> >   drivers/iommu/io-pgtable-arm.c  |  63 ----
> >   drivers/iommu/io-pgtable-dart.c | 580 ++++++++++++++++++++++++++++++++
> >   drivers/iommu/io-pgtable.c      |   2 +
> >   6 files changed, 584 insertions(+), 65 deletions(-)
> >   create mode 100644 drivers/iommu/io-pgtable-dart.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1fc9ead83d2a..028b7e31e589 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1848,6 +1848,7 @@ F:	drivers/clk/clk-apple-nco.c
> >   F:	drivers/i2c/busses/i2c-pasemi-core.c
> >   F:	drivers/i2c/busses/i2c-pasemi-platform.c
> >   F:	drivers/iommu/apple-dart.c
> > +F:	drivers/iommu/io-pgtable-dart.c
> >   F:	drivers/irqchip/irq-apple-aic.c
> >   F:	drivers/mailbox/apple-mailbox.c
> >   F:	drivers/nvme/host/apple.c
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index c79a0df090c0..ed6d8260f330 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -294,7 +294,6 @@ config APPLE_DART
> >   	tristate "Apple DART IOMMU Support"
> >   	depends on ARCH_APPLE || (COMPILE_TEST && !GENERIC_ATOMIC64)
> >   	select IOMMU_API
> > -	select IOMMU_IO_PGTABLE_LPAE
> 
> You still need to select the base IO_PGTABLE. FWIW I think that's probably
> the only crucial issue here - lots more comments below, but they're
> primarily things that could all be unpicked later.

fixed locally. io-pgtable-dart / dart build is now splitted to allow 
building dart as module.

> >   	default ARCH_APPLE
> >   	help
> >   	  Support for Apple DART (Device Address Resolution Table) IOMMUs
> > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> > index 44475a9b3eea..bd68c93bbd62 100644
> > --- a/drivers/iommu/Makefile
> > +++ b/drivers/iommu/Makefile
> > @@ -29,4 +29,4 @@ obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
> >   obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> >   obj-$(CONFIG_IOMMU_SVA) += iommu-sva-lib.o io-pgfault.o
> >   obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
> > -obj-$(CONFIG_APPLE_DART) += apple-dart.o
> > +obj-$(CONFIG_APPLE_DART) += apple-dart.o io-pgtable-dart.o
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > index 94ff319ae8ac..d7f5e23da643 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -130,9 +130,6 @@
> >   #define ARM_MALI_LPAE_MEMATTR_IMP_DEF	0x88ULL
> >   #define ARM_MALI_LPAE_MEMATTR_WRITE_ALLOC 0x8DULL
> > -#define APPLE_DART_PTE_PROT_NO_WRITE (1<<7)
> > -#define APPLE_DART_PTE_PROT_NO_READ (1<<8)
> > -
> >   /* IOPTE accessors */
> >   #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))
> > @@ -406,15 +403,6 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
> >   {
> >   	arm_lpae_iopte pte;
> > -	if (data->iop.fmt == APPLE_DART) {
> > -		pte = 0;
> > -		if (!(prot & IOMMU_WRITE))
> > -			pte |= APPLE_DART_PTE_PROT_NO_WRITE;
> > -		if (!(prot & IOMMU_READ))
> > -			pte |= APPLE_DART_PTE_PROT_NO_READ;
> > -		return pte;
> > -	}
> > -
> >   	if (data->iop.fmt == ARM_64_LPAE_S1 ||
> >   	    data->iop.fmt == ARM_32_LPAE_S1) {
> >   		pte = ARM_LPAE_PTE_nG;
> > @@ -1107,52 +1095,6 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
> >   	return NULL;
> >   }
> > -static struct io_pgtable *
> > -apple_dart_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
> > -{
> > -	struct arm_lpae_io_pgtable *data;
> > -	int i;
> > -
> > -	if (cfg->oas > 36)
> > -		return NULL;
> > -
> > -	data = arm_lpae_alloc_pgtable(cfg);
> > -	if (!data)
> > -		return NULL;
> > -
> > -	/*
> > -	 * The table format itself always uses two levels, but the total VA
> > -	 * space is mapped by four separate tables, making the MMIO registers
> > -	 * an effective "level 1". For simplicity, though, we treat this
> > -	 * equivalently to LPAE stage 2 concatenation at level 2, with the
> > -	 * additional TTBRs each just pointing at consecutive pages.
> > -	 */
> > -	if (data->start_level < 1)
> > -		goto out_free_data;
> > -	if (data->start_level == 1 && data->pgd_bits > 2)
> > -		goto out_free_data;
> > -	if (data->start_level > 1)
> > -		data->pgd_bits = 0;
> > -	data->start_level = 2;
> > -	cfg->apple_dart_cfg.n_ttbrs = 1 << data->pgd_bits;
> > -	data->pgd_bits += data->bits_per_level;
> > -
> > -	data->pgd = __arm_lpae_alloc_pages(ARM_LPAE_PGD_SIZE(data), GFP_KERNEL,
> > -					   cfg);
> > -	if (!data->pgd)
> > -		goto out_free_data;
> > -
> > -	for (i = 0; i < cfg->apple_dart_cfg.n_ttbrs; ++i)
> > -		cfg->apple_dart_cfg.ttbr[i] =
> > -			virt_to_phys(data->pgd + i * ARM_LPAE_GRANULE(data));
> > -
> > -	return &data->iop;
> > -
> > -out_free_data:
> > -	kfree(data);
> > -	return NULL;
> > -}
> > -
> >   struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
> >   	.alloc	= arm_64_lpae_alloc_pgtable_s1,
> >   	.free	= arm_lpae_free_pgtable,
> > @@ -1178,11 +1120,6 @@ struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
> >   	.free	= arm_lpae_free_pgtable,
> >   };
> > -struct io_pgtable_init_fns io_pgtable_apple_dart_init_fns = {
> > -	.alloc	= apple_dart_alloc_pgtable,
> > -	.free	= arm_lpae_free_pgtable,
> > -};
> > -
> >   #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST
> >   static struct io_pgtable_cfg *cfg_cookie __initdata;
> > diff --git a/drivers/iommu/io-pgtable-dart.c b/drivers/iommu/io-pgtable-dart.c
> > new file mode 100644
> > index 000000000000..0c5222942c65
> > --- /dev/null
> > +++ b/drivers/iommu/io-pgtable-dart.c
> > @@ -0,0 +1,580 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Apple DART page table allocator.
> > + *
> > + * Copyright (C) 2022 The Asahi Linux Contributors
> > + *
> > + * Based on io-pgtable-arm.
> > + *
> > + * Copyright (C) 2014 ARM Limited
> > + *
> > + * Author: Will Deacon <will.deacon@....com>
> > + */
> > +
> > +#define pr_fmt(fmt)	"dart io-pgtable: " fmt
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/bitops.h>
> > +#include <linux/io-pgtable.h>
> > +#include <linux/kernel.h>
> > +#include <linux/sizes.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#include <asm/barrier.h>
> > +
> > +#define DART_MAX_ADDR_BITS		52
> 
> Surely 36, no?
> 
> > +#define DART_MAX_LEVELS		3
> 
> It might be simpler in the long run to drop the awkward pretence and handle
> these in their "native" form of multiple 2-level tables.

ack, removed the recursive map, unmap and translation
 
> > +
> > +/* Struct accessors */
> > +#define io_pgtable_to_data(x)						\
> > +	container_of((x), struct dart_io_pgtable, iop)
> > +
> > +#define io_pgtable_ops_to_data(x)					\
> > +	io_pgtable_to_data(io_pgtable_ops_to_pgtable(x))
> > +
> > +/*
> > + * Calculate the right shift amount to get to the portion describing level l
> > + * in a virtual address mapped by the pagetable in d.
> > + */
> > +#define DART_LVL_SHIFT(l, d)						\
> > +	(((DART_MAX_LEVELS - (l)) * (d)->bits_per_level) +		\
> > +	ilog2(sizeof(dart_iopte)))
> > +
> > +#define DART_GRANULE(d)						\
> > +	(sizeof(dart_iopte) << (d)->bits_per_level)
> > +#define DART_PGD_SIZE(d)					\
> > +	(sizeof(dart_iopte) << (d)->pgd_bits)
> > +
> > +#define DART_PTES_PER_TABLE(d)					\
> > +	(DART_GRANULE(d) >> ilog2(sizeof(dart_iopte)))
> > +
> > +/*
> > + * Calculate the index at level l used to map virtual address a using the
> > + * pagetable in d.
> > + */
> > +#define DART_PGD_IDX(l, d)						\
> > +	((l) == (d)->start_level ? (d)->pgd_bits - (d)->bits_per_level : 0)
> > +
> > +#define DART_LVL_IDX(a, l, d)						\
> > +	(((u64)(a) >> DART_LVL_SHIFT(l, d)) &				\
> > +	 ((1 << ((d)->bits_per_level + DART_PGD_IDX(l, d))) - 1))
> > +
> > +/* Calculate the block/page mapping size at level l for pagetable in d. */
> > +#define DART_BLOCK_SIZE(l, d)	(1ULL << DART_LVL_SHIFT(l, d))
> 
> If you don't actually have block mappings, much of the above is really more
> complicated than you need.

removed

> > +
> > +#define APPLE_DART1_PADDR_MASK	GENMASK_ULL(35, 12)
> > +
> > +/* Apple DART1 protection bits */
> > +#define APPLE_DART1_PTE_PROT_NO_READ	BIT(8)
> > +#define APPLE_DART1_PTE_PROT_NO_WRITE	BIT(7)
> > +#define APPLE_DART1_PTE_PROT_SP_DIS	BIT(1)
> > +
> > +/* marks PTE as valid */
> > +#define APPLE_DART_PTE_VALID		BIT(0)
> > +
> > +/* IOPTE accessors */
> > +#define iopte_deref(pte, d) __va(iopte_to_paddr(pte, d))
> > +
> > +struct dart_io_pgtable {
> > +	struct io_pgtable	iop;
> > +
> > +	int			pgd_bits;
> > +	int			start_level;
> 
> If you don't have a variable number of levels, this is a constant.

removed

> > +	int			bits_per_level;
> > +
> > +	void			*pgd;
> > +};
> > +
> > +typedef u64 dart_iopte;
> > +
> > +static inline bool iopte_leaf(dart_iopte pte, int lvl,
> > +			      enum io_pgtable_fmt fmt)
> > +{
> > +	return (lvl == (DART_MAX_LEVELS - 1)) && (pte & APPLE_DART_PTE_VALID);
> > +}
> > +
> > +static dart_iopte paddr_to_iopte(phys_addr_t paddr,
> > +				     struct dart_io_pgtable *data)
> > +{
> > +	return paddr & APPLE_DART1_PADDR_MASK;
> > +}
> > +
> > +static phys_addr_t iopte_to_paddr(dart_iopte pte,
> > +				  struct dart_io_pgtable *data)
> > +{
> > +	return pte & APPLE_DART1_PADDR_MASK;
> > +}
> > +
> > +static void *__dart_alloc_pages(size_t size, gfp_t gfp,
> > +				    struct io_pgtable_cfg *cfg)
> > +{
> > +	struct device *dev = cfg->iommu_dev;
> > +	int order = get_order(size);
> > +	struct page *p;
> > +
> > +	VM_BUG_ON((gfp & __GFP_HIGHMEM));
> > +	p = alloc_pages_node(dev ? dev_to_node(dev) : NUMA_NO_NODE,
> > +			     gfp | __GFP_ZERO, order);
> > +	if (!p)
> > +		return NULL;
> > +
> > +	return page_address(p);
> > +}
> > +
> > +static void __dart_free_pages(void *pages, size_t size)
> > +{
> > +	free_pages((unsigned long)pages, get_order(size));
> > +}
> > +
> > +static void __dart_init_pte(struct dart_io_pgtable *data,
> > +				phys_addr_t paddr, dart_iopte prot,
> > +				int lvl, int num_entries, dart_iopte *ptep)
> > +{
> > +	dart_iopte pte = prot;
> > +	size_t sz = DART_BLOCK_SIZE(lvl, data);
> > +	int i;
> > +
> > +	if (lvl == DART_MAX_LEVELS - 1)
> > +		pte |= APPLE_DART1_PTE_PROT_SP_DIS;
> > +
> > +	pte |= APPLE_DART_PTE_VALID;
> > +
> > +	for (i = 0; i < num_entries; i++)
> > +		ptep[i] = pte | paddr_to_iopte(paddr + i * sz, data);
> > +}
> > +
> > +static int dart_init_pte(struct dart_io_pgtable *data,
> > +			     unsigned long iova, phys_addr_t paddr,
> > +			     dart_iopte prot, int lvl, int num_entries,
> > +			     dart_iopte *ptep)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < num_entries; i++)
> > +		if (iopte_leaf(ptep[i], lvl, data->iop.fmt)) {
> > +			/* We require an unmap first */
> > +			WARN_ON(iopte_leaf(ptep[i], lvl, data->iop.fmt));
> > +			return -EEXIST;
> > +		}
> > +
> > +	__dart_init_pte(data, paddr, prot, lvl, num_entries, ptep);
> > +	return 0;
> > +}
> > +
> > +static dart_iopte dart_install_table(dart_iopte *table,
> > +					     dart_iopte *ptep,
> > +					     dart_iopte curr,
> > +					     struct dart_io_pgtable *data)
> > +{
> > +	dart_iopte old, new;
> > +
> > +	new = paddr_to_iopte(__pa(table), data) | APPLE_DART_PTE_VALID;
> > +
> > +	/*
> > +	 * Ensure the table itself is visible before its PTE can be.
> > +	 * Whilst we could get away with cmpxchg64_release below, this
> > +	 * doesn't have any ordering semantics when !CONFIG_SMP.
> > +	 */
> > +	dma_wmb();
> > +
> > +	old = cmpxchg64_relaxed(ptep, curr, new);
> > +
> > +	return old;
> > +}
> > +
> > +static int __dart_map(struct dart_io_pgtable *data, unsigned long iova,
> > +			  phys_addr_t paddr, size_t size, size_t pgcount,
> > +			  dart_iopte prot, int lvl, dart_iopte *ptep,
> > +			  gfp_t gfp, size_t *mapped)
> > +{
> > +	dart_iopte *cptep, pte;
> > +	size_t block_size = DART_BLOCK_SIZE(lvl, data);
> > +	size_t tblsz = DART_GRANULE(data);
> > +	struct io_pgtable_cfg *cfg = &data->iop.cfg;
> > +	int ret = 0, num_entries, max_entries, map_idx_start;
> > +
> > +	/* Find our entry at the current level */
> > +	map_idx_start = DART_LVL_IDX(iova, lvl, data);
> > +	ptep += map_idx_start;
> > +
> > +	/* If we can install a leaf entry at this level, then do so */
> > +	if (size == block_size) {
> 
> It's a bit confusing to retain this logic when it's reduced to a rather
> long-winded obfuscatiuon of "if (lvl == 2)".

simplified due to a rewrite to a non-recursive form.

> > +		max_entries = DART_PTES_PER_TABLE(data) - map_idx_start;
> > +		num_entries = min_t(int, pgcount, max_entries);
> > +		ret = dart_init_pte(data, iova, paddr, prot, lvl, num_entries, ptep);
> > +		if (!ret && mapped)
> > +			*mapped += num_entries * size;
> > +
> > +		return ret;
> > +	}
> > +
> > +	/* We can't allocate tables at the final level */
> > +	if (WARN_ON(lvl >= DART_MAX_LEVELS - 1))
> > +		return -EINVAL;
> > +
> > +	/* Grab a pointer to the next level */
> > +	pte = READ_ONCE(*ptep);
> > +	if (!pte) {
> > +		cptep = __dart_alloc_pages(tblsz, gfp, cfg);
> > +		if (!cptep)
> > +			return -ENOMEM;
> > +
> > +		pte = dart_install_table(cptep, ptep, 0, data);
> > +		if (pte)
> > +			__dart_free_pages(cptep, tblsz);
> > +	}
> > +
> > +	if (pte && !iopte_leaf(pte, lvl, data->iop.fmt)) {
> > +		cptep = iopte_deref(pte, data);
> > +	} else if (pte) {
> > +		/* We require an unmap first */
> > +		WARN_ON(pte);
> 
> Similarly, you can't ever even get here, since the iopte_leaf() above can
> never be true. It would be significantly simpler to swap the two halves of
> this function and lose the recursion. The things you need to do at each
> level - resolve the pgd, look up or allocate the l1 table, install the l2
> pte - have almost no functional overlap, so simple procedural code would be
> far more suitable.

yes, done

> > +		return -EEXIST;
> > +	}
> > +
> > +	/* Rinse, repeat */
> > +	return __dart_map(data, iova, paddr, size, pgcount, prot, lvl + 1,
> > +			      cptep, gfp, mapped);
> > +}
> > +
> > +static dart_iopte dart_prot_to_pte(struct dart_io_pgtable *data,
> > +					   int prot)
> > +{
> > +	dart_iopte pte = 0;
> > +
> > +	if (!(prot & IOMMU_WRITE))
> > +		pte |= APPLE_DART1_PTE_PROT_NO_WRITE;
> > +	if (!(prot & IOMMU_READ))
> > +		pte |= APPLE_DART1_PTE_PROT_NO_READ;
> > +
> > +	return pte;
> > +}
> > +
> > +static int dart_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
> > +			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
> > +			      int iommu_prot, gfp_t gfp, size_t *mapped)
> > +{
> > +	struct dart_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > +	struct io_pgtable_cfg *cfg = &data->iop.cfg;
> > +	dart_iopte *ptep = data->pgd;
> > +	int ret, lvl = data->start_level;
> > +	dart_iopte prot;
> > +	long iaext = (s64)iova >> cfg->ias;
> 
> Do you have a split address space?

no, removed

> > +	if (WARN_ON(!pgsize || (pgsize & cfg->pgsize_bitmap) != pgsize))
> 
> Or effectively, "pgsize != cfg->pgsize_bitmap".

done

> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(iaext || paddr >> cfg->oas))
> > +		return -ERANGE;
> > +
> > +	/* If no access, then nothing to do */
> > +	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> > +		return 0;
> > +
> > +	prot = dart_prot_to_pte(data, iommu_prot);
> > +	ret = __dart_map(data, iova, paddr, pgsize, pgcount, prot, lvl,
> > +			     ptep, gfp, mapped);
> > +	/*
> > +	 * Synchronise all PTE updates for the new mapping before there's
> > +	 * a chance for anything to kick off a table walk for the new iova.
> > +	 */
> > +	wmb();
> > +
> > +	return ret;
> > +}
> > +
> > +static int dart_map(struct io_pgtable_ops *ops, unsigned long iova,
> > +			phys_addr_t paddr, size_t size, int iommu_prot, gfp_t gfp)
> > +{
> > +	return dart_map_pages(ops, iova, paddr, size, 1, iommu_prot, gfp,
> > +				  NULL);
> > +}
> 
> There's no user for these wrappers, surely?

I didn't realize those are optional, removed
 
> > +static void __dart_free_pgtable(struct dart_io_pgtable *data, int lvl,
> > +				    dart_iopte *ptep)
> > +{
> > +	dart_iopte *start, *end;
> > +	unsigned long table_size;
> > +
> > +	if (lvl == data->start_level)
> > +		table_size = DART_PGD_SIZE(data);
> > +	else
> > +		table_size = DART_GRANULE(data);
> > +
> > +	start = ptep;
> > +
> > +	/* Only leaf entries at the last level */
> > +	if (lvl == DART_MAX_LEVELS - 1)
> > +		end = ptep;
> > +	else
> > +		end = (void *)ptep + table_size;
> > +
> > +	while (ptep != end) {
> > +		dart_iopte pte = *ptep++;
> > +
> > +		if (!pte || iopte_leaf(pte, lvl, data->iop.fmt))
> > +			continue;
> > +
> > +		__dart_free_pgtable(data, lvl + 1, iopte_deref(pte, data));
> > +	}
> > +
> > +	__dart_free_pages(start, table_size);
> > +}
> > +
> > +static size_t __dart_unmap(struct dart_io_pgtable *data,
> > +			       struct iommu_iotlb_gather *gather,
> > +			       unsigned long iova, size_t size, size_t pgcount,
> > +			       int lvl, dart_iopte *ptep)
> > +{
> > +	dart_iopte pte;
> > +	struct io_pgtable *iop = &data->iop;
> > +	int i = 0, num_entries, max_entries, unmap_idx_start;
> > +
> > +	/* Something went horribly wrong and we ran out of page table */
> > +	if (WARN_ON(lvl == DART_MAX_LEVELS))
> > +		return 0;
> > +
> > +	unmap_idx_start = DART_LVL_IDX(iova, lvl, data);
> > +	ptep += unmap_idx_start;
> > +	pte = READ_ONCE(*ptep);
> > +	if (WARN_ON(!pte))
> > +		return 0;
> > +
> > +	/* If the size matches this level, we're in the right place */
> > +	if (size == DART_BLOCK_SIZE(lvl, data)) {
> > +		max_entries = DART_PTES_PER_TABLE(data) - unmap_idx_start;
> > +		num_entries = min_t(int, pgcount, max_entries);
> > +
> > +		while (i < num_entries) {
> > +			pte = READ_ONCE(*ptep);
> > +			if (WARN_ON(!pte))
> > +				break;
> > +
> > +			/* clear pte */
> > +			*ptep = 0;
> > +
> > +			if (!iopte_leaf(pte, lvl, iop->fmt)) {
> 
> Again, this can't ever happen, and being non-recursive would let you see the
> wood for the trees. I suppose you could in principle still optimise for the
> case of "pgcount == DART_PTES_PER_TABLE" with a slightly different code
> structure, but I highly doubt it's worth the bother.

removed

> > +				/* Also flush any partial walks */
> > +				io_pgtable_tlb_flush_walk(iop, iova + i * size, size,
> > +							  DART_GRANULE(data));
> > +				__dart_free_pgtable(data, lvl + 1, iopte_deref(pte, data));
> > +			} else if (!iommu_iotlb_gather_queued(gather)) {
> > +				io_pgtable_tlb_add_page(iop, gather, iova + i * size, size);
> > +			}
> > +
> > +			ptep++;
> > +			i++;
> > +		}
> > +
> > +		return i * size;
> > +	}
> > +
> > +	/* Keep on walkin' */
> > +	ptep = iopte_deref(pte, data);
> > +	return __dart_unmap(data, gather, iova, size, pgcount, lvl + 1, ptep);
> > +}
> > +
> > +static size_t dart_unmap_pages(struct io_pgtable_ops *ops, unsigned long iova,
> > +				   size_t pgsize, size_t pgcount,
> > +				   struct iommu_iotlb_gather *gather)
> > +{
> > +	struct dart_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > +	struct io_pgtable_cfg *cfg = &data->iop.cfg;
> > +	dart_iopte *ptep = data->pgd;
> > +	long iaext = (s64)iova >> cfg->ias;
> > +
> > +	if (WARN_ON(!pgsize || (pgsize & cfg->pgsize_bitmap) != pgsize || !pgcount))
> > +		return 0;
> > +
> > +	if (WARN_ON(iaext))
> > +		return 0;
> > +
> > +	return __dart_unmap(data, gather, iova, pgsize, pgcount,
> > +				data->start_level, ptep);
> > +}
> > +
> > +static size_t dart_unmap(struct io_pgtable_ops *ops, unsigned long iova,
> > +			     size_t size, struct iommu_iotlb_gather *gather)
> > +{
> > +	return dart_unmap_pages(ops, iova, size, 1, gather);
> > +}
> > +
> > +static phys_addr_t dart_iova_to_phys(struct io_pgtable_ops *ops,
> > +					 unsigned long iova)
> > +{
> > +	struct dart_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > +	dart_iopte pte, *ptep = data->pgd;
> > +	int lvl = data->start_level;
> > +
> > +	do {
> > +		/* Valid IOPTE pointer? */
> > +		if (!ptep)
> > +			return 0;
> > +
> > +		/* Grab the IOPTE we're interested in */
> > +		ptep += DART_LVL_IDX(iova, lvl, data);
> > +		pte = READ_ONCE(*ptep);
> > +
> > +		/* Valid entry? */
> > +		if (!pte)
> > +			return 0;
> > +
> > +		/* Leaf entry? */
> > +		if (iopte_leaf(pte, lvl, data->iop.fmt))
> > +			goto found_translation;
> > +
> > +		/* Take it to the next level */
> > +		ptep = iopte_deref(pte, data);
> > +	} while (++lvl < DART_MAX_LEVELS);
> 
> I imagine the compiler can unroll this loop already, but once again I'd
> argue that it's probably more readable to spell out the pgd and table
> lookups directly. When you have a fixed number of levels to walk, and know
> that you'll never find a valid translation before the last one, life is *so*
> much easier.

Agreed, much easier to follow and a net reduction in code size despite 
the "unrolling"

> > +
> > +	/* Ran out of page tables to walk */
> > +	return 0;
> > +
> > +found_translation:
> > +	iova &= (DART_BLOCK_SIZE(lvl, data) - 1);
> > +	return iopte_to_paddr(pte, data) | iova;
> > +}
> > +
> > +static void dart_restrict_pgsizes(struct io_pgtable_cfg *cfg)
> > +{
> > +	unsigned long granule, page_sizes;
> > +	unsigned int max_addr_bits = 48;
> > +
> > +	/*
> > +	 * We need to restrict the supported page sizes to match the
> > +	 * translation regime for a particular granule. Aim to match
> > +	 * the CPU page size if possible, otherwise prefer smaller sizes.
> > +	 * While we're at it, restrict the block sizes to match the
> > +	 * chosen granule.
> > +	 */
> > +	if (cfg->pgsize_bitmap & PAGE_SIZE)
> > +		granule = PAGE_SIZE;
> > +	else if (cfg->pgsize_bitmap & ~PAGE_MASK)
> > +		granule = 1UL << __fls(cfg->pgsize_bitmap & ~PAGE_MASK);
> > +	else if (cfg->pgsize_bitmap & PAGE_MASK)
> > +		granule = 1UL << __ffs(cfg->pgsize_bitmap & PAGE_MASK);
> > +	else
> > +		granule = 0;
> > +
> > +	switch (granule) {
> > +	case SZ_4K:
> > +		page_sizes = (SZ_4K | SZ_2M | SZ_1G);
> > +		break;
> > +	case SZ_16K:
> > +		page_sizes = (SZ_16K | SZ_32M);
> > +		break;
> > +	default:
> > +		page_sizes = 0;
> > +	}
> > +
> > +	cfg->pgsize_bitmap &= page_sizes;
> > +	cfg->ias = min(cfg->ias, max_addr_bits);
> > +	cfg->oas = min(cfg->oas, max_addr_bits);
> 
> Why is any of this here?
> 
> > +}
> > +
> > +static struct dart_io_pgtable *
> > +dart_alloc_pgtable(struct io_pgtable_cfg *cfg)
> > +{
> > +	struct dart_io_pgtable *data;
> > +	int bits_per_level, levels, va_bits, pg_shift;
> > +
> > +	dart_restrict_pgsizes(cfg);
> > +
> > +	if (!(cfg->pgsize_bitmap & (SZ_4K | SZ_16K)))
> > +		return NULL;
> > +
> > +	if (cfg->ias > DART_MAX_ADDR_BITS)
> > +		return NULL;
> > +
> > +	if (cfg->oas > DART_MAX_ADDR_BITS)
> > +		return NULL;
> 
> You already checked this in the caller below.
> 
> > +	pg_shift = __ffs(cfg->pgsize_bitmap);
> > +	bits_per_level = pg_shift - ilog2(sizeof(dart_iopte));
> > +
> > +	va_bits = cfg->ias - pg_shift;
> > +	levels = DIV_ROUND_UP(va_bits, bits_per_level);
> > +	if (levels > DART_MAX_LEVELS)
> > +		return NULL;
> > +
> > +	data = kmalloc(sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return NULL;
> > +
> > +	data->bits_per_level = bits_per_level;
> > +	data->start_level = DART_MAX_LEVELS - levels;
> > +
> > +	/* Calculate the actual size of our pgd (without concatenation) */
> > +	data->pgd_bits = va_bits - (data->bits_per_level * (levels - 1));
> > +
> > +	data->iop.ops = (struct io_pgtable_ops) {
> > +		.map		= dart_map,
> > +		.map_pages	= dart_map_pages,
> > +		.unmap		= dart_unmap,
> > +		.unmap_pages	= dart_unmap_pages,
> > +		.iova_to_phys	= dart_iova_to_phys,
> > +	};
> > +
> > +	return data;
> > +}
> > +
> > +static struct io_pgtable *
> > +apple_dart_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
> > +{
> > +	struct dart_io_pgtable *data;
> > +	int i;
> > +
> > +	if (!cfg->coherent_walk)
> > +		return NULL;
> > +
> > +	if (cfg->oas > 36)
> > +		return NULL;
> > +
> > +	data = dart_alloc_pgtable(cfg);
> > +	if (!data)
> > +		return NULL;
> > +
> > +	/*
> > +	 * The table format itself always uses two levels, but the total VA
> > +	 * space is mapped by four separate tables, making the MMIO registers
> > +	 * an effective "level 1". For simplicity, though, we treat this
> > +	 * equivalently to LPAE stage 2 concatenation at level 2, with the
> > +	 * additional TTBRs each just pointing at consecutive pages.
> > +	 */
> > +	if (data->start_level == 0 && data->pgd_bits > 2)
> > +		goto out_free_data;
> > +	if (data->start_level > 0)
> > +		data->pgd_bits = 0;
> > +	data->start_level = 1;
> > +	cfg->apple_dart_cfg.n_ttbrs = 1 << data->pgd_bits;
> > +	data->pgd_bits += data->bits_per_level;
> > +
> > +	data->pgd = __dart_alloc_pages(DART_PGD_SIZE(data), GFP_KERNEL,
> > +					   cfg);
> > +	if (!data->pgd)
> > +		goto out_free_data;
> 
> If recursive map/unmap goes away then you'll also be free to drop the forced
> concatenation and allocate pgds individually, which should then have further
> knock-on simplification effects elsewhere, primarily __dart_free_pgtable() I
> think.

Done although I seemed to have missed the simplification. for 16k iommu 
pages the concatenation should have been never used as a single table 
holds enoough address space. DART in the M2 has only one TTBR. Those two 
additional bits extend the address space for 4k pages to 32-bit.

> 
> Still, for the split from io-pgtable-arm, and with the Kconfig fixed,
> 
> Acked-by: Robin Murphy <rpbin.murphy@....com>

thanks

Janne

Powered by blists - more mailing lists