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

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

On 8/15/2026 2:15 AM, Daniel Mentz wrote:
> On Thu, Aug 13, 2026 at 11:13 PM Vijayanand Jitta
> <vijayanand.jitta@oss.qualcomm.com> wrote:
>>
>>
>>
>> On 8/11/2026 10:34 AM, Daniel Mentz wrote:
>>> On Mon, Aug 3, 2026 at 11:19 PM 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..23a238de53ed5 100644
>>>> --- a/drivers/iommu/io-pgtable-arm.c
>>>> +++ b/drivers/iommu/io-pgtable-arm.c
>>>> [...]
>>>> +static unsigned long arm_lpae_get_cont_sizes(struct io_pgtable_cfg *cfg)
> 
> I'm wondering if we need this function at all. I'm trying to
> understand that would happen if we advertise page sizes that cannot
> possibly be used given the constraints imposed by cfg->ias and
> cfg->oas.
> arm_lpae_read_and_clear_dirty() appears to check if the end of the
> iova range is out-of-bounds (WARN_ON((iova + size - 1) &
> ~(BIT(cfg->ias) - 1))), but function arm_lpae_map_pages() appears to
> not have such a check.
> 
> If we need to restrict the "cont sizes", we could consider Will's
> suggestion: Add all the sizes to cfg->pgsize_bitmap in
> arm_lpae_restrict_pgsizes, and then subsequently clamp it like so
> 
> cfg->pgsize_bitmap &= (BIT(cfg->ias) - 1))
> cfg->pgsize_bitmap &= (BIT(cfg->oas) - 1))
> 
> That would be shorter than the 39-line arm_lpae_get_cont_sizes function.
> 

Agree , I think both arm_lpae_get_cont_sizes and arm_lpae_cont_size_fits can
be removed.

Instead I'll add something like below to arm_lpae_restrict_pgsizes as suggested.

+ if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT)) {
+        unsigned long sizes = cfg->pgsize_bitmap;
+
+        while (sizes) {
+                unsigned long size = BIT(__ffs(sizes));
+
+                cfg->pgsize_bitmap |= arm_lpae_num_cont(size) * size;
+                sizes &= ~size;
+        }
+  }

  cfg->ias = min(cfg->ias, max_addr_bits);
  cfg->oas = min(cfg->oas, max_addr_bits);

+ cfg->pgsize_bitmap &= (BIT(cfg->ias) - 1);
+ cfg->pgsize_bitmap &= (BIT(cfg->oas) - 1);

>>>> +/*
>>>> + * Install num_entries leaf entries starting at ptep (index map_idx_start
>>>> + * within the current table), tagging arm_lpae_num_cont()-sized groups with
>>>> + * the contiguous hint where both idx and paddr are aligned to the group
>>>> + * size. Entries in a misaligned group are installed without the hint.
>>>> + *
>>>> + * idx and paddr both advance by block_size per entry, so their alignment
>>>> + * relative to the group size is invariant across a run of entries within
>>>> + * this call: once a group qualifies (or fails to), every later whole group
>>>> + * does too, up to num_entries. This merges each such run into a single
>>>> + * arm_lpae_init_pte() call instead of one call per group.
>>>> + */
>>>
>>> Can you provide an example for when this function installs descriptors
>>> where the contiguous bit is only set on a subset of them. I would
>>> assume that the contiguous bit is either set for all descriptors or
>>> none of them.
>>>
>>
>> That assumption doesn't hold in general -- it's only true when the
>> map request happens to start and end on a cont_size boundary. For an
>> arbitrary map_pages() call it usually doesn't.
> 
> I believe you won't see arbitrary map_pages() calls. I understand that
> these calls are exclusively coming from __iommu_map_domain_pgtbl()
> which uses iommu_pgsize() to determine optimal page sizes.
> 
>> Example, 4K granule (num_cont = 16, cont_size = 64K),
>> iova = paddr = 0x1000, pgcount = 34:
>>
>>    - idx 1..15  (off != 0, misaligned prefix):        installed plain
>>    - idx 16..31 (off == 0, paddr now 64K-aligned):     installed w/ CONT
>>    - idx 32..34 (off == 0, remaining < num_cont):      installed plain
> 
> In the example you provided, I expect that you'll receive three
> separate calls from __iommu_map_domain_pgtbl:
>  * idx 1..15 with pgsize 4KB
>  * one call with pgsize 64KB
>  * idx 32..34 with pgsize 4KB
> 
> If I took your argument further, I could argue that we'd also have to
> check if we can put down a block mapping if iova = paddr = 0x0 and
> pgcount = 512, but we're not doing that either.
> 
> Could you provide the input parameters to the iommu_map() call that
> resulted in the parameters you provided i.e. iova = paddr = 0x1000,
> pgcount = 34:
> 

You're right -- for the iommu_map()/__iommu_map_domain_pgtbl() path, iommu_pgsize()
already splits the request at the boundaries you describe before install_leaf() ever
sees it, so install_leaf() doesn't need to handle a mixed prefix/CONT-group/suffix
chunk for that caller.

