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

[Robin Murphy <robin.murphy@arm.com>]

On 2022-03-23 16:06, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Robin Murphy [mailto:robin.murphy@arm.com]
>> Sent: 22 March 2022 19:09
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
>> iommu@lists.linux-foundation.org
>> Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
>> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
>> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
>> will@kernel.org; wanghuiqiang <wanghuiqiang@huawei.com>
>> 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);
> }
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu