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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@xxxxxxx]
> Sent: 08 October 2021 14:04
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx;
> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> Cc: Linuxarm <linuxarm@xxxxxxxxxx>; lorenzo.pieralisi@xxxxxxx;
> joro@xxxxxxxxxx; will@xxxxxxxxxx; wanghuiqiang
> <wanghuiqiang@xxxxxxxxxx>; Guohanjun (Hanjun Guo)
> <guohanjun@xxxxxxxxxx>; steven.price@xxxxxxx; Sami.Mujawar@xxxxxxx;
> jon@xxxxxxxxxxxxx; eric.auger@xxxxxxxxxx; yangyicong
> <yangyicong@xxxxxxxxxx>
> Subject: Re: [PATCH v7 3/9] iommu/dma: Introduce generic helper to retrieve
> RMR info
> 
> 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@xxxxxxxxxx>
> > ---
> >   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?

Right. I think we can do that. 

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

Yes, definitely not an error return. I will change that.

Thanks,
Shameer

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