[PATCH/RFC 0/2] iommu/ipmmu-vmsa: IPMMU slave device whitelist

[PATCH/RFC 0/2] iommu/ipmmu-vmsa: IPMMU slave device whitelist

[Magnus Damm]

iommu/ipmmu-vmsa: IPMMU slave device whitelist

[PATCH/RFC 1/2] arm64: mm: Silently allow devices lacking IOMMU group
[PATCH/RFC 2/2] iommu/ipmmu-vmsa: Opt-in slave devices based on ES version

Here's a little prototype that shows how DT integration of IPMMU details
may be integrated and merged upstream based on SoC data sheet ahead of
time followed by enablement in the IPMMU driver code once the appropriate
SoC ES version has been released and the hardware has been tested.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Developed on top of renesas-drivers-2017-01-10-v4.10-rc3

 arch/arm64/mm/dma-mapping.c |   10 +++++++++-
 drivers/iommu/ipmmu-vmsa.c  |   24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

[PATCH/RFC 1/2] arm64: mm: Silently allow devices lacking IOMMU group

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Consider failure of iommu_get_domain_for_dev() as non-critical and
get rid of the warning printout. This allows IOMMU properties to be
included in the DTB even though the kernel is configured with
CONFIG_IOMMU_API=n or in case a particular IOMMU driver refuses to
enable IOMMU support for a certain slave device and returns error
from the ->add_device() callback.

This is only a cosmetic change that removes console warning printouts.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 arch/arm64/mm/dma-mapping.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

