[PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype

[PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3.

Not for upstream merge.

Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se>
---

 Applies on top of renesas-devel-20181019-v4.19-rc8

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

--- 0001/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2018-10-22 02:46:30.139880557 +0900
@@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st
 
 static bool ipmmu_slave_whitelist(struct device *dev)
 {
+	/* R-Car M3-N/V3H/E3 Ethernet-AVB */
+	if (!strcmp(dev_name(dev), "e6800000.ethernet"))
+		return true;
+
 	/* By default, do not allow use of IPMMU */
 	return false;
 }

Re: [PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype

[Geert Uytterhoeven]

Hi Magnus,

Thanks for your patch!

On Sun, Oct 21, 2018 at 7:56 PM Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm@opensource.se>
>
> For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3.
>
> Not for upstream merge.
>
> Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
>
>  Applies on top of renesas-devel-20181019-v4.19-rc8
>
>  drivers/iommu/ipmmu-vmsa.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> --- 0001/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c     2018-10-22 02:46:30.139880557 +0900
> @@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st
>
>  static bool ipmmu_slave_whitelist(struct device *dev)
>  {
> +       /* R-Car M3-N/V3H/E3 Ethernet-AVB */
> +       if (!strcmp(dev_name(dev), "e6800000.ethernet"))
> +               return true;

I'm afraid the whitelisting doesn't work that way: with the above check, it will
be enabled on all R-Car Gen3 SoCs.

> +
>         /* By default, do not allow use of IPMMU */
>         return false;
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

RE: [PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype

[Yoshihiro Shimoda]

Hi Magnus-san, Geert-san,

> From: Geert Uytterhoeven, Sent: Sunday, October 28, 2018 10:33 PM
> 
> Hi Magnus,
> 
> Thanks for your patch!
> 
> On Sun, Oct 21, 2018 at 7:56 PM Magnus Damm <magnus.damm@gmail.com> wrote:
> > From: Magnus Damm <damm@opensource.se>
> >
> > For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3.
> >
> > Not for upstream merge.
> >
> > Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se>
> > ---
> >
> >  Applies on top of renesas-devel-20181019-v4.19-rc8
> >
> >  drivers/iommu/ipmmu-vmsa.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > --- 0001/drivers/iommu/ipmmu-vmsa.c
> > +++ work/drivers/iommu/ipmmu-vmsa.c     2018-10-22 02:46:30.139880557 +0900
> > @@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st
> >
> >  static bool ipmmu_slave_whitelist(struct device *dev)
> >  {
> > +       /* R-Car M3-N/V3H/E3 Ethernet-AVB */
> > +       if (!strcmp(dev_name(dev), "e6800000.ethernet"))
> > +               return true;
> 
> I'm afraid the whitelisting doesn't work that way: with the above check, it will
> be enabled on all R-Car Gen3 SoCs.

I agree with Geert-san.
So, how about adding .revision into the soc_rcar_gen3 like a whitelist of SoCs first as following?
I believe almost all R-Car Gen3 SoCs can use IPMMU safety, except H3 ES2.0 or older and M3-W ES1.*.

---
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index b98a031..7a528b8 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev)
 }
 
 static const struct soc_device_attribute soc_rcar_gen3[] = {
-	{ .soc_id = "r8a7795", },
-	{ .soc_id = "r8a7796", },
+	{ .soc_id = "r8a7795", .revision = "ES3.*" },
 	{ .soc_id = "r8a77965", },
 	{ .soc_id = "r8a77970", },
+	{ .soc_id = "r8a77990", },
 	{ .soc_id = "r8a77995", },
 	{ /* sentinel */ }
 };

Best regards,
Yoshihiro Shimoda


> > +
> >         /* By default, do not allow use of IPMMU */
> >         return false;
> >  }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

Re: [PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Sunday, October 28, 2018 10:33 PM
> > On Sun, Oct 21, 2018 at 7:56 PM Magnus Damm <magnus.damm@gmail.com> wrote:
> > > From: Magnus Damm <damm@opensource.se>
> > >
> > > For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3.
> > >
> > > Not for upstream merge.
> > >
> > > Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se>
> > > ---
> > >
> > >  Applies on top of renesas-devel-20181019-v4.19-rc8
> > >
> > >  drivers/iommu/ipmmu-vmsa.c |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > --- 0001/drivers/iommu/ipmmu-vmsa.c
> > > +++ work/drivers/iommu/ipmmu-vmsa.c     2018-10-22 02:46:30.139880557 +0900
> > > @@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st
> > >
> > >  static bool ipmmu_slave_whitelist(struct device *dev)
> > >  {
> > > +       /* R-Car M3-N/V3H/E3 Ethernet-AVB */
> > > +       if (!strcmp(dev_name(dev), "e6800000.ethernet"))
> > > +               return true;
> >
> > I'm afraid the whitelisting doesn't work that way: with the above check, it will
> > be enabled on all R-Car Gen3 SoCs.
>
> I agree with Geert-san.
> So, how about adding .revision into the soc_rcar_gen3 like a whitelist of SoCs first as following?
> I believe almost all R-Car Gen3 SoCs can use IPMMU safety, except H3 ES2.0 or older and M3-W ES1.*.

Thanks for the information!

> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev)
>  }
>
>  static const struct soc_device_attribute soc_rcar_gen3[] = {
> -       { .soc_id = "r8a7795", },
> -       { .soc_id = "r8a7796", },
> +       { .soc_id = "r8a7795", .revision = "ES3.*" },
>         { .soc_id = "r8a77965", },
>         { .soc_id = "r8a77970", },
> +       { .soc_id = "r8a77990", },
>         { .soc_id = "r8a77995", },
>         { /* sentinel */ }
>  };

Given the above, I think the time is ripe to convert this from a whitelist to a
blacklist?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

RE: [PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype

[Yoshihiro Shimoda]

Hi Geert-san,

Thank you for the comment!

> From: Geert Uytterhoeven, Sent: Monday, November 12, 2018 5:19 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
<snip>
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev)
> >  }
> >
> >  static const struct soc_device_attribute soc_rcar_gen3[] = {
> > -       { .soc_id = "r8a7795", },
> > -       { .soc_id = "r8a7796", },
> > +       { .soc_id = "r8a7795", .revision = "ES3.*" },
> >         { .soc_id = "r8a77965", },
> >         { .soc_id = "r8a77970", },
> > +       { .soc_id = "r8a77990", },
> >         { .soc_id = "r8a77995", },
> >         { /* sentinel */ }
> >  };
> 
> Given the above, I think the time is ripe to convert this from a whitelist to a
> blacklist?

About the SoCs, I think so. (I updated example patch below and it seems better :) )
However, I would like to keep ipmmu_slave_whitelist to avoid any trouble for now...

---
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index b98a031..ab24128 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -757,12 +757,10 @@ static bool ipmmu_slave_whitelist(struct device *dev)
 	return false;
 }
 
