RE: [PATCH v8 05/11] ACPI/IORT: Add a helper to retrieve RMR memory regions

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

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@xxxxxxx]
> Sent: 22 March 2022 19:09
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx;
> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> Cc: jon@xxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>;
> steven.price@xxxxxxx; Guohanjun (Hanjun Guo) <guohanjun@xxxxxxxxxx>;
> yangyicong <yangyicong@xxxxxxxxxx>; Sami.Mujawar@xxxxxxx;
> will@xxxxxxxxxx; wanghuiqiang <wanghuiqiang@xxxxxxxxxx>
> Subject: Re: [PATCH v8 05/11] ACPI/IORT: Add a helper to retrieve RMR
> memory regions
> 
> On 2022-02-21 15:43, Shameer Kolothum via iommu wrote:
> > Add helper functions (iort_iommu_get/put_rmrs()) that
> > retrieves/releases RMR memory descriptors associated
> > with a given IOMMU. This will be used by IOMMU drivers
> > to set up necessary mappings.
> >
> > Invoke it from the generic iommu helper functions.
> 
> iommu_dma_get_resv_regions() already exists - please extend that rather
> than adding a parallel implementation of the same thing but different.
> IORT should export a single get_resv_regions helper which combines the
> new RMRs with the existing MSI workaround, and a separate "do I need to
> bypass this StreamID" helper for the SMMU drivers to call directly at
> reset time, since the latter isn't really an iommu-dma responsibility.

Right. I actually had couple of back and forth on the interfaces and settled
on this mainly because it just requires a single interface from IORT for both
get_resv_regions() and SMMU driver reset bypass path.
ie, iort_iommu_get/put_rmrs()).

But agree the above comment is also valid. 

> I'm happy to do that just by shuffling wrappers around for now - we can
> come back and streamline the code properly afterwards - but the sheer
> amount of indirection currently at play here is so hard to follow that
> it's not even all that easy to see how it's crossing abstraction levels
> improperly.

Please find below the revised ones. Please take a look and let me know.

Thanks,
Shameer

iommu-dma:

void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list) {

        if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))
                iort_iommu_get_resv_regions(dev, list);
}

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);
}

iort:

void iort_iommu_get_resv_regions(struct device *dev, struct list_head *head) {
        struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);

        iort_iommu_msi_get_resv_regions(dev, head);
        iort_iommu_get_rmrs(fwspec->iommu_fwnode, dev, head);
}

void iort_iommu_put_resv_regions(struct device *dev, struct list_head *head) {
   ./*Free both RMRs and HW MSI ones */
}

/* The below ones will be directly called from SMMU drivers during reset */
void iort_get_rmr_sids(struct fwnode_handle *iommu_fwnode, struct list_head *head) {
        iort_iommu_get_rmrs(iommu_fwnode, NULL, head); }
}

void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode, struct list_head *head) {
        iort_iommu_put_resv_regions(NULL, head);
}