--- 0001/arch/arm64/mm/dma-mapping.c
+++ work/arch/arm64/mm/dma-mapping.c	2017-01-23 20:54:40.060607110 +0900
@@ -827,11 +827,19 @@ static bool do_iommu_attach(struct devic
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 
 	/*
+	 * In case IOMMU support is excluded from the kernel or if the device
+	 * is not hooked up to any IOMMU group then be silent and keep the
+	 * old dma_ops.
+	 */
+	if (!domain)
+		return false;
+
+	/*
 	 * If the IOMMU driver has the DMA domain support that we require,
 	 * then the IOMMU core will have already configured a group for this
 	 * device, and allocated the default domain for that group.
 	 */
-	if (!domain || iommu_dma_init_domain(domain, dma_base, size, dev)) {
+	if (iommu_dma_init_domain(domain, dma_base, size, dev)) {
 		pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA ops\n",
 			dev_name(dev));
 		return false;

[PATCH/RFC 1/2] arm64: mm: Silently allow devices lacking IOMMU group

[Robin Murphy]

Hi Magnus,

On 23/01/17 12:12, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Consider failure of iommu_get_domain_for_dev() as non-critical and
> get rid of the warning printout. This allows IOMMU properties to be
> included in the DTB even though the kernel is configured with
> CONFIG_IOMMU_API=n or in case a particular IOMMU driver refuses to
> enable IOMMU support for a certain slave device and returns error
> from the ->add_device() callback.
> 
> This is only a cosmetic change that removes console warning printouts.

The warning is there for a reason - at this point, we *expected* the
device to be using an IOMMU for DMA, so a failure is significant. Rather
than masking genuine failures in other cases because your case
deliberately breaks that expectation, simply change the expectation -
i.e. rather than letting of_xlate() succeed then failing add_device()
later, reject the of_xlate() call up-front such that the DMA layer never
gets told about the IOMMU in the first place.

Robin.

> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  arch/arm64/mm/dma-mapping.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> --- 0001/arch/arm64/mm/dma-mapping.c
> +++ work/arch/arm64/mm/dma-mapping.c	2017-01-23 20:54:40.060607110 +0900
> @@ -827,11 +827,19 @@ static bool do_iommu_attach(struct devic
>  	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>  
>  	/*
> +	 * In case IOMMU support is excluded from the kernel or if the device
> +	 * is not hooked up to any IOMMU group then be silent and keep the
> +	 * old dma_ops.
> +	 */
> +	if (!domain)
> +		return false;
> +
> +	/*
>  	 * If the IOMMU driver has the DMA domain support that we require,
>  	 * then the IOMMU core will have already configured a group for this
>  	 * device, and allocated the default domain for that group.
>  	 */
> -	if (!domain || iommu_dma_init_domain(domain, dma_base, size, dev)) {
> +	if (iommu_dma_init_domain(domain, dma_base, size, dev)) {
>  		pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA ops\n",
>  			dev_name(dev));
>  		return false;
> _______________________________________________
> iommu mailing list
> iommu at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 

[PATCH/RFC 1/2] arm64: mm: Silently allow devices lacking IOMMU group

[Sricharan]

Hi Robin,

>> Consider failure of iommu_get_domain_for_dev() as non-critical and
>> get rid of the warning printout. This allows IOMMU properties to be
>> included in the DTB even though the kernel is configured with
>> CONFIG_IOMMU_API=n or in case a particular IOMMU driver refuses to
>> enable IOMMU support for a certain slave device and returns error
>> from the ->add_device() callback.
>>
>> This is only a cosmetic change that removes console warning printouts.
>
>The warning is there for a reason - at this point, we *expected* the
>device to be using an IOMMU for DMA, so a failure is significant. Rather
>than masking genuine failures in other cases because your case
>deliberately breaks that expectation, simply change the expectation -
>i.e. rather than letting of_xlate() succeed then failing add_device()
>later, reject the of_xlate() call up-front such that the DMA layer never
>gets told about the IOMMU in the first place.
>
>Robin.
>

With the iommu probe deferral patches, this behavior would change
where the arch_setup_dma_ops would never be called if there is
an error from xlate or add_device. But also the error value from
xlate/add_device is returned back and the probe of the device
would fail for any error. So if there can be cases like above, where
the xlate/add_device callbacks can return error for specific reasons,
should only EPROBE_DEFER be considered and rest of the errors
be filtered out with a WARN probably ?

Regards,
 Sricharan


>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  arch/arm64/mm/dma-mapping.c |   10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> --- 0001/arch/arm64/mm/dma-mapping.c
>> +++ work/arch/arm64/mm/dma-mapping.c	2017-01-23 20:54:40.060607110 +0900
>> @@ -827,11 +827,19 @@ static bool do_iommu_attach(struct devic
>>  	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>>
>>  	/*
>> +	 * In case IOMMU support is excluded from the kernel or if the device
>> +	 * is not hooked up to any IOMMU group then be silent and keep the
>> +	 * old dma_ops.
>> +	 */
>> +	if (!domain)
>> +		return false;
>> +
>> +	/*
>>  	 * If the IOMMU driver has the DMA domain support that we require,
>>  	 * then the IOMMU core will have already configured a group for this
>>  	 * device, and allocated the default domain for that group.
>>  	 */
>> -	if (!domain || iommu_dma_init_domain(domain, dma_base, size, dev)) {
>> +	if (iommu_dma_init_domain(domain, dma_base, size, dev)) {
>>  		pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA ops\n",
>>  			dev_name(dev));
>>  		return false;
>> _______________________________________________
>> iommu mailing list
>> iommu at lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
>
>_______________________________________________
>iommu mailing list
>iommu at lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH/RFC 1/2] arm64: mm: Silently allow devices lacking IOMMU group

[Magnus Damm]

Hi Robin,

On Mon, Jan 23, 2017 at 9:34 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> Hi Magnus,
>
> On 23/01/17 12:12, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Consider failure of iommu_get_domain_for_dev() as non-critical and
>> get rid of the warning printout. This allows IOMMU properties to be
>> included in the DTB even though the kernel is configured with
>> CONFIG_IOMMU_API=n or in case a particular IOMMU driver refuses to
>> enable IOMMU support for a certain slave device and returns error
>> from the ->add_device() callback.
>>
>> This is only a cosmetic change that removes console warning printouts.
>
> The warning is there for a reason - at this point, we *expected* the
> device to be using an IOMMU for DMA, so a failure is significant. Rather
> than masking genuine failures in other cases because your case
> deliberately breaks that expectation, simply change the expectation -
> i.e. rather than letting of_xlate() succeed then failing add_device()
> later, reject the of_xlate() call up-front such that the DMA layer never
> gets told about the IOMMU in the first place.

Thanks for pointing me in the right direction! I will try to handle
this in xlate() instead.

Cheers,

/ magnus

[PATCH/RFC 2/2] iommu/ipmmu-vmsa: Opt-in slave devices based on ES version

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Match on r8a7795 ES2 and enable a certain DMA controller.
In other cases the IPMMU driver remains disabled.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

--- 0001/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-01-23 20:57:02.620607110 +0900
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/sys_soc.h>
 
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 #include <asm/dma-iommu.h>
@@ -1002,16 +1003,39 @@ static void ipmmu_domain_free_dma(struct
 	}
 }
 
+static const struct soc_device_attribute r8a7795es2[] = {
+	{ .soc_id = "r8a7795", .revision = "ES2.*" },
+	{ /* sentinel */ }
+};
+
+static int ipmmu_slave_whitelist(struct device *dev)
+{
+	/* Opt-in slave devices based on SoC and ES version */
+	if (soc_device_match(r8a7795es2)) {
+		if (!strcmp(dev_name(dev), "e7310000.dma-controller"))
+			return 0;
+	}
+
+	/* By default, do not allow use of IPMMU */
+	return -ENODEV;
+}
+
 static int ipmmu_add_device_dma(struct device *dev)
 {
 	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
 	struct iommu_group *group;
+	int ret;
 
 	/* only accept devices with iommus property */
 	if (of_count_phandle_with_args(dev->of_node, "iommus",
 				       "#iommu-cells") < 0)
 		return -ENODEV;
 
+	/* opt-in devices based on SoC and ES version */
+	ret = ipmmu_slave_whitelist(dev);
+	if (ret)
+		return ret;
+
 	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group))
 		return PTR_ERR(group);

[PATCH/RFC 2/2] iommu/ipmmu-vmsa: Opt-in slave devices based on ES version

[Geert Uytterhoeven]

Hi Magnus,

On Mon, Jan 23, 2017 at 1:12 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> Match on r8a7795 ES2 and enable a certain DMA controller.
> In other cases the IPMMU driver remains disabled.
>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
>
>  drivers/iommu/ipmmu-vmsa.c |   24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> --- 0001/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-01-23 20:57:02.620607110 +0900
> @@ -23,6 +23,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> +#include <linux/sys_soc.h>
>
>  #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
>  #include <asm/dma-iommu.h>
> @@ -1002,16 +1003,39 @@ static void ipmmu_domain_free_dma(struct
>         }
>  }
>
> +static const struct soc_device_attribute r8a7795es2[] = {
> +       { .soc_id = "r8a7795", .revision = "ES2.*" },
> +       { /* sentinel */ }
> +};
> +
> +static int ipmmu_slave_whitelist(struct device *dev)
> +{
> +       /* Opt-in slave devices based on SoC and ES version */
> +       if (soc_device_match(r8a7795es2)) {
> +               if (!strcmp(dev_name(dev), "e7310000.dma-controller"))
> +                       return 0;
> +       }

I have two comments about the construct above:
  1. IPMMU will be disabled on all non-r8a7795 SoCs.
     Is that what you want?
  2. Usually we match on the old broken versions instead (e.g. against
     "ES1.*"), as (1) it marks more clearly support for old SoCs, and
                (2) it makes it easier to remove the check later when these
                    old SoCs are deemed extinct later.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH/RFC 2/2] iommu/ipmmu-vmsa: Opt-in slave devices based on ES version

[Magnus Damm]

Hi Geert,

On Mon, Jan 23, 2017 at 9:50 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Mon, Jan 23, 2017 at 1:12 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Match on r8a7795 ES2 and enable a certain DMA controller.
>> In other cases the IPMMU driver remains disabled.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  drivers/iommu/ipmmu-vmsa.c |   24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> --- 0001/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-01-23 20:57:02.620607110 +0900
>> @@ -23,6 +23,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/sizes.h>
>>  #include <linux/slab.h>
>> +#include <linux/sys_soc.h>
>>
>>  #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
>>  #include <asm/dma-iommu.h>
>> @@ -1002,16 +1003,39 @@ static void ipmmu_domain_free_dma(struct
>>         }
>>  }
>>
>> +static const struct soc_device_attribute r8a7795es2[] = {
>> +       { .soc_id = "r8a7795", .revision = "ES2.*" },
>> +       { /* sentinel */ }
>> +};
>> +
>> +static int ipmmu_slave_whitelist(struct device *dev)
>> +{
>> +       /* Opt-in slave devices based on SoC and ES version */
>> +       if (soc_device_match(r8a7795es2)) {
>> +               if (!strcmp(dev_name(dev), "e7310000.dma-controller"))
>> +                       return 0;
>> +       }
>
> I have two comments about the construct above:
>   1. IPMMU will be disabled on all non-r8a7795 SoCs.
>      Is that what you want?

Sort of. This patch is just an example to stir up some discussion
about this topic. I realize this code as-is changes R-Car Gen2
behavior (that is merged upstream) so perhaps we should keep devices
enabled for those SoCs.

>   2. Usually we match on the old broken versions instead (e.g. against
>      "ES1.*"), as (1) it marks more clearly support for old SoCs, and
>                 (2) it makes it easier to remove the check later when these
>                     old SoCs are deemed extinct later.

Right, if I understand correctly then you're saying opt-out might be
better instead of opt-in. In case of IPMMU and R-Car Gen3 I believe it
might be less work to use opt-in rather than excluding not-yet-working
stuff. =)

With this series I would like to propose to disconnect the DT
integration timing from the enablement of IPMMU support for slave
devices. If we can enable the IPMMU in DT early on we can reduce
potential out-of-tree IPMMU enablement DT patches. So with the DT
bindings fixed and accurate data sheet we can merge DT bits ahead of
enablement time. And then use run time logic to determine what to
enable based on test results.

As you are aware, currently we have used the presence of "iommus" in
DT to determine if a device is going to be enabled or not. So if the
IPMMU Kconfig bits enable the IPMMU driver and the "iommus" DT
property tie a certain slave device to the IPMMU then we will make use
of IPMMU for a certain device. Currently we assume it will work on all
ES versions that use that particular DTB.

However ES specific hardware errata together with a wide range of ES
versions for r8a7795 and r8a7796 (and whatever SoCs and ES versions
that comes next) makes it difficult to use DT like above to enable
stuff seemingly on one ES version without potentially breaking other
ES versions. I would like to share DT files between the ES versions as
much as possible but still only enable IPMMU support for devices that
are known to work.

Let me know if you think it makes sense to enable DT in a different
way than my proposal.

I'll have a look at putting the white list code in ->xlate() instead
of ->add_device().

Thanks for your comments!

Cheers,

/ magnus

[PATCH/RFC 2/2] iommu/ipmmu-vmsa: Opt-in slave devices based on ES version

[Geert Uytterhoeven]

Hi Magnus,

On Tue, Jan 24, 2017 at 10:38 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Mon, Jan 23, 2017 at 9:50 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Mon, Jan 23, 2017 at 1:12 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> From: Magnus Damm <damm+renesas@opensource.se>
>>>
>>> Match on r8a7795 ES2 and enable a certain DMA controller.
>>> In other cases the IPMMU driver remains disabled.
>>>
>>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>>> ---
>>>
>>>  drivers/iommu/ipmmu-vmsa.c |   24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> --- 0001/drivers/iommu/ipmmu-vmsa.c
>>> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-01-23 20:57:02.620607110 +0900
>>> @@ -23,6 +23,7 @@
>>>  #include <linux/platform_device.h>
>>>  #include <linux/sizes.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/sys_soc.h>
>>>
>>>  #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
>>>  #include <asm/dma-iommu.h>
>>> @@ -1002,16 +1003,39 @@ static void ipmmu_domain_free_dma(struct
>>>         }
>>>  }
>>>
>>> +static const struct soc_device_attribute r8a7795es2[] = {
>>> +       { .soc_id = "r8a7795", .revision = "ES2.*" },
>>> +       { /* sentinel */ }
>>> +};
>>> +
>>> +static int ipmmu_slave_whitelist(struct device *dev)
>>> +{
>>> +       /* Opt-in slave devices based on SoC and ES version */
>>> +       if (soc_device_match(r8a7795es2)) {
>>> +               if (!strcmp(dev_name(dev), "e7310000.dma-controller"))
>>> +                       return 0;
>>> +       }
>>
>> I have two comments about the construct above:
>>   1. IPMMU will be disabled on all non-r8a7795 SoCs.
>>      Is that what you want?
>
> Sort of. This patch is just an example to stir up some discussion
> about this topic. I realize this code as-is changes R-Car Gen2
> behavior (that is merged upstream) so perhaps we should keep devices
> enabled for those SoCs.

Indeed.
Note that we don't have any "iommus" in upstream R-Car Gen2 DTSes.

>>   2. Usually we match on the old broken versions instead (e.g. against
>>      "ES1.*"), as (1) it marks more clearly support for old SoCs, and
>>                 (2) it makes it easier to remove the check later when these
>>                     old SoCs are deemed extinct later.
>
> Right, if I understand correctly then you're saying opt-out might be
> better instead of opt-in. In case of IPMMU and R-Car Gen3 I believe it
> might be less work to use opt-in rather than excluding not-yet-working
> stuff. =)