That said, install_leaf() is shared by other callers that reach it through
ops->map_pages() directly, without going through iommu_pgsize(). panthor_vm_map_pages()
(drivers/gpu/drm/panthor/panthor_mmu.c) is one -- it allocates its io_pgtable_ops via
alloc_io_pgtable_ops(ARM_64_LPAE_S1, ...), same as any other LPAE consumer, but does its
own chunking with a local get_pgsize() that only ever returns SZ_4K or SZ_2M, with no
notion of the 64K/32M CONT boundaries. That can hand install_leaf() exactly the mixed
iova=paddr=0x1000, pgcount=34 shape in a single call (panfrost's map loop uses the same
get_pgsize() and hits the same case). So the prefix/aligned-group/suffix handling in
install_leaf() is still needed for that path.


>>
>> One arm_lpae_install_leaf() call, three chunks, CONT set on only the
>> middle one.
>>
>>>> +static int arm_lpae_install_leaf(struct arm_lpae_io_pgtable *data,
>>>> +                                unsigned long iova, phys_addr_t paddr,
>>>> +                                arm_lpae_iopte prot, int lvl,
>>>> +                                int map_idx_start, int num_entries, int num_cont,
>>>> +                                arm_lpae_iopte *ptep, size_t *mapped)
>>>> +{
>>>> +       size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
>>>> +       size_t cont_size = num_cont * block_size;
>>>> +       int done = 0;
>>>> +
>>>> +       while (done < num_entries) {
>>>> +               int idx = map_idx_start + done;
>>>> +               int remaining = num_entries - done;
>>>> +               int off = idx % num_cont;
>>>> +               arm_lpae_iopte pte = prot;
>>>> +               int chunk, ret;
>>>> +
>>>> +               if (off) {
>>>> +                       /* Misaligned prefix: advance to the next boundary */
>>>> +                       chunk = min_t(int, num_cont - off, remaining);
>>>> +               } else if (remaining >= num_cont && IS_ALIGNED(paddr, cont_size)) {
>>>> +                       /* Aligned: merge every full group in this run */
>>>> +                       chunk = remaining - remaining % num_cont;
>>>> +                       pte |= ARM_LPAE_PTE_CONT;
>>>> +               } else {
>>>> +                       /*
>>>> +                        * Aligned idx but paddr doesn't line up with cont_size,
>>>> +                        * or too short for a full group. That holds for the
>>>> +                        * rest of this call too, so install the remainder
>>>> +                        * plain in one go.
>>>> +                        */
>>>> +                       chunk = remaining;
>>>> +               }
>>>> +
>>>> +               ret = arm_lpae_init_pte(data, iova, paddr, pte, lvl, chunk, ptep);
>>>> +               if (ret)
>>>> +                       return ret;
>>>> +
>>>> +               *mapped += chunk * block_size;
>>>> +               ptep += chunk;
>>>> +               iova += chunk * block_size;
>>>> +               paddr += chunk * block_size;
>>>> +               done += chunk;
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  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,21 +608,44 @@ 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;
>>>> +       bool cont_hint_enabled = !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_NO_CONT_HINT);
>>>> +       int num_entries, max_entries, map_idx_start;
>>>> +       int num_cont = cont_hint_enabled ? arm_lpae_num_cont(block_size) : 1;
>>>> +       bool use_cont = cont_hint_enabled && num_cont > 1;
>>>>
>>>>         /* Find our entry at the current level */
>>>>         map_idx_start = ARM_LPAE_LVL_IDX(iova, lvl, data);
>>>>         ptep += map_idx_start;
>>>>
>>>> +       /*
>>>> +        * Normalize an exact whole-CONT-group request down to the
>>>> +        * equivalent block_size/pgcount so it funnels through the same
>>>> +        * leaf path below. arm_lpae_install_leaf() independently decides,
>>>> +        * per sub-chunk, whether the CONT hint actually applies.
>>>> +        */
>>>> +       if (use_cont && size == block_size * num_cont) {
>>>> +               pgcount *= num_cont;
>>>> +               size = block_size;
>>>
>>> This appears to me as if you're throwing away information about
>>> whether this mapping request is suitable for the contiguous bit, and
>>> then in arm_lpae_install_leaf(), you're trying to recover that
>>> information. Can't you just do "prot |=ARM_LPAE_PTE_CONT" here and
>>> then completely avoid the logic in arm_lpae_install_leaf?
>>>
>>
>> That optimization only applies to the exact-whole-group case already
>> handled above (size == block_size * num_cont). A single map_pages()
>> call can also cover the general case shown above, where a misaligned
>> prefix/suffix surrounds one or more aligned groups within the same
>> call. Setting prot |= ARM_LPAE_PTE_CONT unconditionally here would
>> incorrectly tag those misaligned entries with the hint.
> 
> Due to how __iommu_map_domain_pgtbl and iommu_pgsize operate, I don't
> expect to see the prefixes and suffixes that you are describing.
> Instead, I expect we'll see separate calls to __arm_lpae_map(): One
> for the prefix, one for the set of aligned groups and another one for
> the suffix.
> 

Agreed, Replied in above comment.

Thanks,
Vijay>>
>> arm_lpae_install_leaf()'s off/remaining logic is what detects those
>> group boundaries per chunk, so I don't think we can drop it in favor
>> of always setting prot |= CONT at this call site. The size ==
>> block_size * num_cont check here is just a fast path for the common
>> whole-group case, avoiding a walk through install_leaf() for something
>> the caller has already told us.
>>