mbox series

[v9,0/5] iommu: Support mappings/reservations in reserved-memory regions

Message ID 20220923123557.866972-1-thierry.reding@gmail.com
Headers show
Series iommu: Support mappings/reservations in reserved-memory regions | expand

Message

Thierry Reding Sept. 23, 2022, 12:35 p.m. UTC
From: Thierry Reding <treding@nvidia.com>

Hi,

This version has several fixes over the previous v8, which can be found
here:

  https://lore.kernel.org/all/20220905170833.396892-1-thierry.reding@gmail.com/

An example is included in the DT bindings, but here is an extract of
what I've used to test this:

        reserved-memory {
                #address-cells = <2>;
                #size-cells = <2>;
                ranges;

                /*
                 * Creates an identity mapping for the framebuffer that
                 * the firmware has setup to scan out a bootsplash from.
                 */
                fb: framebuffer@92cb2000 {
                        reg = <0x0 0x92cb2000 0x0 0x00800000>;
                        iommu-addresses = <&dc0 0x0 0x92cb2000 0x0 0x00800000>;
                };

                /*
                 * Creates a reservation in the IOVA space to prevent
                 * any buffers from being mapped to that region. Note
                 * that on Tegra the range is actually quite different
                 * from this, but it would conflict with the display
                 * driver that I tested this against, so this is just
                 * a dummy region for testing.
                 */
                adsp: reservation-adsp {
                        iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00010000>;
                };
        };

        host1x@50000000 {
                dc@54200000 {
                        memory-region = <&fb>, <&adsp>;
                };
        };

This is abbreviated a little to focus on the essentials. Note also that
the ADSP reservation is not actually used on this device and the driver
for this doesn't exist yet, but I wanted to include this variant for
testing, because we'll want to use these bindings for the reservation
use-case as well at some point.

I've also been able to make use of this binding and the IOMMU code in
conjunction with the simple-framebuffer driver to hand over a display
configuration set up by UEFI to the Linux kernel.

Janne has confirmed[0] this to be suitable for indirect mappings as
well, though these patches don't implement that feature yet. Potential
extensions to this have been discussed but are not yet included at this
time to not further complicate things.

Thierry

[0]: https://lore.kernel.org/all/20220909144504.GA4024@jannau.net/

Navneet Kumar (1):
  iommu/tegra-smmu: Support managed domains

Thierry Reding (4):
  dt-bindings: reserved-memory: Document iommu-addresses
  iommu: Implement of_iommu_get_resv_regions()
  iommu: dma: Use of_iommu_get_resv_regions()
  iommu/tegra-smmu: Add support for reserved regions

 .../reserved-memory/reserved-memory.yaml      |  70 ++++++++++++
 drivers/iommu/dma-iommu.c                     |   3 +
 drivers/iommu/of_iommu.c                      | 104 ++++++++++++++++++
 drivers/iommu/tegra-smmu.c                    |  86 ++++++++++++---
 include/linux/of_iommu.h                      |   8 ++
 5 files changed, 254 insertions(+), 17 deletions(-)

Comments

Thierry Reding Oct. 7, 2022, 12:51 p.m. UTC | #1
On Fri, Sep 23, 2022 at 02:35:52PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Hi,
> 
> This version has several fixes over the previous v8, which can be found
> here:
> 
>   https://lore.kernel.org/all/20220905170833.396892-1-thierry.reding@gmail.com/
> 
> An example is included in the DT bindings, but here is an extract of
> what I've used to test this:
> 
>         reserved-memory {
>                 #address-cells = <2>;
>                 #size-cells = <2>;
>                 ranges;
> 
>                 /*
>                  * Creates an identity mapping for the framebuffer that
>                  * the firmware has setup to scan out a bootsplash from.
>                  */
>                 fb: framebuffer@92cb2000 {
>                         reg = <0x0 0x92cb2000 0x0 0x00800000>;
>                         iommu-addresses = <&dc0 0x0 0x92cb2000 0x0 0x00800000>;
>                 };
> 
>                 /*
>                  * Creates a reservation in the IOVA space to prevent
>                  * any buffers from being mapped to that region. Note
>                  * that on Tegra the range is actually quite different
>                  * from this, but it would conflict with the display
>                  * driver that I tested this against, so this is just
>                  * a dummy region for testing.
>                  */
>                 adsp: reservation-adsp {
>                         iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00010000>;
>                 };
>         };
> 
>         host1x@50000000 {
>                 dc@54200000 {
>                         memory-region = <&fb>, <&adsp>;
>                 };
>         };
> 
> This is abbreviated a little to focus on the essentials. Note also that
> the ADSP reservation is not actually used on this device and the driver
> for this doesn't exist yet, but I wanted to include this variant for
> testing, because we'll want to use these bindings for the reservation
> use-case as well at some point.
> 
> I've also been able to make use of this binding and the IOMMU code in
> conjunction with the simple-framebuffer driver to hand over a display
> configuration set up by UEFI to the Linux kernel.
> 
> Janne has confirmed[0] this to be suitable for indirect mappings as
> well, though these patches don't implement that feature yet. Potential
> extensions to this have been discussed but are not yet included at this
> time to not further complicate things.
> 
> Thierry
> 
> [0]: https://lore.kernel.org/all/20220909144504.GA4024@jannau.net/
> 
> Navneet Kumar (1):
>   iommu/tegra-smmu: Support managed domains
> 
> Thierry Reding (4):
>   dt-bindings: reserved-memory: Document iommu-addresses
>   iommu: Implement of_iommu_get_resv_regions()
>   iommu: dma: Use of_iommu_get_resv_regions()
>   iommu/tegra-smmu: Add support for reserved regions
> 
>  .../reserved-memory/reserved-memory.yaml      |  70 ++++++++++++
>  drivers/iommu/dma-iommu.c                     |   3 +
>  drivers/iommu/of_iommu.c                      | 104 ++++++++++++++++++
>  drivers/iommu/tegra-smmu.c                    |  86 ++++++++++++---
>  include/linux/of_iommu.h                      |   8 ++
>  5 files changed, 254 insertions(+), 17 deletions(-)

Joerg, if there are no further concerns on this, can you pick this up
once v6.1-rc1 is released?

Thanks,
Thierry
Robin Murphy Oct. 7, 2022, 1:45 p.m. UTC | #2
On 2022-09-23 13:35, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This adds the "iommu-addresses" property to reserved-memory nodes, which
> allow describing the interaction of memory regions with IOMMUs. Two use-
> cases are supported:
> 
>    1. Static mappings can be described by pairing the "iommu-addresses"
>       property with a "reg" property. This is mostly useful for adopting
>       firmware-allocated buffers via identity mappings. One common use-
>       case where this is required is if early firmware or bootloaders
>       have set up a bootsplash framebuffer that a display controller is
>       actively scanning out from during the operating system boot
>       process.
> 
>    2. If an "iommu-addresses" property exists without a "reg" property,
>       the reserved-memory node describes an IOVA reservation. Such memory
>       regions are excluded from the IOVA space available to operating
>       system drivers and can be used for regions that must not be used to
>       map arbitrary buffers.

Bah, I've only just realised: don't we also need to change the "oneOf: 
required: ..." schema to permit "iommu-addresses" without "reg" or "size"?

Thanks,
Robin.

> Each mapping or reservation is tied to a specific device via a phandle
> to the device's device tree node. This allows a reserved-memory region
> to be reused across multiple devices.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v9:
> - add Reviewed-by tags
> 
> Changes in v8:
> - include validation warnings that had crept into an unrelated patch
> 
> Changes in v7:
> - keep reserved-memory.txt to avoid broken references
> 
> Changes in v6:
> - add device phandle to iommu-addresses property in examples
> - remove now unused dt-bindings/reserved-memory.h header
> 
>   .../reserved-memory/reserved-memory.yaml      | 70 +++++++++++++++++++
>   1 file changed, 70 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> index 44f72bcf1782..fb48d016e368 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> @@ -52,6 +52,30 @@ properties:
>         Address and Length pairs. Specifies regions of memory that are
>         acceptable to allocate from.
>   
> +  iommu-addresses:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: >
> +      A list of phandle and specifier pairs that describe static IO virtual
> +      address space mappings and carveouts associated with a given reserved
> +      memory region. The phandle in the first cell refers to the device for
> +      which the mapping or carveout is to be created.
> +
> +      The specifier consists of an address/size pair and denotes the IO
> +      virtual address range of the region for the given device. The exact
> +      format depends on the values of the "#address-cells" and "#size-cells"
> +      properties of the device referenced via the phandle.
> +
> +      When used in combination with a "reg" property, an IOVA mapping is to
> +      be established for this memory region. One example where this can be
> +      useful is to create an identity mapping for physical memory that the
> +      firmware has configured some hardware to access (such as a bootsplash
> +      framebuffer).
> +
> +      If no "reg" property is specified, the "iommu-addresses" property
> +      defines carveout regions in the IOVA space for the given device. This
> +      can be useful if a certain memory region should not be mapped through
> +      the IOMMU.
> +
>     no-map:
>       type: boolean
>       description: >
> @@ -97,4 +121,50 @@ oneOf:
>   
>   additionalProperties: true
>   
> +examples:
> +  - |
> +    / {
> +      compatible = "foo";
> +      model = "foo";
> +
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      reserved-memory {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        ranges;
> +
> +        adsp_resv: reservation-adsp {
> +          /*
> +           * Restrict IOVA mappings for ADSP buffers to the 512 MiB region
> +           * from 0x40000000 - 0x5fffffff. Anything outside is reserved by
> +           * the ADSP for I/O memory and private memory allocations.
> +           */
> +          iommu-addresses = <&adsp 0x0 0x00000000 0x00 0x40000000>,
> +                            <&adsp 0x0 0x60000000 0xff 0xa0000000>;
> +        };
> +
> +        fb: framebuffer@90000000 {
> +          reg = <0x0 0x90000000 0x0 0x00800000>;
> +          iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00800000>;
> +        };
> +      };
> +
> +      bus@0 {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges = <0x0 0x0 0x0 0x40000000>;
> +
> +        adsp: adsp@2990000 {
> +          reg = <0x2990000 0x2000>;
> +          memory-region = <&adsp_resv>;
> +        };
> +
> +        dc0: display@15200000 {
> +          reg = <0x15200000 0x10000>;
> +          memory-region = <&fb>;
> +        };
> +      };
> +    };
>   ...
Robin Murphy Oct. 7, 2022, 1:48 p.m. UTC | #3
On 2022-09-23 13:35, Thierry Reding wrote:
> From: Navneet Kumar <navneetk@nvidia.com>
> 
> Allow creating identity and DMA API compatible IOMMU domains. When
> creating a DMA API compatible domain, make sure to also create the
> required cookie.

Nit: this description is now confusingly outdated.

> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v5:
> - remove DMA cookie initialization that's now no longer needed
> 
>   drivers/iommu/tegra-smmu.c | 38 +++++++++++++++++++++-----------------
>   1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 57b4f2b37447..7ad993330634 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -20,6 +20,8 @@
>   #include <soc/tegra/ahb.h>
>   #include <soc/tegra/mc.h>
>   
> +#include "dma-iommu.h"
> +
>   struct tegra_smmu_group {
>   	struct list_head list;
>   	struct tegra_smmu *smmu;
> @@ -277,7 +279,9 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>   {
>   	struct tegra_smmu_as *as;
>   
> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> +	    type != IOMMU_DOMAIN_DMA &&
> +	    type != IOMMU_DOMAIN_IDENTITY)

Since there's apparently no actual handling of IOMMU_DOMAIN_IDENTITY 
being added anywhere, AFAICS it's still going to set up an address space 
for translation with nothing mapped in its pagetable, which is pretty 
much the opposite of what's required :/

>   		return NULL;
>   
>   	as = kzalloc(sizeof(*as), GFP_KERNEL);
> @@ -287,25 +291,16 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>   	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
>   
>   	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
> -	if (!as->pd) {
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->pd)
> +		goto free_as;
>   
>   	as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
> -	if (!as->count) {
> -		__free_page(as->pd);
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->count)
> +		goto free_pd_range;
>   
>   	as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
> -	if (!as->pts) {
> -		kfree(as->count);
> -		__free_page(as->pd);
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->pts)
> +		goto free_pts;

Nit: all this part is now just unrelated refactoring.

Thanks,
Robin.

>   
>   	spin_lock_init(&as->lock);
>   
> @@ -315,6 +310,15 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>   	as->domain.geometry.force_aperture = true;
>   
>   	return &as->domain;
> +
> +free_pts:
> +	kfree(as->pts);
> +free_pd_range:
> +	__free_page(as->pd);
> +free_as:
> +	kfree(as);
> +
> +	return NULL;
>   }
>   
>   static void tegra_smmu_domain_free(struct iommu_domain *domain)
> @@ -1012,7 +1016,7 @@ static const struct iommu_ops tegra_smmu_ops = {
>   	.probe_device = tegra_smmu_probe_device,
>   	.release_device = tegra_smmu_release_device,
>   	.device_group = tegra_smmu_device_group,
> -	.get_resv_regions = of_iommu_get_resv_regions,
> +	.get_resv_regions = iommu_dma_get_resv_regions,
>   	.of_xlate = tegra_smmu_of_xlate,
>   	.pgsize_bitmap = SZ_4K,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
Thierry Reding Oct. 7, 2022, 1:54 p.m. UTC | #4
On Fri, Oct 07, 2022 at 02:45:31PM +0100, Robin Murphy wrote:
> On 2022-09-23 13:35, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > This adds the "iommu-addresses" property to reserved-memory nodes, which
> > allow describing the interaction of memory regions with IOMMUs. Two use-
> > cases are supported:
> > 
> >    1. Static mappings can be described by pairing the "iommu-addresses"
> >       property with a "reg" property. This is mostly useful for adopting
> >       firmware-allocated buffers via identity mappings. One common use-
> >       case where this is required is if early firmware or bootloaders
> >       have set up a bootsplash framebuffer that a display controller is
> >       actively scanning out from during the operating system boot
> >       process.
> > 
> >    2. If an "iommu-addresses" property exists without a "reg" property,
> >       the reserved-memory node describes an IOVA reservation. Such memory
> >       regions are excluded from the IOVA space available to operating
> >       system drivers and can be used for regions that must not be used to
> >       map arbitrary buffers.
> 
> Bah, I've only just realised: don't we also need to change the "oneOf:
> required: ..." schema to permit "iommu-addresses" without "reg" or "size"?

Hm... good point. I think at least we'll want another:

     - required:
         - iommu-addresses

in there. I wonder if we also need to avoid the combination of "size"
and "iommu-addresses". When "size" is specified, is it guaranteed that
those regions will be allocated before the direct mapping needs to be
created?

Thierry
Robin Murphy Oct. 7, 2022, 2:21 p.m. UTC | #5
On 2022-10-07 14:54, Thierry Reding wrote:
> On Fri, Oct 07, 2022 at 02:45:31PM +0100, Robin Murphy wrote:
>> On 2022-09-23 13:35, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> This adds the "iommu-addresses" property to reserved-memory nodes, which
>>> allow describing the interaction of memory regions with IOMMUs. Two use-
>>> cases are supported:
>>>
>>>     1. Static mappings can be described by pairing the "iommu-addresses"
>>>        property with a "reg" property. This is mostly useful for adopting
>>>        firmware-allocated buffers via identity mappings. One common use-
>>>        case where this is required is if early firmware or bootloaders
>>>        have set up a bootsplash framebuffer that a display controller is
>>>        actively scanning out from during the operating system boot
>>>        process.
>>>
>>>     2. If an "iommu-addresses" property exists without a "reg" property,
>>>        the reserved-memory node describes an IOVA reservation. Such memory
>>>        regions are excluded from the IOVA space available to operating
>>>        system drivers and can be used for regions that must not be used to
>>>        map arbitrary buffers.
>>
>> Bah, I've only just realised: don't we also need to change the "oneOf:
>> required: ..." schema to permit "iommu-addresses" without "reg" or "size"?
> 
> Hm... good point. I think at least we'll want another:
> 
>       - required:
>           - iommu-addresses
> 
> in there. I wonder if we also need to avoid the combination of "size"
> and "iommu-addresses". When "size" is specified, is it guaranteed that
> those regions will be allocated before the direct mapping needs to be
> created?

Well, it couldn't really be a direct mapping anyway. In general I don't 
think that combination makes any sense, since the presence of 
"iommu-addresses" means one of two things; either it says the IOVA range 
is carved out for some special purpose or just unusable, in which case 
allocating any memory to back it would surely be pointless, or it's 
saying don't touch these addresses because the device is already 
accessing them, thus the underlying physical memory must be allocated 
somewhere already.

Thanks,
Robin.
Thierry Reding Oct. 7, 2022, 3:22 p.m. UTC | #6
On Fri, Oct 07, 2022 at 03:21:46PM +0100, Robin Murphy wrote:
> On 2022-10-07 14:54, Thierry Reding wrote:
> > On Fri, Oct 07, 2022 at 02:45:31PM +0100, Robin Murphy wrote:
> > > On 2022-09-23 13:35, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > This adds the "iommu-addresses" property to reserved-memory nodes, which
> > > > allow describing the interaction of memory regions with IOMMUs. Two use-
> > > > cases are supported:
> > > > 
> > > >     1. Static mappings can be described by pairing the "iommu-addresses"
> > > >        property with a "reg" property. This is mostly useful for adopting
> > > >        firmware-allocated buffers via identity mappings. One common use-
> > > >        case where this is required is if early firmware or bootloaders
> > > >        have set up a bootsplash framebuffer that a display controller is
> > > >        actively scanning out from during the operating system boot
> > > >        process.
> > > > 
> > > >     2. If an "iommu-addresses" property exists without a "reg" property,
> > > >        the reserved-memory node describes an IOVA reservation. Such memory
> > > >        regions are excluded from the IOVA space available to operating
> > > >        system drivers and can be used for regions that must not be used to
> > > >        map arbitrary buffers.
> > > 
> > > Bah, I've only just realised: don't we also need to change the "oneOf:
> > > required: ..." schema to permit "iommu-addresses" without "reg" or "size"?
> > 
> > Hm... good point. I think at least we'll want another:
> > 
> >       - required:
> >           - iommu-addresses
> > 
> > in there. I wonder if we also need to avoid the combination of "size"
> > and "iommu-addresses". When "size" is specified, is it guaranteed that
> > those regions will be allocated before the direct mapping needs to be
> > created?
> 
> Well, it couldn't really be a direct mapping anyway. In general I don't
> think that combination makes any sense, since the presence of
> "iommu-addresses" means one of two things; either it says the IOVA range is
> carved out for some special purpose or just unusable, in which case
> allocating any memory to back it would surely be pointless, or it's saying
> don't touch these addresses because the device is already accessing them,
> thus the underlying physical memory must be allocated somewhere already.

I thought perhaps there could be cases where it is known that a
controller needs to access memory in a certain I/O virtual region but
doesn't actually care where that lives in physical memory and also does
not rely on that memory have been previously set up (pre-filled, or
whatever). Say you've got a micro-controller in a system that needs its
firmware in a given region, but the OS can set up that region without
any other limitations. One could use "size" and "iommu-addresses" to
make sure the region is allocated with a specific size and located in a
specific I/O virtual region. Not sure if that's perhaps a bit exotic,
though.

Thierry
Thierry Reding Oct. 7, 2022, 3:40 p.m. UTC | #7
On Fri, Oct 07, 2022 at 02:48:19PM +0100, Robin Murphy wrote:
> On 2022-09-23 13:35, Thierry Reding wrote:
> > From: Navneet Kumar <navneetk@nvidia.com>
> > 
> > Allow creating identity and DMA API compatible IOMMU domains. When
> > creating a DMA API compatible domain, make sure to also create the
> > required cookie.
> 
> Nit: this description is now confusingly outdated.
> 
> > Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v5:
> > - remove DMA cookie initialization that's now no longer needed
> > 
> >   drivers/iommu/tegra-smmu.c | 38 +++++++++++++++++++++-----------------
> >   1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index 57b4f2b37447..7ad993330634 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -20,6 +20,8 @@
> >   #include <soc/tegra/ahb.h>
> >   #include <soc/tegra/mc.h>
> > +#include "dma-iommu.h"
> > +
> >   struct tegra_smmu_group {
> >   	struct list_head list;
> >   	struct tegra_smmu *smmu;
> > @@ -277,7 +279,9 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
> >   {
> >   	struct tegra_smmu_as *as;
> > -	if (type != IOMMU_DOMAIN_UNMANAGED)
> > +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> > +	    type != IOMMU_DOMAIN_DMA &&
> > +	    type != IOMMU_DOMAIN_IDENTITY)
> 
> Since there's apparently no actual handling of IOMMU_DOMAIN_IDENTITY being
> added anywhere, AFAICS it's still going to set up an address space for
> translation with nothing mapped in its pagetable, which is pretty much the
> opposite of what's required :/

Yeah, I think we can safely skip identity domains. I don't think I've
ever seen them get used on Tegra.

> 
> >   		return NULL;
> >   	as = kzalloc(sizeof(*as), GFP_KERNEL);
> > @@ -287,25 +291,16 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
> >   	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
> >   	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
> > -	if (!as->pd) {
> > -		kfree(as);
> > -		return NULL;
> > -	}
> > +	if (!as->pd)
> > +		goto free_as;
> >   	as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
> > -	if (!as->count) {
> > -		__free_page(as->pd);
> > -		kfree(as);
> > -		return NULL;
> > -	}
> > +	if (!as->count)
> > +		goto free_pd_range;
> >   	as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
> > -	if (!as->pts) {
> > -		kfree(as->count);
> > -		__free_page(as->pd);
> > -		kfree(as);
> > -		return NULL;
> > -	}
> > +	if (!as->pts)
> > +		goto free_pts;
> 
> Nit: all this part is now just unrelated refactoring.

Okay, I'll see if I can pull this into a separate patch. Or perhaps just
drop it.

Frankly, at this point it's unlikely that all of this will ever be
deployed on Tegra210 (and earlier) that used this SMMU, so I've included
this particular patch here primarily because Tegra210 was originally
meant to be the primary target. This patch is now close to 4 years
old...

Thierry

> 
> Thanks,
> Robin.
> 
> >   	spin_lock_init(&as->lock);
> > @@ -315,6 +310,15 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
> >   	as->domain.geometry.force_aperture = true;
> >   	return &as->domain;
> > +
> > +free_pts:
> > +	kfree(as->pts);
> > +free_pd_range:
> > +	__free_page(as->pd);
> > +free_as:
> > +	kfree(as);
> > +
> > +	return NULL;
> >   }
> >   static void tegra_smmu_domain_free(struct iommu_domain *domain)
> > @@ -1012,7 +1016,7 @@ static const struct iommu_ops tegra_smmu_ops = {
> >   	.probe_device = tegra_smmu_probe_device,
> >   	.release_device = tegra_smmu_release_device,
> >   	.device_group = tegra_smmu_device_group,
> > -	.get_resv_regions = of_iommu_get_resv_regions,
> > +	.get_resv_regions = iommu_dma_get_resv_regions,
> >   	.of_xlate = tegra_smmu_of_xlate,
> >   	.pgsize_bitmap = SZ_4K,
> >   	.default_domain_ops = &(const struct iommu_domain_ops) {
Robin Murphy Oct. 7, 2022, 4:25 p.m. UTC | #8
On 2022-10-07 16:22, Thierry Reding wrote:
> On Fri, Oct 07, 2022 at 03:21:46PM +0100, Robin Murphy wrote:
>> On 2022-10-07 14:54, Thierry Reding wrote:
>>> On Fri, Oct 07, 2022 at 02:45:31PM +0100, Robin Murphy wrote:
>>>> On 2022-09-23 13:35, Thierry Reding wrote:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> This adds the "iommu-addresses" property to reserved-memory nodes, which
>>>>> allow describing the interaction of memory regions with IOMMUs. Two use-
>>>>> cases are supported:
>>>>>
>>>>>      1. Static mappings can be described by pairing the "iommu-addresses"
>>>>>         property with a "reg" property. This is mostly useful for adopting
>>>>>         firmware-allocated buffers via identity mappings. One common use-
>>>>>         case where this is required is if early firmware or bootloaders
>>>>>         have set up a bootsplash framebuffer that a display controller is
>>>>>         actively scanning out from during the operating system boot
>>>>>         process.
>>>>>
>>>>>      2. If an "iommu-addresses" property exists without a "reg" property,
>>>>>         the reserved-memory node describes an IOVA reservation. Such memory
>>>>>         regions are excluded from the IOVA space available to operating
>>>>>         system drivers and can be used for regions that must not be used to
>>>>>         map arbitrary buffers.
>>>>
>>>> Bah, I've only just realised: don't we also need to change the "oneOf:
>>>> required: ..." schema to permit "iommu-addresses" without "reg" or "size"?
>>>
>>> Hm... good point. I think at least we'll want another:
>>>
>>>        - required:
>>>            - iommu-addresses
>>>
>>> in there. I wonder if we also need to avoid the combination of "size"
>>> and "iommu-addresses". When "size" is specified, is it guaranteed that
>>> those regions will be allocated before the direct mapping needs to be
>>> created?
>>
>> Well, it couldn't really be a direct mapping anyway. In general I don't
>> think that combination makes any sense, since the presence of
>> "iommu-addresses" means one of two things; either it says the IOVA range is
>> carved out for some special purpose or just unusable, in which case
>> allocating any memory to back it would surely be pointless, or it's saying
>> don't touch these addresses because the device is already accessing them,
>> thus the underlying physical memory must be allocated somewhere already.
> 
> I thought perhaps there could be cases where it is known that a
> controller needs to access memory in a certain I/O virtual region but
> doesn't actually care where that lives in physical memory and also does
> not rely on that memory have been previously set up (pre-filled, or
> whatever). Say you've got a micro-controller in a system that needs its
> firmware in a given region, but the OS can set up that region without
> any other limitations. One could use "size" and "iommu-addresses" to
> make sure the region is allocated with a specific size and located in a
> specific I/O virtual region. Not sure if that's perhaps a bit exotic,
> though.

Yeah, that was the closest case I could think of as well, but I'd really 
rather not encourage people to abuse DT that way. If a kernel driver is 
loading firmware and initialising the device from scratch then it should 
be able to sort everything out at runtime without DT involvement. Even 
if the firmware is somehow massive enough to warrant an early dynamic 
carveout rather than a regular page/CMA allocation, there's still no 
good excuse for the driver not to manage its own address space constraints.

On the other hand if the device really does need its firmware at a 
specific hard-coded address than that would need a fixed physical 
reservation anyway, since the DT can't assume that the OS is definitely 
going to use IOMMU translation.

Thanks,
Robin.