-static const struct soc_device_attribute soc_rcar_gen3[] = {
-	{ .soc_id = "r8a7795", },
-	{ .soc_id = "r8a7796", },
-	{ .soc_id = "r8a77965", },
-	{ .soc_id = "r8a77970", },
-	{ .soc_id = "r8a77995", },
+static const struct soc_device_attribute soc_rcar_gen3_blacklist[] = {
+	{ .soc_id = "r8a7795", .revision = "ES1.*" },
+	{ .soc_id = "r8a7795", .revision = "ES2.*" },
+	{ .soc_id = "r8a7796", .revision = "ES1.*" },
 	{ /* sentinel */ }
 };
 
@@ -770,7 +768,8 @@ static int ipmmu_of_xlate(struct device *dev,
 			  struct of_phandle_args *spec)
 {
 	/* For R-Car Gen3 use a white list to opt-in slave devices */
-	if (soc_device_match(soc_rcar_gen3) && !ipmmu_slave_whitelist(dev))
+	if (!soc_device_match(soc_rcar_gen3_blacklist) &&
+	    !ipmmu_slave_whitelist(dev))
 		return -ENODEV;
 
 	iommu_fwspec_add_ids(dev, spec->args, 1);
---
Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

Re: [PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype

[Simon Horman]

On Mon, Nov 12, 2018 at 09:18:41AM +0100, Geert Uytterhoeven wrote:
> Hi Shimoda-san,
> 
> On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Geert Uytterhoeven, Sent: Sunday, October 28, 2018 10:33 PM
> > > On Sun, Oct 21, 2018 at 7:56 PM Magnus Damm <magnus.damm@gmail.com> wrote:
> > > > From: Magnus Damm <damm@opensource.se>
> > > >
> > > > For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3.
> > > >
> > > > Not for upstream merge.
> > > >
> > > > Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se>
> > > > ---
> > > >
> > > >  Applies on top of renesas-devel-20181019-v4.19-rc8
> > > >
> > > >  drivers/iommu/ipmmu-vmsa.c |    4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > --- 0001/drivers/iommu/ipmmu-vmsa.c
> > > > +++ work/drivers/iommu/ipmmu-vmsa.c     2018-10-22 02:46:30.139880557 +0900
> > > > @@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st
> > > >
> > > >  static bool ipmmu_slave_whitelist(struct device *dev)
> > > >  {
> > > > +       /* R-Car M3-N/V3H/E3 Ethernet-AVB */
> > > > +       if (!strcmp(dev_name(dev), "e6800000.ethernet"))
> > > > +               return true;
> > >
> > > I'm afraid the whitelisting doesn't work that way: with the above check, it will
> > > be enabled on all R-Car Gen3 SoCs.
> >
> > I agree with Geert-san.
> > So, how about adding .revision into the soc_rcar_gen3 like a whitelist of SoCs first as following?
> > I believe almost all R-Car Gen3 SoCs can use IPMMU safety, except H3 ES2.0 or older and M3-W ES1.*.
> 
> Thanks for the information!
> 
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev)
> >  }
> >
> >  static const struct soc_device_attribute soc_rcar_gen3[] = {
> > -       { .soc_id = "r8a7795", },
> > -       { .soc_id = "r8a7796", },
> > +       { .soc_id = "r8a7795", .revision = "ES3.*" },
> >         { .soc_id = "r8a77965", },
> >         { .soc_id = "r8a77970", },
> > +       { .soc_id = "r8a77990", },
> >         { .soc_id = "r8a77995", },
> >         { /* sentinel */ }
> >  };
> 
> Given the above, I think the time is ripe to convert this from a whitelist to a
> blacklist?

My understanding is that the motivation for the whitelist was to allow us
to control enabling this device on a per-SOC basis in a very deliberate
maner to avoid enabling non-working hardware/firmware/driver combinations.

In moving to a whitelist I believe we would be saying that the risk of
such non-working combinations has subsided and can satisfactorily be
managed by a whitelist.

Are we comfortable in saying that?

Re: [PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Nov 12, 2018 at 9:53 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Monday, November 12, 2018 5:19 PM
> > On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> <snip>
> > > --- a/drivers/iommu/ipmmu-vmsa.c
> > > +++ b/drivers/iommu/ipmmu-vmsa.c
> > > @@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev)
> > >  }
> > >
> > >  static const struct soc_device_attribute soc_rcar_gen3[] = {
> > > -       { .soc_id = "r8a7795", },
> > > -       { .soc_id = "r8a7796", },
> > > +       { .soc_id = "r8a7795", .revision = "ES3.*" },
> > >         { .soc_id = "r8a77965", },
> > >         { .soc_id = "r8a77970", },
> > > +       { .soc_id = "r8a77990", },
> > >         { .soc_id = "r8a77995", },
> > >         { /* sentinel */ }
> > >  };
> >
> > Given the above, I think the time is ripe to convert this from a whitelist to a
> > blacklist?
>
> About the SoCs, I think so. (I updated example patch below and it seems better :) )
> However, I would like to keep ipmmu_slave_whitelist to avoid any trouble for now...

