Re: [PATCH v7 3/9] iommu/dma: Introduce generic helper to retrieve RMR info

[Robin Murphy <robin.murphy@arm.com>]

On 2021-08-05 09:07, Shameer Kolothum wrote:
> Reserved Memory Regions(RMR) associated with an IOMMU can be
> described through ACPI IORT tables in systems with devices
> that require a unity mapping or bypass for those
> regions.
> 
> Introduce a generic interface so that IOMMU drivers can retrieve
> and set up necessary mappings.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   drivers/iommu/dma-iommu.c | 29 +++++++++++++++++++++++++++++
>   include/linux/dma-iommu.h | 13 +++++++++++++
>   2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 98ba927aee1a..2fa2445e9070 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -174,6 +174,35 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
>   }
>   EXPORT_SYMBOL(iommu_put_dma_cookie);
>   
> +/**
> + *
> + * iommu_dma_get_rmrs - Retrieve Reserved Memory Regions(RMRs) associated
> + *                      with a given IOMMU
> + * @iommu_fwnode: fwnode associated with IOMMU
> + * @list: RMR list to be populated
> + *
> + */
> +int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
> +		       struct list_head *list)
> +{
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(iommu_dma_get_rmrs);
> +
> +/**
> + *
> + * iommu_dma_put_rmrs - Release Reserved Memory Regions(RMRs) associated
> + *                      with a given IOMMU
> + * @iommu_fwnode: fwnode associated with IOMMU
> + * @list: RMR list
> + *
> + */
> +void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
> +			struct list_head *list)
> +{
> +}
> +EXPORT_SYMBOL(iommu_dma_put_rmrs);

Unless there's something special you expect to need to do here, can we 
just uphold the prevailing expectation that resv_regions are kmalloc()ed 
and can be freed directly by the generic function?

> +
>   /**
>    * iommu_dma_get_resv_regions - Reserved region driver helper
>    * @dev: Device from iommu_get_resv_regions()
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> index 758ca4694257..3b7b2d096c6e 100644
> --- a/include/linux/dma-iommu.h
> +++ b/include/linux/dma-iommu.h
> @@ -42,12 +42,16 @@ void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
>   
>   extern bool iommu_dma_forcedac;
>   
> +int iommu_dma_get_rmrs(struct fwnode_handle *iommu, struct list_head *list);
> +void iommu_dma_put_rmrs(struct fwnode_handle *iommu, struct list_head *list);
> +
>   #else /* CONFIG_IOMMU_DMA */
>   
>   struct iommu_domain;
>   struct msi_desc;
>   struct msi_msg;
>   struct device;
> +struct fwnode_handle;
>   
>   static inline void iommu_setup_dma_ops(struct device *dev, u64 dma_base,
>   				       u64 dma_limit)
> @@ -83,5 +87,14 @@ static inline void iommu_dma_get_resv_regions(struct device *dev, struct list_he
>   {
>   }
>   
> +static int iommu_dma_get_rmrs(struct fwnode_handle *iommu, struct list_head *list)
> +{
> +	return -ENODEV;

Hmm, if this needs to be stubbed at all then returning an error seems 
like probably the wrong thing to do. I guess it's for 32-bit builds of 
arm-smmu? It is not an error if the firmware describes no RMRs because 
there are no RMRs, so it hardly deserves to be an error if there are no 
RMRs simply because the firmware isn't ACPI.

Robin.

> +}
> +
> +static void iommu_dma_put_rmrs(struct fwnode_handle *iommu, struct list_head *list)
> +{
> +}
> +
>   #endif	/* CONFIG_IOMMU_DMA */
>   #endif	/* __DMA_IOMMU_H */
>