Re: [PATCH v4 07/15] iommupt: Add map_pages op

[Randy Dunlap <rdunlap@infradead.org>]

On 8/26/25 10:18 AM, Jason Gunthorpe wrote:
> 
> Tested-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/generic_pt/iommu_pt.h | 480 ++++++++++++++++++++++++++++
>  include/linux/generic_pt/iommu.h    |  58 ++++
>  2 files changed, 538 insertions(+)
> 
> diff --git a/drivers/iommu/generic_pt/iommu_pt.h b/drivers/iommu/generic_pt/iommu_pt.h
> index 53901a4a977935..ee762dde6fd531 100644
> --- a/drivers/iommu/generic_pt/iommu_pt.h
> +++ b/drivers/iommu/generic_pt/iommu_pt.h

[snip]


> +
> +/**
> + * map_range() - Install translation for an IOVA range

Maybe I don't understand the macros (haven't looked lately).

Function name appears to be map_pages(), not map_range().

> + * @iommu_table: Table to manipulate
> + * @iova: IO virtual address to start
> + * @paddr: Physical/Output address to start
> + * @len: Length of the range starting from @iova
> + * @prot: A bitmap of IOMMU_READ/WRITE/CACHE/NOEXEC/MMIO
> + * @gfp: GFP flags for any memory allocations
> + * @gather: Gather struct that must be flushed on return
> + *

and the @params above don't match the actual function arguments. (?)

> + * The range starting at IOVA will have paddr installed into it. The rage is
> + * automatically segmented into optimally sized table entries, and can have any
> + * valid alignment.
> + *
> + * On error the caller will probably want to invoke unmap on the range from iova
> + * up to the amount indicated by @mapped to return the table back to an
> + * unchanged state.
> + *
> + * Context: The caller must hold a write range lock that includes the whole
> + * range.
> + *
> + * Returns: -ERRNO on failure, 0 on success. The number of bytes of VA that were
> + * mapped are added to @mapped, @mapped is not zerod first.
> + */
> +int DOMAIN_NS(map_pages)(struct iommu_domain *domain, unsigned long iova,
> +			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
> +			 int prot, gfp_t gfp, size_t *mapped)
> +{


> diff --git a/include/linux/generic_pt/iommu.h b/include/linux/generic_pt/iommu.h
> index 382596b70e394e..2ca62812b5a152 100644
> --- a/include/linux/generic_pt/iommu.h
> +++ b/include/linux/generic_pt/iommu.h
> @@ -11,6 +11,7 @@
>  
>  struct iommu_iotlb_gather;
>  struct pt_iommu_ops;
> +struct pt_iommu_flush_ops;
>  
>  /**
>   * DOC: IOMMU Radix Page Table
> @@ -43,6 +44,12 @@ struct pt_iommu {
>  	 */
>  	const struct pt_iommu_ops *ops;
>  
> +	/**
> +	 * @hw_flush_ops - Function pointers provided by the HW driver to flush
> +	 * HW caches after changes to the page table.

All of these "@param -" should be "@param:" (or "@param :" if you prefer that.)
Otherwise a kernel-doc build warning happens for each one of them.

(others deleted...)


>  
> +/**
> + * struct pt_iommu_flush_ops - HW IOTLB cache flushing operations
> + *
> + * The IOMMU driver should implement these using container_of(iommu_table) to
> + * get to it's iommu_domain dervied structure. All ops can be called in atomic
> + * contexts as they are buried under DMA API calls.
> + */
> +struct pt_iommu_flush_ops {
> +	/**
> +	 * change_top() - Update the top of table pointer

This should be
	 * @change_top: Update the top of table pointer
since it is a struct member, not a function. Otherwise it causes
build warnings.


> +	 * @iommu_table: Table to operate on
> +	 * @top_paddr: New CPU physical address of the top pointer
> +	 * @top_level: IOMMU PT level of the new top

We don't really have a way to document parameters of a callback function
inside a struct, but for now kernel-doc.py won't complain about it.
(Somehow kernel-doc.pl did once upon a time and then that became dead code.)

> +	 *
> +	 * Called under the get_top_lock() spinlock. The driver must update all
> +	 * HW references to this domain with a new top address and
> +	 * configuration. On return mappings placed in the new top must be
> +	 * reachable by the HW.
> +	 *
> +	 * top_level encodes the level in IOMMU PT format, level 0 is the
> +	 * smallest page size increasing from there. This has to be translated
> +	 * to any HW specific format. During this call the new top will not be
> +	 * visible to any other API.
> +	 *
> +	 * This op is only used by PT_FEAT_DYNAMIC_TOP, and is required if
> +	 * enabled.
> +	 */
> +	void (*change_top)(struct pt_iommu *iommu_table, phys_addr_t top_paddr,
> +			   unsigned int top_level);
> +
> +	/**
> +	 * get_top_lock() - Return a lock to hold when changing the table top

This one should be
	 * @get_top_lock: <description>

This is just a struct.member so it should be documented like one; otherwise
it causes a kernel warning.


> +	 * @iommu_table: Table to operate on
> +	 *
> +	 * page table from being stored in HW. The lock will be held prior
> +	 * to calling change_top() and released once the top is fully visible.
> +	 *
> +	 * Typically this would be a lock that protects the iommu_domain's
> +	 * attachment list.
> +	 *
> +	 * This op is only used by PT_FEAT_DYNAMIC_TOP, and is required if
> +	 * enabled.
> +	 */
> +	spinlock_t *(*get_top_lock)(struct pt_iommu *iommu_table);
> +};
> +

-- 
~Randy