OK, IC.

> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -757,12 +757,10 @@ static bool ipmmu_slave_whitelist(struct device *dev)
>         return false;
>  }
>
> -static const struct soc_device_attribute soc_rcar_gen3[] = {
> -       { .soc_id = "r8a7795", },
> -       { .soc_id = "r8a7796", },
> -       { .soc_id = "r8a77965", },
> -       { .soc_id = "r8a77970", },
> -       { .soc_id = "r8a77995", },
> +static const struct soc_device_attribute soc_rcar_gen3_blacklist[] = {
> +       { .soc_id = "r8a7795", .revision = "ES1.*" },
> +       { .soc_id = "r8a7795", .revision = "ES2.*" },

I think you can combine both lines using "ES[12].*", too.
But it may be considered less readable, and not greppable for e.g. "ES1".

> +       { .soc_id = "r8a7796", .revision = "ES1.*" },
>         { /* sentinel */ }
>  };
>
> @@ -770,7 +768,8 @@ static int ipmmu_of_xlate(struct device *dev,
>                           struct of_phandle_args *spec)
>  {
>         /* For R-Car Gen3 use a white list to opt-in slave devices */
> -       if (soc_device_match(soc_rcar_gen3) && !ipmmu_slave_whitelist(dev))
> +       if (!soc_device_match(soc_rcar_gen3_blacklist) &&
> +           !ipmmu_slave_whitelist(dev))
>                 return -ENODEV;

Ah, this has the side effect of applying ipmmu_slave_whitelist() on R-Car
Gen2, too, which is probably not what we want?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

RE: [PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Monday, November 12, 2018 6:08 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Nov 12, 2018 at 9:53 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Geert Uytterhoeven, Sent: Monday, November 12, 2018 5:19 PM
> > > On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
<snip>
> > @@ -770,7 +768,8 @@ static int ipmmu_of_xlate(struct device *dev,
> >                           struct of_phandle_args *spec)
> >  {
> >         /* For R-Car Gen3 use a white list to opt-in slave devices */
> > -       if (soc_device_match(soc_rcar_gen3) && !ipmmu_slave_whitelist(dev))
> > +       if (!soc_device_match(soc_rcar_gen3_blacklist) &&
> > +           !ipmmu_slave_whitelist(dev))
> >                 return -ENODEV;
> 
> Ah, this has the side effect of applying ipmmu_slave_whitelist() on R-Car
> Gen2, too, which is probably not what we want?

Oops! You're correct. The current code behavior is:
 - Gen2 doesn't use any whitelist.
 - Gen3 uses the whitelist.

If we apply this my patch above:
 - Gen2 and Gen3 use the whitelist anyway.

This is not our expected, so I think we keep to use the current style.
# Note that my first suggestion [1] also broke the code because
# r8a7795 ES2.0 or older and r8a7796 don't use the whitelist...
# 
# [1] https://www.spinics.net/lists/linux-renesas-soc/msg35048.html

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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