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

Commit Message

Shameerali Kolothum Thodi May 24, 2021, 11:02 a.m. UTC

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 <[e-mail]>
---
 drivers/acpi/arm64/iort.c | 50 +++++++++++++++++++++++++++++++++++++++
 drivers/iommu/dma-iommu.c |  4 ++++
 include/linux/acpi_iort.h |  7 ++++++
 3 files changed, 61 insertions(+)

Comments

Laurentiu Tudor May 26, 2021, 7:53 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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?

---
Thanks & Best Regards, Laurentiu

Shameerali Kolothum Thodi May 26, 2021, 4:36 p.m. UTC | #2

> -----Original Message-----
> From: Laurentiu Tudor [mailto:[e-mail]]
> Sent: 26 May 2021 08:53
> To: Shameerali Kolothum Thodi <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]
> Cc: [e-mail]; Linuxarm <[e-mail]>;
> [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
> yangyicong <[e-mail]>; [e-mail];
> [e-mail]; wanghuiqiang <[e-mail]>
> 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
> <[e-mail]>
> > ---
> >  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).

Thanks,
Shameer

Laurentiu Tudor May 26, 2021, 5:11 p.m. UTC | #3

On 5/26/2021 7:36 PM, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Laurentiu Tudor [mailto:[e-mail]]
>> Sent: 26 May 2021 08:53
>> To: Shameerali Kolothum Thodi <[e-mail]>;
>> [e-mail]; [e-mail];
>> [e-mail]
>> Cc: [e-mail]; Linuxarm <[e-mail]>;
>> [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
>> yangyicong <[e-mail]>; [e-mail];
>> [e-mail]; wanghuiqiang <[e-mail]>
>> 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
>> <[e-mail]>
>>> ---
>>>  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

Jon Nettleton May 27, 2021, 4:25 a.m. UTC | #4

On Wed, May 26, 2021 at 6:36 PM Shameerali Kolothum Thodi
<[e-mail]> wrote:
>
>
>
> > -----Original Message-----
> > From: Laurentiu Tudor [mailto:[e-mail]]
> > Sent: 26 May 2021 08:53
> > To: Shameerali Kolothum Thodi <[e-mail]>;
> > [e-mail]; [e-mail];
> > [e-mail]
> > Cc: [e-mail]; Linuxarm <[e-mail]>;
> > [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
> > yangyicong <[e-mail]>; [e-mail];
> > [e-mail]; wanghuiqiang <[e-mail]>
> > 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
> > <[e-mail]>
> > > ---
> > >  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).

We probably need to bring this back up to the ACPI forums. This is probably
something that should be attached to the memory range node which does have
4 extra reserved bytes.

-Jon

>
> Thanks,
> Shameer
>
>

Jon Nettleton June 3, 2021, 12:27 p.m. UTC | #5

On Wed, May 26, 2021 at 7:11 PM Laurentiu Tudor <[e-mail]> wrote:
>
>
>
> On 5/26/2021 7:36 PM, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Laurentiu Tudor [mailto:[e-mail]]
> >> Sent: 26 May 2021 08:53
> >> To: Shameerali Kolothum Thodi <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> Cc: [e-mail]; Linuxarm <[e-mail]>;
> >> [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
> >> yangyicong <[e-mail]>; [e-mail];
> >> [e-mail]; wanghuiqiang <[e-mail]>
> >> 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
> >> <[e-mail]>
> >>> ---
> >>>  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.

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

-Jon

Laurentiu Tudor June 3, 2021, 12:32 p.m. UTC | #6

Hi Jon,

On 6/3/2021 3:27 PM, Jon Nettleton wrote:
> On Wed, May 26, 2021 at 7:11 PM Laurentiu Tudor <[e-mail]> wrote:
>>
>>
>>
>> On 5/26/2021 7:36 PM, Shameerali Kolothum Thodi wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Laurentiu Tudor [mailto:[e-mail]]
>>>> Sent: 26 May 2021 08:53
>>>> To: Shameerali Kolothum Thodi <[e-mail]>;
>>>> [e-mail]; [e-mail];
>>>> [e-mail]
>>>> Cc: [e-mail]; Linuxarm <[e-mail]>;
>>>> [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
>>>> yangyicong <[e-mail]>; [e-mail];
>>>> [e-mail]; wanghuiqiang <[e-mail]>
>>>> 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
>>>> <[e-mail]>
>>>>> ---
>>>>>  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

Robin Murphy June 14, 2021, 10:35 a.m. UTC | #7

On 2021-05-26 17:36, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Laurentiu Tudor [mailto:[e-mail]]
>> Sent: 26 May 2021 08:53
>> To: Shameerali Kolothum Thodi <[e-mail]>;
>> [e-mail]; [e-mail];
>> [e-mail]
>> Cc: [e-mail]; Linuxarm <[e-mail]>;
>> [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
>> yangyicong <[e-mail]>; [e-mail];
>> [e-mail]; wanghuiqiang <[e-mail]>
>> 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
>> <[e-mail]>
>>> ---
>>>   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).

Right, it's reserved for the device to use *somehow* - that's all we 
know, so it's not our place to try and enforce any restrictive 
permissions. All it could possibly achieve is the ability to log an 
error to say "a thing did an unknown thing in a way we didn't expect!", 
and there's hardly much value in that.

Robin.

Robin Murphy June 14, 2021, 11:23 a.m. UTC | #8

On 2021-05-24 12:02, 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 <[e-mail]>
> ---
>   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;
> +		struct iommu_resv_region *region;
> +		enum iommu_resv_type type;
> +		struct acpi_iort_rmr_desc *rmr_desc;
> +
> +		if (e->smmu != iommu)
> +			continue;
> +
> +		rmr_desc = e->rmr_desc;
> +		if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> +			type = IOMMU_RESV_DIRECT_RELAXABLE;
> +		else
> +			type = IOMMU_RESV_DIRECT;

Wasn't the idea that we can do all this during the initial parsing, i.e. 
we don't even have iort_rmr_entry, we store them as iommu_resv_regions 
and simply kmemdup() or whatever at this point?

Robin.

> +
> +		region = iommu_alloc_resv_region(rmr_desc->base_address,
> +						 rmr_desc->length,
> +						 prot, type);
> +		if (region) {
> +			region->fw_data.rmr.flags = e->flags;
> +			region->fw_data.rmr.sid = e->sid;
> +			list_add_tail(&region->list, head);
> +			rmrs++;
> +		}
> +	}
> +
> +	return (rmrs == 0) ? -ENODEV : 0;
> +}
> +
>   /**
>    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
>    * @dev: Device from iommu_get_resv_regions()
> @@ -1108,6 +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
>   const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
>   						const u32 *input_id)
>   { return NULL; }
> +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head *head)
> +{ return -ENODEV; }
>   #endif
>   
>   static int nc_dma_get_range(struct device *dev, u64 *size)
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 229ec65d98be..f893d460cfa4 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
>   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
>   		       struct list_head *list)
>   {
> +	if (!is_of_node(iommu_fwnode))
> +		return iort_iommu_get_rmrs(iommu_fwnode, list);
> +
>   	return -EINVAL;
>   }
>   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
>   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
>   			struct list_head *list)
>   {
> +	generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
>   }
>   EXPORT_SYMBOL(iommu_dma_put_rmrs);
>   
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 1a12baa58e40..e8c45fa59531 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -39,6 +39,8 @@ const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
>   						const u32 *id_in);
>   int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head);
>   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> +			struct list_head *list);
>   #else
>   static inline void acpi_iort_init(void) { }
>   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
>   
>   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
>   { return PHYS_ADDR_MAX; }
> +
> +static inline
> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> +			struct list_head *list)
> +{ return -ENODEV; }
>   #endif
>   
>   #endif /* __ACPI_IORT_H__ */
>

Shameerali Kolothum Thodi June 14, 2021, 12:49 p.m. UTC | #9

> -----Original Message-----
> From: Robin Murphy [mailto:[e-mail]]
> Sent: 14 June 2021 12:23
> To: Shameerali Kolothum Thodi <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]
> Cc: [e-mail]; Linuxarm <[e-mail]>;
> [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
> yangyicong <[e-mail]>; [e-mail];
> wanghuiqiang <[e-mail]>
> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> regions
> 
> On 2021-05-24 12:02, 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
> <[e-mail]>
> > ---
> >   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;
> > +		struct iommu_resv_region *region;
> > +		enum iommu_resv_type type;
> > +		struct acpi_iort_rmr_desc *rmr_desc;
> > +
> > +		if (e->smmu != iommu)
> > +			continue;
> > +
> > +		rmr_desc = e->rmr_desc;
> > +		if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > +			type = IOMMU_RESV_DIRECT_RELAXABLE;
> > +		else
> > +			type = IOMMU_RESV_DIRECT;
> 
> Wasn't the idea that we can do all this during the initial parsing, i.e.
> we don't even have iort_rmr_entry, we store them as iommu_resv_regions
> and simply kmemdup() or whatever at this point?


Hmm... Not yet. I removed struct iommu_rmr() from v4. But yes, it looks like
we can get rid of iort_rmr_entry as well. Will give it a go in next.

Thanks,
Shameer

> Robin.
> 
> > +
> > +		region = iommu_alloc_resv_region(rmr_desc->base_address,
> > +						 rmr_desc->length,
> > +						 prot, type);
> > +		if (region) {
> > +			region->fw_data.rmr.flags = e->flags;
> > +			region->fw_data.rmr.sid = e->sid;
> > +			list_add_tail(&region->list, head);
> > +			rmrs++;
> > +		}
> > +	}
> > +
> > +	return (rmrs == 0) ? -ENODEV : 0;
> > +}
> > +
> >   /**
> >    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> >    * @dev: Device from iommu_get_resv_regions()
> > @@ -1108,6 +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct
> device *dev, struct list_head *head)
> >   const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
> >   						const u32 *input_id)
> >   { return NULL; }
> > +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head
> *head)
> > +{ return -ENODEV; }
> >   #endif
> >
> >   static int nc_dma_get_range(struct device *dev, u64 *size)
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 229ec65d98be..f893d460cfa4 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
> >   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
> >   		       struct list_head *list)
> >   {
> > +	if (!is_of_node(iommu_fwnode))
> > +		return iort_iommu_get_rmrs(iommu_fwnode, list);
> > +
> >   	return -EINVAL;
> >   }
> >   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
> >   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
> >   			struct list_head *list)
> >   {
> > +	generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
> >   }
> >   EXPORT_SYMBOL(iommu_dma_put_rmrs);
> >
> > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> > index 1a12baa58e40..e8c45fa59531 100644
> > --- a/include/linux/acpi_iort.h
> > +++ b/include/linux/acpi_iort.h
> > @@ -39,6 +39,8 @@ const struct iommu_ops
> *iort_iommu_configure_id(struct device *dev,
> >   						const u32 *id_in);
> >   int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head
> *head);
> >   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > +			struct list_head *list);
> >   #else
> >   static inline void acpi_iort_init(void) { }
> >   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> > @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct device
> *dev, struct list_head *head)
> >
> >   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
> >   { return PHYS_ADDR_MAX; }
> > +
> > +static inline
> > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > +			struct list_head *list)
> > +{ return -ENODEV; }
> >   #endif
> >
> >   #endif /* __ACPI_IORT_H__ */
> >

Jon Nettleton June 29, 2021, 5:34 p.m. UTC | #10

On Mon, Jun 14, 2021 at 2:49 PM Shameerali Kolothum Thodi
<[e-mail]> wrote:
>
>
>
> > -----Original Message-----
> > From: Robin Murphy [mailto:[e-mail]]
> > Sent: 14 June 2021 12:23
> > To: Shameerali Kolothum Thodi <[e-mail]>;
> > [e-mail]; [e-mail];
> > [e-mail]
> > Cc: [e-mail]; Linuxarm <[e-mail]>;
> > [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
> > yangyicong <[e-mail]>; [e-mail];
> > wanghuiqiang <[e-mail]>
> > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> > regions
> >
> > On 2021-05-24 12:02, 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
> > <[e-mail]>
> > > ---
> > >   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;
> > > +           struct iommu_resv_region *region;
> > > +           enum iommu_resv_type type;
> > > +           struct acpi_iort_rmr_desc *rmr_desc;
> > > +
> > > +           if (e->smmu != iommu)
> > > +                   continue;
> > > +
> > > +           rmr_desc = e->rmr_desc;
> > > +           if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > > +                   type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > +           else
> > > +                   type = IOMMU_RESV_DIRECT;
> >

I have been looking at this a bit more and looking at the history of
RMR_REMAP_PERMITTED.  Based on the history I have found it
seems to me like this would be the better options for prot.

@@ -840,7 +840,7 @@ int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
                return -ENODEV;

        list_for_each_entry(e, &iort_rmr_list, list) {
-               int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+               int prot = IOMMU_READ | IOMMU_WRITE;
                struct iommu_resv_region *region;
                enum iommu_resv_type type;
                struct acpi_iort_rmr_desc *rmr_desc;
@@ -849,10 +849,13 @@ int iort_iommu_get_rmrs(struct fwnode_handle
*iommu_fwnode,
                        continue;

                rmr_desc = e->rmr_desc;
-               if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
+               if (e->flags & IOMMU_RMR_REMAP_PERMITTED) {
                        type = IOMMU_RESV_DIRECT_RELAXABLE;
-               else
+                       prot |= IOMMU_CACHE;
+               } else {
                        type = IOMMU_RESV_DIRECT;
+                       prot |= IOMMU_MMIO;
+               }

                region = iommu_alloc_resv_region(rmr_desc->base_address,
                                                 rmr_desc->length,

any input on this?  My reasoning is that IOMMU_RESV_DIRECT is specifically
referenced for things like MSI doorbell and in all the examples needs
IOMMU_MMIO, while REMAP_PERMITTED seems to be used for allocated
system memory that is then used for device specific reserved regions which
commits say would be like a GPU or USB device.

-Jon

> > Wasn't the idea that we can do all this during the initial parsing, i.e.
> > we don't even have iort_rmr_entry, we store them as iommu_resv_regions
> > and simply kmemdup() or whatever at this point?
>
>
> Hmm... Not yet. I removed struct iommu_rmr() from v4. But yes, it looks like
> we can get rid of iort_rmr_entry as well. Will give it a go in next.
>
> Thanks,
> Shameer
>
> > Robin.
> >
> > > +
> > > +           region = iommu_alloc_resv_region(rmr_desc->base_address,
> > > +                                            rmr_desc->length,
> > > +                                            prot, type);
> > > +           if (region) {
> > > +                   region->fw_data.rmr.flags = e->flags;
> > > +                   region->fw_data.rmr.sid = e->sid;
> > > +                   list_add_tail(&region->list, head);
> > > +                   rmrs++;
> > > +           }
> > > +   }
> > > +
> > > +   return (rmrs == 0) ? -ENODEV : 0;
> > > +}
> > > +
> > >   /**
> > >    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> > >    * @dev: Device from iommu_get_resv_regions()
> > > @@ -1108,6 +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct
> > device *dev, struct list_head *head)
> > >   const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
> > >                                             const u32 *input_id)
> > >   { return NULL; }
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head
> > *head)
> > > +{ return -ENODEV; }
> > >   #endif
> > >
> > >   static int nc_dma_get_range(struct device *dev, u64 *size)
> > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > > index 229ec65d98be..f893d460cfa4 100644
> > > --- a/drivers/iommu/dma-iommu.c
> > > +++ b/drivers/iommu/dma-iommu.c
> > > @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
> > >   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > >                    struct list_head *list)
> > >   {
> > > +   if (!is_of_node(iommu_fwnode))
> > > +           return iort_iommu_get_rmrs(iommu_fwnode, list);
> > > +
> > >     return -EINVAL;
> > >   }
> > >   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > >   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
> > >                     struct list_head *list)
> > >   {
> > > +   generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
> > >   }
> > >   EXPORT_SYMBOL(iommu_dma_put_rmrs);
> > >
> > > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> > > index 1a12baa58e40..e8c45fa59531 100644
> > > --- a/include/linux/acpi_iort.h
> > > +++ b/include/linux/acpi_iort.h
> > > @@ -39,6 +39,8 @@ const struct iommu_ops
> > *iort_iommu_configure_id(struct device *dev,
> > >                                             const u32 *id_in);
> > >   int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head
> > *head);
> > >   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > +                   struct list_head *list);
> > >   #else
> > >   static inline void acpi_iort_init(void) { }
> > >   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> > > @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct device
> > *dev, struct list_head *head)
> > >
> > >   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
> > >   { return PHYS_ADDR_MAX; }
> > > +
> > > +static inline
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > +                   struct list_head *list)
> > > +{ return -ENODEV; }
> > >   #endif
> > >
> > >   #endif /* __ACPI_IORT_H__ */
> > >

Jon Nettleton July 4, 2021, 7:38 a.m. UTC | #11

On Tue, Jun 29, 2021 at 7:34 PM Jon Nettleton <[e-mail]> wrote:
>
> On Mon, Jun 14, 2021 at 2:49 PM Shameerali Kolothum Thodi
> <[e-mail]> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Robin Murphy [mailto:[e-mail]]
> > > Sent: 14 June 2021 12:23
> > > To: Shameerali Kolothum Thodi <[e-mail]>;
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > Cc: [e-mail]; Linuxarm <[e-mail]>;
> > > [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
> > > yangyicong <[e-mail]>; [e-mail];
> > > wanghuiqiang <[e-mail]>
> > > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> > > regions
> > >
> > > On 2021-05-24 12:02, 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
> > > <[e-mail]>
> > > > ---
> > > >   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;
> > > > +           struct iommu_resv_region *region;
> > > > +           enum iommu_resv_type type;
> > > > +           struct acpi_iort_rmr_desc *rmr_desc;
> > > > +
> > > > +           if (e->smmu != iommu)
> > > > +                   continue;
> > > > +
> > > > +           rmr_desc = e->rmr_desc;
> > > > +           if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > > > +                   type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > > +           else
> > > > +                   type = IOMMU_RESV_DIRECT;
> > >
>
> I have been looking at this a bit more and looking at the history of
> RMR_REMAP_PERMITTED.  Based on the history I have found it
> seems to me like this would be the better options for prot.
>
> @@ -840,7 +840,7 @@ int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
>                 return -ENODEV;
>
>         list_for_each_entry(e, &iort_rmr_list, list) {
> -               int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +               int prot = IOMMU_READ | IOMMU_WRITE;
>                 struct iommu_resv_region *region;
>                 enum iommu_resv_type type;
>                 struct acpi_iort_rmr_desc *rmr_desc;
> @@ -849,10 +849,13 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
>                         continue;
>
>                 rmr_desc = e->rmr_desc;
> -               if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> +               if (e->flags & IOMMU_RMR_REMAP_PERMITTED) {
>                         type = IOMMU_RESV_DIRECT_RELAXABLE;
> -               else
> +                       prot |= IOMMU_CACHE;
> +               } else {
>                         type = IOMMU_RESV_DIRECT;
> +                       prot |= IOMMU_MMIO;
> +               }
>
>                 region = iommu_alloc_resv_region(rmr_desc->base_address,
>                                                  rmr_desc->length,
>
> any input on this?  My reasoning is that IOMMU_RESV_DIRECT is specifically
> referenced for things like MSI doorbell and in all the examples needs
> IOMMU_MMIO, while REMAP_PERMITTED seems to be used for allocated
> system memory that is then used for device specific reserved regions which
> commits say would be like a GPU or USB device.
>
> -Jon
>
> > > Wasn't the idea that we can do all this during the initial parsing, i.e.
> > > we don't even have iort_rmr_entry, we store them as iommu_resv_regions
> > > and simply kmemdup() or whatever at this point?
> >
> >
> > Hmm... Not yet. I removed struct iommu_rmr() from v4. But yes, it looks like
> > we can get rid of iort_rmr_entry as well. Will give it a go in next.
> >
> > Thanks,
> > Shameer
> >
> > > Robin.
> > >
> > > > +
> > > > +           region = iommu_alloc_resv_region(rmr_desc->base_address,
> > > > +                                            rmr_desc->length,
> > > > +                                            prot, type);
> > > > +           if (region) {
> > > > +                   region->fw_data.rmr.flags = e->flags;
> > > > +                   region->fw_data.rmr.sid = e->sid;
> > > > +                   list_add_tail(&region->list, head);
> > > > +                   rmrs++;
> > > > +           }
> > > > +   }
> > > > +
> > > > +   return (rmrs == 0) ? -ENODEV : 0;
> > > > +}
> > > > +
> > > >   /**
> > > >    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> > > >    * @dev: Device from iommu_get_resv_regions()
> > > > @@ -1108,6 +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct
> > > device *dev, struct list_head *head)
> > > >   const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
> > > >                                             const u32 *input_id)
> > > >   { return NULL; }
> > > > +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head
> > > *head)
> > > > +{ return -ENODEV; }
> > > >   #endif
> > > >
> > > >   static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > > > index 229ec65d98be..f893d460cfa4 100644
> > > > --- a/drivers/iommu/dma-iommu.c
> > > > +++ b/drivers/iommu/dma-iommu.c
> > > > @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
> > > >   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > >                    struct list_head *list)
> > > >   {
> > > > +   if (!is_of_node(iommu_fwnode))
> > > > +           return iort_iommu_get_rmrs(iommu_fwnode, list);
> > > > +
> > > >     return -EINVAL;
> > > >   }
> > > >   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > > @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > >   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
> > > >                     struct list_head *list)
> > > >   {
> > > > +   generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
> > > >   }
> > > >   EXPORT_SYMBOL(iommu_dma_put_rmrs);
> > > >
> > > > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> > > > index 1a12baa58e40..e8c45fa59531 100644
> > > > --- a/include/linux/acpi_iort.h
> > > > +++ b/include/linux/acpi_iort.h
> > > > @@ -39,6 +39,8 @@ const struct iommu_ops
> > > *iort_iommu_configure_id(struct device *dev,
> > > >                                             const u32 *id_in);
> > > >   int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head
> > > *head);
> > > >   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > +                   struct list_head *list);
> > > >   #else
> > > >   static inline void acpi_iort_init(void) { }
> > > >   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> > > > @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct device
> > > *dev, struct list_head *head)
> > > >
> > > >   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
> > > >   { return PHYS_ADDR_MAX; }
> > > > +
> > > > +static inline
> > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > +                   struct list_head *list)
> > > > +{ return -ENODEV; }
> > > >   #endif
> > > >
> > > >   #endif /* __ACPI_IORT_H__ */
> > > >

Ping.  Any comments on this?

-Jon

Shameerali Kolothum Thodi July 5, 2021, 9:10 a.m. UTC | #12

> -----Original Message-----
> From: Jon Nettleton [mailto:[e-mail]]
> Sent: 04 July 2021 08:39
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: Robin Murphy <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; Linuxarm <[e-mail]>;
> [e-mail]; Guohanjun (Hanjun Guo) <[e-mail]>;
> yangyicong <[e-mail]>; [e-mail];
> wanghuiqiang <[e-mail]>
> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> regions
> 
> On Tue, Jun 29, 2021 at 7:34 PM Jon Nettleton <[e-mail]> wrote:
> >
> > On Mon, Jun 14, 2021 at 2:49 PM Shameerali Kolothum Thodi
> > <[e-mail]> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Robin Murphy [mailto:[e-mail]]
> > > > Sent: 14 June 2021 12:23
> > > > To: Shameerali Kolothum Thodi
> > > > <[e-mail]>;
> > > > [e-mail]; [e-mail];
> > > > [e-mail]
> > > > Cc: [e-mail]; Linuxarm <[e-mail]>;
> > > > [e-mail]; Guohanjun (Hanjun Guo)
> > > > <[e-mail]>; yangyicong <[e-mail]>;
> > > > [e-mail]; wanghuiqiang <[e-mail]>
> > > > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve
> > > > RMR memory regions
> > > >
> > > > On 2021-05-24 12:02, 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
> > > > <[e-mail]>
> > > > > ---
> > > > >   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;
> > > > > +           struct iommu_resv_region *region;
> > > > > +           enum iommu_resv_type type;
> > > > > +           struct acpi_iort_rmr_desc *rmr_desc;
> > > > > +
> > > > > +           if (e->smmu != iommu)
> > > > > +                   continue;
> > > > > +
> > > > > +           rmr_desc = e->rmr_desc;
> > > > > +           if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > > > > +                   type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > > > +           else
> > > > > +                   type = IOMMU_RESV_DIRECT;
> > > >
> >
> > I have been looking at this a bit more and looking at the history of
> > RMR_REMAP_PERMITTED.  Based on the history I have found it seems to
> me
> > like this would be the better options for prot.
> >
> > @@ -840,7 +840,7 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
> >                 return -ENODEV;
> >
> >         list_for_each_entry(e, &iort_rmr_list, list) {
> > -               int prot = IOMMU_READ | IOMMU_WRITE |
> IOMMU_NOEXEC | IOMMU_MMIO;
> > +               int prot = IOMMU_READ | IOMMU_WRITE;
> >                 struct iommu_resv_region *region;
> >                 enum iommu_resv_type type;
> >                 struct acpi_iort_rmr_desc *rmr_desc; @@ -849,10
> > +849,13 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
> >                         continue;
> >
> >                 rmr_desc = e->rmr_desc;
> > -               if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > +               if (e->flags & IOMMU_RMR_REMAP_PERMITTED) {
> >                         type = IOMMU_RESV_DIRECT_RELAXABLE;
> > -               else
> > +                       prot |= IOMMU_CACHE;
> > +               } else {
> >                         type = IOMMU_RESV_DIRECT;
> > +                       prot |= IOMMU_MMIO;
> > +               }
> >
> >                 region =
> iommu_alloc_resv_region(rmr_desc->base_address,
> >                                                  rmr_desc->length,
> >
> > any input on this?  My reasoning is that IOMMU_RESV_DIRECT is
> > specifically referenced for things like MSI doorbell and in all the
> > examples needs IOMMU_MMIO, while REMAP_PERMITTED seems to be used
> for
> > allocated system memory that is then used for device specific reserved
> > regions which commits say would be like a GPU or USB device.

I am Ok to make those changes but not sure we can make the above assumptions
based on the way it is currently used. 

Hi Robin,

Any thoughts?

Thanks,
Shameer

> >
> > -Jon
> >
> > > > Wasn't the idea that we can do all this during the initial parsing, i.e.
> > > > we don't even have iort_rmr_entry, we store them as
> > > > iommu_resv_regions and simply kmemdup() or whatever at this point?
> > >
> > >
> > > Hmm... Not yet. I removed struct iommu_rmr() from v4. But yes, it
> > > looks like we can get rid of iort_rmr_entry as well. Will give it a go in next.
> > >
> > > Thanks,
> > > Shameer
> > >
> > > > Robin.
> > > >
> > > > > +
> > > > > +           region =
> iommu_alloc_resv_region(rmr_desc->base_address,
> > > > > +                                            rmr_desc->length,
> > > > > +                                            prot, type);
> > > > > +           if (region) {
> > > > > +                   region->fw_data.rmr.flags = e->flags;
> > > > > +                   region->fw_data.rmr.sid = e->sid;
> > > > > +                   list_add_tail(&region->list, head);
> > > > > +                   rmrs++;
> > > > > +           }
> > > > > +   }
> > > > > +
> > > > > +   return (rmrs == 0) ? -ENODEV : 0; }
> > > > > +
> > > > >   /**
> > > > >    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> > > > >    * @dev: Device from iommu_get_resv_regions() @@ -1108,6
> > > > > +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct
> > > > device *dev, struct list_head *head)
> > > > >   const struct iommu_ops *iort_iommu_configure_id(struct device
> *dev,
> > > > >                                             const u32
> *input_id)
> > > > >   { return NULL; }
> > > > > +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct
> > > > > +list_head
> > > > *head)
> > > > > +{ return -ENODEV; }
> > > > >   #endif
> > > > >
> > > > >   static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > > diff --git a/drivers/iommu/dma-iommu.c
> > > > > b/drivers/iommu/dma-iommu.c index 229ec65d98be..f893d460cfa4
> > > > > 100644
> > > > > --- a/drivers/iommu/dma-iommu.c
> > > > > +++ b/drivers/iommu/dma-iommu.c
> > > > > @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
> > > > >   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > >                    struct list_head *list)
> > > > >   {
> > > > > +   if (!is_of_node(iommu_fwnode))
> > > > > +           return iort_iommu_get_rmrs(iommu_fwnode, list);
> > > > > +
> > > > >     return -EINVAL;
> > > > >   }
> > > > >   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > > > @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > > >   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > >                     struct list_head *list)
> > > > >   {
> > > > > +   generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
> > > > >   }
> > > > >   EXPORT_SYMBOL(iommu_dma_put_rmrs);
> > > > >
> > > > > diff --git a/include/linux/acpi_iort.h
> > > > > b/include/linux/acpi_iort.h index 1a12baa58e40..e8c45fa59531
> > > > > 100644
> > > > > --- a/include/linux/acpi_iort.h
> > > > > +++ b/include/linux/acpi_iort.h
> > > > > @@ -39,6 +39,8 @@ const struct iommu_ops
> > > > *iort_iommu_configure_id(struct device *dev,
> > > > >                                             const u32 *id_in);
> > > > >   int iort_iommu_msi_get_resv_regions(struct device *dev, struct
> > > > > list_head
> > > > *head);
> > > > >   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> > > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > > +                   struct list_head *list);
> > > > >   #else
> > > > >   static inline void acpi_iort_init(void) { }
> > > > >   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> > > > > @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct
> > > > > device
> > > > *dev, struct list_head *head)
> > > > >
> > > > >   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
> > > > >   { return PHYS_ADDR_MAX; }
> > > > > +
> > > > > +static inline
> > > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > > +                   struct list_head *list) { return -ENODEV; }
> > > > >   #endif
> > > > >
> > > > >   #endif /* __ACPI_IORT_H__ */
> > > > >
> 
> Ping.  Any comments on this?
> 
> -Jon

Patch

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;
+		struct iommu_resv_region *region;
+		enum iommu_resv_type type;
+		struct acpi_iort_rmr_desc *rmr_desc;
+
+		if (e->smmu != iommu)
+			continue;
+
+		rmr_desc = e->rmr_desc;
+		if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
+			type = IOMMU_RESV_DIRECT_RELAXABLE;
+		else
+			type = IOMMU_RESV_DIRECT;
+
+		region = iommu_alloc_resv_region(rmr_desc->base_address,
+						 rmr_desc->length,
+						 prot, type);
+		if (region) {
+			region->fw_data.rmr.flags = e->flags;
+			region->fw_data.rmr.sid = e->sid;
+			list_add_tail(&region->list, head);
+			rmrs++;
+		}
+	}
+
+	return (rmrs == 0) ? -ENODEV : 0;
+}
+
 /**
  * iort_iommu_msi_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
@@ -1108,6 +1156,8 @@  int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
 const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
 						const u32 *input_id)
 { return NULL; }
+int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head *head)
+{ return -ENODEV; }
 #endif
 
 static int nc_dma_get_range(struct device *dev, u64 *size)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 229ec65d98be..f893d460cfa4 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -185,6 +185,9 @@  EXPORT_SYMBOL(iommu_put_dma_cookie);
 int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
 		       struct list_head *list)
 {
+	if (!is_of_node(iommu_fwnode))
+		return iort_iommu_get_rmrs(iommu_fwnode, list);
+
 	return -EINVAL;
 }
 EXPORT_SYMBOL(iommu_dma_get_rmrs);
@@ -200,6 +203,7 @@  EXPORT_SYMBOL(iommu_dma_get_rmrs);
 void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
 			struct list_head *list)
 {
+	generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
 }
 EXPORT_SYMBOL(iommu_dma_put_rmrs);
 
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 1a12baa58e40..e8c45fa59531 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -39,6 +39,8 @@  const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
 						const u32 *id_in);
 int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head);
 phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
+int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
+			struct list_head *list);
 #else
 static inline void acpi_iort_init(void) { }
 static inline u32 iort_msi_map_id(struct device *dev, u32 id)
@@ -59,6 +61,11 @@  int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
 
 static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
 { return PHYS_ADDR_MAX; }
+
+static inline
+int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
+			struct list_head *list)
+{ return -ENODEV; }
 #endif
 
 #endif /* __ACPI_IORT_H__ */