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

Robin Murphy <robin.murphy@xxxxxxx> · Fri, 25 Mar 2022 17:49:39 +0000

On 2022-03-23 16:06, Shameerali Kolothum Thodi wrote:

-----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.

Yeah, that looks like it should work!

Cheers,
Robin.

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