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

On Mon, Aug 3, 2026 at 11:19 PM Vijayanand Jitta
<[email protected]> wrote:
> +static unsigned long arm_lpae_get_cont_sizes(struct io_pgtable_cfg *cfg)
> +{
> +       unsigned long pg_size, blk_size, l1_blk_size, cont_sizes = 0;
> +       unsigned long cont_leaf_size, cont_blk_size, cont_l1_blk_size;
> +       int pg_shift, bits_per_level;
> +
> +       if (!cfg->pgsize_bitmap || (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT))
> +               return 0;
> +
> +       pg_shift = __ffs(cfg->pgsize_bitmap);
> +       bits_per_level = pg_shift - ilog2(sizeof(arm_lpae_iopte));
> +       pg_size = 1UL << pg_shift;
> +       blk_size = pg_size << bits_per_level;
> +       l1_blk_size = blk_size << bits_per_level;
> +
> +       cont_leaf_size = arm_lpae_num_cont(pg_size) * pg_size;
> +       if ((cfg->pgsize_bitmap & pg_size) &&
> +           arm_lpae_cont_size_fits(cfg, cont_leaf_size))
> +               cont_sizes |= cont_leaf_size;
> +
> +       if (cfg->pgsize_bitmap & blk_size) {
> +               cont_blk_size = arm_lpae_num_cont(blk_size) * blk_size;
> +               if (arm_lpae_cont_size_fits(cfg, cont_blk_size))
> +                       cont_sizes |= cont_blk_size;
> +       }
> +
> +       /*
> +        * l1_blk_size is only set in pgsize_bitmap if level-1 blocks are
> +        * supported for this granule (not 16K/64K, per
> +        * arm_lpae_restrict_pgsizes()), so no extra gating is needed here.
> +        */
> +       if (cfg->pgsize_bitmap & l1_blk_size) {
> +               cont_l1_blk_size = arm_lpae_num_cont(l1_blk_size) * l1_blk_size;

Our AI model is saying that this might overflow cont_l1_blk_size on 32
bit platforms i.e. 16 * 1G doesn't fit into a 32 bit type. It says
that cont_l1_blk_size will be truncated to 0, and
arm_lpae_cont_size_fits() then calls ilog2(0) which is undefined.

> +               if (arm_lpae_cont_size_fits(cfg, cont_l1_blk_size))
> +                       cont_sizes |= cont_l1_blk_size;
> +       }
> +
> +       return cont_sizes;
> +}
[...]
> @@ -660,6 +829,8 @@ 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);
>         int i = 0, num_entries, max_entries, unmap_idx_start;
>
>         /* Something went horribly wrong and we ran out of page table */
> @@ -674,10 +845,22 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>                 return 0;
>         }
>
> +       /*
> +        * Normalize an exact whole-CONT-group request down to the
> +        * equivalent block_size/pgcount, mirroring __arm_lpae_map().
> +        */
> +       if (!(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT) &&
> +           num_cont > 1 && size == block_size * num_cont) {
> +               pgcount *= num_cont;
> +               size = block_size;
> +       }
> +
>         /* If the size matches this level, we're in the right place */
> -       if (size == ARM_LPAE_BLOCK_SIZE(lvl, data)) {
> +       if (size == block_size) {
> +               size_t cont_size = num_cont * 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);
>
>                 /* Find and handle non-leaf entries */

This comment is no longer accurate. The handling now extends beyond
non-leaf entries.

>                 for (i = 0; i < num_entries; i++) {
> @@ -687,6 +870,40 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>                                 break;
>                         }
>
> +                       /*
> +                        * A real CONT group must always be invalidated as a
> +                        * unit, so reject an unmap that splits one. Check the
> +                        * PTE's own CONT bit rather than the caller's size,
> +                        * since a legitimate unmap can span multiple prior
> +                        * iommu_map() calls and its size alone doesn't say how
> +                        * the underlying PTEs were grouped. Only the first and
> +                        * last entries can straddle a group boundary; an
> +                        * interior CONT-tagged entry's group is necessarily
> +                        * fully covered by this unmap, since groups can't
> +                        * overlap without also covering everything between
> +                        * them.
> +                        */
> +                       if (pte & ARM_LPAE_PTE_CONT) {
> +                               bool ok = true;
> +
> +                               if (i == 0)
> +                                       ok = ok && IS_ALIGNED(iova, cont_size);
> +                               if (i == num_entries - 1)
> +                                       ok = ok && IS_ALIGNED(iova + (i + 1) * block_size,
> +                                                             cont_size);
> +
> +                               /*
> +                                * Stop short of this entry instead of returning
> +                                * 0: entries before i may already have had
> +                                * non-leaf sub-tables torn down above, so the
> +                                * caller needs the real unmapped count, and the
> +                                * loop exit below still clears/gathers entries
> +                                * [0, i) correctly.
> +                                */
> +                               if (WARN_ON_ONCE(!ok))

Consider aligning with the following WARN_ONCE in the same function:

WARN_ONCE(true, "Unmap of a partial large IOPTE is not allowed");

> +                                       break;

I think this behavior is inconsistent: when a problem is detected at
the beginning of the unmap range, you return without modifying the
table, whereas if it's detected at the end, the code proceeds with
unmapping and leaves the table misconfigured. Could these checks be
performed before entering the loop?

> +                       }
> +
>                         if (!iopte_leaf(pte, lvl, iop->fmt)) {
>                                 __arm_lpae_clear_pte(&ptep[i], &iop->cfg, 1);
>