[PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions

[Laurentiu Tudor]

Hi Jon,

On 6/3/2021 3:27 PM, Jon Nettleton wrote:
> On Wed, May 26, 2021 at 7:11 PM Laurentiu Tudor <laurentiu.tudor at nxp.com> wrote:
>>
>>
>>
>> On 5/26/2021 7:36 PM, Shameerali Kolothum Thodi wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Laurentiu Tudor [mailto:laurentiu.tudor at nxp.com]
>>>> Sent: 26 May 2021 08:53
>>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>;
>>>> linux-arm-kernel at lists.infradead.org; linux-acpi at vger.kernel.org;
>>>> iommu at lists.linux-foundation.org
>>>> Cc: jon at solid-run.com; Linuxarm <linuxarm at huawei.com>;
>>>> steven.price at arm.com; Guohanjun (Hanjun Guo) <guohanjun at huawei.com>;
>>>> yangyicong <yangyicong at huawei.com>; Sami.Mujawar at arm.com;
>>>> robin.murphy at arm.com; wanghuiqiang <wanghuiqiang at huawei.com>
>>>> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
>>>> regions
>>>>
>>>> Hi Shameer,
>>>>
>>>> On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
>>>>> Add a helper function that retrieves RMR memory descriptors
>>>>> associated with a given IOMMU. This will be used by IOMMU
>>>>> drivers to setup necessary mappings.
>>>>>
>>>>> Now that we have this, invoke it from the generic helper
>>>>> interface.
>>>>>
>>>>> Signed-off-by: Shameer Kolothum
>>>> <shameerali.kolothum.thodi at huawei.com>
>>>>> ---
>>>>>  drivers/acpi/arm64/iort.c | 50
>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>  drivers/iommu/dma-iommu.c |  4 ++++
>>>>>  include/linux/acpi_iort.h |  7 ++++++
>>>>>  3 files changed, 61 insertions(+)
>>>>>
>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>>> index fea1ffaedf3b..01917caf58de 100644
>>>>> --- a/drivers/acpi/arm64/iort.c
>>>>> +++ b/drivers/acpi/arm64/iort.c
>>>>> @@ -12,6 +12,7 @@
>>>>>
>>>>>  #include <linux/acpi_iort.h>
>>>>>  #include <linux/bitfield.h>
>>>>> +#include <linux/dma-iommu.h>
>>>>>  #include <linux/iommu.h>
>>>>>  #include <linux/kernel.h>
>>>>>  #include <linux/list.h>
>>>>> @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
>>>> device *dev)
>>>>>     return err;
>>>>>  }
>>>>>
>>>>> +/**
>>>>> + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
>>>> IOMMU
>>>>> + * @iommu: fwnode for the IOMMU
>>>>> + * @head: RMR list head to be populated
>>>>> + *
>>>>> + * Returns: 0 on success, <0 failure
>>>>> + */
>>>>> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
>>>>> +                   struct list_head *head)
>>>>> +{
>>>>> +   struct iort_rmr_entry *e;
>>>>> +   struct acpi_iort_node *iommu;
>>>>> +   int rmrs = 0;
>>>>> +
>>>>> +   iommu = iort_get_iort_node(iommu_fwnode);
>>>>> +   if (!iommu || list_empty(&iort_rmr_list))
>>>>> +           return -ENODEV;
>>>>> +
>>>>> +   list_for_each_entry(e, &iort_rmr_list, list) {
>>>>> +           int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
>>>> IOMMU_MMIO;
>>>>
>>>> We have a case with an IP block that needs EXEC rights on its reserved
>>>> memory, so could you please drop the IOMMU_NOEXEC flag?
>>>
>>> Ok, I think I can drop that one if there are no other concerns. I was not quite
>>> sure what to include here in the first place as the IORT spec is not giving any
>>> further details about the RMR regions(May be the flags field can be extended to
>>> describe these details).
>>>
>>
>> That would be great, given that some preliminary investigations on my
>> side revealed that our IP block seems to be quite sensitive to memory
>> attributes. I need to spend some more time on this but at first sight
>> looks like it needs cacheable, normal memory (not mmio mapping).
>>
>> ---
>> Thanks & Best Regards, Laurentiu
> 
> Laurentiu,
> 
> Is this regarding the mc-bin memory block or another IP?  I am currently
> running this patchset with IOMMU_NOEXEC under ACPI without any problems.

It's the MC firmware needing EXEC rights on its reserved memory. On my
side, with IOMMU_NOEXEC, as soon as the direct mappings are created I
get SMMU faults triggered by the FW.

> If so maybe we can touch base off list and align on the implementation.

Sure, just let me know when you have the time.

---
Best Regards, Laurentiu