Re: [PATCH v5] iommu/io-pgtable-arm: Add support for contiguous hint bit

[Daniel Mentz]

On Mon, Sep 21, 2026 at 4:44 AM Vijayanand Jitta
<vijayanand.jitta@xxxxxxxxxxxxxxxx> wrote:
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 476c0e25631af..01d98959c514f 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -86,6 +86,21 @@
>  /* Software bit for solving coherency races */
>  #define ARM_LPAE_PTE_SW_SYNC           (((arm_lpae_iopte)1) << 55)
>
> +/* PTE Contiguous Bit */
> +#define ARM_LPAE_PTE_CONT              (((arm_lpae_iopte)1) << 52)
> +
> +/*
> + * Contiguous hint group sizes per granule:
> + *
> + *------------------------------------------------------------------
> + *| Page Size | CONT PTE |  Block  | CONT Block | L1 Block | CONT L1 |
> + *------------------------------------------------------------------
> + *|     4K    |   64K    |   2M    |    32M     |    1G    |   16G   |
> + *|    16K    |    2M    |  32M    |     1G     |          |         |
> + *|    64K    |    2M    | 512M    |    16G     |          |         |
> + *------------------------------------------------------------------
> + */

I find this comment redundant. People can find this information in the
Arm architecture specification.

> +static int arm_lpae_num_cont(size_t size)
> +{
> +       switch (size) {
> +       case SZ_4K:
> +       case SZ_2M:
> +       case SZ_1G:
> +               return 16;

I'm thinking that if you use something like

return BITS_PER_TYPE(size_t) >= 64 ? 16 : 1

i.e. return 16 only on 64 bit platforms, then you can avoid those
overflow checks in various places. Same for the SZ_512M cases.

> +       case SZ_64K:
> +       case SZ_32M:
> +       case SZ_512M:
> +               return 32;
> +       case SZ_16K:
> +               return 128;
> +       default:
> +               return 1;
> +       }
> +}
> +
>  static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
>                           phys_addr_t paddr, size_t size, size_t pgcount,
>                           arm_lpae_iopte prot, int lvl, arm_lpae_iopte *ptep,
> @@ -462,20 +495,41 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
>         size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
>         size_t tblsz = ARM_LPAE_GRANULE(data);
>         struct io_pgtable_cfg *cfg = &data->iop.cfg;
> -       int ret = 0, num_entries, max_entries, map_idx_start;
> +       int num_cont = arm_lpae_num_cont(block_size);
> +       size_t cont_size = 0, entries_per_map;
> +       int num_entries, max_entries, map_idx_start;
> +       bool cont = false;
> +
> +       if (num_cont > 1 && block_size <= SIZE_MAX / num_cont)
> +               cont_size = num_cont * block_size;
>
>         /* Find our entry at the current level */
>         map_idx_start = ARM_LPAE_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) {
> +       if (size == block_size ||
> +           (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT) &&

I'm thinking that the check for IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT is
redundant. If this quirk is active, none of the contiguous sizes were
advertised, so no one should call this function with any of the
contiguous sizes.

> +            size == cont_size)) {
> +               int ret;
> +
> +               cont = size == cont_size;
> +               if (cont && (!IS_ALIGNED(iova, size) || !IS_ALIGNED(paddr, size)))

These alignment checks are also redundant. We can rely on the caller
to pass properly aligned values. It's also inconsistent, because it
verifies alignment only for contiguous sizes.

> +                       return -EINVAL;
> +
> +               entries_per_map = size / block_size;

Can't you just do

pgcount *= num_cont;

Wouldn't that be easier?

>                 max_entries = arm_lpae_max_entries(map_idx_start, data);
> -               num_entries = min_t(int, pgcount, max_entries);
> -               ret = arm_lpae_init_pte(data, iova, paddr, prot, lvl, num_entries, ptep);
> +               num_entries = min_t(size_t, pgcount,
> +                                   max_entries / entries_per_map) * entries_per_map;
> +               if (!num_entries)
> +                       return -EINVAL;

I believe this check is also redundant. Can we remove it?

> +               if (cont)
> +                       prot |= ARM_LPAE_PTE_CONT;
> +
> +               ret = arm_lpae_init_pte(data, iova, paddr, prot, lvl,
> +                                       num_entries, ptep);
>                 if (!ret)
> -                       *mapped += num_entries * size;
> -
> +                       *mapped += num_entries * block_size;
>                 return ret;
>         }
>
> @@ -660,12 +714,18 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>  {
>         arm_lpae_iopte pte;
>         struct io_pgtable *iop = &data->iop;
> +       size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
> +       int num_cont = arm_lpae_num_cont(block_size);
> +       size_t cont_size = 0, entries_per_map;
>         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 == ARM_LPAE_MAX_LEVELS))
>                 return 0;
>
> +       if (num_cont > 1 && block_size <= SIZE_MAX / num_cont)
> +               cont_size = num_cont * block_size;
> +
>         unmap_idx_start = ARM_LPAE_LVL_IDX(iova, lvl, data);
>         ptep += unmap_idx_start;
>         pte = READ_ONCE(*ptep);
> @@ -675,9 +735,27 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>         }
>
>         /* If the size matches this level, we're in the right place */
> -       if (size == ARM_LPAE_BLOCK_SIZE(lvl, data)) {
> +       if (size == block_size ||
> +           (!(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT) &&

Checking for IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT is redundant. Can we remove it?

> +            size == cont_size)) {
> +               entries_per_map = size / block_size;
>                 max_entries = arm_lpae_max_entries(unmap_idx_start, data);
> -               num_entries = min_t(int, pgcount, max_entries);
> +               num_entries = min_t(size_t, pgcount,
> +                                   max_entries / entries_per_map) * entries_per_map;
> +               if (!num_entries)
> +                       return 0;
> +
> +               /*
> +                * A CONT group must be invalidated as a unit. Reject a request that
> +                * starts or ends inside a tagged group before changing any PTEs.
> +                */
> +               if ((READ_ONCE(*ptep) & ARM_LPAE_PTE_CONT &&
> +                    !IS_ALIGNED(iova, cont_size)) ||
> +                   (READ_ONCE(ptep[num_entries - 1]) & ARM_LPAE_PTE_CONT &&
> +                    !IS_ALIGNED(iova + num_entries * block_size, cont_size))) {
> +                       WARN_ONCE(true, "Unmap of a partial CONT IOPTE group is not allowed");
> +                       return 0;
> +               }
>
>                 /* Find and handle non-leaf entries */
>                 for (i = 0; i < num_entries; i++) {
> @@ -691,7 +769,8 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>                                 __arm_lpae_clear_pte(&ptep[i], &iop->cfg, 1);
>
>                                 /* Also flush any partial walks */
> -                               io_pgtable_tlb_flush_walk(iop, iova + i * size, size,
> +                               io_pgtable_tlb_flush_walk(iop,
> +                                                         iova + i * block_size, block_size,
>                                                           ARM_LPAE_GRANULE(data));
>                                 __arm_lpae_free_pgtable(data, lvl + 1, iopte_deref(pte, data));
>                         }
> @@ -702,9 +781,10 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>
>                 if (gather && !iommu_iotlb_gather_queued(gather))
>                         for (int j = 0; j < i; j++)
> -                               io_pgtable_tlb_add_page(iop, gather, iova + j * size, size);
> +                               io_pgtable_tlb_add_page(iop, gather,
> +                                                       iova + j * block_size, block_size);
>
> -               return i * size;
> +               return i * block_size;
>         } else if (iopte_leaf(pte, lvl, iop->fmt)) {
>                 WARN_ONCE(true, "Unmap of a partial large IOPTE is not allowed");
>                 return 0;
> @@ -943,8 +1023,23 @@ static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)

I want to re-iterate what I wrote earlier: I think we should change
this function's name. This is what our AI model has to say:

Originally, arm_lpae_restrict_pgsizes() performed a purely monotonic reduction:

  1. Identified the translation granule (e.g. matching CPU PAGE_SIZE).
  2. Performed a bitwise-AND (cfg->pgsize_bitmap &= page_sizes) to
discard non-granule sizes.
  3. Clamped ias and oas.

With this commit, it now:

  1. Restricts to the base granule sizes (&= page_sizes).
  2. Expands the bitmap with synthesized contiguous sizes (|= num_cont * size).
  3. Restricts again against ias and oas (&= GENMASK_ULL(...)).

Calling a function ..._restrict_... when it actively synthesizes and
injects new page sizes violates the principle of least astonishment.

>         }
>
>         cfg->pgsize_bitmap &= page_sizes;
> +       if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT)) {
> +               unsigned long sizes = cfg->pgsize_bitmap;
> +
> +               while (sizes) {
> +                       unsigned long size = BIT(__ffs(sizes));
> +                       int num_cont = arm_lpae_num_cont(size);
> +
> +                       if (size <= ULONG_MAX / num_cont)
> +                               cfg->pgsize_bitmap |= num_cont * size;
> +                       sizes &= ~size;
> +               }
> +       }
> +
>         cfg->ias = min(cfg->ias, max_addr_bits);
>         cfg->oas = min(cfg->oas, max_addr_bits);
> +       cfg->pgsize_bitmap &= GENMASK_ULL(cfg->ias, 0);
> +       cfg->pgsize_bitmap &= GENMASK_ULL(cfg->oas, 0);
>  }

Gemini added the following. I have to admit, though, that I'm not
familiar enough with dirty bit tracking to determine if this is a real
concern.

Interaction with Hardware Dirty Tracking (IO_PGTABLE_QUIRK_ARM_HD)

When Hardware Dirty Tracking (ARM_LPAE_PTE_DBM) is enabled on Stage-1 tables:
 * In visit_dirty() / arm_lpae_read_and_clear_dirty(), dirty bits are
queried and cleared on a page-by-page granularity
(iopte_set_writeable_clean(ptep)).
 * If a single 4KB page within a 64KB CONT group is marked clean while
neighboring pages remain marked dirty/writeable, the descriptors in
that group will differ in access permissions (AP[2]).
 * According to Arm ARM D8.3.1, all descriptors in a contiguous block
must share identical permissions and attributes. If dirty tracking is
active on a domain, consider whether IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT
should be set or whether CONT sizes should be suppressed when
IO_PGTABLE_QUIRK_ARM_HD is active.