RE: [PATCH v7 1/9] iommu: Introduce a union to struct iommu_resv_region

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

> -----Original Message-----
> From: Jon Nettleton [mailto:jon@solid-run.com]
> Sent: 09 October 2021 07:58
> To: Robin Murphy <robin.murphy@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-arm-kernel <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling
> List <linux-acpi@vger.kernel.org>; Linux IOMMU
> <iommu@lists.linux-foundation.org>; Linuxarm <linuxarm@huawei.com>;
> Steven Price <steven.price@arm.com>; Guohanjun (Hanjun Guo)
> <guohanjun@huawei.com>; yangyicong <yangyicong@huawei.com>; Sami
> Mujawar <Sami.Mujawar@arm.com>; Will Deacon <will@kernel.org>;
> wanghuiqiang <wanghuiqiang@huawei.com>
> Subject: Re: [PATCH v7 1/9] iommu: Introduce a union to struct
> iommu_resv_region
> 
> On Fri, Oct 8, 2021 at 2:14 PM Robin Murphy <robin.murphy@arm.com>
> wrote:
> >
> > On 2021-08-05 09:07, Shameer Kolothum wrote:
> > > A union is introduced to struct iommu_resv_region to hold any
> > > firmware specific data. This is in preparation to add support for
> > > IORT RMR reserve regions and the union now holds the RMR specific
> > > information.
> > >
> > > Signed-off-by: Shameer Kolothum
> > > <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > >   include/linux/iommu.h | 11 +++++++++++
> > >   1 file changed, 11 insertions(+)
> > >
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> > > 32d448050bf7..bd0e4641c569 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -114,6 +114,13 @@ enum iommu_resv_type {
> > >       IOMMU_RESV_SW_MSI,
> > >   };
> > >
> > > +struct iommu_iort_rmr_data {
> > > +#define IOMMU_RMR_REMAP_PERMITTED    (1 << 0)
> > > +     u32 flags;
> > > +     u32 sid;        /* Stream Id associated with RMR entry */
> > > +     void *smmu;     /* Associated IORT SMMU node pointer */
> > > +};
> >
> > Do we really need to duplicate all this data? AFAICS we could just
> > save the acpi_iort_rmr pointer in the iommu_resv_region (with a
> > forward declaration here if necessary) and defer parsing its actual
> > mappings until the point where we can directly consume the results.
> 
> From earlier discussions on this patchset, the original goal was also for
> device-tree mechanisms to be able to hook into this code to support similar
> RMR's and SMMU initialization, just not through the ACPI / IORT path.

Yes. IIRC, there were some earlier attempts to have DT support for reserved regions
and there was a suggestion to provide generic interfaces so that when DT solution
comes up it is easier to add the support.

Thanks,
Shameer

> >
> > Robin.
> >
> > > +
> > >   /**
> > >    * struct iommu_resv_region - descriptor for a reserved memory region
> > >    * @list: Linked list pointers
> > > @@ -121,6 +128,7 @@ enum iommu_resv_type {
> > >    * @length: Length of the region in bytes
> > >    * @prot: IOMMU Protection flags (READ/WRITE/...)
> > >    * @type: Type of the reserved region
> > > + * @rmr: ACPI IORT RMR specific data
> > >    */
> > >   struct iommu_resv_region {
> > >       struct list_head        list;
> > > @@ -128,6 +136,9 @@ struct iommu_resv_region {
> > >       size_t                  length;
> > >       int                     prot;
> > >       enum iommu_resv_type    type;
> > > +     union {
> > > +             struct iommu_iort_rmr_data rmr;
> > > +     } fw_data;
> > >   };
> > >
> > >   /**
> > >