RE: [PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR reserved regions

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

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: 07 April 2022 15:00
> To: Robin Murphy <robin.murphy@arm.com>
> Cc: Christoph Hellwig <hch@infradead.org>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; jon@solid-run.com; Linuxarm
> <linuxarm@huawei.com>; steven.price@arm.com; linux-acpi@vger.kernel.org;
> iommu@lists.linux-foundation.org; wanghuiqiang
> <wanghuiqiang@huawei.com>; Guohanjun (Hanjun Guo)
> <guohanjun@huawei.com>; yangyicong <yangyicong@huawei.com>;
> Sami.Mujawar@arm.com; will@kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR
> reserved regions
> 
> On Thu, Apr 07, 2022 at 02:53:38PM +0100, Robin Murphy wrote:
> > > Why can't this just go into generic_iommu_put_resv_regions?  The idea
> > > that the iommu low-level drivers need to call into dma-iommu which is
> > > a consumer of the IOMMU API is odd.  Especially if that just calls out
> > > to ACPI code and generic IOMMU code only anyway.
> >
> > Because assuming ACPI means IORT is not generic. Part of the aim in adding
> > the union to iommu_resv_region is that stuff like AMD's unity_map_entry
> and
> > Intel's dmar_rmrr_unit can be folded into it as well, and their reserved
> > region handling correspondingly simplified too.
> >
> > The iommu_dma_{get,put}_resv_region() helpers are kind of intended to be
> > specific to the fwnode mechanism which deals with IORT and devicetree
> (once
> > the reserved region bindings are fully worked out).
> 
> But IORT is not driver₋specific code.  So we'll need a USE_IORT flag
> somewhere in core IOMMU code instead of trying to stuff this into
> driver operations.  and dma-iommu mostly certainly implies IORT even
> less than ACPI.

Sorry for the delayed response(was on holidays). So if we move the
iommu_dma_put_resv_region() call to generic_iommu_put_resv_regions() ,
will that address the concerns here?

I think it will resolve the issue in 05/11 as well pointed out by Christoph
where we end up not releasing reserved regions when
CONFIG_IOMMU_DMA is not set.

Something like below,

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2c45b85b9fc..d08204a25ba8 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2605,6 +2605,8 @@ void generic_iommu_put_resv_regions(struct
device *dev, struct list_head *list)
 {
        struct iommu_resv_region *entry, *next;

+       iommu_dma_put_resv_regions(dev, list);
+
        list_for_each_entry_safe(entry, next, list, list)
                kfree(entry);
 }

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 5811233dc9fb..8cb1e419db49 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -393,8 +393,6 @@ void iommu_dma_put_resv_regions(struct device
*dev, struct list_head *list)
 {
        if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))
                iort_iommu_put_resv_regions(dev, list);
-
-       generic_iommu_put_resv_regions(dev, list);
 }

Please let me know if this is not good enough.

Thanks,
Shameer