Unfortunately that may be true :-(

> With this series I would like to propose to disconnect the DT
> integration timing from the enablement of IPMMU support for slave
> devices. If we can enable the IPMMU in DT early on we can reduce
> potential out-of-tree IPMMU enablement DT patches. So with the DT
> bindings fixed and accurate data sheet we can merge DT bits ahead of
> enablement time. And then use run time logic to determine what to
> enable based on test results.
>
> As you are aware, currently we have used the presence of "iommus" in
> DT to determine if a device is going to be enabled or not. So if the
> IPMMU Kconfig bits enable the IPMMU driver and the "iommus" DT
> property tie a certain slave device to the IPMMU then we will make use
> of IPMMU for a certain device. Currently we assume it will work on all
> ES versions that use that particular DTB.
>
> However ES specific hardware errata together with a wide range of ES
> versions for r8a7795 and r8a7796 (and whatever SoCs and ES versions
> that comes next) makes it difficult to use DT like above to enable
> stuff seemingly on one ES version without potentially breaking other
> ES versions. I would like to share DT files between the ES versions as
> much as possible but still only enable IPMMU support for devices that
> are known to work.
>
> Let me know if you think it makes sense to enable DT in a different
> way than my proposal.

That makes perfect sense to me: DT describes (ideal production) hardware,
and errata are handled in C code and tables.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds