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

[Vijayanand Jitta <vijayanand.jitta@oss.qualcomm.com>]

On 9/25/2026 2:06 AM, Daniel Mentz wrote:
> On Mon, Sep 21, 2026 at 4:44 AM Vijayanand Jitta
> <vijayanand.jitta@oss.qualcomm.com> 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.
> 

Ack, will remove this.

>> +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.
> 

Ack. will update this as suggested.

>> +       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.
> 

Ack.

>> +            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.
> 

Ack.

>> +                       return -EINVAL;
>> +
>> +               entries_per_map = size / block_size;
> 
> Can't you just do
> 
> pgcount *= num_cont;
> 
> Wouldn't that be easier?
> 

There could be potential overflow with pgcount * num_cont. So, I went with division first.


>>                 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?
> 

Ack.

>> +               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?
> 

Ack.
>> +            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.
> 

Ack, Will rename it to arm_lpae_adjust_pgsizes, looks fine ?

Thanks,
Vijay

>>         }
>>
>>         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)IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT
>> +                               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);
>>  }IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT
> 
> 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.